New issue
Advanced search Search tips

Issue 843860 link

Starred by 3 users

Issue metadata

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

Blocking:
issue 706331



Sign in to add a comment

ServiceWorkerSubresourceLoaderFactory should work on worker for sync loading

Project Member Reported by kinuko@chromium.org, May 17 2018

Issue description

Current sync-loading logic that lets the issuing thread block on SyncLoadContext on another thread doesn't work well with off-main-thread sync loading with Network Service / S13nServiceWorker, as ServiceWorker's subresourceloader, which needs to handle the request, lives on the issuer's thread which is being blocked during sync loading.

We probably need to make SWSubresourceLoader{,Factory} work on the same thread as the SyncLoadContext.  This is blocking enabling bigger migration for off-main-thread loading (given that we're shipping Network Service / S13nServiceWorker).  See  issue 706331  #c39 for more details.
 

Comment 1 by kinuko@chromium.org, May 17 2018

Labels: -Pri-3 Proj-Servicification Pri-2

Comment 2 by dxie@chromium.org, May 17 2018

Labels: Proj-Servicification-Canary OS-All

Comment 3 by kinuko@chromium.org, May 22 2018

Owner: kinuko@chromium.org
Status: Assigned (was: Available)
Project Member

Comment 4 by bugdroid1@chromium.org, May 23 2018

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

commit 13ad0c14313a98543ba25c34ff89fb639ce98fdc
Author: Kinuko Yasuda <kinuko@chromium.org>
Date: Wed May 23 00:49:23 2018

Run ServiceWorkerSubresourceLoader on IO thread for Workers

In order to avoid potential dead-locks with synchronous fetches.

The same can be done for fetches from main threads too, but it's not
really necessary as currently synchronous loads from the main thread
(i.e. sync XHR) don't go through service workers (and also is deprecated).

On workers on the other hand sync XHR is not deprecated (and existing service
worker code supports interception), and importScripts() also need to
go through service workers.

Bug:  843860 
Change-Id: I068be4b4bfb7e37cd2b2aabb0be848931ee107c7
Reviewed-on: https://chromium-review.googlesource.com/1065402
Commit-Queue: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560861}
[modify] https://crrev.com/13ad0c14313a98543ba25c34ff89fb639ce98fdc/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/13ad0c14313a98543ba25c34ff89fb639ce98fdc/content/renderer/service_worker/controller_service_worker_connector.cc
[modify] https://crrev.com/13ad0c14313a98543ba25c34ff89fb639ce98fdc/content/renderer/service_worker/controller_service_worker_connector.h
[modify] https://crrev.com/13ad0c14313a98543ba25c34ff89fb639ce98fdc/content/renderer/service_worker/service_worker_subresource_loader.h
[modify] https://crrev.com/13ad0c14313a98543ba25c34ff89fb639ce98fdc/content/renderer/service_worker/worker_fetch_context_impl.cc
[modify] https://crrev.com/13ad0c14313a98543ba25c34ff89fb639ce98fdc/content/renderer/service_worker/worker_fetch_context_impl.h
[modify] https://crrev.com/13ad0c14313a98543ba25c34ff89fb639ce98fdc/content/renderer/shared_worker/embedded_shared_worker_stub.cc

Comment 5 by kinuko@chromium.org, May 23 2018

Status: Fixed (was: Assigned)
I think it's fixed, hopefully.

Comment 6 by falken@chromium.org, May 23 2018

Labels: M-68

Comment 7 by bashi@chromium.org, Jun 1 2018

Cc: bashi@chromium.org
 Issue 846228  has been merged into this issue.

Sign in to add a comment