SourceMap files and resources downloaded by the frontend are currently downloaded synchronously. That is poor and in some cases I've even seen a 10s timeout resulting in a 10s hang in the UI. Most of the time when the frontend wants to download a resource it will want to download the resource through the backend. And even have caching behavior. For instance if you're remotely debugging an application that loads web content from its hard disk (file:///) attempting to load the SourceMap resources from the host machine (frontend) won't work. and worst case could produce unexpected results.
Created attachment 192569 [details] [PATCH] Proposed Async XHR through Backend with cross origin requests allowed So I spoke with bradee-oh and he suggested I just try using an XHR through the backend page. This seems to work in the general case, but got a bit messy in the inspector code (since the result is a load through the inspected page we have to block sending "willSend / didReceive / finished / failed" delegates from being sent to the frontend for these loads. There are some further issues that I would need to work out if we decide to go this route, but before I go any further I'd like to see if anyone has thoughts about this approach. yurys, pfeldman, any comments about this approach? Does it sound acceptable, or is it too messy (if so, any suggestions)?
<rdar://problem/13238886>
(In reply to comment #0) > SourceMap files and resources downloaded by the frontend are currently downloaded synchronously. That is poor and in some cases I've even seen a 10s timeout resulting in a 10s hang in the UI. Fully agree, we need a way to load source maps asynchronously. > > Most of the time when the frontend wants to download a resource it will want to download the resource through the backend. And even have caching behavior. For instance if you're remotely debugging an application that loads web content from its hard disk (file:///) attempting to load the SourceMap resources from the host machine (frontend) won't work. and worst case could produce unexpected results. Could you please elaborate on this scenario? Why would you need to remotely debug an application that is served from file system from another host in the real world? For debugging a web page on a mobile phone it seems reasonable to load resource from the host machine - it would be faster and I don't see a real case where it wouldn't work.
Comment on attachment 192569 [details] [PATCH] Proposed Async XHR through Backend with cross origin requests allowed View in context: https://bugs.webkit.org/attachment.cgi?id=192569&action=review > Source/WebCore/inspector/InspectorResourceAgent.cpp:216 > + if (!m_frontendLoadRequestIdentifier.isNull()) { Some comments to this patch: 1) I don't see a reason to filter out those inspector requests from network panel. 2) A simpler (less intrusive) way to implement this would be to create an XMLHttpRequest object and add event listener to it. 3) Instead of adding a command and an event to the protocol it would be better to use an "async" command. See Database.executeSQL for an example.
(In reply to comment #3) > (In reply to comment #0) > > Most of the time when the frontend wants to download a resource it will want to download the resource through the backend. And even have caching behavior. For instance if you're remotely debugging an application that loads web content from its hard disk (file:///) attempting to load the SourceMap resources from the host machine (frontend) won't work. and worst case could produce unexpected results. > > Could you please elaborate on this scenario? Why would you need to remotely debug an application that is served from file system from another host in the real world? > > For debugging a web page on a mobile phone it seems reasonable to load resource from the host machine - it would be faster and I don't see a real case where it wouldn't work. The source map might be located in an app bundle on the device (back-end) and no content related to the app is on the front-end machine.
> The source map might be located in an app bundle on the device (back-end) and no content related to the app is on the front-end machine. So to make sure I understand, you want to ask debug target to send you back something located on its filesystem. Debug target's responsibility is to make sure this filesystem access is sufficiently sandboxed. I.e.: Client -> Remote Debugging Protocol -> WebCore -> WebKit -> FileSystem -> WebCore -> Remote Debugging Protocol Response -> Client. Sounds like too much work fo supporting specific deployment scenario. Is this for release deployment or for debugging purposes only? Alternatively, you could use data urls for source maps and hence get rid of the sources lookup altogether.
The inspecte page (different device or not) has credentials, cookies, etc. that might also affect access to source maps an original sources. So the app scenario is not the only case of this. The app scenario also simplifies development, not released binaries. It allows developers to have their maps, compined source and original source in one location without configuring anything the Inspector.
(In reply to comment #4) > 1) I don't see a reason to filter out those inspector requests from network panel. > 2) A simpler (less intrusive) way to implement this would be to create an XMLHttpRequest object and add event listener to it. > 3) Instead of adding a command and an event to the protocol it would be better to use an "async" command. See Database.executeSQL for an example. Good suggestions. I still think the frontend would not want to see the network requests / timing information for downloading say a source map file, but the source map resources that would probably be fine. I did not know about "async" commands, thanks for pointing that out. (In reply to comment #6) > > The source map might be located in an app bundle on the device (back-end) and no content related to the app is on the front-end machine. > > So to make sure I understand, you want to ask debug target to send you back something located on its filesystem. Debug target's responsibility is to make sure this filesystem access is sufficiently sandboxed. I.e.: > > Client -> Remote Debugging Protocol -> WebCore -> WebKit -> FileSystem -> WebCore -> Remote Debugging Protocol Response -> Client. Correct. You raise a good point about security though. I'll need to think about that. This API is slightly different from running an XHR through the JS Console (already possible). The exception here is that the API being proposed AllowsCrossOrigin access, and maybe that is enough to be a cause for concern. > Sounds like too much work for supporting specific deployment scenario. Is this for release deployment or for debugging purposes only? An app bundle my include their own myapp:// URL protocol and hooks. So a page with a <script src="myapp://script.js"> with "//@ sourceMappingURL=script.map.js" would resolve the source map to "myapp://script.map.js". That URL would be loadable on the debug target but would not be loadable (or be potentially incorrect) from the host. > Alternatively, you could use data urls for source maps and hence get rid of the sources lookup altogether. That is true, that can significantly increase the file-size of the script loaded. Developers can make it so data url source maps are only included while they are debugging their applications and absent in production / release builds. This sounds acceptable, but requires the developer to do a bit more work.
For all the shit Chrome folks gave us about this, they just did pretty much this exact same thing in Blink. https://chromium.googlesource.com/chromium/blink/+/5bf5cf554eb1059711a450d8dc8ddbf0eef817f3 We should proceed with this approach. I like their simplified version of just one method in the protocol, unless we have reason for doing it our own way. Filtering from the Timeline?
I think they single method approach is best. FrontendRequestId is not needed since method calls in the protocol can have async returns now.
(In reply to comment #10) > We should proceed with this approach. I like their simplified version of just one method in the protocol, unless we have reason for doing it our own way. Filtering from the Timeline? Correct. I did filtering so that resources requested through this API don't show up in the Resources Sidebar / Timeline as if it was a resource loaded by the page; because in reality it was a resource loaded by the frontend via the inspected page. I thinking showing the resources that way can be misleading. However we can revisit that decision. We can still do the single request approach, and I would prefer it. I just wasn't aware that it existed at the time I wrote this patch.
Ok, I agree we should filter them out. Lets do that and a single method.
Note that there is no way to access file:/// resources using this protocol method in Blink. We thought of this solution as a reasonable compromise given that files can be accessed using workspace in Blink.
(In reply to comment #13) > Note that there is no way to access file:/// resources using this protocol method in Blink. We thought of this solution as a reasonable compromise given that files can be accessed using workspace in Blink. What if you need to access a file:/// resource? E.g. a sourceMappingURL to a file:// path?
Blink would not support this case unless workspace is used.
Created attachment 226167 [details] Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=226167&action=review Would be nice to have at least a protocol test for this. Especially if we add support for a "base64Encoded" param. r=me > Source/WebCore/inspector/InspectorResourceAgent.cpp:310 > + auto hiddenResult = m_hiddenRequestIdentifiers.find(identifier); > + if (hiddenResult != m_hiddenRequestIdentifiers.end()) { > + m_hiddenRequestIdentifiers.remove(hiddenResult); > + return; > + } HashSet::remove(ValueType) does exactly this, and returns a boolean if it removed or not. true if found, false if not. So this could be: if (m_hiddenRequestIdentifiers.remove(identifier)) return; I'll leave it up to you if you find that non-obvious. > Source/WebCore/inspector/InspectorResourceAgent.cpp:340 > + auto hiddenResult = m_hiddenRequestIdentifiers.find(identifier); > + if (hiddenResult != m_hiddenRequestIdentifiers.end()) { > + m_hiddenRequestIdentifiers.remove(hiddenResult); > + return; > + } Ditto. > Source/WebCore/inspector/InspectorResourceAgent.cpp:638 > +class LoadResourceListener : public EventListener { Nit: final > Source/WebCore/inspector/InspectorResourceAgent.cpp:659 > + m_callback->sendFailure("Error loading resource"); Nit: These sendFailure calls can be ASCIILiteral. > Source/WebCore/inspector/InspectorResourceAgent.cpp:673 > + } It would be great to handle non-text content here by base64 encoding it. InspectorPageAgent uses (hasTextContent) to check if the resource needs to be base64 encoded or not. It checks the CachedResource type / InspectorPageAgent::ResourceType type. If need be, we could assume certain mime types are text (text/, *xml*) and assume anything else is not. It is not strictly necessary to do this now, but if we decide to use this in the frontend for a non-text resource, it would be good if it worked with older backends. > Source/WebCore/inspector/InspectorResourceAgent.cpp:693 > + *errorString = "No Document instance for the specified frame"; Nit: These errorString assignments can be ASCIILiteral. > Source/WebCore/inspector/InspectorResourceAgent.cpp:697 > + URL url = document->completeURL(urlString); Style: I would put this down below, next to where it is used. > Source/WebCore/inspector/protocol/Network.json:203 > + { "name": "content", "type": "string", "description": "Resource content." }, > + { "name": "mimeType", "type": "string", "description": "Resource mimeType." } I think ideally this should have a "base64Encoded" parameter: { "name": "base64Encoded", "type": "boolean", "description": "True, if content was sent as base64." } Currently we always assume the content is text and not base64Encoded. But in the future, we may attempt to load a resource, like an image, that may not be text. > Source/WebCore/platform/network/ResourceRequestBase.h:144 > + // Whether this request should be hidden from the Inspector. > + bool hiddenFromInspector() const { return m_hiddenFromInspector; } > + void setHiddenFromInspector(bool hiddenFromInspector) { m_hiddenFromInspector = hiddenFromInspector; } Nit: ENABLE(INSPECTOR)? > Source/WebInspectorUI/UserInterface/Models/SourceMapResource.js:76 > + // FIXME: We don't know the MIME-type for inline content. Guess by analyzing the content? > + // Guess by using the type of the original resource? > + sourceMapResourceLoaded.call(this, null, inlineContent, "text/javascript"); Ugh, this is why I proposed inlineContent be a dataURL on the source map dev list =/ > Source/WebInspectorUI/UserInterface/Models/SourceMapResource.js:96 > + // FIXME: Add support for picking the best MIME-type. Right now the file extension is the best bet. > + mimeType = fileExtensionMIMEType; I agree. I don't think servers will be sending the right file extension right now for things like SCSS/Sass/Less/TypeScript. File extension is probably preferred.
Created attachment 226204 [details] Patch
Adding base64Encoded is not trival. The CachedResource type is always XHR, which is treated as text by the page agent. Making it a MIME-type check makes sense.
Comment on attachment 226204 [details] Patch Rejecting attachment 226204 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 226204, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: /ResourceRequestBase.h patching file Source/WebCore/xml/XMLHttpRequest.cpp patching file Source/WebCore/xml/XMLHttpRequest.h patching file Source/WebInspectorUI/UserInterface/Controllers/SourceMapManager.js patching file Source/WebInspectorUI/UserInterface/Models/SourceMapResource.js patching file Source/WebInspectorUI/UserInterface/Protocol/InspectorWebBackendCommands.js Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.appspot.com/results/6150659757834240
https://trac.webkit.org/r165331