Summary: | Web Inspector: REGRESSION: Inline SourceMap resources show empty content when opened. | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jonathan Wells <jonowells> | ||||||||||||||||
Component: | Web Inspector | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | graouts, joepeck, jonowells, mattbaker, nvasilyev, timothy, webkit-bug-importer | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Attachments: |
|
Description
Jonathan Wells
2015-02-03 17:43:39 PST
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.
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> |