New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 764224 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 766476

Blocking:
issue 715640



Sign in to add a comment

S13nServiceWorker: Make redirects work

Project Member Reported by kinuko@chromium.org, Sep 12 2017

Issue description

There could be two different places to make it work:

1. Redirect for main resource navigation: this needs to be handled in NavigationURLLoaderNetworkService plus each loaders and request handlers. Currently MichealN is working on the change that plumbs the basic skeleton and make it work with AppCache.  (https://chromium-review.googlesource.com/c/chromium/src/+/654143 is the patch, the CL probably gives you some ideas about how this needs to be handled)

In this case it's possible that a redirect coming from, say, URLLoader in Network Service, may need to be handled by different URLLoader like Service Worker.

2. Redirect for sub-resource loading: this needs to be handled in ServiceWorkerSubresourceLoader and the code around it.  In this case we don't need to think about handling redirects that come from Network Service, but Service Worker may return a redirect response and we may need to start over the response handling. (For reference, AppCache's corresponding patch is: https://chromium-review.googlesource.com/c/chromium/src/+/633922)

In both cases the very basic handling would be something like: store redirected request info in OnReceiveRedirect(), and then perform the actual redirect handling in FollowRedirect().  I don't have a clear idea about how much code / logic can be shared between main-resource and sub-resource cases yet.
 

Comment 1 by kinuko@chromium.org, Sep 12 2017

Summary: S13nServiceWorker: Make redirects work (was: S13nServiceWorker: Make redirect works)

Comment 2 by kinuko@chromium.org, Sep 12 2017

Components: Blink>ServiceWorker Internals>Network>Service

Comment 3 by kinuko@chromium.org, Sep 12 2017

Labels: Proj-Servicification

Comment 4 by kinuko@chromium.org, Sep 13 2017

(Fyi, Michael's appcache patch is landed)

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

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

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

Status: Started (was: Assigned)
Created a cl for main resource navigation: https://chromium-review.googlesource.com/c/chromium/src/+/668265

Comment 7 by horo@chromium.org, Sep 19 2017

Blockedon: 766476
Project Member

Comment 8 by bugdroid1@chromium.org, Sep 21 2017

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

commit 5204c87e3a6178b2898f254c2404930a9638b7c5
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Thu Sep 21 16:28:17 2017

Support redirect for main resource navigation with S13nServiceWorker.

I locally tested these redirect patterns in WPT's navigation-redirect.https.html
with --enable-features=NetworkService flag, after registering the SW without the
flag.

  - Normal redirect to same-origin scope.
  - Normal redirect to other-origin scope.
  - SW-fallbacked redirect to same-origin out-scope.
  - SW-fallbacked redirect to same-origin same-scope.
  - SW-fallbacked redirect to same-origin other-scope.
  - SW-fallbacked redirect to other-origin out-scope.
  - SW-fallbacked redirect to other-origin in-scope.
  - SW-generated redirect to same-origin out-scope.
  - SW-generated redirect to same-origin same-scope.
  - SW-generated redirect to same-origin other-scope.
  - SW-generated redirect to other-origin out-scope.
  - SW-generated redirect to other-origin in-scope.

But other patters in navigation-redirect.https.html failed. It is because there
is another issue in handling manual redirect mode requests in NetworkService.

BUG= 764224 

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: I1f4b602e6dd4887aabac770fa2b9d4a6dfb3aeb8
Reviewed-on: https://chromium-review.googlesource.com/668265
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda (slow) <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503467}
[modify] https://crrev.com/5204c87e3a6178b2898f254c2404930a9638b7c5/content/browser/loader/navigation_url_loader_network_service.cc
[modify] https://crrev.com/5204c87e3a6178b2898f254c2404930a9638b7c5/content/browser/service_worker/service_worker_url_loader_job.cc
[modify] https://crrev.com/5204c87e3a6178b2898f254c2404930a9638b7c5/content/browser/service_worker/service_worker_url_loader_job_unittest.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Sep 28 2017

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

commit ec9880b061e7fa0b1ee19545110853997e435299
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Thu Sep 28 22:25:21 2017

Share the ComputeRedirectInfo logic with URLRequestJob and ServiceWorkerURLLoaderJob.

Bug:  764224 
Change-Id: I7ea1dffdbde10ba03880c6ea88cfd721f30af57d
Reviewed-on: https://chromium-review.googlesource.com/677902
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505192}
[modify] https://crrev.com/ec9880b061e7fa0b1ee19545110853997e435299/content/browser/service_worker/service_worker_url_loader_job.cc
[modify] https://crrev.com/ec9880b061e7fa0b1ee19545110853997e435299/content/public/common/referrer.cc
[modify] https://crrev.com/ec9880b061e7fa0b1ee19545110853997e435299/content/public/common/referrer.h
[modify] https://crrev.com/ec9880b061e7fa0b1ee19545110853997e435299/net/BUILD.gn
[modify] https://crrev.com/ec9880b061e7fa0b1ee19545110853997e435299/net/url_request/redirect_info.cc
[modify] https://crrev.com/ec9880b061e7fa0b1ee19545110853997e435299/net/url_request/redirect_info.h
[add] https://crrev.com/ec9880b061e7fa0b1ee19545110853997e435299/net/url_request/redirect_info_unittest.cc
[modify] https://crrev.com/ec9880b061e7fa0b1ee19545110853997e435299/net/url_request/url_request_job.cc
[modify] https://crrev.com/ec9880b061e7fa0b1ee19545110853997e435299/net/url_request/url_request_job.h
[modify] https://crrev.com/ec9880b061e7fa0b1ee19545110853997e435299/net/url_request/url_request_job_unittest.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Oct 3 2017

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

commit 8e66ad255c0fd51ef0c7bdc713efdd9378c1d921
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Tue Oct 03 11:01:30 2017

Add MIME sniffing for SW test and make redirect tests not to rely on MIME sniffing

Bug:  771118 ,  764224 
Change-Id: Ia7a942644736c21412535a9d01bb183794c202cb
Reviewed-on: https://chromium-review.googlesource.com/697484
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506004}
[modify] https://crrev.com/8e66ad255c0fd51ef0c7bdc713efdd9378c1d921/third_party/WebKit/LayoutTests/FlagExpectations/enable-features=NetworkService
[add] https://crrev.com/8e66ad255c0fd51ef0c7bdc713efdd9378c1d921/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/mime-sniffing.https.html
[add] https://crrev.com/8e66ad255c0fd51ef0c7bdc713efdd9378c1d921/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/mime-sniffing-worker.js
[modify] https://crrev.com/8e66ad255c0fd51ef0c7bdc713efdd9378c1d921/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/navigation-redirect-out-scope.py
[modify] https://crrev.com/8e66ad255c0fd51ef0c7bdc713efdd9378c1d921/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/navigation-redirect-scope1.py
[modify] https://crrev.com/8e66ad255c0fd51ef0c7bdc713efdd9378c1d921/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/navigation-redirect-scope2.py

Project Member

Comment 11 by bugdroid1@chromium.org, Oct 3 2017

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

commit 868d1a6b0a1cb29227644988725c83d0fce78679
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Tue Oct 03 17:33:29 2017

S13nServiceWorker: Fix navigation redirect related tests

Fix the failure of navigation-redirect.https.html and navigation-redirect.https.html by
setting request/credentials/redirect mode in NavigationURLLoaderNetworkService.

Bug:  764224 
Change-Id: I0b061c26e3bd0a5d7de9afa8d813439d977212bb
Reviewed-on: https://chromium-review.googlesource.com/697086
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506088}
[modify] https://crrev.com/868d1a6b0a1cb29227644988725c83d0fce78679/content/browser/loader/navigation_url_loader_network_service.cc
[modify] https://crrev.com/868d1a6b0a1cb29227644988725c83d0fce78679/content/browser/service_worker/service_worker_url_loader_job.cc
[modify] https://crrev.com/868d1a6b0a1cb29227644988725c83d0fce78679/content/browser/service_worker/service_worker_url_loader_job_unittest.cc
[modify] https://crrev.com/868d1a6b0a1cb29227644988725c83d0fce78679/third_party/WebKit/LayoutTests/FlagExpectations/enable-features=NetworkService

Project Member

Comment 12 by bugdroid1@chromium.org, Oct 4 2017

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

commit 5eabfa378e8657b0f54ceeca48626f72c8d3b1ca
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Wed Oct 04 01:05:20 2017

Update FlagExpectations of NetworkService

https://chromium-review.googlesource.com/697086 changed the result
of navigation-preload/redirect.https.html from Crash to Failure.

Bug:  764224 
Change-Id: Icf8990adf28ead713b6b38684174825a549c63b0
Reviewed-on: https://chromium-review.googlesource.com/699251
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506261}
[modify] https://crrev.com/5eabfa378e8657b0f54ceeca48626f72c8d3b1ca/third_party/WebKit/LayoutTests/FlagExpectations/enable-features=NetworkService

Project Member

Comment 13 by bugdroid1@chromium.org, Oct 11 2017

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

commit f45d5c38068ce3aa28a032ba241046dfc0e9ff55
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Wed Oct 11 05:19:00 2017

Make navigation-preload/redirect.https.html not to rely on MIME sniffing

Bug:  771118 ,  764224 
Change-Id: I11aca4ce5675a633cbc722f0c362c0dc4df0b2da
Reviewed-on: https://chromium-review.googlesource.com/711555
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507900}
[modify] https://crrev.com/f45d5c38068ce3aa28a032ba241046dfc0e9ff55/third_party/WebKit/LayoutTests/FlagExpectations/enable-features=NetworkService
[modify] https://crrev.com/f45d5c38068ce3aa28a032ba241046dfc0e9ff55/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigation-preload/resources/redirect-scope.py

Project Member

Comment 14 by bugdroid1@chromium.org, Oct 13 2017

Project Member

Comment 15 by bugdroid1@chromium.org, Oct 16 2017

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

commit 9e2ec4dfde8f3c4c3075a1aac6f66455dfb9ef6d
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Mon Oct 16 15:15:55 2017

S13nServiceWorker: Support redirect in ServiceWorkerSubresourceLoader

This CL supports the SW generated redirect responses by implementing
ServiceWorkerSubresourceLoader::FollowRedirect().

This CL factor out the logic of updating HTTP request for redirection as
net::RedirectUtil::UpdateHttpRequest(). This method is used from
URLRequest::Redirect(), URLLoaderRequestController::FollowRedirect() and
ServiceWorkerSubresourceLoader::FollowRedirect().

Bug:  764224 
Change-Id: Idce34ca122c6a58dd1f20d49e739b839a215fae9
Reviewed-on: https://chromium-review.googlesource.com/695241
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509048}
[modify] https://crrev.com/9e2ec4dfde8f3c4c3075a1aac6f66455dfb9ef6d/content/browser/loader/navigation_url_loader_network_service.cc
[modify] https://crrev.com/9e2ec4dfde8f3c4c3075a1aac6f66455dfb9ef6d/content/browser/service_worker/service_worker_url_loader_job.cc
[modify] https://crrev.com/9e2ec4dfde8f3c4c3075a1aac6f66455dfb9ef6d/content/child/service_worker/service_worker_subresource_loader.cc
[modify] https://crrev.com/9e2ec4dfde8f3c4c3075a1aac6f66455dfb9ef6d/content/child/service_worker/service_worker_subresource_loader.h
[modify] https://crrev.com/9e2ec4dfde8f3c4c3075a1aac6f66455dfb9ef6d/content/child/service_worker/service_worker_subresource_loader_unittest.cc
[modify] https://crrev.com/9e2ec4dfde8f3c4c3075a1aac6f66455dfb9ef6d/content/common/service_worker/service_worker_loader_helpers.cc
[modify] https://crrev.com/9e2ec4dfde8f3c4c3075a1aac6f66455dfb9ef6d/content/common/service_worker/service_worker_loader_helpers.h
[modify] https://crrev.com/9e2ec4dfde8f3c4c3075a1aac6f66455dfb9ef6d/net/BUILD.gn
[add] https://crrev.com/9e2ec4dfde8f3c4c3075a1aac6f66455dfb9ef6d/net/url_request/redirect_util.cc
[add] https://crrev.com/9e2ec4dfde8f3c4c3075a1aac6f66455dfb9ef6d/net/url_request/redirect_util.h
[add] https://crrev.com/9e2ec4dfde8f3c4c3075a1aac6f66455dfb9ef6d/net/url_request/redirect_util_unittest.cc
[modify] https://crrev.com/9e2ec4dfde8f3c4c3075a1aac6f66455dfb9ef6d/net/url_request/url_request.cc
[modify] https://crrev.com/9e2ec4dfde8f3c4c3075a1aac6f66455dfb9ef6d/third_party/WebKit/LayoutTests/FlagExpectations/enable-features=NetworkService

Comment 16 by horo@chromium.org, Oct 17 2017

Status: Fixed (was: Started)
Components: -Internals>Network>Service Internals>Services>Network
Apologies, applied the wrong component in bulk.

Sign in to add a comment