Regression from http://trac.webkit.org/changeset/177791. Happens with base64 encoded SourceMaps only.
<rdar://problem/19709617>
<rdar://problem/19510507>
Created attachment 246000 [details] [PATCH] Attempted fix See screenshots for fix result.
Created attachment 246001 [details] [SCREENSHOT] patched.
Created attachment 246003 [details] [SCREENSHOT] broken behavior.
<rdar://problem/19566353>
Comment on attachment 246000 [details] [PATCH] Attempted fix View in context: https://bugs.webkit.org/attachment.cgi?id=246000&action=review > Source/WebInspectorUI/UserInterface/Models/SourceMapResource.js:106 > + function sourceMapResourceLoadError(error) > { > + // There was an error calling NetworkAgent.loadResource. > + throw new Error(error); > + } I don't think we want to throw an exception in our code when this happens. Doesn't this break iOS 6 and iOS 7 inspection of a page with Source Map contents? > Source/WebInspectorUI/UserInterface/Models/SourceMapResource.js:113 > + var error = parameters.error; > + var content = parameters.content; > + var mimeType = parameters.mimeType; > + var statusCode = parameters.status; This could be written more simply: var {error, content, mimeType, statusCode} = parameters;
Comment on attachment 246000 [details] [PATCH] Attempted fix View in context: https://bugs.webkit.org/attachment.cgi?id=246000&action=review >> Source/WebInspectorUI/UserInterface/Models/SourceMapResource.js:106 >> + } > > I don't think we want to throw an exception in our code when this happens. Doesn't this break iOS 6 and iOS 7 inspection of a page with Source Map contents? Why would that happen? >> Source/WebInspectorUI/UserInterface/Models/SourceMapResource.js:113 >> + var statusCode = parameters.status; > > This could be written more simply: > > var {error, content, mimeType, statusCode} = parameters; Good call!
Comment on attachment 246000 [details] [PATCH] Attempted fix View in context: https://bugs.webkit.org/attachment.cgi?id=246000&action=review >>> Source/WebInspectorUI/UserInterface/Models/SourceMapResource.js:106 >>> + } >> >> I don't think we want to throw an exception in our code when this happens. Doesn't this break iOS 6 and iOS 7 inspection of a page with Source Map contents? > > Why would that happen? So it turns out iOS 6 and iOS 7 don't even register that there is a source map present, even with inline source maps. The SourceMapResource constructor is never called.
Comment on attachment 246000 [details] [PATCH] Attempted fix View in context: https://bugs.webkit.org/attachment.cgi?id=246000&action=review >>>> Source/WebInspectorUI/UserInterface/Models/SourceMapResource.js:106 >>>> + } >>> >>> I don't think we want to throw an exception in our code when this happens. Doesn't this break iOS 6 and iOS 7 inspection of a page with Source Map contents? >> >> Why would that happen? > > So it turns out iOS 6 and iOS 7 don't even register that there is a source map present, even with inline source maps. The SourceMapResource constructor is never called. Yeah, it looks like previously in SourceMapManager._loadAndParseSourceMap we would have done nothing if !NetworkAgent.loadResource. We should probably update that to better handle data URLs.
Created attachment 246060 [details] [PATCH] Fix based on feedback.
Comment on attachment 246060 [details] [PATCH] Fix based on feedback. View in context: https://bugs.webkit.org/attachment.cgi?id=246060&action=review > Source/WebInspectorUI/UserInterface/Models/SourceMapResource.js:107 > + // There was an error calling NetworkAgent.loadResource. > + // FIXME: https://bugs.webkit.org/show_bug.cgi?id=141266 needs to change this condition to be compatible > + // with legacy iOS versions. As it is those versions would hit this code path and stop execution. > + throw new Error(error); I'm still not sure why this needs to throw. We don't want to break the UI. > Source/WebInspectorUI/UserInterface/Models/SourceMapResource.js:133 > + sourceMapResourceLoadError.call(this, "No NetworkAgent.loadResource"); Can we just remove this call or use sourceMapResourceNotAvailable with empty data and a 500 status code or something?
Comment on attachment 246060 [details] [PATCH] Fix based on feedback. View in context: https://bugs.webkit.org/attachment.cgi?id=246060&action=review >> Source/WebInspectorUI/UserInterface/Models/SourceMapResource.js:107 >> + throw new Error(error); > > I'm still not sure why this needs to throw. We don't want to break the UI. This should have been console.error(). >> Source/WebInspectorUI/UserInterface/Models/SourceMapResource.js:133 >> + sourceMapResourceLoadError.call(this, "No NetworkAgent.loadResource"); > > Can we just remove this call or use sourceMapResourceNotAvailable with empty data and a 500 status code or something? Sure, maybe with a console.assert() along with it.
Created attachment 246078 [details] [PATCH] Fix with further feedback. This will no longer halt execution if NetworkAgent.loadResource is unavailable or fails to work. Also WebInspector.SourceCode.prototype._processContent wasn't handling error messages correctly which has been fixed. The result can bee seen in screenshots being attached now.
Created attachment 246079 [details] [SCREENSHOT] patched NetworkAgent error. Shows what happens if the NetworkAgent should be unavailable or return an error state.
Created attachment 246080 [details] [SCREENSHOT] patched NetworkAgent error assertion. Assertion failing when NetworkAgent is missing (inspecting the inspector).
Comment on attachment 246078 [details] [PATCH] Fix with further feedback. View in context: https://bugs.webkit.org/attachment.cgi?id=246078&action=review > Source/WebInspectorUI/UserInterface/Models/SourceMapResource.js:106 > + function sourceMapResourceLoadError(error) > { > + // There was an error calling NetworkAgent.loadResource. > + console.error(error); > + } You don't use sourceMapResourceLoadError since you added the console.assert. Should remove.
Comment on attachment 246078 [details] [PATCH] Fix with further feedback. View in context: https://bugs.webkit.org/attachment.cgi?id=246078&action=review >> Source/WebInspectorUI/UserInterface/Models/SourceMapResource.js:106 >> + } > > You don't use sourceMapResourceLoadError since you added the console.assert. Should remove. It could if NetworkAgent.requestContent has an unexpected failure. See line 143 of the same file. However I suppose this could also just call sourceMapResourceNotAvailable.
Comment on attachment 246078 [details] [PATCH] Fix with further feedback. View in context: https://bugs.webkit.org/attachment.cgi?id=246078&action=review >>> Source/WebInspectorUI/UserInterface/Models/SourceMapResource.js:106 >>> + } >> >> You don't use sourceMapResourceLoadError since you added the console.assert. Should remove. > > It could if NetworkAgent.requestContent has an unexpected failure. See line 143 of the same file. However I suppose this could also just call sourceMapResourceNotAvailable. Oh, I see. It probably would be best to call sourceMapResourceNotAvailable so the UI reflects the error. Otherwise, it will likely show a spinner indefinitely?
Committed r179713: <http://trac.webkit.org/changeset/179713>