New issue
Advanced search Search tips

Issue 836129 link

Starred by 1 user

Issue metadata

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

Blocking:
issue 715640



Sign in to add a comment

NetS13nServiceWorker: Support extension service workers

Project Member Reported by falken@chromium.org, Apr 24 2018

Issue description

When NetworkService is enabled, support extensions registering service workers from chrome-extension:// urls. Currently we go to the network service to load these which fails since we should be using ExtensionURLLoaderFactory.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 25 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ae65b291893488bc1ca4fc9e6abc6bff160d4382

commit ae65b291893488bc1ca4fc9e6abc6bff160d4382
Author: Matt Falkenhagen <falken@chromium.org>
Date: Wed Apr 25 16:03:27 2018

NetworkService: Add basic support for extension service workers.

Extensions can register service workers from URLs like
chrome-extension://blahblahblah/sw.js. To load such URLs we must
use ExtensionURLLoaderFactory instead of the NetworkService.

This patch allows extension service workers to be registered.
But it doesn't yet allow subresource requests to fallback to
the ExtensionURLLoaderFactory.

This enables 41 of 49 failing browser tests.

Bug:  836129 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I21f513d892a813ba41c2414a7f0734d1d46898b2
Reviewed-on: https://chromium-review.googlesource.com/1025674
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553585}
[modify] https://crrev.com/ae65b291893488bc1ca4fc9e6abc6bff160d4382/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/ae65b291893488bc1ca4fc9e6abc6bff160d4382/chrome/browser/chrome_content_browser_client.h
[modify] https://crrev.com/ae65b291893488bc1ca4fc9e6abc6bff160d4382/chrome/browser/extensions/extension_protocols_unittest.cc
[modify] https://crrev.com/ae65b291893488bc1ca4fc9e6abc6bff160d4382/content/browser/loader/navigation_url_loader_network_service.cc
[modify] https://crrev.com/ae65b291893488bc1ca4fc9e6abc6bff160d4382/content/browser/service_worker/embedded_worker_instance.cc
[modify] https://crrev.com/ae65b291893488bc1ca4fc9e6abc6bff160d4382/content/browser/service_worker/embedded_worker_instance.h
[modify] https://crrev.com/ae65b291893488bc1ca4fc9e6abc6bff160d4382/content/browser/service_worker/embedded_worker_instance_unittest.cc
[modify] https://crrev.com/ae65b291893488bc1ca4fc9e6abc6bff160d4382/content/browser/service_worker/service_worker_new_script_loader.cc
[modify] https://crrev.com/ae65b291893488bc1ca4fc9e6abc6bff160d4382/content/browser/service_worker/service_worker_new_script_loader.h
[modify] https://crrev.com/ae65b291893488bc1ca4fc9e6abc6bff160d4382/content/browser/service_worker/service_worker_new_script_loader_unittest.cc
[modify] https://crrev.com/ae65b291893488bc1ca4fc9e6abc6bff160d4382/content/browser/service_worker/service_worker_provider_host.cc
[modify] https://crrev.com/ae65b291893488bc1ca4fc9e6abc6bff160d4382/content/browser/service_worker/service_worker_provider_host.h
[modify] https://crrev.com/ae65b291893488bc1ca4fc9e6abc6bff160d4382/content/browser/service_worker/service_worker_script_loader_factory.cc
[modify] https://crrev.com/ae65b291893488bc1ca4fc9e6abc6bff160d4382/content/browser/service_worker/service_worker_script_loader_factory.h
[modify] https://crrev.com/ae65b291893488bc1ca4fc9e6abc6bff160d4382/content/browser/service_worker/service_worker_test_utils.cc
[modify] https://crrev.com/ae65b291893488bc1ca4fc9e6abc6bff160d4382/content/browser/service_worker/service_worker_version.cc
[modify] https://crrev.com/ae65b291893488bc1ca4fc9e6abc6bff160d4382/content/public/browser/content_browser_client.cc
[modify] https://crrev.com/ae65b291893488bc1ca4fc9e6abc6bff160d4382/content/public/browser/content_browser_client.h
[modify] https://crrev.com/ae65b291893488bc1ca4fc9e6abc6bff160d4382/extensions/browser/extension_protocols.cc
[modify] https://crrev.com/ae65b291893488bc1ca4fc9e6abc6bff160d4382/extensions/browser/extension_protocols.h
[modify] https://crrev.com/ae65b291893488bc1ca4fc9e6abc6bff160d4382/extensions/shell/browser/shell_content_browser_client.cc
[modify] https://crrev.com/ae65b291893488bc1ca4fc9e6abc6bff160d4382/extensions/shell/browser/shell_content_browser_client.h
[modify] https://crrev.com/ae65b291893488bc1ca4fc9e6abc6bff160d4382/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

Project Member

Comment 2 by bugdroid1@chromium.org, Apr 26 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c43dbd15b1445ac94f9c16f31886f6e7ba9b57cc

commit c43dbd15b1445ac94f9c16f31886f6e7ba9b57cc
Author: Matt Falkenhagen <falken@chromium.org>
Date: Thu Apr 26 05:46:03 2018

S13nServiceWorker: Save the ptr to non_network_loader_factory.

ServiceWorkerNewScriptLoader was using the ptr then letting it fall
out of scope. ServiceWorkerScriptLoaderFactory still held a ptr to the
factory which is strongly bound, so the factory shouldn't die anyway,
but it seems safer if ServiceWorkerNewScriptLoader holds the cloned
ptr too throughout its life.

I'm not sure if this is actually needed for ExtensionURLLoaderFactory,
but it seems possible to write a factory that would fail if the
cloned ptr falls out of scope.

Bug:  836129 
Change-Id: Id7b4ab9d72bb758fe639e963e2747f915e67aea3
Reviewed-on: https://chromium-review.googlesource.com/1029271
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553929}
[modify] https://crrev.com/c43dbd15b1445ac94f9c16f31886f6e7ba9b57cc/content/browser/service_worker/service_worker_new_script_loader.cc
[modify] https://crrev.com/c43dbd15b1445ac94f9c16f31886f6e7ba9b57cc/content/browser/service_worker/service_worker_new_script_loader.h

Project Member

Comment 3 by bugdroid1@chromium.org, May 10 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/62798c60492048987a8c95b986a5f56765fb448b

commit 62798c60492048987a8c95b986a5f56765fb448b
Author: Matt Falkenhagen <falken@chromium.org>
Date: Thu May 10 06:10:35 2018

NetworkService: Support subresource requests to extension service workers.

This CL makes it so ServiceWorkerSubresourceLoaderFactory uses a
factory created from a ChildURLLoaderFactoryBundle rather than the direct
network factory for "network fallback" after service worker does
not provide a response to a FetchEvent.

The direct network factory didn't work in the case of
chrome-extension:// URLs, since network service does not understand
such URLs.

To create the fallback factory, we use the default
ChildURLLoaderFactoryBundle of the RenderFrameImpl, except remove the
"default factory" from that bundle. The default factory can be something
like the AppCache loader factory, which we don't want to hit when
performing network fallback.

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: If6ae1d6fc161cae8ec4574417a0d8954215f878e
Bug:  836129 
Reviewed-on: https://chromium-review.googlesource.com/1032354
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557465}
[modify] https://crrev.com/62798c60492048987a8c95b986a5f56765fb448b/content/renderer/loader/child_url_loader_factory_bundle.cc
[modify] https://crrev.com/62798c60492048987a8c95b986a5f56765fb448b/content/renderer/loader/child_url_loader_factory_bundle.h
[modify] https://crrev.com/62798c60492048987a8c95b986a5f56765fb448b/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/62798c60492048987a8c95b986a5f56765fb448b/content/renderer/service_worker/service_worker_network_provider.cc
[modify] https://crrev.com/62798c60492048987a8c95b986a5f56765fb448b/content/renderer/service_worker/service_worker_network_provider.h
[modify] https://crrev.com/62798c60492048987a8c95b986a5f56765fb448b/content/renderer/service_worker/service_worker_provider_context.cc
[modify] https://crrev.com/62798c60492048987a8c95b986a5f56765fb448b/content/renderer/service_worker/service_worker_provider_context.h
[modify] https://crrev.com/62798c60492048987a8c95b986a5f56765fb448b/content/renderer/service_worker/service_worker_subresource_loader.cc
[modify] https://crrev.com/62798c60492048987a8c95b986a5f56765fb448b/content/renderer/service_worker/service_worker_subresource_loader.h
[modify] https://crrev.com/62798c60492048987a8c95b986a5f56765fb448b/content/renderer/service_worker/worker_fetch_context_impl.cc
[modify] https://crrev.com/62798c60492048987a8c95b986a5f56765fb448b/content/renderer/service_worker/worker_fetch_context_impl.h
[modify] https://crrev.com/62798c60492048987a8c95b986a5f56765fb448b/content/renderer/shared_worker/embedded_shared_worker_stub.cc
[modify] https://crrev.com/62798c60492048987a8c95b986a5f56765fb448b/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

Comment 4 by falken@chromium.org, May 14 2018

Cc: jam@chromium.org
Labels: M-68
Status: Fixed (was: Started)
Closing this as the browser tests now pass. The remaining failing test ServiceWorkerTest.MimeHandlerView has to do with NetworkService + mimeHandler which seems independent of service workers. jam@ said he will take that.
Project Member

Comment 5 by bugdroid1@chromium.org, May 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/83f1f8b146164249e9c710ba00c33357ccfc5905

commit 83f1f8b146164249e9c710ba00c33357ccfc5905
Author: Matt Falkenhagen <falken@chromium.org>
Date: Fri May 18 08:11:08 2018

service worker: Use URLLoaderFactoryBundle for loading new scripts.

Before this CL, the service worker script loader factories had both
a network loader factory and non-network loader factory, and chose
which one to use based on the URL scheme. A URLLoaderFactoryBundle can
be used for that instead, which simplifies code as the factories
just need a single abstract SharedURLLoaderFactory.

Change-Id: Id9664138601327c65886e81198a4dd15f71af092
Bug:  836129 
Reviewed-on: https://chromium-review.googlesource.com/1059986
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559835}
[modify] https://crrev.com/83f1f8b146164249e9c710ba00c33357ccfc5905/content/browser/service_worker/embedded_worker_instance.cc
[modify] https://crrev.com/83f1f8b146164249e9c710ba00c33357ccfc5905/content/browser/service_worker/embedded_worker_instance.h
[modify] https://crrev.com/83f1f8b146164249e9c710ba00c33357ccfc5905/content/browser/service_worker/embedded_worker_instance_unittest.cc
[modify] https://crrev.com/83f1f8b146164249e9c710ba00c33357ccfc5905/content/browser/service_worker/service_worker_navigation_loader_unittest.cc
[modify] https://crrev.com/83f1f8b146164249e9c710ba00c33357ccfc5905/content/browser/service_worker/service_worker_new_script_loader.cc
[modify] https://crrev.com/83f1f8b146164249e9c710ba00c33357ccfc5905/content/browser/service_worker/service_worker_new_script_loader.h
[modify] https://crrev.com/83f1f8b146164249e9c710ba00c33357ccfc5905/content/browser/service_worker/service_worker_new_script_loader_unittest.cc
[modify] https://crrev.com/83f1f8b146164249e9c710ba00c33357ccfc5905/content/browser/service_worker/service_worker_provider_host.cc
[modify] https://crrev.com/83f1f8b146164249e9c710ba00c33357ccfc5905/content/browser/service_worker/service_worker_provider_host.h
[modify] https://crrev.com/83f1f8b146164249e9c710ba00c33357ccfc5905/content/browser/service_worker/service_worker_script_loader_factory.cc
[modify] https://crrev.com/83f1f8b146164249e9c710ba00c33357ccfc5905/content/browser/service_worker/service_worker_script_loader_factory.h
[modify] https://crrev.com/83f1f8b146164249e9c710ba00c33357ccfc5905/content/browser/service_worker/service_worker_version.cc

Sign in to add a comment