Closed Bug 1200677 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox43 --- wontfix
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(2 files, 5 obsolete files)

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.
Assignee: nobody → josh
Status: NEW → ASSIGNED
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.
Do we intercept again after a navigation redirects?  I seem to recall a lot of these tests are checking for variations of that.
What's the status of this?
Depends on: 1219085
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.
Oh, wait.  There is another test present here.  Lets see if we can land that.
Summary: Import new blink SW redirection wpt tests → Import navigation-redirect.https.html
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)
Attachment #8689576 - Attachment is patch: true
Attached patch bug1200677_interdiff.patch (obsolete) — Splinter Review
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+
(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.
I managed to fix the race, but we have a bug in the platform code for those cases.
Attached patch bug1200677_interdiff_2.patch (obsolete) — Splinter Review
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)
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+
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+
(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)
I'm going to look at this some more.  I'm not sure this is the right fix any more.
Flags: needinfo?(josh)
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.
Attachment #8689682 - Attachment is obsolete: true
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)
Folded patch including interdiff 3.
Attachment #8689576 - Attachment is obsolete: true
Attachment #8689585 - Attachment is obsolete: true
Attachment #8689680 - Attachment is obsolete: true
Attachment #8690336 - Flags: review?(josh) → review+
https://hg.mozilla.org/mozilla-central/rev/9a6ee2a16bd6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Blocks: 1229369
You need to log in before you can comment on or make changes to this bug.