Bug 164425 - Worker Script Loads (new Worker(...), importScripts(...)) should be classified as Scripts not Raw requests
Summary: Worker Script Loads (new Worker(...), importScripts(...)) should be classifie...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks: 164427 164860 165724
  Show dependency treegraph
 
Reported: 2016-11-04 13:17 PDT by Joseph Pecoraro
Modified: 2017-07-31 16:55 PDT (History)
14 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (105.70 KB, patch)
2016-12-16 11:43 PST, Joseph Pecoraro
joepeck: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Proposed Fix (105.68 KB, patch)
2016-12-16 12:11 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-yosemite (1.67 MB, application/zip)
2016-12-16 13:15 PST, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (1.04 MB, application/zip)
2016-12-16 13:18 PST, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-yosemite (942.33 KB, application/zip)
2016-12-16 13:21 PST, Build Bot
no flags Details
Patch (107.34 KB, patch)
2017-07-01 03:41 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.40 MB, application/zip)
2017-07-01 04:45 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews100 for mac-elcapitan (1.24 MB, application/zip)
2017-07-01 04:52 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-elcapitan (1.81 MB, application/zip)
2017-07-01 05:15 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (1.08 MB, application/zip)
2017-07-03 15:00 PDT, Build Bot
no flags Details
Patch (107.81 KB, patch)
2017-07-07 04:17 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (1.11 MB, application/zip)
2017-07-07 05:27 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.11 MB, application/zip)
2017-07-07 05:28 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (1008.61 KB, application/zip)
2017-07-07 05:52 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-elcapitan (1.83 MB, application/zip)
2017-07-07 05:53 PDT, Build Bot
no flags Details
Patch (114.88 KB, patch)
2017-07-14 03:48 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (121.74 KB, patch)
2017-07-29 12:07 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (114.94 KB, patch)
2017-07-29 12:54 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (114.88 KB, patch)
2017-07-29 13:15 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2016-11-04 13:17:35 PDT
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.
Comment 1 Radar WebKit Bug Importer 2016-11-04 13:17:57 PDT
<rdar://problem/29117281>
Comment 2 Joseph Pecoraro 2016-11-04 13:21:32 PDT
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.
Comment 3 youenn fablet 2016-11-20 13:27:15 PST
If adding a new loading path from DTL proves to be difficult/takes time, using initiator might be a temporary short-term solution.
Comment 4 Joseph Pecoraro 2016-12-16 11:43:33 PST
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.
Comment 5 Joseph Pecoraro 2016-12-16 11:45:44 PST
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.
Comment 6 Joseph Pecoraro 2016-12-16 12:11:33 PST
Created attachment 297340 [details]
[PATCH] Proposed Fix
Comment 7 Joseph Pecoraro 2016-12-16 12:12:06 PST
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.
Comment 8 WebKit Commit Bot 2016-12-16 12:14:12 PST
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.
Comment 9 Build Bot 2016-12-16 13:15:37 PST
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
Comment 10 Build Bot 2016-12-16 13:15:41 PST
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
Comment 11 Build Bot 2016-12-16 13:18:47 PST
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
Comment 12 Build Bot 2016-12-16 13:18:52 PST
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
Comment 13 Build Bot 2016-12-16 13:21:55 PST
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
Comment 14 Build Bot 2016-12-16 13:21:59 PST
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
Comment 15 Brian Burg 2016-12-20 11:53:19 PST
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.
Comment 16 Yusuke Suzuki 2017-06-28 07:49:08 PDT
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.
Comment 17 youenn fablet 2017-06-28 08:25:22 PDT
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.
Comment 18 Yusuke Suzuki 2017-07-01 03:36:40 PDT
(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.
Comment 19 Yusuke Suzuki 2017-07-01 03:41:17 PDT
Created attachment 314380 [details]
Patch
Comment 20 Build Bot 2017-07-01 03:43:46 PDT
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.
Comment 21 Build Bot 2017-07-01 04:45:06 PDT
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
Comment 22 Build Bot 2017-07-01 04:45:08 PDT
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
Comment 23 Build Bot 2017-07-01 04:52:40 PDT
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
Comment 24 Build Bot 2017-07-01 04:52:45 PDT
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
Comment 25 Build Bot 2017-07-01 05:15:20 PDT
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
Comment 26 Build Bot 2017-07-01 05:15:21 PDT
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
Comment 27 youenn fablet 2017-07-01 10:00:50 PDT
> 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.
Comment 28 Build Bot 2017-07-03 15:00:14 PDT
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
Comment 29 Build Bot 2017-07-03 15:00:16 PDT
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
Comment 30 Yusuke Suzuki 2017-07-07 04:17:15 PDT
Created attachment 314827 [details]
Patch
Comment 31 Build Bot 2017-07-07 04:22:53 PDT
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.
Comment 32 Build Bot 2017-07-07 05:27:10 PDT
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
Comment 33 Build Bot 2017-07-07 05:27:12 PDT
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
Comment 34 Build Bot 2017-07-07 05:28:06 PDT
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
Comment 35 Build Bot 2017-07-07 05:28:07 PDT
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
Comment 36 Build Bot 2017-07-07 05:52:35 PDT
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
Comment 37 Build Bot 2017-07-07 05:52:37 PDT
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
Comment 38 Build Bot 2017-07-07 05:53:02 PDT
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
Comment 39 Build Bot 2017-07-07 05:53:03 PDT
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
Comment 40 Yusuke Suzuki 2017-07-14 03:48:58 PDT
Created attachment 315415 [details]
Patch

WIP
Comment 41 Build Bot 2017-07-14 03:51:19 PDT
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.
Comment 42 Yusuke Suzuki 2017-07-29 12:07:22 PDT
Created attachment 316720 [details]
Patch
Comment 43 Build Bot 2017-07-29 12:09:47 PDT
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.
Comment 44 Yusuke Suzuki 2017-07-29 12:54:23 PDT
Created attachment 316724 [details]
Patch
Comment 45 Build Bot 2017-07-29 13:08:09 PDT
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.
Comment 46 Yusuke Suzuki 2017-07-29 13:15:56 PDT
Created attachment 316725 [details]
Patch
Comment 47 Build Bot 2017-07-29 13:18:43 PDT
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.
Comment 48 youenn fablet 2017-07-31 16:55:40 PDT
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?