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. If adding a new loading path from DTL proves to be difficult/takes time, using initiator might be a temporary short-term solution. 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. Created attachment 297340 [details]
[PATCH] Proposed Fix
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.
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 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 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 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 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 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. Created attachment 314380 [details]
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.
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 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 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 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 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 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.
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 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
Created attachment 314827 [details]
Patch
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
Created attachment 315415 [details]
Patch
WIP
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.
Created attachment 316720 [details]
Patch
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.
Created attachment 316724 [details]
Patch
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.
Created attachment 316725 [details]
Patch
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? |
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.