New issue
Advanced search Search tips

Issue 706331 link

Starred by 7 users
Status: Fixed
Owner:
japhet@chromium.org
Closed: Jul 25
Cc:
falken@chromium.org
yhirano@chromium.org
tzik@chromium.org
shimazu@chromium.org
horo@chromium.org
bashi@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 68105
issue 443374
issue 840348
issue 843860
Blocking:
issue 538751
issue 835717



Sign in to add a comment

Support synchronous off-main-thread fetch on workers

Project Member Reported by horo@chromium.org, Mar 29 2017 
In  issue 443374 , we are planing to support asynchronous off-main-thread fetch on workers.
But we should also support synchronous off-main-thread fetch such as importScripts() and sync XHR.
If we can support it, we can remove WorkerThreadableLoader's posting task logic.
It is very important from the point of view of code health.

Comment 1 by horo@chromium.org, Mar 29 2017

Blockedon: 443374

Comment 2 by yhirano@chromium.org, Mar 29 2017

Cc: yhirano@chromium.org tzik@chromium.org

Comment 3 by tyoshino@chromium.org, Apr 12 2017

Blockedon: 68105

Comment 4 by horo@chromium.org, Aug 29 2017

Cc: horo@chromium.org

Comment 5 by horo@chromium.org, Sep 12 2017

Owner: horo@chromium.org
Status: Assigned (was: Available)

Comment 6 by horo@chromium.org, Sep 12 2017 

I created a proof-of-concept patch: https://chromium-review.googlesource.com/c/chromium/src/+/657787

Comment 7 by kinuko@chromium.org, Sep 29 2017

Blocking: 538751

Comment 8 by horo@chromium.org, Dec 20 2017

Status: Started (was: Assigned)
Project Member

Comment 9 by bugdroid1@chromium.org, Dec 21 2017 

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

commit 8685966927ced0bfc55129b863394040bb9ace58
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Thu Dec 21 15:41:26 2017

Use DocumentThreadableLoader for sync loading from worker thread.

This change makes the sync loading on worker thread off the main thread.

I will create another CL to:
 - Rename DocumentThreadableLoader.
 - Remove WorkerThreadableLoader.


Bug:  706331 
Change-Id: Ia625c667eb1367d44976478dd0272317b52e60ea
Reviewed-on: https://chromium-review.googlesource.com/657787
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525696}
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/content/renderer/loader/resource_dispatcher.cc
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/content/renderer/loader/resource_dispatcher.h
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/content/renderer/loader/sync_load_context.cc
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/content/renderer/loader/sync_load_context.h
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/content/renderer/loader/sync_load_response.cc
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/content/renderer/loader/sync_load_response.h
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/content/renderer/loader/url_loader_client_impl.cc
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/content/renderer/loader/url_loader_client_impl.h
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/content/renderer/loader/web_url_loader_impl.cc
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/content/renderer/loader/web_url_loader_impl.h
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/content/renderer/loader/web_url_loader_impl_unittest.cc
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/content/renderer/service_worker/service_worker_fetch_context_impl.cc
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/content/renderer/service_worker/service_worker_fetch_context_impl.h
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/content/renderer/service_worker/worker_fetch_context_impl.cc
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/content/renderer/service_worker/worker_fetch_context_impl.h
[delete] https://crrev.com/d7ae21d5bad97d486539c7edeed21f975acc7420/third_party/WebKit/LayoutTests/external/wpt/XMLHttpRequest/send-sync-response-event-order-expected.txt
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/workers/access-control-basic-get-fail-non-simple-expected.txt
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/workers/cross-origin-unsupported-url-expected.txt
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/third_party/WebKit/Source/core/DEPS
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/third_party/WebKit/Source/core/exported/WebFrameTest.cpp
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/third_party/WebKit/Source/core/loader/ThreadableLoader.cpp
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.cpp
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.h
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/third_party/WebKit/Source/platform/WaitableEvent.h
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/third_party/WebKit/Source/platform/exported/WebURLRequest.cpp
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/third_party/WebKit/Source/platform/loader/fetch/FetchParameters.cpp
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/third_party/WebKit/Source/platform/loader/fetch/RawResource.cpp
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/third_party/WebKit/Source/platform/loader/fetch/RawResource.h
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/third_party/WebKit/Source/platform/loader/fetch/ResourceLoader.cpp
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/third_party/WebKit/Source/platform/testing/weburl_loader_mock.cc
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/third_party/WebKit/Source/platform/testing/weburl_loader_mock.h
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/third_party/WebKit/Tools/Scripts/audit-non-blink-usage.py
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/third_party/WebKit/public/platform/WebURLLoader.h
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/third_party/WebKit/public/platform/WebURLRequest.h
[modify] https://crrev.com/8685966927ced0bfc55129b863394040bb9ace58/third_party/WebKit/public/platform/WebWorkerFetchContext.h
Project Member

Comment 10 by bugdroid1@chromium.org, Dec 23 2017 

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

commit 35cc2b256f2598350915742eefcc505ab902b636
Author: Marijn Kruisselbrink <mek@chromium.org>
Date: Sat Dec 23 00:46:13 2017

Revert "Use DocumentThreadableLoader for sync loading from worker thread."

This reverts commit 8685966927ced0bfc55129b863394040bb9ace58.

Reason for revert: threading related crashes in sync XHR code

Original change's description:
> Use DocumentThreadableLoader for sync loading from worker thread.
> 
> This change makes the sync loading on worker thread off the main thread.
> 
> I will create another CL to:
>  - Rename DocumentThreadableLoader.
>  - Remove WorkerThreadableLoader.
> 
> 
> Bug:  706331 
> Change-Id: Ia625c667eb1367d44976478dd0272317b52e60ea
> Reviewed-on: https://chromium-review.googlesource.com/657787
> Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#525696}

TBR=horo@chromium.org,kinuko@chromium.org,haraken@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  706331 ,  797374 
Change-Id: Ibd2be711ed693084c45d45fda7c51a2d3170361a
Reviewed-on: https://chromium-review.googlesource.com/843145
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526111}
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/content/renderer/loader/resource_dispatcher.cc
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/content/renderer/loader/resource_dispatcher.h
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/content/renderer/loader/sync_load_context.cc
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/content/renderer/loader/sync_load_context.h
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/content/renderer/loader/sync_load_response.cc
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/content/renderer/loader/sync_load_response.h
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/content/renderer/loader/url_loader_client_impl.cc
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/content/renderer/loader/url_loader_client_impl.h
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/content/renderer/loader/web_url_loader_impl.cc
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/content/renderer/loader/web_url_loader_impl.h
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/content/renderer/loader/web_url_loader_impl_unittest.cc
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/content/renderer/service_worker/service_worker_fetch_context_impl.cc
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/content/renderer/service_worker/service_worker_fetch_context_impl.h
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/content/renderer/service_worker/worker_fetch_context_impl.cc
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/content/renderer/service_worker/worker_fetch_context_impl.h
[add] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/third_party/WebKit/LayoutTests/external/wpt/XMLHttpRequest/send-sync-response-event-order-expected.txt
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/workers/access-control-basic-get-fail-non-simple-expected.txt
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/workers/cross-origin-unsupported-url-expected.txt
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/third_party/WebKit/Source/core/DEPS
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/third_party/WebKit/Source/core/exported/WebFrameTest.cpp
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/third_party/WebKit/Source/core/loader/ThreadableLoader.cpp
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.cpp
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.h
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/third_party/WebKit/Source/platform/WaitableEvent.h
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/third_party/WebKit/Source/platform/exported/WebURLRequest.cpp
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/third_party/WebKit/Source/platform/loader/fetch/FetchParameters.cpp
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/third_party/WebKit/Source/platform/loader/fetch/RawResource.cpp
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/third_party/WebKit/Source/platform/loader/fetch/RawResource.h
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/third_party/WebKit/Source/platform/loader/fetch/ResourceLoader.cpp
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/third_party/WebKit/Source/platform/testing/weburl_loader_mock.cc
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/third_party/WebKit/Source/platform/testing/weburl_loader_mock.h
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/third_party/WebKit/Tools/Scripts/audit-non-blink-usage.py
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/third_party/WebKit/public/platform/WebURLLoader.h
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/third_party/WebKit/public/platform/WebURLRequest.h
[modify] https://crrev.com/35cc2b256f2598350915742eefcc505ab902b636/third_party/WebKit/public/platform/WebWorkerFetchContext.h

Comment 11 by falken@chromium.org, Mar 8 2018 

 Issue 817701  has been merged into this issue.
Project Member

Comment 12 by bugdroid1@chromium.org, Mar 26 2018 

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

commit 5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Mon Mar 26 10:24:30 2018

Revert "Revert "Use DocumentThreadableLoader for sync loading from worker thread.""

This reverts commit 35cc2b256f2598350915742eefcc505ab902b636 and
removes "completed_event_->Signal()" from SyncLoadContext::OnReceivedRedirect().
https://chromium-review.googlesource.com/c/chromium/src/+/657787/23/content/renderer/loader/sync_load_context.cc#81

"completed_event_->Signal()" was called twice when SyncLoadContext receives
a cross origin redirect response and caused crashes ( crbug.com/797374 ).

Bug:  706331 ,  797374 
Change-Id: I0562752b4be305a83ac4a0997c50687189d7f6cb

Original change's description:
> Revert "Use DocumentThreadableLoader for sync loading from worker thread."
>
> This reverts commit 8685966927ced0bfc55129b863394040bb9ace58.
>
> Reason for revert: threading related crashes in sync XHR code
>
> Original change's description:
> > Use DocumentThreadableLoader for sync loading from worker thread.
> >
> > This change makes the sync loading on worker thread off the main thread.
> >
> > I will create another CL to:
> >  - Rename DocumentThreadableLoader.
> >  - Remove WorkerThreadableLoader.
> >
> >
> > Bug:  706331 
> > Change-Id: Ia625c667eb1367d44976478dd0272317b52e60ea
> > Reviewed-on: https://chromium-review.googlesource.com/657787
> > Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
> > Reviewed-by: Kentaro Hara <haraken@chromium.org>
> > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#525696}
>
> TBR=horo@chromium.org,kinuko@chromium.org,haraken@chromium.org
>
> # Not skipping CQ checks because original CL landed > 1 day ago.
>
> Bug:  706331 ,  797374 
> Change-Id: Ibd2be711ed693084c45d45fda7c51a2d3170361a
> Reviewed-on: https://chromium-review.googlesource.com/843145
> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
> Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#526111}

Change-Id: I0562752b4be305a83ac4a0997c50687189d7f6cb
Reviewed-on: https://chromium-review.googlesource.com/974989
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545754}
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/content/renderer/loader/resource_dispatcher.cc
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/content/renderer/loader/resource_dispatcher.h
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/content/renderer/loader/sync_load_context.cc
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/content/renderer/loader/sync_load_context.h
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/content/renderer/loader/sync_load_response.cc
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/content/renderer/loader/sync_load_response.h
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/content/renderer/loader/url_loader_client_impl.cc
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/content/renderer/loader/url_loader_client_impl.h
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/content/renderer/loader/web_url_loader_impl.cc
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/content/renderer/loader/web_url_loader_impl.h
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/content/renderer/loader/web_url_loader_impl_unittest.cc
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/content/renderer/service_worker/service_worker_fetch_context_impl.cc
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/content/renderer/service_worker/service_worker_fetch_context_impl.h
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/content/renderer/service_worker/worker_fetch_context_impl.cc
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/content/renderer/service_worker/worker_fetch_context_impl.h
[delete] https://crrev.com/71e4caf1294020283a44cada9aa710fef337c717/third_party/WebKit/LayoutTests/external/wpt/xhr/send-sync-response-event-order-expected.txt
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/workers/access-control-basic-get-fail-non-simple-expected.txt
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/workers/cross-origin-unsupported-url-expected.txt
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/third_party/WebKit/Source/core/DEPS
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/third_party/WebKit/Source/core/exported/WebFrameTest.cpp
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/third_party/WebKit/Source/core/loader/ThreadableLoader.cpp
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.cpp
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.h
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/third_party/WebKit/Source/platform/WaitableEvent.h
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/third_party/WebKit/Source/platform/exported/WebURLRequest.cpp
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/third_party/WebKit/Source/platform/loader/fetch/FetchParameters.cpp
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/third_party/WebKit/Source/platform/loader/fetch/RawResource.cpp
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/third_party/WebKit/Source/platform/loader/fetch/RawResource.h
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/third_party/WebKit/Source/platform/loader/fetch/ResourceLoader.cpp
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/third_party/WebKit/Source/platform/testing/weburl_loader_mock.cc
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/third_party/WebKit/Source/platform/testing/weburl_loader_mock.h
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/third_party/WebKit/Tools/Scripts/audit-non-blink-usage.py
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/third_party/WebKit/public/platform/WebURLLoader.h
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/third_party/WebKit/public/platform/WebURLRequest.h
[modify] https://crrev.com/5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c/third_party/WebKit/public/platform/WebWorkerFetchContext.h

Comment 13 by horo@chromium.org, Mar 27 2018 

I finished moving Sync XHR on workers off the main thread.

And now I'm trying to move importScripts on workers off the main thread to depricate WorkerThreadableLoader.

But it requires to make SyncLoadContext support cross origin redirection and CSP checking of the redirected URL.

Currently SyncLoadContext is used only for sync XHR which doesn't support cross origin redirection.
But importScripts need to support cross origin redirection and CSP checking of the redirected URL.
The tests for CSP on workers are in wpt/content-security-policy/inside-worker/.

SyncLoadContext is implemented under content/, and it is created in a separate thread which is created in ResourceDispatcher::StartSync().
But the CSP logic is implemented under Blink, and the CSP information is in the worker thread.

We need to find a way to make SyncLoadContext support of cross origin redirection and CSP checking.

Comment 14 by yhirano@chromium.org, Mar 27 2018 

How about moving the redirect handling back to blink? That should be more aligned with the async case.

Comment 15 by horo@chromium.org, Mar 27 2018 

Are you saying about [1] "call the OnReceivedRedirect callback from SyncLoadContext::OnReceivedRedirect()" or [2] "return the redirect response from SyncLoadContext::OnReceivedRedirect() and check the response in ResourceLoader::RequestSynchronously() or somewhere in Blink"?

ResourceDispatcher::OnReceivedRedirect() handles the return value of RequestPeer::OnReceivedRedirect() to decide whether to follow the redirection or not. So the redirection check must be in the same thread as ResourceDispatcher.

So for [1] case, we have to run the OnReceivedRedirect callback on the "separate thread" for ResourceDispatcher.
And for [2] case, we need to implement the redirection following logic (such as sending a new request with checking the redirection count limit) in Blink without relying on the following logic in net/.

Comment 16 by yhirano@chromium.org, Mar 27 2018 

I've talked about [2]. We have the redirect handling logic in blink, so I think we can reuse it for the sync case.

Comment 17 by horo@chromium.org, Mar 27 2018 

Ah, I see.
DocumentThreadableLoader::RedirectReceivedBlinkCORS() has the logic.
Thank you.
Project Member

Comment 18 by bugdroid1@chromium.org, Mar 28 2018 

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

commit 9bfedaf10f21ac73f0a2a45c33d8ba76d326ac74
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Wed Mar 28 05:31:34 2018

Move the ownership of terminate_sync_load_event_ to ThreadedMessagingProxyBase

Currently the WaitableEvent for terminating sync XHR on worker thread from the
main thread is owned by WorkerFetchContext. But this is not safe because
the WorkerFetchContext may be already deleted before
ThreadedMessagingProxyBase::TerminateGlobalScope() is called when
WorkerGlobalScope.close() is called on the worker thread.

So this CL moves the ownership of the WaitableEvent to ThreadedMessagingProxyBase.

Bug: 826464, 706331 
Change-Id: If07d028e362b6462e3d710408023b0a10dad2d63
Reviewed-on: https://chromium-review.googlesource.com/982776
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546417}
[modify] https://crrev.com/9bfedaf10f21ac73f0a2a45c33d8ba76d326ac74/content/renderer/service_worker/service_worker_fetch_context_impl.cc
[modify] https://crrev.com/9bfedaf10f21ac73f0a2a45c33d8ba76d326ac74/content/renderer/service_worker/service_worker_fetch_context_impl.h
[modify] https://crrev.com/9bfedaf10f21ac73f0a2a45c33d8ba76d326ac74/content/renderer/service_worker/worker_fetch_context_impl.cc
[modify] https://crrev.com/9bfedaf10f21ac73f0a2a45c33d8ba76d326ac74/content/renderer/service_worker/worker_fetch_context_impl.h
[modify] https://crrev.com/9bfedaf10f21ac73f0a2a45c33d8ba76d326ac74/third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp
[modify] https://crrev.com/9bfedaf10f21ac73f0a2a45c33d8ba76d326ac74/third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.cpp
[modify] https://crrev.com/9bfedaf10f21ac73f0a2a45c33d8ba76d326ac74/third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.h
[modify] https://crrev.com/9bfedaf10f21ac73f0a2a45c33d8ba76d326ac74/third_party/WebKit/public/platform/WebWorkerFetchContext.h

Comment 19 by horo@chromium.org, Mar 29 2018 

I created a test patch (http://crrev.com/c/981959) to use ThreadableLoader from
WorkerClassicScriptLoader::LoadSynchronously(), and found 3 blocking issues to
move importScripts() off the main thread.

[1] Service Worker support
 Currently the service worker handling in the browser process is skipped if the
 request is a sync request. If WorkerClassicScriptLoader calls
 ThreadableLoader::LoadResourceSynchronously(), |is_sync_load| in
 ResourceDispatcherHostImpl::ContinuePendingBeginRequest() will be true in the
 browser process. So the request will not go to the service worker.
 https://chromium.googlesource.com/chromium/src/+blame/5145a01/content/browser/loader/resource_dispatcher_host_impl.cc#1247

 I think we can fix this problem by setting |skip_service_worker| flag in the
 renderer process for sync requests from the main thread.

 Failed tests:
  external/wpt/fetch/api/request/destination/fetch-destination-no-load-event.https.html
  external/wpt/service-workers/service-worker/shared-worker-controlled.https.html


[2] Redirect support and CSP checking
 See the comment 13.
 Failed tests:
  http/tests/workers/shared-worker-importScripts.html
  http/tests/workers/worker-importScripts-onerror-redirect-to-crossorigin.html
  http/tests/workers/worker-importScripts.html

[3] RewriteLayoutTestsURL support.
 RewriteLayoutTestsURL is called from WebFrameTestClient::WillSendRequest() to
 rewrite the request URL such as "file:///gen/" when the request is handled by
 FrameFetchContext. But WorkerFetchContext doesn't have the mechanism. So if
 importScripts() is moved off the main thread, the URL is not rewitten.

 We need to implement the rewite mechanism on the worker thread.
 
 Failed tests:
  mojo/bind-intercepted-interface-in-worker.html
  shapedetection/detection-on-worker.html
Project Member

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

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

commit bb9fd1a8249db7ca306513e8d8baf41d3e770674
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Mon Apr 02 03:27:45 2018

Set SkipServiceWorker flag for synchronous loads from the main thread.

Before 5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c, synchronous XHR on worker
was handled by service workers. It is because the |is_sync_load| was false
when the sync request is from worker thread. But after the CL, the
|is_sync_load| flag for the sync request from worker became true, so the request
will not go to the service worker.

This CL will fix this by
 - Set the SkipServiceWorker flag for synchronous loads from the main thread
   in the renderer process. (FetchParameters.cpp)
 - Don't set skip_service_worker even if is_sync_load is true in the browser
   process. (resource_dispatcher_host_impl.cc)

Bug:  706331 , 827473 
Change-Id: I186bc97f3f8d298e0a04942d0ec4b708b3022cc1
Reviewed-on: https://chromium-review.googlesource.com/989376
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547407}
[modify] https://crrev.com/bb9fd1a8249db7ca306513e8d8baf41d3e770674/content/browser/loader/resource_dispatcher_host_impl.cc
[add] https://crrev.com/bb9fd1a8249db7ca306513e8d8baf41d3e770674/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-request-xhr-sync-on-worker.https.html
[add] https://crrev.com/bb9fd1a8249db7ca306513e8d8baf41d3e770674/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/fetch-request-xhr-sync-on-worker-worker.js
[modify] https://crrev.com/bb9fd1a8249db7ca306513e8d8baf41d3e770674/third_party/WebKit/Source/platform/loader/fetch/FetchParameters.cpp

Comment 21 by falken@chromium.org, Apr 6 2018 

(Copying horo@'s comment on an internal doc)

Remaining work for importScripts() off-main-thread:
 - Redirection support
 - CSP checking after redirect
 - Rewriting URL for LayoutTests

Comment 22 by nhiroki@chromium.org, Apr 23 2018

Blocking: 835717
Project Member

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

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

commit 98cf914b57604ab170479d74eb63bf50696b6c2c
Author: Nate Chapin <japhet@chromium.org>
Date: Tue May 01 20:45:15 2018

Rewrite layout test urls for worker fetches

* Add a mechanic to inject a url rewriter function into
  WorkerFetchContextImpl through layouttest_support.h
* Switch url rewriting to not require any copied state from the
  browser process. Use PathService directly instead.
* Make RewriteLayoutTestsURL static and move it to blink_test_helpers.h
  for easy injection.

Bug:  706331 
Change-Id: I2e680ab39eb2aa1822724f4d82155ef1156b8f24
Reviewed-on: https://chromium-review.googlesource.com/1022811
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Commit-Queue: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555177}
[modify] https://crrev.com/98cf914b57604ab170479d74eb63bf50696b6c2c/content/public/test/layouttest_support.h
[modify] https://crrev.com/98cf914b57604ab170479d74eb63bf50696b6c2c/content/renderer/service_worker/worker_fetch_context_impl.cc
[modify] https://crrev.com/98cf914b57604ab170479d74eb63bf50696b6c2c/content/renderer/service_worker/worker_fetch_context_impl.h
[modify] https://crrev.com/98cf914b57604ab170479d74eb63bf50696b6c2c/content/shell/browser/layout_test/blink_test_controller.cc
[modify] https://crrev.com/98cf914b57604ab170479d74eb63bf50696b6c2c/content/shell/browser/layout_test/layout_test_content_browser_client.cc
[modify] https://crrev.com/98cf914b57604ab170479d74eb63bf50696b6c2c/content/shell/common/layout_test.mojom
[modify] https://crrev.com/98cf914b57604ab170479d74eb63bf50696b6c2c/content/shell/common/shell_messages.h
[modify] https://crrev.com/98cf914b57604ab170479d74eb63bf50696b6c2c/content/shell/renderer/layout_test/blink_test_helpers.cc
[modify] https://crrev.com/98cf914b57604ab170479d74eb63bf50696b6c2c/content/shell/renderer/layout_test/blink_test_helpers.h
[modify] https://crrev.com/98cf914b57604ab170479d74eb63bf50696b6c2c/content/shell/renderer/layout_test/blink_test_runner.cc
[modify] https://crrev.com/98cf914b57604ab170479d74eb63bf50696b6c2c/content/shell/renderer/layout_test/layout_test_content_renderer_client.cc
[modify] https://crrev.com/98cf914b57604ab170479d74eb63bf50696b6c2c/content/shell/renderer/layout_test/layout_test_render_thread_observer.cc
[modify] https://crrev.com/98cf914b57604ab170479d74eb63bf50696b6c2c/content/shell/renderer/layout_test/layout_test_render_thread_observer.h
[modify] https://crrev.com/98cf914b57604ab170479d74eb63bf50696b6c2c/content/test/layouttest_support.cc
Project Member

Comment 24 by bugdroid1@chromium.org, May 7 2018 

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

commit 0817c1db4b405492ea24c6f9fda733480002a1ec
Author: Nate Chapin <japhet@chromium.org>
Date: Mon May 07 20:11:30 2018

Do partial redirect handling in the renderer process for sync requests

Historically, blink has not received any intermediate resource loading
callbacks for sync requests. It simply blocked until the load finished
or failed, which meant that redirect security checks performed by blink
would be skipped (and in some cases we would try to reconstruct or do
a partial security check after the fact). SyncLoadContext allows us to
do more of these security checks before the redirect is sent. This
allows us to come closer to doing the correct thing for sync request
in general, and allows us to move some worker requests off-main-thread,
where before they had to proxy sync requests through the main thread in
order to get both proper redirect handling and the correct synchronous
behavior.

This CL "cancels" sync requests on redirect. WebURLLoaderImpl then
notifies blink of the redirect, which sends FetchContext callbacks (but
not ResourceClient callbacks). This allows us to do things CSP checking
on redirect. If blink permits the redirect, WebURLLoaderImpl manually
follows the redirect by issuing a new sync request.

ResourceClient callbacks aren't performed becuase sync requests have
historically not been associated with a ResourceClient. Doing so would
be a much large refactor, and would require careful handling to make sure
we didn't enter JS inside what is supposed to be a synchronous blocking
function.

We therefore need to continue to perform a rudimentary CORS check in
SyncLoadContext, which blocks cross-origin redirects if the
FetchRequestMode was anything other than kNoCors. This revealed a couple
of XML/XSLT callsites that should have been setting a FetchRequestMode but
weren't.

Bug:  706331 

Change-Id: I4ed54cb3ccd119b8a5d192b077bef8d03745e67e
Reviewed-on: https://chromium-review.googlesource.com/1015237
Commit-Queue: Nate Chapin <japhet@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556546}
[modify] https://crrev.com/0817c1db4b405492ea24c6f9fda733480002a1ec/content/renderer/loader/sync_load_context.cc
[modify] https://crrev.com/0817c1db4b405492ea24c6f9fda733480002a1ec/content/renderer/loader/sync_load_context.h
[modify] https://crrev.com/0817c1db4b405492ea24c6f9fda733480002a1ec/content/renderer/loader/sync_load_response.h
[modify] https://crrev.com/0817c1db4b405492ea24c6f9fda733480002a1ec/content/renderer/loader/web_url_loader_impl.cc
[modify] https://crrev.com/0817c1db4b405492ea24c6f9fda733480002a1ec/content/renderer/loader/web_url_loader_impl.h
[modify] https://crrev.com/0817c1db4b405492ea24c6f9fda733480002a1ec/content/renderer/loader/web_url_loader_impl_unittest.cc
[modify] https://crrev.com/0817c1db4b405492ea24c6f9fda733480002a1ec/third_party/WebKit/LayoutTests/http/tests/appcache/fallback.html
[modify] https://crrev.com/0817c1db4b405492ea24c6f9fda733480002a1ec/third_party/blink/public/platform/web_url_loader.h
[modify] https://crrev.com/0817c1db4b405492ea24c6f9fda733480002a1ec/third_party/blink/renderer/core/xml/parser/xml_document_parser.cc
[modify] https://crrev.com/0817c1db4b405492ea24c6f9fda733480002a1ec/third_party/blink/renderer/core/xml/xslt_processor_libxslt.cc
[modify] https://crrev.com/0817c1db4b405492ea24c6f9fda733480002a1ec/third_party/blink/renderer/platform/loader/fetch/resource_loader.cc
[modify] https://crrev.com/0817c1db4b405492ea24c6f9fda733480002a1ec/third_party/blink/renderer/platform/testing/weburl_loader_mock.cc
[modify] https://crrev.com/0817c1db4b405492ea24c6f9fda733480002a1ec/third_party/blink/renderer/platform/testing/weburl_loader_mock.h
Project Member

Comment 25 by bugdroid1@chromium.org, May 8 2018 

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

commit 545913666a2b2aee8330c05fe7ffc1001ba41e21
Author: Nate Chapin <japhet@chromium.org>
Date: Tue May 08 16:15:05 2018

Use off-main-thread loading for WorkerClassicScriptLoader::LoadSynchronously

This removes the last usage of WorkerThreadableLoader. I'll delete it in a
followup.

Bug:  706331 
Change-Id: I6d452c5ae4a4e6ea99d14dd9d4ba048c66c2cbdc
Reviewed-on: https://chromium-review.googlesource.com/1048133
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Commit-Queue: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556819}
[modify] https://crrev.com/545913666a2b2aee8330c05fe7ffc1001ba41e21/third_party/blink/renderer/core/workers/worker_classic_script_loader.cc
Project Member

Comment 26 by bugdroid1@chromium.org, May 8 2018 

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

commit d6bb2dcc13ac5d402d5f3bbcf8106cd3f2c7f1f0
Author: Matt Menke <mmenke@chromium.org>
Date: Tue May 08 21:43:46 2018

Disable some service-worker and a fetch test with the network service.

I believe these most likely broken by
https://chromium-review.googlesource.com/1048133

TBR=japhet@chromium.org, jam@chromium.org
NOTRY=true

Bug:  729848 ,  706331 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I4b3525af8d2f06fbccf311e40a9f64e7af70ffcd
Reviewed-on: https://chromium-review.googlesource.com/1049831
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556972}
[modify] https://crrev.com/d6bb2dcc13ac5d402d5f3bbcf8106cd3f2c7f1f0/third_party/WebKit/LayoutTests/FlagExpectations/enable-features=NetworkService
Project Member

Comment 27 by bugdroid1@chromium.org, May 8 2018 

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

commit fe527cd3c9af27d2356845b8eb1bdb6d475e9a8d
Author: Nate Chapin <japhet@chromium.org>
Date: Tue May 08 23:18:04 2018

Revert "Use off-main-thread loading for WorkerClassicScriptLoader::LoadSynchronously"

This reverts commit 545913666a2b2aee8330c05fe7ffc1001ba41e21.

Reason for revert: broke some layout tests with NetworkService enabled:
https://chromium-review.googlesource.com/c/chromium/src/+/1049831

Original change's description:
> Use off-main-thread loading for WorkerClassicScriptLoader::LoadSynchronously
> 
> This removes the last usage of WorkerThreadableLoader. I'll delete it in a
> followup.
> 
> Bug:  706331 
> Change-Id: I6d452c5ae4a4e6ea99d14dd9d4ba048c66c2cbdc
> Reviewed-on: https://chromium-review.googlesource.com/1048133
> Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
> Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
> Commit-Queue: Nate Chapin <japhet@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#556819}

TBR=falken@chromium.org,horo@chromium.org,kinuko@chromium.org,japhet@chromium.org,nhiroki@chromium.org

Change-Id: Ie703d84972b1b40e35f8c7ade24d9b6d0c3f3d36
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  706331 
Reviewed-on: https://chromium-review.googlesource.com/1050847
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557000}
[modify] https://crrev.com/fe527cd3c9af27d2356845b8eb1bdb6d475e9a8d/third_party/blink/renderer/core/workers/worker_classic_script_loader.cc
Project Member

Comment 28 by bugdroid1@chromium.org, May 8 2018 

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

commit 9aa30376941de5eccd1375675cb6b3dbedbeacaf
Author: Nate Chapin <japhet@chromium.org>
Date: Tue May 08 23:19:41 2018

Revert "Disable some service-worker and a fetch test with the network service."

This reverts commit d6bb2dcc13ac5d402d5f3bbcf8106cd3f2c7f1f0.

Reason for revert: offending CL reverted: https://chromium-review.googlesource.com/c/chromium/src/+/1050847

Original change's description:
> Disable some service-worker and a fetch test with the network service.
> 
> I believe these most likely broken by
> https://chromium-review.googlesource.com/1048133
> 
> TBR=japhet@chromium.org, jam@chromium.org
> NOTRY=true
> 
> Bug:  729848 ,  706331 
> Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
> Change-Id: I4b3525af8d2f06fbccf311e40a9f64e7af70ffcd
> Reviewed-on: https://chromium-review.googlesource.com/1049831
> Commit-Queue: Matt Menke <mmenke@chromium.org>
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Reviewed-by: Matt Menke <mmenke@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#556972}

TBR=falken@chromium.org,jam@chromium.org,japhet@chromium.org,mmenke@chromium.org

Change-Id: Ib6b665dba5810a5c50f2c7081a0647bcd22e58bc
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  729848 ,  706331 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Reviewed-on: https://chromium-review.googlesource.com/1050500
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557001}
[modify] https://crrev.com/9aa30376941de5eccd1375675cb6b3dbedbeacaf/third_party/WebKit/LayoutTests/FlagExpectations/enable-features=NetworkService

Comment 29 by horo@chromium.org, May 10 2018

Cc: falken@chromium.org shimazu@chromium.org bashi@chromium.org
Ah, I understood why some service worker related tests with NetworkService were
broken by https://chromium-review.googlesource.com/1048133.

This is because:
 - In a service worker thread, or in a (dedicated or shared) worker thread which
   is controled by a service worker, a ServiceWorkerSubresourceLoaderFactory
   handles resource requests.
 - The ServiceWorkerSubresourceLoaderFactory lives on the worker thread.
 - When "importScripts()" is called in the worker thread,
   ResourceDispatcher::StartSync() calls url_loader_factory->Clone().
 - url_loader_factory is a WrapperSharedURLLoaderFactory which has a mojo
   pointer to the ServiceWorkerSubresourceLoaderFactory.
 - ServiceWorkerSubresourceLoaderFactory::Clone() is not synchronously called,
   but a task to call Clone() is PostTasked.
 - But "completed_event.Wait()" blocks the worker thread.
 - So the ServiceWorkerSubresourceLoaderFactory::Clone() will not be called
   forever.

I don't have a good solution for that...

Comment 30 by falken@chromium.org, May 10 2018 

Is the error because of importScripts() from a dedicated/shared worker, or from a service worker? importScripts() from SW should go to a ServiceWorkerScriptLoaderFactory. importScripts() from dedicated/shared worker should  go to ServiceWorkerSubresourceLoaderFactory.

Comment 31 by horo@chromium.org, May 10 2018 

Ah sorry. I misunderstood.
importScripts() from a service worker may have another problem.

I just debugged using fetch-destination-no-load-event.https.html which calls importScripts() from a dedicated worker.
Project Member

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

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

commit 1b5126a68c8620c18c515d5aa1e7bc61394c99af
Author: Nate Chapin <japhet@chromium.org>
Date: Thu May 10 17:37:05 2018

Revert "Do partial redirect handling in the renderer process for sync requests"

This reverts commit 0817c1db4b405492ea24c6f9fda733480002a1ec.

Reason for revert: Canceling and redirecting causes a variety of small, strange behaviors, revert while landing a better solution (https://chromium-review.googlesource.com/c/chromium/src/+/1053237)

Original change's description:
> Do partial redirect handling in the renderer process for sync requests
> 
> Historically, blink has not received any intermediate resource loading
> callbacks for sync requests. It simply blocked until the load finished
> or failed, which meant that redirect security checks performed by blink
> would be skipped (and in some cases we would try to reconstruct or do
> a partial security check after the fact). SyncLoadContext allows us to
> do more of these security checks before the redirect is sent. This
> allows us to come closer to doing the correct thing for sync request
> in general, and allows us to move some worker requests off-main-thread,
> where before they had to proxy sync requests through the main thread in
> order to get both proper redirect handling and the correct synchronous
> behavior.
> 
> This CL "cancels" sync requests on redirect. WebURLLoaderImpl then
> notifies blink of the redirect, which sends FetchContext callbacks (but
> not ResourceClient callbacks). This allows us to do things CSP checking
> on redirect. If blink permits the redirect, WebURLLoaderImpl manually
> follows the redirect by issuing a new sync request.
> 
> ResourceClient callbacks aren't performed becuase sync requests have
> historically not been associated with a ResourceClient. Doing so would
> be a much large refactor, and would require careful handling to make sure
> we didn't enter JS inside what is supposed to be a synchronous blocking
> function.
> 
> We therefore need to continue to perform a rudimentary CORS check in
> SyncLoadContext, which blocks cross-origin redirects if the
> FetchRequestMode was anything other than kNoCors. This revealed a couple
> of XML/XSLT callsites that should have been setting a FetchRequestMode but
> weren't.
> 
> Bug:  706331 
> 
> Change-Id: I4ed54cb3ccd119b8a5d192b077bef8d03745e67e
> Reviewed-on: https://chromium-review.googlesource.com/1015237
> Commit-Queue: Nate Chapin <japhet@chromium.org>
> Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#556546}

TBR=falken@chromium.org,horo@chromium.org,kinuko@chromium.org,yhirano@chromium.org,japhet@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  706331 
Change-Id: Id42378ac08f67861aac524ab618b782010970577
Reviewed-on: https://chromium-review.googlesource.com/1054108
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557568}
[modify] https://crrev.com/1b5126a68c8620c18c515d5aa1e7bc61394c99af/content/renderer/loader/sync_load_context.cc
[modify] https://crrev.com/1b5126a68c8620c18c515d5aa1e7bc61394c99af/content/renderer/loader/sync_load_context.h
[modify] https://crrev.com/1b5126a68c8620c18c515d5aa1e7bc61394c99af/content/renderer/loader/sync_load_response.h
[modify] https://crrev.com/1b5126a68c8620c18c515d5aa1e7bc61394c99af/content/renderer/loader/web_url_loader_impl.cc
[modify] https://crrev.com/1b5126a68c8620c18c515d5aa1e7bc61394c99af/content/renderer/loader/web_url_loader_impl.h
[modify] https://crrev.com/1b5126a68c8620c18c515d5aa1e7bc61394c99af/content/renderer/loader/web_url_loader_impl_unittest.cc
[modify] https://crrev.com/1b5126a68c8620c18c515d5aa1e7bc61394c99af/third_party/WebKit/LayoutTests/http/tests/appcache/fallback.html
[modify] https://crrev.com/1b5126a68c8620c18c515d5aa1e7bc61394c99af/third_party/blink/public/platform/web_url_loader.h
[modify] https://crrev.com/1b5126a68c8620c18c515d5aa1e7bc61394c99af/third_party/blink/renderer/core/xml/parser/xml_document_parser.cc
[modify] https://crrev.com/1b5126a68c8620c18c515d5aa1e7bc61394c99af/third_party/blink/renderer/core/xml/xslt_processor_libxslt.cc
[modify] https://crrev.com/1b5126a68c8620c18c515d5aa1e7bc61394c99af/third_party/blink/renderer/platform/loader/fetch/resource_loader.cc
[modify] https://crrev.com/1b5126a68c8620c18c515d5aa1e7bc61394c99af/third_party/blink/renderer/platform/testing/weburl_loader_mock.cc
[modify] https://crrev.com/1b5126a68c8620c18c515d5aa1e7bc61394c99af/third_party/blink/renderer/platform/testing/weburl_loader_mock.h

Comment 33 by horo@chromium.org, May 16 2018

Blockedon: 840348

Comment 34 by horo@chromium.org, May 16 2018 

There was a bug report that my change "5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c" which made ThreadableLoader use DocumentThreadableLoader for sync loading from worker thread had broken an existing site. ( issue 840348 )
I think this is because DocumentThreadableLoader::LoadResourceSynchronously() doesn't support CORS after redirect.

It is difficult to make DocumentThreadableLoader::LoadResourceSynchronously() support it.
So I created a CL to revive WorkerThreadableLoader for sync XHR.
https://chromium-review.googlesource.com/c/chromium/src/+/1060793

I think out-of-renderer-CORS project will make it easy.
So let's use out-of-renderer-CORS for sync xhr on worker when out-of-renderer-CORS will be ready.

Comment 35 by toyoshim@chromium.org, May 16 2018

Labels: OOR-CORS
lgtm, and sorry for the CORS plan being delayed.
Let me set OOR-CORS label to keep my eyes on this.
Project Member

Comment 36 by bugdroid1@chromium.org, May 16 2018 

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

commit 14b1861dc42443535a48b97c09126f1f59cd4088
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Wed May 16 09:44:48 2018

Revive WorkerThreadableLoader for sync XHR again

The cl "5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c" which made ThreadableLoader
use DocumentThreadableLoader for sync loading from worker thread had broken
existing site.  https://crbug.com/840348 

This is because DocumentThreadableLoader::LoadResourceSynchronously() doesn't
support CORS after redirect.

It is difficult to make DocumentThreadableLoader::LoadResourceSynchronously()
support it. So This CL revives WorkerThreadableLoader for sync XHR.

Bug:  840348 ,  706331 
Change-Id: Ib41fc442ddca508011431609cad1b6b6f9bda537
Reviewed-on: https://chromium-review.googlesource.com/1060793
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559023}
[modify] https://crrev.com/14b1861dc42443535a48b97c09126f1f59cd4088/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/workers/access-control-basic-get-fail-non-simple-expected.txt
[modify] https://crrev.com/14b1861dc42443535a48b97c09126f1f59cd4088/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/workers/cross-origin-unsupported-url-expected.txt
[add] https://crrev.com/14b1861dc42443535a48b97c09126f1f59cd4088/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/workers/resources/sync-cors-after-redirect-on-worker-worker.js
[add] https://crrev.com/14b1861dc42443535a48b97c09126f1f59cd4088/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/workers/sync-cors-after-redirect-on-worker.html
[modify] https://crrev.com/14b1861dc42443535a48b97c09126f1f59cd4088/third_party/blink/renderer/core/loader/threadable_loader.cc
Project Member

Comment 37 by bugdroid1@chromium.org, May 16 2018 

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

commit 4b00991ee27817b60b2a142261bd1d89ca253615
Author: Łukasz Anforowicz <lukasza@chromium.org>
Date: Wed May 16 15:11:09 2018

Revert "Revive WorkerThreadableLoader for sync XHR again"

This reverts commit 14b1861dc42443535a48b97c09126f1f59cd4088.

Reason for revert: This CL introduced a layout test that seems to be consistently failing on Win10 starting in https://ci.chromium.org/buildbot/chromium.webkit/WebKit%20Win10/35033 in http/tests/xmlhttprequest/workers/sync-cors-after-redirect-on-worker.html:

FAIL Synchronous CORS XHR after redirect on worker assert_equals: expected "PASS: Cross-domain access allowed.\n" but got "PASS: Cross-domain access allowed.\r\n"

Original change's description:
> Revive WorkerThreadableLoader for sync XHR again
> 
> The cl "5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c" which made ThreadableLoader
> use DocumentThreadableLoader for sync loading from worker thread had broken
> existing site.  https://crbug.com/840348 
> 
> This is because DocumentThreadableLoader::LoadResourceSynchronously() doesn't
> support CORS after redirect.
> 
> It is difficult to make DocumentThreadableLoader::LoadResourceSynchronously()
> support it. So This CL revives WorkerThreadableLoader for sync XHR.
> 
> Bug:  840348 ,  706331 
> Change-Id: Ib41fc442ddca508011431609cad1b6b6f9bda537
> Reviewed-on: https://chromium-review.googlesource.com/1060793
> Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#559023}

TBR=horo@chromium.org,kinuko@chromium.org

Change-Id: I1605a0b591d59737fa09951e6f8d5f3d907d52b4
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  840348 ,  706331 
Reviewed-on: https://chromium-review.googlesource.com/1061894
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559101}
[modify] https://crrev.com/4b00991ee27817b60b2a142261bd1d89ca253615/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/workers/access-control-basic-get-fail-non-simple-expected.txt
[modify] https://crrev.com/4b00991ee27817b60b2a142261bd1d89ca253615/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/workers/cross-origin-unsupported-url-expected.txt
[delete] https://crrev.com/f5f34321c9cc5fc976bb8dbb514ead9597ed9d73/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/workers/resources/sync-cors-after-redirect-on-worker-worker.js
[delete] https://crrev.com/f5f34321c9cc5fc976bb8dbb514ead9597ed9d73/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/workers/sync-cors-after-redirect-on-worker.html
[modify] https://crrev.com/4b00991ee27817b60b2a142261bd1d89ca253615/third_party/blink/renderer/core/loader/threadable_loader.cc
Project Member

Comment 38 by bugdroid1@chromium.org, May 16 2018 

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

commit b1e172c8d0e85286f9025f291c3fc3e719bc30ff
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Wed May 16 15:26:48 2018

Reland "Revive WorkerThreadableLoader for sync XHR again"

This reverts commit 4b00991ee27817b60b2a142261bd1d89ca253615.

Reason for revert: It is easy to fix the test failure.

Original change's description:
> Revert "Revive WorkerThreadableLoader for sync XHR again"
> 
> This reverts commit 14b1861dc42443535a48b97c09126f1f59cd4088.
> 
> Reason for revert: This CL introduced a layout test that seems to be consistently failing on Win10 starting in https://ci.chromium.org/buildbot/chromium.webkit/WebKit%20Win10/35033 in http/tests/xmlhttprequest/workers/sync-cors-after-redirect-on-worker.html:
> 
> FAIL Synchronous CORS XHR after redirect on worker assert_equals: expected "PASS: Cross-domain access allowed.\n" but got "PASS: Cross-domain access allowed.\r\n"
> 
> Original change's description:
> > Revive WorkerThreadableLoader for sync XHR again
> > 
> > The cl "5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c" which made ThreadableLoader
> > use DocumentThreadableLoader for sync loading from worker thread had broken
> > existing site.  https://crbug.com/840348 
> > 
> > This is because DocumentThreadableLoader::LoadResourceSynchronously() doesn't
> > support CORS after redirect.
> > 
> > It is difficult to make DocumentThreadableLoader::LoadResourceSynchronously()
> > support it. So This CL revives WorkerThreadableLoader for sync XHR.
> > 
> > Bug:  840348 ,  706331 
> > Change-Id: Ib41fc442ddca508011431609cad1b6b6f9bda537
> > Reviewed-on: https://chromium-review.googlesource.com/1060793
> > Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
> > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#559023}
> 
> TBR=horo@chromium.org,kinuko@chromium.org
> 
> Change-Id: I1605a0b591d59737fa09951e6f8d5f3d907d52b4
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug:  840348 ,  706331 
> Reviewed-on: https://chromium-review.googlesource.com/1061894
> Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
> Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#559101}

TBR=horo@chromium.org,kinuko@chromium.org,lukasza@chromium.org

Change-Id: I6d078b4124defec2f9d7514b95eed00ff6f685f5
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  840348 ,  706331 
Reviewed-on: https://chromium-review.googlesource.com/1061435
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559107}
[modify] https://crrev.com/b1e172c8d0e85286f9025f291c3fc3e719bc30ff/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/workers/access-control-basic-get-fail-non-simple-expected.txt
[modify] https://crrev.com/b1e172c8d0e85286f9025f291c3fc3e719bc30ff/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/workers/cross-origin-unsupported-url-expected.txt
[add] https://crrev.com/b1e172c8d0e85286f9025f291c3fc3e719bc30ff/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/workers/resources/sync-cors-after-redirect-on-worker-worker.js
[add] https://crrev.com/b1e172c8d0e85286f9025f291c3fc3e719bc30ff/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/workers/sync-cors-after-redirect-on-worker.html
[modify] https://crrev.com/b1e172c8d0e85286f9025f291c3fc3e719bc30ff/third_party/blink/renderer/core/loader/threadable_loader.cc

Comment 39 by horo@chromium.org, May 17 2018 

There are following dependencies now.

 [1] Move redirect handling from SyncLoadContext::OnReceivedRedirect() to ResourceDispatcher::StartSync().
     https://crrev.com/c/1053237
 [2] Have cloned ServiceWorkerscriptLoaderFactory in the service worker thread.
     https://crrev.com/c/1054168
 [3] Make ServiceWorkerSubresourceLoaderFactory on worker work with sync.
     ServiceWorkerSubresourceLoaderFactory lives on the worker which is controlled by a service worker when NetworkService is enabled.
 [4] Use DocumentThreadableLoader::LoadResourceSynchronously() from WorkerClassicScriptLoader::LoadSynchronously()
     This depends on [1], [2] and [3].
     This makes importScripts() off-main-thread.
 [5] Make ThreadableLoader::LoadResourceSynchronously() support CORS.
     There could be 2 plans:
       - Implement CORS logic in DocumentThreadableLoader::LoadResourceSynchronously()
       - Wait for out-of-renderer-CORS to be ready,
 [6] Use DocumentThreadableLoader::LoadResourceSynchronously() from ThreadableLoader::LoadResourceSynchronously()
     This depends on [3] and [5].
     This makes Sync XHR on worker off-main-thread.
 [7] Remove WorkerThreadableLoader.
     This depends on [4] and [6].
 [8] Remove the shadow page of workers.
     This depends on [7].
 [9] Make starting service worker off-main-thread.
     This depends on [8].
 [19] Nested worker support.
      Removing WorkerThreadableLoader [7] makes this easier.
      There may be a workaround without [7].

[1]
   ➘
[2]→[4]
   ➚  ➘
[3]    [7]→[8]→[9]
   ➘  ➚   ➘
[5]→[6]    [10]

Comment 40 by horo@chromium.org, May 17 2018 

s/[19]/[10]/

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

Blockedon: 843860

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

Filed a bug for [3].
Project Member

Comment 43 by bugdroid1@chromium.org, May 17 2018 

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

commit 4bd024a4e885ced5d79f19c4e950c7ffb9ee29b0
Author: Nate Chapin <japhet@chromium.org>
Date: Thu May 17 18:32:06 2018

Do partial redirect handling in the renderer process for sync requests (attempt #2)

https://chromium.googlesource.com/chromium/src.git/+/0817c1db4b405492ea24c6f9fda733480002a1ec
introduced some degree of blink redirect handling for sync requests.
It did so by cancelling and manually following redirects if blink
permitted. This causes observable side effects throughout the codebase,
and was reverted.

This CL instead defers the redirect until blink has had a chance to
process it. The flow is now:
* Redirect is received by SyncLoadContext on its helper thread.
  Redirect is approved but deferred, and ResourceDispatcher on the
  blocking thread is signaled for "completion".
* ResourceDispatcher unblocks and checks whether a redirect was
  received. If there was a redirect, it notifies its RequestPeer,
  which calls in to blink and checks the redirect. ResourceDispatcher
  then resets its WaitableEvent and instructs SyncLoadContext to
  either follow or cancel the redirect.
* Repeat previous step until all redirects have been processed. The
  sync request then proceeds as normal, only unblocking the
  ResourceDispatcher when the request is entirely done.

Bug:  706331 
Change-Id: I9f5d0fdcdcc0e022dbc8a3be8fe1dd0f41443f4a
Reviewed-on: https://chromium-review.googlesource.com/1053237
Commit-Queue: Nate Chapin <japhet@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559617}
[modify] https://crrev.com/4bd024a4e885ced5d79f19c4e950c7ffb9ee29b0/content/renderer/loader/resource_dispatcher.cc
[modify] https://crrev.com/4bd024a4e885ced5d79f19c4e950c7ffb9ee29b0/content/renderer/loader/resource_dispatcher.h
[modify] https://crrev.com/4bd024a4e885ced5d79f19c4e950c7ffb9ee29b0/content/renderer/loader/sync_load_context.cc
[modify] https://crrev.com/4bd024a4e885ced5d79f19c4e950c7ffb9ee29b0/content/renderer/loader/sync_load_context.h
[modify] https://crrev.com/4bd024a4e885ced5d79f19c4e950c7ffb9ee29b0/content/renderer/loader/sync_load_response.h
[modify] https://crrev.com/4bd024a4e885ced5d79f19c4e950c7ffb9ee29b0/content/renderer/loader/web_url_loader_impl.cc
[modify] https://crrev.com/4bd024a4e885ced5d79f19c4e950c7ffb9ee29b0/content/renderer/loader/web_url_loader_impl.h
[modify] https://crrev.com/4bd024a4e885ced5d79f19c4e950c7ffb9ee29b0/content/renderer/loader/web_url_loader_impl_unittest.cc
[modify] https://crrev.com/4bd024a4e885ced5d79f19c4e950c7ffb9ee29b0/third_party/blink/public/platform/web_url_loader.h
[modify] https://crrev.com/4bd024a4e885ced5d79f19c4e950c7ffb9ee29b0/third_party/blink/renderer/core/xml/parser/xml_document_parser.cc
[modify] https://crrev.com/4bd024a4e885ced5d79f19c4e950c7ffb9ee29b0/third_party/blink/renderer/core/xml/xslt_processor_libxslt.cc
[modify] https://crrev.com/4bd024a4e885ced5d79f19c4e950c7ffb9ee29b0/third_party/blink/renderer/platform/loader/fetch/resource_loader.cc
[modify] https://crrev.com/4bd024a4e885ced5d79f19c4e950c7ffb9ee29b0/third_party/blink/renderer/platform/testing/weburl_loader_mock.cc
[modify] https://crrev.com/4bd024a4e885ced5d79f19c4e950c7ffb9ee29b0/third_party/blink/renderer/platform/testing/weburl_loader_mock.h

Comment 44 by japhet@chromium.org, May 17 2018 

Re: comment #39, I've posted https://chromium-review.googlesource.com/c/chromium/src/+/1064796 to try to ensure this issue doesn't block nested worker support.
Project Member

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

Labels: merge-merged-3396
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6375357dec9578dc907cacc22824c0923754adbf

commit 6375357dec9578dc907cacc22824c0923754adbf
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Fri May 18 00:42:09 2018

[Merge to M67] Reland "Revive WorkerThreadableLoader for sync XHR again"

TBR=kinuko@chromium.org

This reverts commit 4b00991ee27817b60b2a142261bd1d89ca253615.

Reason for revert: It is easy to fix the test failure.

Original change's description:
> Revert "Revive WorkerThreadableLoader for sync XHR again"
>
> This reverts commit 14b1861dc42443535a48b97c09126f1f59cd4088.
>
> Reason for revert: This CL introduced a layout test that seems to be consistently failing on Win10 starting in https://ci.chromium.org/buildbot/chromium.webkit/WebKit%20Win10/35033 in http/tests/xmlhttprequest/workers/sync-cors-after-redirect-on-worker.html:
>
> FAIL Synchronous CORS XHR after redirect on worker assert_equals: expected "PASS: Cross-domain access allowed.\n" but got "PASS: Cross-domain access allowed.\r\n"
>
> Original change's description:
> > Revive WorkerThreadableLoader for sync XHR again
> >
> > The cl "5e1b52dd7e828fb2e4bd69f599c0c30eec3e873c" which made ThreadableLoader
> > use DocumentThreadableLoader for sync loading from worker thread had broken
> > existing site.  https://crbug.com/840348 
> >
> > This is because DocumentThreadableLoader::LoadResourceSynchronously() doesn't
> > support CORS after redirect.
> >
> > It is difficult to make DocumentThreadableLoader::LoadResourceSynchronously()
> > support it. So This CL revives WorkerThreadableLoader for sync XHR.
> >
> > Bug:  840348 ,  706331 
> > Change-Id: Ib41fc442ddca508011431609cad1b6b6f9bda537
> > Reviewed-on: https://chromium-review.googlesource.com/1060793
> > Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
> > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#559023}
>
> TBR=horo@chromium.org,kinuko@chromium.org
>
> Change-Id: I1605a0b591d59737fa09951e6f8d5f3d907d52b4
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug:  840348 ,  706331 
> Reviewed-on: https://chromium-review.googlesource.com/1061894
> Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
> Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#559101}

TBR=horo@chromium.org,kinuko@chromium.org,lukasza@chromium.org

(cherry picked from commit b1e172c8d0e85286f9025f291c3fc3e719bc30ff)

Change-Id: I6d078b4124defec2f9d7514b95eed00ff6f685f5
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  840348 ,  706331 
Reviewed-on: https://chromium-review.googlesource.com/1061435
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#559107}
Reviewed-on: https://chromium-review.googlesource.com/1064652
Cr-Commit-Position: refs/branch-heads/3396@{#633}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/6375357dec9578dc907cacc22824c0923754adbf/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/workers/access-control-basic-get-fail-non-simple-expected.txt
[modify] https://crrev.com/6375357dec9578dc907cacc22824c0923754adbf/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/workers/cross-origin-unsupported-url-expected.txt
[add] https://crrev.com/6375357dec9578dc907cacc22824c0923754adbf/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/workers/resources/sync-cors-after-redirect-on-worker-worker.js
[add] https://crrev.com/6375357dec9578dc907cacc22824c0923754adbf/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/workers/sync-cors-after-redirect-on-worker.html
[modify] https://crrev.com/6375357dec9578dc907cacc22824c0923754adbf/third_party/blink/renderer/core/loader/threadable_loader.cc
Project Member

Comment 46 by bugdroid1@chromium.org, May 29 2018 

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

commit b5c5c86c21a0bf51cc910e0b62611aea45aa0b0d
Author: Nate Chapin <japhet@chromium.org>
Date: Tue May 29 17:47:40 2018

CORS for sync redirects

Historically, synchronous fetches have had no associated ResourceClient.
This made no difference, because synchronous fetches blocked until the
fetch completed, so there would be no callbacks to listen to until the
function was ready to return anyway.

However, now we perform the synchronous fetch on a utility thread, and
https://chromium.googlesource.com/chromium/src.git/+/4bd024a4e885ced5d79f19c4e950c7ffb9ee29b0
allows us to unblock and re-block the requesting thread in order to
perform redirect checks. Before this CL, the redirect handling can only
process checks that don't require access to the ResourceClient (like CSP).
This CL allows DocumentThreadableLoader to register itself as a
ResourceClient on a sync fetch, so that it can perform CORS checks
on sync requests.

Because DocumentThreadableLoader now gets ResourceClient callbacks
for sync requests, DocumentThreadableLoader::LoadRequestSync is
substantially simpler.

Bug:  706331 
Change-Id: I3031b8068b3501f7f8f7bf19e4ab46ad7b59e83a
Reviewed-on: https://chromium-review.googlesource.com/1067745
Commit-Queue: Nate Chapin <japhet@chromium.org>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562496}
[modify] https://crrev.com/b5c5c86c21a0bf51cc910e0b62611aea45aa0b0d/content/renderer/loader/sync_load_context.cc
[modify] https://crrev.com/b5c5c86c21a0bf51cc910e0b62611aea45aa0b0d/content/renderer/loader/sync_load_context.h
[delete] https://crrev.com/66d69d2841126c91f2bcf9660163e450fe217109/third_party/WebKit/LayoutTests/external/wpt/xhr/access-control-and-redirects-expected.txt
[modify] https://crrev.com/b5c5c86c21a0bf51cc910e0b62611aea45aa0b0d/third_party/WebKit/LayoutTests/fast/xmlhttprequest/xmlhttprequest-recursive-sync-event-expected.txt
[modify] https://crrev.com/b5c5c86c21a0bf51cc910e0b62611aea45aa0b0d/third_party/WebKit/LayoutTests/fast/xmlhttprequest/xmlhttprequest-recursive-sync-event.html
[modify] https://crrev.com/b5c5c86c21a0bf51cc910e0b62611aea45aa0b0d/third_party/WebKit/LayoutTests/http/tests/security/xss-DENIED-xsl-external-entity-redirect-expected.txt
[modify] https://crrev.com/b5c5c86c21a0bf51cc910e0b62611aea45aa0b0d/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/redirect-cross-origin-post-sync-expected.txt
[modify] https://crrev.com/b5c5c86c21a0bf51cc910e0b62611aea45aa0b0d/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/redirect-cross-origin-sync-double-expected.txt
[modify] https://crrev.com/b5c5c86c21a0bf51cc910e0b62611aea45aa0b0d/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/redirect-cross-origin-sync-expected.txt
[modify] https://crrev.com/b5c5c86c21a0bf51cc910e0b62611aea45aa0b0d/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/redirect-cross-origin-tripmine-expected.txt
[modify] https://crrev.com/b5c5c86c21a0bf51cc910e0b62611aea45aa0b0d/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-unsafe-redirect-expected.txt
[modify] https://crrev.com/b5c5c86c21a0bf51cc910e0b62611aea45aa0b0d/third_party/blink/renderer/core/loader/document_threadable_loader.cc
[modify] https://crrev.com/b5c5c86c21a0bf51cc910e0b62611aea45aa0b0d/third_party/blink/renderer/platform/loader/fetch/raw_resource.cc
[modify] https://crrev.com/b5c5c86c21a0bf51cc910e0b62611aea45aa0b0d/third_party/blink/renderer/platform/loader/fetch/raw_resource.h
[modify] https://crrev.com/b5c5c86c21a0bf51cc910e0b62611aea45aa0b0d/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc
[modify] https://crrev.com/b5c5c86c21a0bf51cc910e0b62611aea45aa0b0d/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.h
[modify] https://crrev.com/b5c5c86c21a0bf51cc910e0b62611aea45aa0b0d/third_party/blink/renderer/platform/loader/fetch/resource_loader.cc
Project Member

Comment 48 by bugdroid1@chromium.org, May 31 2018 

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

commit e7832567df269a5025a6dd4d87c7e6878e05fd72
Author: Nate Chapin <japhet@chromium.org>
Date: Thu May 31 19:56:07 2018

Plumb ServiceWorkerScriptLoaderFactory to SW thread

This allows importScripts() to work off-main-thread in a service
worker with NetworkService enabled.

TBR=kinuko@chromium.org

Bug:  706331 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I34810127e168be91d857a1c1dfbfcfbd6891da1d
Reviewed-on: https://chromium-review.googlesource.com/1054168
Commit-Queue: Nate Chapin <japhet@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563357}
[modify] https://crrev.com/e7832567df269a5025a6dd4d87c7e6878e05fd72/content/browser/service_worker/service_worker_script_loader_factory.cc
[modify] https://crrev.com/e7832567df269a5025a6dd4d87c7e6878e05fd72/content/browser/service_worker/service_worker_script_loader_factory.h
[modify] https://crrev.com/e7832567df269a5025a6dd4d87c7e6878e05fd72/content/renderer/service_worker/service_worker_context_client.cc
[modify] https://crrev.com/e7832567df269a5025a6dd4d87c7e6878e05fd72/content/renderer/service_worker/service_worker_context_client.h
[modify] https://crrev.com/e7832567df269a5025a6dd4d87c7e6878e05fd72/content/renderer/service_worker/service_worker_fetch_context_impl.cc
[modify] https://crrev.com/e7832567df269a5025a6dd4d87c7e6878e05fd72/content/renderer/service_worker/service_worker_fetch_context_impl.h
[modify] https://crrev.com/e7832567df269a5025a6dd4d87c7e6878e05fd72/third_party/blink/public/platform/web_worker_fetch_context.h
[modify] https://crrev.com/e7832567df269a5025a6dd4d87c7e6878e05fd72/third_party/blink/public/web/modules/serviceworker/web_service_worker_context_client.h
[modify] https://crrev.com/e7832567df269a5025a6dd4d87c7e6878e05fd72/third_party/blink/renderer/core/loader/worker_fetch_context.cc
[modify] https://crrev.com/e7832567df269a5025a6dd4d87c7e6878e05fd72/third_party/blink/renderer/core/loader/worker_fetch_context.h
[modify] https://crrev.com/e7832567df269a5025a6dd4d87c7e6878e05fd72/third_party/blink/renderer/modules/exported/web_embedded_worker_impl.cc
Project Member

Comment 49 by bugdroid1@chromium.org, May 31 2018 

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

commit 22dfc3b1b8b1da9d5fb11b26574ff013d67fd583
Author: Nate Chapin <japhet@chromium.org>
Date: Thu May 31 22:43:50 2018

Use off-main-thread loading for WorkerClassicScriptLoader::LoadSynchronously

This removes the last usage of WorkerThreadableLoader. I'll delete it in a
followup.

Bug:  706331 
Change-Id: Idbeea89cb10c80d1ade1b2f5b6acefe89172a96c
Reviewed-on: https://chromium-review.googlesource.com/1081348
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Commit-Queue: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563410}
[modify] https://crrev.com/22dfc3b1b8b1da9d5fb11b26574ff013d67fd583/third_party/blink/renderer/core/workers/worker_classic_script_loader.cc
Project Member

Comment 50 by bugdroid1@chromium.org, Jun 4 2018 

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

commit d64c69b12930bbefa9b1cce65c3d81883deb2b68
Author: Nate Chapin <japhet@chromium.org>
Date: Mon Jun 04 21:01:28 2018

Register ResourceClients with a Resource before starting fetch

Historically, the ordering for async fetch start was:
* Create Resource if we don't have one cached.
* If not reusing a cached Resource, create ResourceLoader,
  associate it with Resource, start fetch.
* Register ResourceClient with Resource, triggering cache hit logic
  if using a cached Resource or the fetch failed during start.

Until https://chromium-review.googlesource.com/c/chromium/src/+/1067745,
sync fetches had no ResourceClient at all. That change registered
a ResourceClient for certain sync requests, but for the the
ResourceClient to be able to inspect callbacks (esp. redirects), the
ResourceClient needed to be registered before starting the fetch.

This CL changes the order, whether sync or async to:
* Create Resource if we don't have one cached.
* Register ResourceClient with Resource, triggering cache hit logic
  if using a cached Resource.
* If not reusing a cached Resource, create ResourceLoader,
  associate it with Resource, start fetch. If the request fails
  during start, post a task to notify the ResourceClient if the
  ResourceClient does not expect synchronous success/failure.

Bug:  706331 
Change-Id: I1294fc9342f0a60a03b11a99c75f490283600eb9
Reviewed-on: https://chromium-review.googlesource.com/1077087
Commit-Queue: Nate Chapin <japhet@chromium.org>
Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564233}
[modify] https://crrev.com/d64c69b12930bbefa9b1cce65c3d81883deb2b68/third_party/blink/renderer/core/loader/image_loader.cc
[modify] https://crrev.com/d64c69b12930bbefa9b1cce65c3d81883deb2b68/third_party/blink/renderer/core/loader/resource/image_resource.cc
[modify] https://crrev.com/d64c69b12930bbefa9b1cce65c3d81883deb2b68/third_party/blink/renderer/core/loader/resource/image_resource_content.cc
[modify] https://crrev.com/d64c69b12930bbefa9b1cce65c3d81883deb2b68/third_party/blink/renderer/core/loader/resource/image_resource_test.cc
[modify] https://crrev.com/d64c69b12930bbefa9b1cce65c3d81883deb2b68/third_party/blink/renderer/platform/loader/fetch/resource.cc
[modify] https://crrev.com/d64c69b12930bbefa9b1cce65c3d81883deb2b68/third_party/blink/renderer/platform/loader/fetch/resource.h
[modify] https://crrev.com/d64c69b12930bbefa9b1cce65c3d81883deb2b68/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc

Comment 51 by nhiroki@chromium.org, Jul 25

Owner: japhet@chromium.org
Updated the dependency graph on c#39 (thank you for making the nice graph, horo@!).

We finished [1-6, 10]. CL for [7] is under review:
https://chromium-review.googlesource.com/c/chromium/src/+/1045522

 [1] Move redirect handling from SyncLoadContext::OnReceivedRedirect() to ResourceDispatcher::StartSync().
 [2] Have cloned ServiceWorkerscriptLoaderFactory in the service worker thread.
 [3] Make ServiceWorkerSubresourceLoaderFactory on worker work with sync.
 [4] Use DocumentThreadableLoader::LoadResourceSynchronously() from WorkerClassicScriptLoader::LoadSynchronously()
 [5] Make ThreadableLoader::LoadResourceSynchronously() support CORS.
 [6] Use DocumentThreadableLoader::LoadResourceSynchronously() from ThreadableLoader::LoadResourceSynchronously()
 [7] Remove WorkerThreadableLoader.
 [10] Nested worker support.

Remaining tasks are [8-9]. I think we could break them down into new service worker case and installed service worker case. Since this issue is mainly about [1-7], I'll make separate doc and issues for the remaining tasks. 

 [8] Remove the shadow page of workers.
     This depends on [7].
 [9] Make starting service worker off-main-thread.
     This depends on [8].

[1]
   ➘
[2]→[4]
   ➚  ➘
[3]    [7]→[8]→[9]
   ➘  ➚   ➘
[5]→[6]    [10]

Comment 52 by falken@chromium.org, Jul 25 

Thanks! It sounds like [8-9] is not quite about supporting OMT fetch from workers.

So we can mark this as fixed (I guess fixed ever since c#50?), and open new bugs for 8 and 9?
Project Member

Comment 53 by bugdroid1@chromium.org, Jul 25 

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

commit 2d43ed4c8d17b477714a42226aa9a80a39d38d3c
Author: Nate Chapin <japhet@chromium.org>
Date: Wed Jul 25 16:06:00 2018

Delete WorkerThreadableLoader, it's unused

Flatten DocumentThreadableLoader into the base class, ThreadableLoader.
Same for DocumentThreadbleLoaderClient and DocumentThreadableLoaderTest.

Bug:  706331 
Change-Id: Ib15340f83bc38e6c0427d829cb0490f7b3ecd2c5
Reviewed-on: https://chromium-review.googlesource.com/1045522
Commit-Queue: Nate Chapin <japhet@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577914}
[modify] https://crrev.com/2d43ed4c8d17b477714a42226aa9a80a39d38d3c/third_party/blink/renderer/core/BUILD.gn
[modify] https://crrev.com/2d43ed4c8d17b477714a42226aa9a80a39d38d3c/third_party/blink/renderer/core/exported/web_associated_url_loader_impl.cc
[modify] https://crrev.com/2d43ed4c8d17b477714a42226aa9a80a39d38d3c/third_party/blink/renderer/core/exported/web_associated_url_loader_impl.h
[modify] https://crrev.com/2d43ed4c8d17b477714a42226aa9a80a39d38d3c/third_party/blink/renderer/core/exported/web_frame_test.cc
[modify] https://crrev.com/2d43ed4c8d17b477714a42226aa9a80a39d38d3c/third_party/blink/renderer/core/fetch/fetch_manager.cc
[modify] https://crrev.com/2d43ed4c8d17b477714a42226aa9a80a39d38d3c/third_party/blink/renderer/core/loader/BUILD.gn
[delete] https://crrev.com/31d4e6b70d4c2271e2e755c2eed49bfda9f04127/third_party/blink/renderer/core/loader/document_threadable_loader.cc
[delete] https://crrev.com/31d4e6b70d4c2271e2e755c2eed49bfda9f04127/third_party/blink/renderer/core/loader/document_threadable_loader.h
[delete] https://crrev.com/31d4e6b70d4c2271e2e755c2eed49bfda9f04127/third_party/blink/renderer/core/loader/document_threadable_loader_client.h
[delete] https://crrev.com/31d4e6b70d4c2271e2e755c2eed49bfda9f04127/third_party/blink/renderer/core/loader/document_threadable_loader_test.cc
[modify] https://crrev.com/2d43ed4c8d17b477714a42226aa9a80a39d38d3c/third_party/blink/renderer/core/loader/threadable_loader.cc
[modify] https://crrev.com/2d43ed4c8d17b477714a42226aa9a80a39d38d3c/third_party/blink/renderer/core/loader/threadable_loader.h
[modify] https://crrev.com/2d43ed4c8d17b477714a42226aa9a80a39d38d3c/third_party/blink/renderer/core/loader/threadable_loader_client.h
[modify] https://crrev.com/2d43ed4c8d17b477714a42226aa9a80a39d38d3c/third_party/blink/renderer/core/loader/threadable_loader_test.cc
[delete] https://crrev.com/31d4e6b70d4c2271e2e755c2eed49bfda9f04127/third_party/blink/renderer/core/loader/worker_threadable_loader.cc
[delete] https://crrev.com/31d4e6b70d4c2271e2e755c2eed49bfda9f04127/third_party/blink/renderer/core/loader/worker_threadable_loader.h
[modify] https://crrev.com/2d43ed4c8d17b477714a42226aa9a80a39d38d3c/third_party/blink/renderer/core/testing/dummy_page_holder.h
[modify] https://crrev.com/2d43ed4c8d17b477714a42226aa9a80a39d38d3c/third_party/blink/renderer/core/workers/worker_classic_script_loader.cc
[modify] https://crrev.com/2d43ed4c8d17b477714a42226aa9a80a39d38d3c/third_party/blink/renderer/core/workers/worker_global_scope.cc
[modify] https://crrev.com/2d43ed4c8d17b477714a42226aa9a80a39d38d3c/third_party/blink/renderer/platform/loader/fetch/raw_resource.cc
[modify] https://crrev.com/2d43ed4c8d17b477714a42226aa9a80a39d38d3c/third_party/blink/renderer/platform/loader/fetch/resource.cc
[modify] https://crrev.com/2d43ed4c8d17b477714a42226aa9a80a39d38d3c/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc
[modify] https://crrev.com/2d43ed4c8d17b477714a42226aa9a80a39d38d3c/third_party/blink/renderer/platform/loader/fetch/resource_loader_options.h

Comment 54 by japhet@chromium.org, Jul 25

Status: Fixed (was: Started)
Now that [7] has landed, yes, this is done. :)
►

Sign in to add a comment