Closed Bug 1122161 Opened 5 years ago Closed 5 years ago

Redirected channels should respect skip service worker flag

Categories

(Core :: DOM: Core & HTML, defect)

33 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: nsm, Assigned: jaoo, Mentored)

References

Details

Attachments

(1 file, 3 obsolete files)

Tracking to ensure we test this.
Nikhil, will you (or someone else) be willing to mentor here please? If so I want to take it. Thanks!
Flags: needinfo?(nsm.nikhil)
Yes. We have to test 2 things.

1) Request is made from a page that has a controlling SW. When the page fetches a url the controlling SW forces a redirect to another location. this other location fetch should also be intercepted by the SW.

2) Request is made from a SW. The fetch will internally get it's SkipServiceWorker flag set. The mochitest server should force a redirect using an SJS (e.g. https://hg.mozilla.org/mozilla-central/diff/ed7e94450c03/dom/workers/test/serviceworkers/redirect_serviceworker.sjs). This one should not go through the SW
Flags: needinfo?(nsm.nikhil)
Assignee: nobody → josea.olivera
Mentor: nsm.nikhil
Status: NEW → ASSIGNED
Attached patch WIP (obsolete) — Splinter Review
Tested first thing from comment 2. I cannibalized several parts of the fetch event test so I guess I'll end up adding this test to the fetch event one.
Attached patch v1 (obsolete) — Splinter Review
This version implement -more or less- the tests explained at comment 2. I'd like to have some feedback please. The test corresponding to the first part of comment 2 fails when running on e10s mode. I not able to know why :(. Is there any issue with redirects (through Response.redirect() function) in the service worker? Thanks!
Attachment #8583240 - Attachment is obsolete: true
Attachment #8583798 - Flags: feedback?(nsm.nikhil)
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #4)
> Created attachment 8583798 [details] [diff] [review]
> v1
> 
> This version implement -more or less- the tests explained at comment 2. I'd
> like to have some feedback please. The test corresponding to the first part
> of comment 2 fails when running on e10s mode. I not able to know why :(. Is
> there any issue with redirects (through Response.redirect() function) in the
> service worker? Thanks!

e10s redirection failures are likely bug 1137287.
See Also: → 1137287
Comment on attachment 8583798 [details] [diff] [review]
v1

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

sorry for the delay.

::: dom/workers/test/serviceworkers/fetch_event_worker.js
@@ +161,5 @@
>      }
> +  } else if (ev.request.url.contains('something.txt')) {
> +    ev.respondWith(Promise.resolve(
> +      Response.redirect(
> +        'http://mochi.test:8888/tests/dom/workers/test/serviceworkers/fetch/somethingelse.txt'

Just use relative url 'fetch/somethingelse.txt'

@@ +169,5 @@
> +    ev.respondWith(Promise.resolve(
> +      new Response('something else response body', {})
> +    ));
> +  } else if (ev.request.url.contains('redirect_serviceworker.sjs')) {
> +    var redirect_url = 'http://mochi.test:8888/tests/dom/workers/test/serviceworkers/redirect_serviceworker.sjs';

relative url.

@@ +174,5 @@
> +    // The redirect_serviceworker.sjs server-side JavaScript file redirects to
> +    // 'http://mochi.test:8888/tests/dom/workers/test/serviceworkers/worker.js'
> +    // That redirect location fetch does not go through this SW. Why?, because
> +    // if the location fetch went through the SW, the SW would need take care of
> +    // it to get the test passing.

This comment should be more of 'The redirected fetch should not go through the SW since the original fetch was initiated from a SW.'
Attachment #8583798 - Flags: feedback?(nsm.nikhil) → feedback+
Attached patch v2 (obsolete) — Splinter Review
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #6)
> Comment on attachment 8583798 [details] [diff] [review]
> v1
> 
> Review of attachment 8583798 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the review.

This version address the review comments made at comment 6.

Since bug 1136780 disabled this tests and they fail when e10s mode due to bug 1137287, how should we deal with this situation? I mean, should we pause here until those bugs get fixed? Let me know please. Thanks!
Attachment #8583798 - Attachment is obsolete: true
Flags: needinfo?(nsm.nikhil)
Let's wait until Bug 1137287 lands.
Flags: needinfo?(nsm.nikhil)
Per comment 8, adding Bug 1137287 dependence.
Depends on: 1137287
Honza, we have more problems after bug 1137287. That bug made us process 302 responses in the parent, but we now need to call the redirected channel's AsyncOpen in the child (where the ServiceWorker is running), rather than in the parent at nsHttpChannel::ContinueProcessRedirection. This looks... complicated to get right.
Flags: needinfo?(honzab.moz)
And hitting IPC redirects again...  I somehow thought you had this under control.  

OK.  Let HttpChannel parent tell the nsHttpChannel simply to not AsyncOpen the new channel (rather throw it away) - this needs to be imlemented.  AsyncOpen on the child will be called from CompleteRedirectSetup.  Unfortunatelly this will need to be done on many places since we have mRedirectChannel->AsyncOpen spread all over around.  Maybe do a cleanup patch first?
Flags: needinfo?(honzab.moz)
Depends on: 1157283
Attachment #8588529 - Attachment is obsolete: true
Bug 1122161 - Redirected channels should respect skip service worker flag. r=nsm
Attachment #8613216 - Flags: review?(nsm.nikhil)
Comment on attachment 8613216 [details]
MozReview Request: Bug 1122161 - Redirected channels should respect skip service worker flag. r=nsm

Bug 1122161 - Redirected channels should respect skip service worker flag. r=nsm
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #14)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e2a61c8ffa4

Too much orange there. It seems the tests Eshan wanted to enable here are causing that orange. I'll mark to skip them. Let's see the try results now.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba3c7a5c50e4
Flags: needinfo?(ehsan)
Bug 1164397 may also help.  Please file a new bug and make it depend on both this one and the other one, and then we can look into re-enabling those tests there.  Thanks!
Flags: needinfo?(ehsan)
Comment on attachment 8613216 [details]
MozReview Request: Bug 1122161 - Redirected channels should respect skip service worker flag. r=nsm

https://reviewboard.mozilla.org/r/9727/#review8659

Ship It!

::: dom/workers/test/serviceworkers/fetch_event_worker.js:195
(Diff revision 2)
> +    ev.respondWith(Promise.resolve(
> +      Response.redirect('fetch/somethingelse.txt')
> +    ));

ev.respondWith(Response.redirect(...))

::: dom/workers/test/serviceworkers/fetch_event_worker.js:201
(Diff revision 2)
> +    ev.respondWith(Promise.resolve(
> +      new Response('something else response body', {})
> +    ));

same here

::: dom/workers/test/serviceworkers/fetch/fetch_tests.js:153
(Diff revision 2)
> +// made from the SW through fecth(). Fetch() fetches a server-side JavaScript

typo fecth -> fetch
In "Fetch() fetches a ..." fetch() should be lowercase
Attachment #8613216 - Flags: review?(nsm.nikhil) → review+
Blocks: 1170937
Comment on attachment 8613216 [details]
MozReview Request: Bug 1122161 - Redirected channels should respect skip service worker flag. r=nsm

Bug 1122161 - Redirected channels should respect skip service worker flag. r=nsm
Attachment #8613216 - Flags: review+
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #20)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=422c33cdd994
> 
> Let's see how that looks like and land it then.

Looking good, lets land it.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/733fbadf4fe3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.