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

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 1

[Josh Matthews [:jdm]]

Attachment: 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.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

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

[(no longer active)]

What's the status of this?

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Oh, wait.  There is another test present here.  Lets see if we can land that.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: 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

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: bug1200677_interdiff.patch

Comment 8

[Josh Matthews [:jdm]]

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?

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

(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.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

I managed to fix the race, but we have a bug in the platform code for those cases.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: 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.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: 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.

Comment 13

[Josh Matthews [:jdm]]

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.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=141f3552f08e

Comment 15

[Josh Matthews [:jdm]]

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?

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

(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?

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

I'm going to look at this some more.  I'm not sure this is the right fix any more.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Attachment: 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

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Import navigation-redirect.https.html for non-e10s. r=jdm

Folded patch including interdiff 3.

Comment 21

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a6ee2a16bd63569fd916976ac1c60635e3eb5c5
Bug 1200677 - Import navigation-redirect.https.html for non-e10s. r=jdm

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9a6ee2a16bd6

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/a7e4b6ab821f

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/a7e4b6ab821f