RESOLVED FIXED 165569
Web Inspector: Cross Origin importScripts() scripts lack source URL, causes issues with Inspector showing Resource
https://bugs.webkit.org/show_bug.cgi?id=165569
Summary Web Inspector: Cross Origin importScripts() scripts lack source URL, causes i...
Joseph Pecoraro
Reported 2016-12-07 16:53:12 PST
Summary: Web Inspector: Show Worker Resources if Worker started with a Blob URL Steps to Reproduce: 1. Inspect http://tesseract.projectnaptha.com 2. Scroll down to demo => Expected to See Worker Resources in Sidebar but they didn't Notes: - We see the Worker Execution Context in the Console, just not the resources in the Sidebar - The page uses Blob URLs to start the Worker: exports.spawnWorker = function spawnWorker(instance, workerOptions) { if (window.Blob && window.URL) { var blob = new Blob(['importScripts("' + workerOptions.workerPath + '");']); var worker = new Worker(window.URL.createObjectURL(blob)); } ... }
Attachments
[PATCH] Proposed Fix (7.64 KB, patch)
2016-12-08 19:59 PST, Joseph Pecoraro
joepeck: review-
bburg: commit-queue-
[PATCH] Proposed Fix (10.17 KB, patch)
2016-12-16 14:42 PST, Joseph Pecoraro
joepeck: review-
buildbot: commit-queue-
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (937.00 KB, application/zip)
2016-12-16 16:23 PST, Build Bot
no flags
[PATCH] Proposed Fix (14.15 KB, patch)
2017-01-03 17:23 PST, Joseph Pecoraro
no flags
Joseph Pecoraro
Comment 1 2016-12-08 15:12:42 PST
Okay, I'm seeing this worker show up and its resources. However it doesn't relate the Script and Resource for importScript because Script doesn't have the expected URL. Investigating.
Joseph Pecoraro
Comment 2 2016-12-08 19:59:50 PST
Created attachment 296627 [details] [PATCH] Proposed Fix I may also write some tests for redirected Worker resources, which this fixes. Namely inspecting Worker resources in this LayoutTest: workers/worker-importScriptsOnError.html
Blaze Burg
Comment 3 2016-12-09 12:15:51 PST
Comment on attachment 296627 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=296627&action=review r=me with some suggestions > LayoutTests/ChangeLog:13 > + and non-same origin resources. If you could also add data: URL that is an encoded function string, then I think that will cover all non-HTTP scheme cases. > LayoutTests/http/tests/inspector/workers/blob-script-with-cross-domain-imported-scripts-expected.txt:8 > +http://localhost:8000/inspector/workers/resources/worker-blob-script.js Do we need to sanitize this? I'm not sure we are supposed to rely on port 8000. > LayoutTests/http/tests/inspector/workers/blob-script-with-cross-domain-imported-scripts.html:9 > + let blob = new Blob([`importScripts(["http://localhost:8000/inspector/workers/resources/worker-blob-script.js"]);`]); Ditto. > LayoutTests/http/tests/inspector/workers/blob-script-with-cross-domain-imported-scripts.html:11 > + worker = new Worker(blobURL); I had no idea you could even do this. o_O > LayoutTests/http/tests/inspector/workers/blob-script-with-cross-domain-imported-scripts.html:23 > + return "blob:<sanitized>"; We should add this to the reusable URL sanitization function. > Source/WebCore/platform/network/ResourceResponseBase.cpp:106 > + opaqueResponse.setURL(response.url()); What happens without this line? I'm not sure where it is being used. Is it passed along to the frontend when the resource gets loaded as part of the blob's data?
Joseph Pecoraro
Comment 4 2016-12-09 13:51:21 PST
Comment on attachment 296627 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=296627&action=review >> LayoutTests/http/tests/inspector/workers/blob-script-with-cross-domain-imported-scripts-expected.txt:8 >> +http://localhost:8000/inspector/workers/resources/worker-blob-script.js > > Do we need to sanitize this? I'm not sure we are supposed to rely on port 8000. I think we can rely on 8000, there are many tests and results in layout tests and EWS seems fine with it. >> Source/WebCore/platform/network/ResourceResponseBase.cpp:106 >> + opaqueResponse.setURL(response.url()); > > What happens without this line? I'm not sure where it is being used. Is it passed along to the frontend when the resource gets loaded as part of the blob's data? Without this line the ResourceResponse given to the ThreadableLoaderClient is completely empty. I think the URL (final redirected URL) is fine. However it may be that somehow Fetch could access this data when it should be empty. So I'd like someone more familiar with Fetch / Loader Tainting / Opaque Responses to take a look. All existing tests that I found passed, but that is never a good argument on its own.
Joseph Pecoraro
Comment 5 2016-12-09 13:52:15 PST
Comment on attachment 296627 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=296627&action=review >> LayoutTests/ChangeLog:13 >> + and non-same origin resources. > > If you could also add data: URL that is an encoded function string, then I think that will cover all non-HTTP scheme cases. You cannot load Workers with a data URL. We maybe be able to do an importScript with a dataURL though, I'll try that.
Joseph Pecoraro
Comment 6 2016-12-09 19:29:22 PST
It looks like this would break a fetch with an "opaque" response: fetch(<cors-URL>, {mode:"cors"}).then((response) => { assert(response.type === "opaque"); assert(response.url === ""); // Fails, this is the URL. }); So I may have to approach this differently. Is there really no test for this?
Joseph Pecoraro
Comment 7 2016-12-09 19:32:08 PST
(In reply to comment #6) > It looks like this would break a fetch with an "opaque" response: > > fetch(<cors-URL>, {mode:"cors"}).then((response) => { > assert(response.type === "opaque"); > assert(response.url === ""); // Fails, this is the URL. > }); Sorry, this should have been: fetch(<cors-URL>, {mode:"no-cors"}) ... To produce an "opaque" response.
Joseph Pecoraro
Comment 8 2016-12-09 19:35:16 PST
Ideally, the imported `imported/w3c/web-platform-tests/fetch/api/basic/mode-no-cors.js` test should check all of the properties of the opaque response, instead of just a few: https://fetch.spec.whatwg.org/#concept-filtered-response-opaque
Joseph Pecoraro
Comment 9 2016-12-09 20:43:23 PST
Okay lets take a step back and review. The issue is that the source URL for a cross-origin script loaded by a Worker's importScripts(...) is not getting set. This means that any time JavaScript would produce the source URL for that imported script its URL would be empty. Namely viewing the resource in Web Inspector but also even in JavaScript exception messages / stack traces. WorkerGlobalScope::importScripts currently uses WorkerScriptLoader::loadSynchronously with FetchOptions::Mode::NoCors. • Using mode Mode::NoCors the ThreadableLoader path means we receive an Opaque (empty) ResourceReponse but does eventually load the data (performs a basic request) • Using mode Mode::Cors the ThreadableLoader path would fail to load the response only if Access-Control-Allow-Origin. Looking at the relevant specifications. importScripts should load scripts pretty much like <script src="..."> loads classic scripts: https://html.spec.whatwg.org/multipage/workers.html#importing-scripts-and-libraries https://html.spec.whatwg.org/multipage/webappapis.html#fetch-a-classic-worker-imported-script https://fetch.spec.whatwg.org/#concept-main-fetch My best understanding of how we actually load <script src="..."> is that we use a SubresourceLoader instead of a DocumentThreadableLoader. Only the ThreadableLoader path does ResourceResponse filtering / tainting. So in the SubresourceLoader case we are still NoCors but we just don't filter the ResourceResponse. So in the end we get what we want. Unless I've missed something this seems like another case that would best be solved by moving WorkerThreadableLoader off of DocumentThreadableLoader. Short term solutions, like a new loader option to say "oh don't filter this" would just be a hack.
Joseph Pecoraro
Comment 10 2016-12-09 20:43:50 PST
s/WorkerThreadableLoader/WorkerScriptLoader/
Joseph Pecoraro
Comment 11 2016-12-09 20:51:14 PST
Joseph Pecoraro
Comment 12 2016-12-09 20:56:00 PST
Retitling since this was not about blob URLs, but was actually about cross origin imported scripts.
Joseph Pecoraro
Comment 13 2016-12-09 20:58:07 PST
Highly related (probably identical) to: <https://webkit.org/b/164425> Worker Script Loads (new Worker(...), importScripts(...)) should be classified as Scripts not Raw requests
Joseph Pecoraro
Comment 14 2016-12-09 21:00:54 PST
I hope to look into this next week. When I first started re-writing WorkerScriptLoader I was unaware of the loading paths for "classic scripts" and "module scripts" (<script src=...>). There is some obvious overlap here. Also worth noting we don't currently have way to start a Worker with Module code. That would be nice considering we have <script type="module"> now. These all seem highly related.
Joseph Pecoraro
Comment 15 2016-12-16 10:57:22 PST
I'm in the process of a large refactoring to make these loads be CachedScript loads. I'm going to put that work up on bug 164425. However, I think for this particular issue we should start with a short term, small risk fix. Something like the above patch but targeted for worker import script loads which are internal only.
Joseph Pecoraro
Comment 16 2016-12-16 14:42:39 PST
Created attachment 297357 [details] [PATCH] Proposed Fix This is a targeted fix for importScripts. It would be addressed by the more risky fix that I suspect I will be iterating on.
Build Bot
Comment 17 2016-12-16 16:23:43 PST
Comment on attachment 297357 [details] [PATCH] Proposed Fix Attachment 297357 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2739861 New failing tests: http/tests/navigation/keyboard-events-during-provisional-navigation.html
Build Bot
Comment 18 2016-12-16 16:23:50 PST
Created attachment 297372 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Blaze Burg
Comment 19 2016-12-20 11:49:35 PST
Comment on attachment 297357 [details] [PATCH] Proposed Fix Inspector side looks good to me. Youenn, can you check the loader changes?
youenn fablet
Comment 20 2016-12-21 02:30:58 PST
Comment on attachment 297357 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=297357&action=review > Source/WebCore/loader/DocumentThreadableLoader.cpp:289 > + // This is a workaround to avoid opaque filtering of importScripts loads. The load details I am not sure using initiator is the right way to implement this. Fetch spec allows retrieving the internal response from the filtered one. WebKit does not do it that way but ThreadableLoaderOptions::opaqueResponse could be used instead. How about updating ThreadableLoaderOptions::opaqueResponse to something like ResponseFilteringPolicy { Enable, Disable } and a third value (OpaqueResponseBodyPolicy::DoNotReceive) if still needed.
Joseph Pecoraro
Comment 21 2017-01-03 16:48:22 PST
> WebKit does not do it that way but ThreadableLoaderOptions::opaqueResponse could be used instead. > How about updating ThreadableLoaderOptions::opaqueResponse to something like > ResponseFilteringPolicy { Enable, Disable } and a third value > (OpaqueResponseBodyPolicy::DoNotReceive) if still needed. I'll start looking into this.
Joseph Pecoraro
Comment 22 2017-01-03 17:23:48 PST
Created attachment 297975 [details] [PATCH] Proposed Fix
Yusuke Suzuki
Comment 23 2017-01-04 00:30:01 PST
Ooops
youenn fablet
Comment 24 2017-01-04 01:01:05 PST
Comment on attachment 297975 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=297975&action=review > Source/WebCore/loader/ThreadableLoader.h:67 > + Disable, Do you know what coding style is saying about commas for the last enum item? We do not do anything consistent in this file. > Source/WebCore/loader/ThreadableLoader.h:79 > + ResponseFilteringPolicy filteringPolicy { ResponseFilteringPolicy::Enable }; We should look to make consistent opaqueResponse and filteringPolicy. Ideally, they should be merged. If not possible, they should have the same default value type. Currently the first is permissive, the second is restrictive. That could be done as a later patch if we want to land this one quickly.
Joseph Pecoraro
Comment 25 2017-01-04 10:57:41 PST
Comment on attachment 297975 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=297975&action=review >> Source/WebCore/loader/ThreadableLoader.h:67 >> + Disable, > > Do you know what coding style is saying about commas for the last enum item? > We do not do anything consistent in this file. I don't think there is any official coding style here. In my opinion, in multi-line cases like this, we should add the trailing comma. It makes future diffs cleaner, its more consistent to read and less prone to error (adding a new thing or reordering things). On a single line none of these advantages apply so the trailing comma is just superfluous. >> Source/WebCore/loader/ThreadableLoader.h:79 >> + ResponseFilteringPolicy filteringPolicy { ResponseFilteringPolicy::Enable }; > > We should look to make consistent opaqueResponse and filteringPolicy. > Ideally, they should be merged. If not possible, they should have the same default value type. > Currently the first is permissive, the second is restrictive. > That could be done as a later patch if we want to land this one quickly. Okay, lets have that in a follow-up. You seem more familiar so it may be better if you do it, but if you want I can investigate it. I didn't quite understand opaqueResponse's purpose in WorkerScriptLoader's path.
WebKit Commit Bot
Comment 26 2017-01-04 11:21:48 PST
Comment on attachment 297975 [details] [PATCH] Proposed Fix Clearing flags on attachment: 297975 Committed r210279: <http://trac.webkit.org/changeset/210279>
WebKit Commit Bot
Comment 27 2017-01-04 11:21:57 PST
All reviewed patches have been landed. Closing bug.
youenn fablet
Comment 28 2017-01-09 08:46:20 PST
> Okay, lets have that in a follow-up. You seem more familiar so it may be > better if you do it, but if you want I can investigate it. I didn't quite > understand opaqueResponse's purpose in WorkerScriptLoader's path. Filed bug 166843 for that purpose
Note You need to log in before you can comment on or make changes to this bug.