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

Issue 691503 link

Starred by 7 users

Issue metadata

Status: Verified
Owner:
Closed: Apr 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

FetchEvent.preloadResponse must resolve with the redirect response if the server responded with a redirect.

Project Member Reported by horo@chromium.org, Feb 13 2017

Issue description

Currently Chrome rejects the FetchEvent.preloadResponse promise with an error when redirected.
https://chromium.googlesource.com/chromium/src/+/2fd0fdd/content/renderer/service_worker/service_worker_context_client.cc#295

But according to the spec, the promise should be resolved with the redirect response.
https://w3c.github.io/ServiceWorker/#handle-fetch

> 1. Fetch preloadRequest.
> To process response for navigationPreloadResponse, run these substeps:
>   1. If navigationPreloadResponse’s type is "error", reject preloadResponse with a TypeError and terminate these substeps.
>   2. Associate preloadResponseObject with navigationPreloadResponse.
>   3. Resolve preloadResponse with navigationPreloadResponse.
 

Comment 1 by ralp...@google.com, Mar 27 2017

Hi - Any updates on this?

Comment 2 by n...@fb.com, Mar 28 2017

Labels: DevRel-Facebook
This doesn't affect Facebook's immediate plans for preload, but it's something that we want to track long term so I'm adding the label.

Comment 3 by horo@chromium.org, Mar 30 2017

Status: Started (was: Assigned)

Comment 4 by horo@chromium.org, Mar 30 2017

Working in progress patch: https://codereview.chromium.org/2790433003/

Comment 5 by ralp...@google.com, Mar 30 2017

horo@, Thanks for taking a stab at this. Since this is a fix to an Original Trial, do you think it will be possible to get this cherrypicked in M57? Barring that, what's the soonest we would be able to get this to prod?

The internal bug for Docs is b/36734243 in which we lay out how this bug makes many things problematic, and will probably block our participation in the Original Trial on this bug.

Thanks!

Comment 6 by horo@chromium.org, Mar 30 2017

This patch for the fix is not a trivial one. I think it is imposiible to merge to M57.

The branch cut day of M59 is Apr 13.
If I can land the patch before the day, it will be available in Stable from Jun 6th.
https://www.chromium.org/developers/calendar
Project Member

Comment 7 by bugdroid1@chromium.org, Apr 2 2017

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

commit 96efe9c104f0618c637c648bd12728418286f254
Author: horo <horo@chromium.org>
Date: Sun Apr 02 00:08:31 2017

Support redirect responses for NavigationPreload

BUG= 691503 

Review-Url: https://codereview.chromium.org/2790433003
Cr-Commit-Position: refs/heads/master@{#461333}

[modify] https://crrev.com/96efe9c104f0618c637c648bd12728418286f254/content/browser/service_worker/service_worker_browsertest.cc
[modify] https://crrev.com/96efe9c104f0618c637c648bd12728418286f254/content/browser/service_worker/service_worker_fetch_dispatcher.cc
[modify] https://crrev.com/96efe9c104f0618c637c648bd12728418286f254/content/renderer/service_worker/service_worker_context_client.cc
[add] https://crrev.com/96efe9c104f0618c637c648bd12728418286f254/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigation-preload/redirect.https.html
[add] https://crrev.com/96efe9c104f0618c637c648bd12728418286f254/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigation-preload/resources/redirect-redirected.html
[add] https://crrev.com/96efe9c104f0618c637c648bd12728418286f254/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigation-preload/resources/redirect-scope.py
[add] https://crrev.com/96efe9c104f0618c637c648bd12728418286f254/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigation-preload/resources/redirect-worker.js
[add] https://crrev.com/96efe9c104f0618c637c648bd12728418286f254/third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/navigation-preload-redirected.html
[modify] https://crrev.com/96efe9c104f0618c637c648bd12728418286f254/third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/navigation-preload-scope.php
[modify] https://crrev.com/96efe9c104f0618c637c648bd12728418286f254/third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/navigation-preload-worker.php
[modify] https://crrev.com/96efe9c104f0618c637c648bd12728418286f254/third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/service-workers-navigation-preload-expected.txt
[modify] https://crrev.com/96efe9c104f0618c637c648bd12728418286f254/third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/service-workers-navigation-preload.html
[modify] https://crrev.com/96efe9c104f0618c637c648bd12728418286f254/third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp

Comment 8 by horo@chromium.org, Apr 3 2017

Labels: -Pri-2 Pri-1
Status: Verified (was: Started)
Note: 96efe9c104f0618c637c648bd12728418286f254 is in 59.0.3061.0.

$ git find-releases 96efe9c104f0618c637c648bd12728418286f254
commit 96efe9c104f0618c637c648bd12728418286f254 was:
  initially in 59.0.3061.0

I verified with Canary 59.0.3061.0.

1. Go https://horo-test.appspot.com/navigationpreload/demo/
2. Click "Register SW"
3. Click "redirect demo" link
Expected: Redirected to https://horo-test.appspot.com/navigationpreload/demo/redirected


Comment 9 by horo@chromium.org, Apr 3 2017

Cc: gov...@chromium.org abdulsyed@chromium.org
Labels: Merge-Request-58 OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows
This issue is blocking to start Original Trial experiment in Docs. (See: #c5)

So I'd like to merge 96efe9c104f0618c637c648bd12728418286f254 and f37399e1cb91a70730938f61008142ce61a00ad6 to M58.
(96efe9c104f0618c637c648bd12728418286f254 depends on f37399e1cb91a70730938f61008142ce61a00ad6)
Project Member

Comment 10 by sheriffbot@chromium.org, Apr 3 2017

Labels: -Merge-Request-58 Hotlist-Merge-Approved Merge-Approved-58
Your change meets the bar and is auto-approved for M58. Please go ahead and merge the CL to branch 3029 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

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

Labels: -merge-approved-58 merge-merged-3029
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/432269484584455b81b68bb58d0a356c0da29456

commit 432269484584455b81b68bb58d0a356c0da29456
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Mon Apr 03 08:10:53 2017

Merge to M58: Resolve preloadResponse of Navigation Preload even if the response body is empty.

BUG= 706765 , 691503 
TEST=virtual/service-worker-navigation-preload-wpt/external/wpt/service-workers/service-worker/navigation-preload/empty-preload-response-body.https.html

Review-Url: https://codereview.chromium.org/2787783002
Cr-Commit-Position: refs/heads/master@{#461082}
(cherry picked from commit f37399e1cb91a70730938f61008142ce61a00ad6)

Review-Url: https://codereview.chromium.org/2791963002 .
Cr-Commit-Position: refs/branch-heads/3029@{#540}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/432269484584455b81b68bb58d0a356c0da29456/content/renderer/service_worker/service_worker_context_client.cc
[add] https://crrev.com/432269484584455b81b68bb58d0a356c0da29456/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigation-preload/empty-preload-response-body.https.html
[add] https://crrev.com/432269484584455b81b68bb58d0a356c0da29456/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigation-preload/resources/empty-preload-response-body-scope.html
[add] https://crrev.com/432269484584455b81b68bb58d0a356c0da29456/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigation-preload/resources/empty-preload-response-body-worker.js
[modify] https://crrev.com/432269484584455b81b68bb58d0a356c0da29456/third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp

Project Member

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

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

commit 461204ca18397f44cd5dee4f9ca6e26c575c7dc8
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Mon Apr 03 08:13:31 2017

Merge to M58: Support redirect responses for NavigationPreload

BUG= 691503 

Review-Url: https://codereview.chromium.org/2790433003
Cr-Commit-Position: refs/heads/master@{#461333}
(cherry picked from commit 96efe9c104f0618c637c648bd12728418286f254)

Review-Url: https://codereview.chromium.org/2795653002 .
Cr-Commit-Position: refs/branch-heads/3029@{#541}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/461204ca18397f44cd5dee4f9ca6e26c575c7dc8/content/browser/service_worker/service_worker_browsertest.cc
[modify] https://crrev.com/461204ca18397f44cd5dee4f9ca6e26c575c7dc8/content/browser/service_worker/service_worker_fetch_dispatcher.cc
[modify] https://crrev.com/461204ca18397f44cd5dee4f9ca6e26c575c7dc8/content/renderer/service_worker/service_worker_context_client.cc
[add] https://crrev.com/461204ca18397f44cd5dee4f9ca6e26c575c7dc8/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigation-preload/redirect.https.html
[add] https://crrev.com/461204ca18397f44cd5dee4f9ca6e26c575c7dc8/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigation-preload/resources/redirect-redirected.html
[add] https://crrev.com/461204ca18397f44cd5dee4f9ca6e26c575c7dc8/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigation-preload/resources/redirect-scope.py
[add] https://crrev.com/461204ca18397f44cd5dee4f9ca6e26c575c7dc8/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigation-preload/resources/redirect-worker.js
[add] https://crrev.com/461204ca18397f44cd5dee4f9ca6e26c575c7dc8/third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/navigation-preload-redirected.html
[modify] https://crrev.com/461204ca18397f44cd5dee4f9ca6e26c575c7dc8/third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/navigation-preload-scope.php
[modify] https://crrev.com/461204ca18397f44cd5dee4f9ca6e26c575c7dc8/third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/navigation-preload-worker.php
[modify] https://crrev.com/461204ca18397f44cd5dee4f9ca6e26c575c7dc8/third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/service-workers-navigation-preload-expected.txt
[modify] https://crrev.com/461204ca18397f44cd5dee4f9ca6e26c575c7dc8/third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/service-workers-navigation-preload.html
[modify] https://crrev.com/461204ca18397f44cd5dee4f9ca6e26c575c7dc8/third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp

Comment 13 Deleted

Comment 14 by horo@chromium.org, Apr 4 2017

Note: Both change were merged to 58.0.3029.52

$ git find-releases 96efe9c104f0618c637c648bd12728418286f254 
commit 96efe9c104f0618c637c648bd12728418286f254 was:
  initially in 59.0.3061.0
  merged to 58.0.3029.52 (as 461204ca18397f44cd5dee4f9ca6e26c575c7dc8)
horo@horox:~/chromium/src$ git find-releases f37399e1cb91a70730938f61008142ce61a00ad6
commit f37399e1cb91a70730938f61008142ce61a00ad6 was:
  initially in 59.0.3058.0
  merged to 58.0.3029.52 (as 432269484584455b81b68bb58d0a356c0da29456)


Sign in to add a comment