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!

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

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.

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!

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

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

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

Let's wait until Bug 1137287 lands.

Per comment 8, adding Bug 1137287 dependence.

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.

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?

Please re-enable these two tests <https://hg.mozilla.org/integration/mozilla-inbound/rev/1189f6ffc65e#l2.27> <https://hg.mozilla.org/integration/mozilla-inbound/rev/1189f6ffc65e#l2.31> when this bug gets fixed. Thanks!

Bug 1122161 - Redirected channels should respect skip service worker flag. r=nsm

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

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!

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

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=422c33cdd994 Let's see how that looks like and land it then.

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