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.
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.
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 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 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.
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.
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
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
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 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.
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.
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.
(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.
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.
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
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
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
> 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.
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
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 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
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 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
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 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
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 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
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
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.
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.
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.
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 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?
2016-12-16 11:43 PST, Joseph Pecoraro
2016-12-16 12:11 PST, Joseph Pecoraro
2016-12-16 13:15 PST, Build Bot
2016-12-16 13:18 PST, Build Bot
2016-12-16 13:21 PST, Build Bot
2017-07-01 03:41 PDT, Yusuke Suzuki
2017-07-01 04:45 PDT, Build Bot
2017-07-01 04:52 PDT, Build Bot
2017-07-01 05:15 PDT, Build Bot
2017-07-03 15:00 PDT, Build Bot
2017-07-07 04:17 PDT, Yusuke Suzuki
2017-07-07 05:27 PDT, Build Bot
2017-07-07 05:28 PDT, Build Bot
2017-07-07 05:52 PDT, Build Bot
2017-07-07 05:53 PDT, Build Bot
2017-07-14 03:48 PDT, Yusuke Suzuki
2017-07-29 12:07 PDT, Yusuke Suzuki
2017-07-29 12:54 PDT, Yusuke Suzuki
2017-07-29 13:15 PDT, Yusuke Suzuki