NetworkService: shared worker blob URL support. |
||||||
Issue descriptionWe might not support creating shared workers using blob URLs under network service. SharedWorkerScriptLoader::MaybeStartLoader // TODO(falken): Support blob urls.
,
Jun 13 2018
,
Jun 13 2018
I'm not sure I quite understand how loading of shared workers works, but yeah, it does look like some extra code will be needed to make blob URLs work correctly. Presumably we'll need to do something similar as we do for navigations, where the renderer process creates a BlobURLToken for the url it is trying to create a worker for, and then that BlobURLToken is converted into a SharedURLLoaderFactory to use (using ChromeBlobStorageContext::URLLoaderFactoryForToken) in the browser process. I'll try to write at least a test case to demonstrate the way it is currently broken.
,
Jun 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dab7b3e524e95bcc2c2ca27c8d6b650115d007c1 commit dab7b3e524e95bcc2c2ca27c8d6b650115d007c1 Author: Marijn Kruisselbrink <mek@chromium.org> Date: Thu Jun 14 16:54:05 2018 Add a web platform test to verify various aspects of creating workers from blobs. Bug: 852249 , 800898 Change-Id: Ic885db66683d40f259f4f8c0ddf82d6ea3b4c144 Reviewed-on: https://chromium-review.googlesource.com/1099361 Commit-Queue: Marijn Kruisselbrink <mek@chromium.org> Reviewed-by: Matt Falkenhagen <falken@chromium.org> Cr-Commit-Position: refs/heads/master@{#567308} [modify] https://crrev.com/dab7b3e524e95bcc2c2ca27c8d6b650115d007c1/third_party/WebKit/LayoutTests/TestExpectations [add] https://crrev.com/dab7b3e524e95bcc2c2ca27c8d6b650115d007c1/third_party/WebKit/LayoutTests/external/wpt/workers/worker-from-blob-url.window.js
,
Jun 19 2018
,
Jun 19 2018
,
Jun 20 2018
I was asked to comment on this but I don't understand all the comments here. From c#2, it sounds like `new SharedWorker(blobUrl)` won't work with NetworkService yet. It looks like mek's test shows that we're timing out in both NetworkService and non-NetworkService, possibly with different causes. Non-NS times out I guess due to issue 800898.
,
Jun 20 2018
Today actually I believe things work equally well with and without NetworkService. That however is primarily because today the NWService code path has two different blob URL implementations. The first attempt introduced all kinds of extra race conditions in places that used to work fine before (around downloads and subresource requests), so I came up with a different way to completely get rid of any of these race conditions. So today with network service enabled `new SharedWorker(blobUrl)` works, because the first attempt code is still around. I do hope to delete that code soon though, in which case it might stop working. If you don't mind, I'll re-assign this bug to me, since I think it should be a pretty easy fix, and something I'll need to make sure is done before deleting the first network service blob URL attempt anyway.
,
Jun 20 2018
Or actually I'm not sure I understand how shared worker script loading is supposed to work... Something like https://chromium-review.googlesource.com/c/chromium/src/+/1108496 is what I was thinking, but that doesn't actually seem to do what I expect. That CL does result in SharedWorkerScriptLoaderFactory::CreateLoaderAndStart being called and forwarding to the correct BlobURLLoaderFactory to load from the correct blob, but then it seems the renderer also requests the blob URL through a different code path later, and it is that second request that is actually used to load the script... So not sure how all this is supposed to work from the shared worker side...
,
Jun 20 2018
Ah, and the fact that it doesn't seem that the request from SharedWorkerScriptLoaderFactory is actually used (and instead the regular subresource codepath is used in the renderer) also means that even if I remove the legacy network service blob URL code it'll keep working just as well as it does today...
,
Jun 20 2018
That's surprising, the renderer should use SWSLF when NetworkService is on. I'd expect SWSLF to be used here: https://cs.chromium.org/chromium/src/content/renderer/shared_worker/embedded_shared_worker_stub.cc?l=140&rcl=af1c729e71ca1cba2cdec95fae9d0a63f43410a8
,
Jun 20 2018
Hum, yea I don't know why there would be two requests as you say in c#9.
,
Jun 20 2018
Ah, never mind, I figured out what was going wrong (basically the blob URL specific code in FrameFetchContext was getting in the way of using the correct factory for these shared worker requests).
,
Jun 21 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9996bf405b80d7093e99543eb39e73f91aa1e601 commit 9996bf405b80d7093e99543eb39e73f91aa1e601 Author: Marijn Kruisselbrink <mek@chromium.org> Date: Thu Jun 21 19:02:03 2018 Use BlobURLToken to fix creating shared workers from blob URLs. This change makes sure that if Mojo Blob URLs and Service Worker s13n are enabled there will be no more race conditions between revoking blob URLs and creating shared workers from those URLs. Cq-Include-Trybots: luci.chromium.try:linux_mojo Bug: 852249 Change-Id: I2ad10fbc2c16abb8e4666afae6893e9faf09f285 Reviewed-on: https://chromium-review.googlesource.com/1108496 Commit-Queue: Marijn Kruisselbrink <mek@chromium.org> Reviewed-by: Matt Falkenhagen <falken@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Cr-Commit-Position: refs/heads/master@{#569344} [modify] https://crrev.com/9996bf405b80d7093e99543eb39e73f91aa1e601/content/browser/shared_worker/shared_worker_connector_impl.cc [modify] https://crrev.com/9996bf405b80d7093e99543eb39e73f91aa1e601/content/browser/shared_worker/shared_worker_connector_impl.h [modify] https://crrev.com/9996bf405b80d7093e99543eb39e73f91aa1e601/content/browser/shared_worker/shared_worker_script_loader.cc [modify] https://crrev.com/9996bf405b80d7093e99543eb39e73f91aa1e601/content/browser/shared_worker/shared_worker_service_impl.cc [modify] https://crrev.com/9996bf405b80d7093e99543eb39e73f91aa1e601/content/browser/shared_worker/shared_worker_service_impl.h [modify] https://crrev.com/9996bf405b80d7093e99543eb39e73f91aa1e601/content/browser/shared_worker/shared_worker_service_impl_unittest.cc [modify] https://crrev.com/9996bf405b80d7093e99543eb39e73f91aa1e601/content/common/shared_worker/shared_worker_connector.mojom [modify] https://crrev.com/9996bf405b80d7093e99543eb39e73f91aa1e601/content/renderer/shared_worker/shared_worker_repository.cc [modify] https://crrev.com/9996bf405b80d7093e99543eb39e73f91aa1e601/content/renderer/shared_worker/shared_worker_repository.h [modify] https://crrev.com/9996bf405b80d7093e99543eb39e73f91aa1e601/third_party/WebKit/LayoutTests/FlagExpectations/enable-features=NetworkService [modify] https://crrev.com/9996bf405b80d7093e99543eb39e73f91aa1e601/third_party/blink/public/web/web_shared_worker_repository_client.h [modify] https://crrev.com/9996bf405b80d7093e99543eb39e73f91aa1e601/third_party/blink/renderer/core/exported/shared_worker_repository_client_impl.cc [modify] https://crrev.com/9996bf405b80d7093e99543eb39e73f91aa1e601/third_party/blink/renderer/core/exported/shared_worker_repository_client_impl.h [modify] https://crrev.com/9996bf405b80d7093e99543eb39e73f91aa1e601/third_party/blink/renderer/core/loader/frame_fetch_context.cc [modify] https://crrev.com/9996bf405b80d7093e99543eb39e73f91aa1e601/third_party/blink/renderer/core/workers/shared_worker.cc [modify] https://crrev.com/9996bf405b80d7093e99543eb39e73f91aa1e601/third_party/blink/renderer/core/workers/shared_worker_repository_client.h
,
Jun 21 2018
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by kinuko@chromium.org
, Jun 13 2018