Import navigation-redirect.https.html and fix interception on navigation redirection

RESOLVED FIXED in Firefox 44, Firefox OS v2.5

Status

()

Core
DOM: Service Workers
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

unspecified
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 wontfix, firefox44 fixed, firefox45 fixed, b2g-v2.5 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

2 years ago
Follow-up from bug 1184607.  There are a couple new redirect tests in the blink tree that we should import.  They test navigations, etc.

  https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-redirect.html&sq=package:chromium
  https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/LayoutTests/http/tests/serviceworker/fetch-request-redirect.html&sq=package:chromium

Obviously there are supporting files needed as well.

These tests have some overlap with the tests I wrote for bug 1184607, but they also test additional cases.  So I think its worth having all three.  Maybe one day we can unify them when they are upstreamed.

Updated

2 years ago
Assignee: nobody → josh
Status: NEW → ASSIGNED

Comment 1

2 years ago
Created attachment 8673376 [details] [diff] [review]
Import two new tests from blink for ServiceWorker interception + redirection behaviour

WIP. navigation-redirection.https.html runs to completion but most of the tests fail right now. I'm going cross-eyed trying to figure out the control flow :( The other test will be more work to get running, since it relies on various other resources that haven't been imported.
(Assignee)

Comment 2

2 years ago
Do we intercept again after a navigation redirects?  I seem to recall a lot of these tests are checking for variations of that.

Comment 3

2 years ago
What's the status of this?

Updated

2 years ago
Depends on: 1219085
(Assignee)

Comment 4

2 years ago
All dependent bugs are fixed.  We'll still have more wpt work to do, but I don't think we need this meta bug any more.
(Assignee)

Comment 5

2 years ago
Oh, wait.  There is another test present here.  Lets see if we can land that.
(Assignee)

Updated

2 years ago
Summary: Import new blink SW redirection wpt tests → Import navigation-redirect.https.html
(Assignee)

Comment 6

2 years ago
Created attachment 8689576 [details] [diff] [review]
Import navigation-redirect.https.html for non-e10s. r=jdm

The test was making assumptions about racing promises and service worker lifetime.  Once I fixed those we pass the test.

Of course, we crash on e10s due to bug 1219469.  So this only enables the test on non-e10s.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fb58850e408
Assignee: josh → bkelly
Attachment #8673376 - Attachment is obsolete: true
Attachment #8689576 - Flags: review?(josh)
(Assignee)

Updated

2 years ago
Attachment #8689576 - Attachment is patch: true
(Assignee)

Comment 7

2 years ago
Created attachment 8689585 [details] [diff] [review]
bug1200677_interdiff.patch

Comment 8

2 years ago
Comment on attachment 8689576 [details] [diff] [review]
Import navigation-redirect.https.html for non-e10s. r=jdm

Review of attachment 8689576 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/mozilla/tests/service-workers/service-worker/resources/navigation-redirect-out-scope.php
@@ +1,1 @@
> +<?php

We can remove this file.

::: testing/web-platform/mozilla/tests/service-workers/service-worker/resources/navigation-redirect-scope1.php
@@ +1,1 @@
> +<?php

We can remove this file.

::: testing/web-platform/mozilla/tests/service-workers/service-worker/resources/navigation-redirect-scope2.php
@@ +1,1 @@
> +<?php

We can remove this file.

::: testing/web-platform/mozilla/tests/service-workers/service-worker/resources/navigation-redirect-worker.js
@@ +51,5 @@
> +          })
> +          .then(function(res) { return cache.put(event.request, res); })
> +          .then(function() { return cache.match(url); });
> +      }
> +      return fetch(event.request);

The thing that I struggled with when working on this test was dealing with this case, since the original test let the request go to the network if the query params didn't match any prior case. Are we sure we're testing what we want with this change?
Attachment #8689576 - Flags: review?(josh) → review+
(Assignee)

Comment 9

2 years ago
(In reply to Josh Matthews [:jdm] from comment #8)
> The thing that I struggled with when working on this test was dealing with
> this case, since the original test let the request go to the network if the
> query params didn't match any prior case. Are we sure we're testing what we
> want with this change?

Hmm.  I see what you mean.  I can do this at the cost of a small race with the cache.put() at the top of the function.
(Assignee)

Comment 10

2 years ago
I managed to fix the race, but we have a bug in the platform code for those cases.
(Assignee)

Comment 11

2 years ago
Created attachment 8689680 [details] [diff] [review]
bug1200677_interdiff_2.patch

This avoids calling event.respondWith().  I have to play games with the waitUntil promises to avoid races with the message event.
Attachment #8689680 - Flags: review?(josh)
(Assignee)

Comment 12

2 years ago
Created attachment 8689682 [details] [diff] [review]
P0 nsHttpChannel should re-intercept navigation redirections. r=jdm

And this lets us pass the no-respondWith test cases.  Basically, we need to bypass the service worker for the internal redirect, but then allow the service worker to intercept for any subsequent redirects.  Just for navigations, I believe.
Attachment #8689682 - Flags: review?(josh)
Comment on attachment 8689680 [details] [diff] [review]
bug1200677_interdiff_2.patch

Review of attachment 8689680 [details] [diff] [review]:
-----------------------------------------------------------------

Nice. I nearly had this solution, but didn't make the connection of using waitUntil in the message listener.
Attachment #8689680 - Flags: review?(josh) → review+
(Assignee)

Comment 14

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=141f3552f08e
(Assignee)

Updated

2 years ago
status-firefox43: --- → wontfix
status-firefox44: --- → affected
status-firefox45: --- → affected
(Assignee)

Updated

2 years ago
Summary: Import navigation-redirect.https.html → Import navigation-redirect.https.html and fix interception on navigation redirection
Comment on attachment 8689682 [details] [diff] [review]
P0 nsHttpChannel should re-intercept navigation redirections. r=jdm

Review of attachment 8689682 [details] [diff] [review]:
-----------------------------------------------------------------

This does implement the solution that you describe in comment 12, hence the r+. However, I am not yet convinced that the test expectations are correct (and therefore that this change is necessary). The Fetch spec has exactly one instance of setting the skip-service-worker flag (ie. HTTP fetch step 4.2) and it's unconditional; is the navigation/subresource distinction encoded in the conditions in HTTP fetch step 3, or am I missing something else in my reading?
Attachment #8689682 - Flags: review?(josh) → review+
(Assignee)

Comment 16

2 years ago
(In reply to Josh Matthews [:jdm] from comment #15)
> This does implement the solution that you describe in comment 12, hence the
> r+. However, I am not yet convinced that the test expectations are correct
> (and therefore that this change is necessary). The Fetch spec has exactly
> one instance of setting the skip-service-worker flag (ie. HTTP fetch step
> 4.2) and it's unconditional; is the navigation/subresource distinction
> encoded in the conditions in HTTP fetch step 3, or am I missing something
> else in my reading?

The issue is that navigations don't use the fetch spec redirection code.  They follow their own redirections.  This encoded in the manual redirect mode which means navigations always get back a non-followed 301 response.  It then presumably crafts a new request to follow the redirect (which does not have the skip service worker flag set).

Of course, this is not obvious to me reading the html spec:

  https://html.spec.whatwg.org/#navigate

Also, you can see this was clearly the intention from comments like this:

  https://github.com/whatwg/fetch/issues/66#issuecomment-121192316

Of course, now that I read step 4.2 again, it seems maybe we should also allow re-interception of respondWith(Response.redirect(newURL)).  I don't think that case would hit step 4.2 either because its a non-null Response returned from Handle Fetch.  I can file a follow-up for that.

What do you think?
Flags: needinfo?(josh)
(Assignee)

Comment 17

2 years ago
I'm going to look at this some more.  I'm not sure this is the right fix any more.
Flags: needinfo?(josh)
(Assignee)

Comment 18

2 years ago
Josh, you're completely right.  I'm sorry I got so confused and convinced about my position.  Clearly I should go on vacation!

Let me fix the tests.  I don't see a problem with our code here based on what the spec currently says.
(Assignee)

Updated

2 years ago
Attachment #8689682 - Attachment is obsolete: true
(Assignee)

Comment 19

2 years ago
Created attachment 8690336 [details] [diff] [review]
bug1200677_interdiff_3.patch

Another interdiff for the test.  Also, I wrote a bug against chrome:

  https://code.google.com/p/chromium/issues/detail?id=559447&thanks=559447&ts=1448069478
Attachment #8690336 - Flags: review?(josh)
(Assignee)

Comment 20

2 years ago
Created attachment 8690337 [details] [diff] [review]
Import navigation-redirect.https.html for non-e10s. r=jdm

Folded patch including interdiff 3.
Attachment #8689576 - Attachment is obsolete: true
Attachment #8689585 - Attachment is obsolete: true
Attachment #8689680 - Attachment is obsolete: true

Updated

2 years ago
Attachment #8690336 - Flags: review?(josh) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a6ee2a16bd63569fd916976ac1c60635e3eb5c5
Bug 1200677 - Import navigation-redirect.https.html for non-e10s. r=jdm

Comment 22

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9a6ee2a16bd6
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(Assignee)

Updated

2 years ago
Blocks: 1229369

Comment 23

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/a7e4b6ab821f
status-firefox44: affected → fixed

Comment 24

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/a7e4b6ab821f
status-b2g-v2.5: --- → fixed
You need to log in before you can comment on or make changes to this bug.