New issue
Advanced search Search tips

Issue 852249 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

NetworkService: shared worker blob URL support.

Project Member Reported by falken@chromium.org, Jun 13 2018

Issue description

We might not support creating shared workers using blob URLs under network service.

SharedWorkerScriptLoader::MaybeStartLoader
  // TODO(falken): Support blob urls.



 

Comment 1 by kinuko@chromium.org, Jun 13 2018

Cc: mek@chromium.org
I think this could be working at least for the cases where we create a blob and blob Url, and then pass the blobUrl to the constructor of a SharedWorker (we can use a blob URL factory obtained in the renderer).  I'm not too clear if nothing to be done for SharedWorker + blob cases though.

Comment 2 by falken@chromium.org, Jun 13 2018

Summary: NetworkService: shared worker blob URL support. (was: NetworkServiec: shared worker blob URL support.)

Comment 3 by mek@chromium.org, 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.

Project Member

Comment 4 by bugdroid1@chromium.org, 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

Comment 5 by dxie@google.com, Jun 19 2018

Labels: Hotlist-KnownIssue

Comment 6 by dxie@google.com, Jun 19 2018

Labels: -Hotlist-KnownIssue

Comment 7 by falken@chromium.org, 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.


Comment 8 by mek@chromium.org, Jun 20 2018

Cc: falken@chromium.org
Owner: mek@chromium.org
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.

Comment 9 by mek@chromium.org, 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...

Comment 10 by mek@chromium.org, 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...
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

Hum, yea I don't know why there would be two requests as you say in c#9.

Comment 13 by mek@chromium.org, 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).
Project Member

Comment 14 by bugdroid1@chromium.org, 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

Comment 15 by mek@chromium.org, Jun 21 2018

Status: Fixed (was: Assigned)

Sign in to add a comment