Web Inspector: Cross Origin importScripts() scripts lack source URL, causes issues with Inspector showing Resource

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

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));
        }
        ...
    }

[Joseph Pecoraro]

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]

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

[BJ Burg]

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 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 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]

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]

(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]

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]

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]

s/WorkerThreadableLoader/WorkerScriptLoader/

[Joseph Pecoraro]

<rdar://problem/29607569>

[Joseph Pecoraro]

Retitling since this was not about blob URLs, but was actually about cross origin imported scripts.

[Joseph Pecoraro]

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]

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]

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]

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 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]

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

[BJ Burg]

Comment on attachment 297357 [details]
[PATCH] Proposed Fix

Inspector side looks good to me. Youenn, can you check the loader changes?

[youenn fablet]

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]

> 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]

Created attachment 297975 [details]
[PATCH] Proposed Fix

[Yusuke Suzuki]

Ooops

[youenn fablet]

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 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 on attachment 297975 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 297975

Committed r210279: <http://trac.webkit.org/changeset/210279>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

> 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