WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
[PATCH] Proposed Fix
(10.17 KB, patch)
2016-12-16 14:42 PST
,
Joseph Pecoraro
joepeck
: review-
buildbot
: commit-queue-
Details
Formatted Diff
Diff
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
Details
[PATCH] Proposed Fix
(14.15 KB, patch)
2017-01-03 17:23 PST
,
Joseph Pecoraro
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
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
<
rdar://problem/29607569
>
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.
Top of Page
Format For Printing
XML
Clone This Bug