Bug 164425 – Worker Script Loads (new Worker(...), importScripts(...)) should be classified as Scripts not Raw requests

• Status: NEW
• Component: Web Inspector

[Joseph Pecoraro]

Summary:
WorkerTarget's mainResource should be a Resource not a Script

Currently, all Worker resource loads are RawResourceRequests. Such resources do not always get saved. In the past that was fine because they are typically ephemeral (XHR,Fetch, etc). However Worker resources are more like <script src="..."> and we should expect Web Inspector to be informed of them. This impacts Web Inspector in a few different ways:

    - If Web Inspector is opened after a page has loaded
      - Worker Script Resources are not guaranteed to exist so the frontend may not know about them
      - Worker Script Resources may show up as a Script instead of a Resource
        - being Script and not Resource, don't get a Resource Details Sidebar
        - being Script and not Resource, the Network tab won't have Resource load data
    - Web Inspector has a workaround converting the type from XHR -> Script in some places

Notes:
- WorkerScriptLoader issues ThreadableLoader requests that it knows will be Scripts. It would be good to type such requests as Script early on.
- Page.getResourceTree iterates over cachedResourceLoader().allCachedResources(). What determines that CachedResources for XHRs are not in that list?
  - Do they get removed from the list early on? Do they not even get added?

I do not know the best approach here, I'd like to talk with people more familiar with the Loader code to see what would be the right strategy.

[Radar WebKit Bug Importer]

<rdar://problem/29117281>

[Joseph Pecoraro]

I'm going to split this up into pieces since really there are two possible fixes here. The first is WebCore loading classifying these as Script. The second can then be updating Web Inspector's frontend to take advantage of the smarter, more consistent, backend.

[youenn fablet]

If adding a new loading path from DTL proves to be difficult/takes time, using initiator might be a temporary short-term solution.

[Joseph Pecoraro]

Created attachment 297336 [details]
[PATCH] Proposed Fix

This should help pave the way for `new Worker(..., {type: "module"})` script loading.

This also would end up fixing bug 164425, because the internal loads performed by `importScripts(...)` do not get the URL "wiped" because it was in effect behaving like a public fetch client.

[Joseph Pecoraro]

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

View in context: https://bugs.webkit.org/attachment.cgi?id=297336&action=review

> Source/WebCore/ChangeLog:17
> +        A new CachedResource::WorkerScript type is added for `new Worker(...)` resources.
> +        It is a CachedScript just like CachedResource::Script. The only difference is

An alternative to this would be to give CachedScript a "isWorker" flag and set it. Not sure if people think that would be better or not.

> Source/WebCore/ChangeLog:30
> +        This introduces ResourceLoaderContext to encapsulate the necessary scripting
> +        environment context information necessary for loading. This can be set on

After implementing this, it looks like ResourceLoaderContext is pretty similar to SecurityContext. We could consider using that instead.

[Joseph Pecoraro]

Created attachment 297340 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

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

View in context: https://bugs.webkit.org/attachment.cgi?id=297340&action=review

> Source/WebCore/ChangeLog:17
> +        A new CachedResource::WorkerScript type is added for `new Worker(...)` resources.
> +        It is a CachedScript just like CachedResource::Script. The only difference is

An alternative to this would be to give CachedScript a "isWorker" flag and set it. Not sure if people think that would be better or not.

> Source/WebCore/ChangeLog:30
> +        This introduces ResourceLoaderContext to encapsulate the necessary scripting
> +        environment context information necessary for loading. This can be set on

After implementing this, it looks like ResourceLoaderContext is pretty similar to SecurityContext. We could consider using that instead.

[WebKit Commit Bot]

Attachment 297340 [details] did not pass style-queue:


ERROR: Source/WebCore/workers/WorkerImportScriptLoader.h:101:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/loader/cache/CachedResourceRequest.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/workers/Worker.cpp:71:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

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

Attachment 297340 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2738902

New failing tests:
inspector/page/searchInResources.html

[Build Bot]

Created attachment 297347 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

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

Attachment 297340 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2738967

New failing tests:
inspector/page/searchInResources.html

[Build Bot]

Created attachment 297348 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

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

Attachment 297340 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2739000

New failing tests:
inspector/page/searchInResources.html

[Build Bot]

Created attachment 297349 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Blaze Burg]

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

Inspector side changes look okay, but this causes several tests to regress. Youenn or someone should look at loader-side changes.

[Yusuke Suzuki]

It seems that test failure can be simply solved by rebaselining the test results. This patch requires loader review.

Youenn or Alex, could you review this patch?
This patch will be the basis of supporting modules for workers.

[youenn fablet]

Patch no longer applies so it would be good to rebaseline it.

Having a quick look at ResourceLoaderContext, I am not sure we actually want that.
There is some overlap with other variables, like the origin...
If we want to go down that road, we should introduce it in its own patch and do the corresponding refactoring.

[Yusuke Suzuki]

(In reply to youenn fablet from comment #17)
> Patch no longer applies so it would be good to rebaseline it.

OK, let's rebaseline.

> 
> Having a quick look at ResourceLoaderContext, I am not sure we actually want
> that.
> There is some overlap with other variables, like the origin...
> If we want to go down that road, we should introduce it in its own patch and
> do the corresponding refactoring.

I think LoaderContext is necessary to keep information of the request in CachedResourceLoader.
It would be fine to split this part from this patch.

[Yusuke Suzuki]

Created attachment 314380 [details]
Patch

[Build Bot]

Attachment 314380 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/cache/CachedResourceLoader.cpp:420:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/workers/WorkerImportScriptLoader.h:101:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 314380 [details]
Patch

Attachment 314380 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4033388

New failing tests:
imported/w3c/web-platform-tests/resource-timing/rt-initiatorType.worker.html
imported/w3c/web-platform-tests/resource-timing/rt-resources-per-worker.html
imported/w3c/web-platform-tests/resource-timing/rt-performance-extensions.worker.html
imported/w3c/web-platform-tests/fetch/nosniff/parsing-nosniff.html
imported/w3c/web-platform-tests/fetch/nosniff/importscripts.html

[Build Bot]

Created attachment 314381 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 314380 [details]
Patch

Attachment 314380 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4033427

New failing tests:
imported/w3c/web-platform-tests/fetch/nosniff/importscripts.html
imported/w3c/web-platform-tests/resource-timing/rt-initiatorType.worker.html
imported/w3c/web-platform-tests/resource-timing/rt-performance-extensions.worker.html
imported/w3c/web-platform-tests/fetch/nosniff/parsing-nosniff.html
imported/w3c/web-platform-tests/fetch/nosniff/worker.html
imported/w3c/web-platform-tests/resource-timing/rt-resources-per-worker.html

[Build Bot]

Created attachment 314382 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 314380 [details]
Patch

Attachment 314380 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4033414

New failing tests:
imported/w3c/web-platform-tests/resource-timing/rt-initiatorType.worker.html
imported/w3c/web-platform-tests/resource-timing/rt-resources-per-worker.html
imported/w3c/web-platform-tests/resource-timing/rt-performance-extensions.worker.html
imported/w3c/web-platform-tests/fetch/nosniff/worker.html

[Build Bot]

Created attachment 314383 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[youenn fablet]

> I think LoaderContext is necessary to keep information of the request in
> CachedResourceLoader.
> It would be fine to split this part from this patch.

In that case, make sure to have information stored in LoaderContext not redundant with information in CachedResourceRequest and CachedResource.

[Build Bot]

Comment on attachment 314380 [details]
Patch

Attachment 314380 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4046766

New failing tests:
imported/w3c/web-platform-tests/resource-timing/rt-initiatorType.worker.html
imported/w3c/web-platform-tests/resource-timing/rt-resources-per-worker.html
imported/w3c/web-platform-tests/fetch/nosniff/importscripts.html
imported/w3c/web-platform-tests/resource-timing/rt-performance-extensions.worker.html
imported/w3c/web-platform-tests/fetch/nosniff/worker.html

[Build Bot]

Created attachment 314522 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Yusuke Suzuki]

Created attachment 314827 [details]
Patch

[Build Bot]

Attachment 314827 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/cache/CachedResourceLoader.cpp:420:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/workers/WorkerImportScriptLoader.h:101:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 314827 [details]
Patch

Attachment 314827 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4069657

New failing tests:
imported/w3c/web-platform-tests/resource-timing/rt-initiatorType.worker.html
imported/w3c/web-platform-tests/resource-timing/rt-resources-per-worker.html
imported/w3c/web-platform-tests/resource-timing/rt-performance-extensions.worker.html

[Build Bot]

Created attachment 314832 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 314827 [details]
Patch

Attachment 314827 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4069660

New failing tests:
imported/w3c/web-platform-tests/resource-timing/rt-initiatorType.worker.html
imported/w3c/web-platform-tests/resource-timing/rt-resources-per-worker.html
imported/w3c/web-platform-tests/resource-timing/rt-performance-extensions.worker.html

[Build Bot]

Created attachment 314833 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 314827 [details]
Patch

Attachment 314827 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4069696

New failing tests:
imported/w3c/web-platform-tests/resource-timing/rt-initiatorType.worker.html
imported/w3c/web-platform-tests/resource-timing/rt-resources-per-worker.html
imported/w3c/web-platform-tests/resource-timing/rt-performance-extensions.worker.html

[Build Bot]

Created attachment 314835 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Build Bot]

Comment on attachment 314827 [details]
Patch

Attachment 314827 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4069692

New failing tests:
imported/w3c/web-platform-tests/resource-timing/rt-initiatorType.worker.html
imported/w3c/web-platform-tests/resource-timing/rt-resources-per-worker.html
imported/w3c/web-platform-tests/resource-timing/rt-performance-extensions.worker.html

[Build Bot]

Created attachment 314836 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Yusuke Suzuki]

Created attachment 315415 [details]
Patch

WIP

[Build Bot]

Attachment 315415 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/cache/CachedResourceLoader.cpp:420:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/workers/WorkerImportScriptLoader.h:102:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 316720 [details]
Patch

[Build Bot]

Attachment 316720 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/cache/CachedResourceLoader.cpp:420:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 316724 [details]
Patch

[Build Bot]

Attachment 316724 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/cache/CachedResourceLoader.cpp:420:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 316725 [details]
Patch

[Build Bot]

Attachment 316725 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/cache/CachedResourceLoader.cpp:420:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Comment on attachment 316725 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316725&action=review

> Source/WebCore/loader/ResourceLoaderContext.h:42
> +    String referrer() const { return m_referrer; }

const String& probably

> Source/WebCore/loader/ResourceLoaderContext.h:77
> +    std::unique_ptr<ContentSecurityPolicy> m_contentSecurityPolicy;

unique ref probably

> Source/WebCore/loader/cache/CachedResource.cpp:120
> +    , m_loaderContext(request.loaderContext())

If we want to be consistent, we should have releaseLoaderContext here.

> Source/WebCore/loader/cache/CachedResource.h:302
> +    RefPtr<ResourceLoaderContext> m_loaderContext;

It is fine to use ResourceLoaderContext for going from one thread to the other, maybe this could also be a field of CachedResourceRequest.

That said, there is no notion of loader context in the fetch algorithm.
It seems to be close to https://fetch.spec.whatwg.org/#concept-request-client which might be good to have.
So maybe we should add this abstraction instead or make clear ResourceLoaderContext is the same as fetch request client and have it into CachedResourceRequest.
ResourceLoaderContext is never used by ResourceLoader so probably we could come up with a better name.

I am less sure about having it inside CachedResource.
We already have CachedResouce::m_origin.
The actual Referer header value is computed and stored within the internal ResourceRequest and might be different from the ResourceLoaderContext one.
We probably need to store CSP but why not adding it as a member of CachedResource as well? or add CachedResourceRequest as a member to CachedResource?