1420672 - Redirect loses hash fragment when going through Service Worker

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect, P2)

Description

[Daniel Huigens [:twiss]]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171123161455

Steps to reproduce:

Open the following URL twice: https://www.airbornos.com/run#test (once to install the Service Worker, a second time to test with SW installed).

That URL redirects to /demo. The Service Worker is fairly complex, but in this case should amount to not much more than `event.respondWith(fetch(event.request).clone())`.


Actual results:

The first time, the redirect keeps the hash fragment (#test) as it should. The second time, it is lost.


Expected results:

The hash should be kept both times.


Regression window: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4afc55e7033cba917b1b55a74bf4941c8a48c660&tochange=2ed5e7fbf39e949693d8a7455d6313b14a7aeaf6
Bug 1391693 seems the likely culprit.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

NI for suspicious regression bug in comment 0

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

I need to implement this code in InterceptedHttpChannel:

https://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#5652

I'll take this and try to fix soon so we can uplift to FF58.  I'll add a WPT test as well.

Comment 3

[Mike Taylor [:miketaylr]]

Hi Ben, do you still think you'll be able to get to this for uplift to 58? Thanks.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Mike Taylor [:miketaylr] (58 Regression Engineering Owner) from comment #3)
> Hi Ben, do you still think you'll be able to get to this for uplift to 58?
> Thanks.

Yes, but I am prioritizing other work before the December break.  I think I can get a test and fix uplifted before the merge which I believe is the 3rd week of January.  (I think we got an extra week when the holiday time was extended for the company.)

Ideally I'll get to this next week, though.  (I'm not going to Austin.)  It just depends on how fast I can get my high priority work fixed.

Comment 5

[Mike Taylor [:miketaylr]]

Thanks for the update, much appreciated!

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Tom said he might have time to do this sooner than January.

Tom, see comment 2 for the code currently in nsHttpChannel that we need to run in InterceptedHttpChannel.  I was thinking of maybe factoring that logic out into a helper method on the HttpBaseChannel class that both nsHttpChannel and InterceptedHttpChannel could call.

We also probably need to add a WPT case.  Hopefully this is just adding to one of the existing redirect test files.

Comment 7

[Tom Tung [:tt, :ttung]]

I'll try to fix this in this week or the next week.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Thanks!

Comment 9

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1420672: Propagate the uri reference to the redirect uri if the redirect uri doesn't have one. r?beklly

Hi Ben, 

This patch does copy the ref of the uri if the redirect uri doesn't have one. 
Could you help me to review it? Thanks! I'll add a wpt test or find a test to verify this later. 

However, there is one thing which I'm sure about. Since the pointer [1] in comment #2 is pointing to where delete the cache entry while redirect the itself, I'm not sure whether I need to copy this logic to the InterceptedHttpChannel as well. Do we need to take care about this in InterceptedHttpChannel? 

[1] https://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#5652

Comment 10

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1420672 - P2: Add wpt tests to verify it. r?bkelly

WPT test. 

Without applying P1, the test "SW-generated redirect to same-origin out-scope with ref" fails. 
With applying P1, the test success.

Comment 11

[Tom Tung [:tt, :ttung]]

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

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Tom Tung [:tt] from comment #9)
> However, there is one thing which I'm sure about. Since the pointer [1] in
> comment #2 is pointing to where delete the cache entry while redirect the
> itself, I'm not sure whether I need to copy this logic to the
> InterceptedHttpChannel as well. Do we need to take care about this in
> InterceptedHttpChannel? 

No, we don't need this.  I think my searchfox link started pointing to slightly different code in nsHttpChannel due to other changes moving things around in the file.  I should figure out how to do permalinks with a rev.  Sorry for the confusion.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8935707 [details] [diff] [review]
Bug 1420672 - P2: Add wpt tests to verify it. r?bkelly

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

::: testing/web-platform/tests/service-workers/service-worker/navigation-redirect.https.html
@@ +155,5 @@
>    }, 'Normal redirect to same-origin scope.');
>  promise_test(function(t) {
>      return setup_environment(t).then(function() {
>          return test_redirect(
> +            OUT_SCOPE + 'url=' + encodeURIComponent(SCOPE1) + '#ref',

Can you add a case that also includes a different hash as part of the redirect location as well?  Something like:

  return test_redirect(
    OUT_SCOPE + 'url=' + encodeURIComponent(SCOPE1 + "ref2") + '#ref',
    SCOPE1 + "#ref2",
    [[SCOPE1 + "#ref2"], [], []]);

I think this should work with our code, but it would be nice to include in the WPT as long as we are here.

Also, I was initially confused about if this "#ref" was on the original or redirected URL at first.  Took me a while to figure out it was getting added to the original URL since it comes right after the `SCOPE1)` in the code.  Having a case that shows a hash on the redirect URL might make it easier to understand.

Thanks!

Comment 14

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1420672 - P1: Propagate the uri reference to the redirect uri if the redirect uri doesn't have one. r=beklly

Comment 15

[Tom Tung [:tt, :ttung]]

Attachment: interdiff patch for tests

Ben, I find out I should also test the real redirect cases rather than just a service worker generated redirect. This is the inter-diff patch for what I add. Could you help me to review? Thanks!

Comment 16

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1420672 - P2: Add wpt tests to verify service workers redirect the hash fragment if the redirect URL doesn't have one. r=bkelly

Address the comment and add two more cases for the real redirect while service worker is intercepting the request.

Comment 17

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=61c69670543d920724266334524a2c367e35060d

Comment 18

[Tom Tung [:tt, :ttung]]

Comment on attachment 8936371 [details] [diff] [review]
Bug 1420672 - P2: Add wpt tests to verify service workers redirect the hash fragment if the redirect URL doesn't have one. r=bkelly

Thanks for the review!

Comment 19

[Tom Tung [:tt, :ttung]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b74f146fbc0a987c2ab259f3dc19cfe41a7ce0b2
Bug 1420672 - P1: Propagate the uri reference to the redirect uri if the redirect uri doesn't have one. r=beklly

https://hg.mozilla.org/integration/mozilla-inbound/rev/976d54360e53438f9f7029b636b4585ee974f891
Bug 1420672 - P2: Add wpt tests to verify service workers redirect the hash fragment if the redirect URL doesn't have one. r=bkelly

Comment 20

[Guilherme Lima]

The merge https://hg.mozilla.org/mozilla-central/rev/93b37aa497c4 didn't set the bugs as fixed.

Comment 21

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/b74f146fbc0a
https://hg.mozilla.org/mozilla-central/rev/976d54360e53

Comment 22

[Julien Cristau [:jcristau]]

Please request uplift to beta when you get a chance.

Comment 23

[Tom Tung [:tt, :ttung]]

Comment on attachment 8936368 [details] [diff] [review]
Bug 1420672 - P1: Propagate the uri reference to the redirect uri if the redirect uri doesn't have one. r=beklly

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1391693
[User impact if declined]: As the comment in comment #0, the website's redirected URL will lose its fragment while the website is controlled by a service worker.
[Is this code covered by automated tests?]: Yes, it's in the P2.
[Has the fix been verified in Nightly?]: Yes, the website's URL described in comment #0 doesn't lose its fragment in current Nightly.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: This patch and patch P2 (Attachment #8936371 [details] [diff]) if it's possible. Patch P2 adds few tests (web-platform test) to verify the behavior is the same as expected.
[Is the change risky?]: No
[Why is the change risky/not risky?]: The patch only makes the logic for propagating fragment apply in the service worker intercepted channel. (Note: It was only applied in the normal HTTP channel in Firefox 58)
[String changes made/needed]: No

Comment 24

[Julien Cristau [:jcristau]]

Comment on attachment 8936368 [details] [diff] [review]
Bug 1420672 - P1: Propagate the uri reference to the redirect uri if the redirect uri doesn't have one. r=beklly

regression fix for service worker, beta58+

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/9ea92fd889d8
https://hg.mozilla.org/releases/mozilla-beta/rev/9e467d43a27a