Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Description • 10 years ago

Tracking to ensure we test this.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 1 • 9 years ago

Nikhil, will you (or someone else) be willing to mentor here please? If so I want to take it. Thanks!

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 2 • 9 years ago

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

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 3 • 9 years ago

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.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 4 • 9 years ago

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!

	Josh Matthews [:jdm]
	Comment 5 • 9 years ago

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

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 6 • 9 years ago

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

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 7 • 9 years ago

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

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 8 • 9 years ago

Let's wait until Bug 1137287 lands.

	Noemí Freire (:noemi)
	Comment 9 • 9 years ago

Per comment 8, adding Bug 1137287 dependence.

	Josh Matthews [:jdm]
	Comment 10 • 9 years ago

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.

	Honza Bambas (:mayhemer)
	Comment 11 • 9 years ago

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?

	(no longer active)
	Comment 12 • 9 years ago

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!

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 13 • 9 years ago

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

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 14 • 9 years ago

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

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 15 • 9 years ago

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

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 16 • 9 years ago

(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

	(no longer active)
	Comment 17 • 9 years ago

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!

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 18 • 9 years ago

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

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 19 • 9 years ago

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

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 20 • 9 years ago

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

Let's see how that looks like and land it then.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 21 • 9 years ago

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

	Pulsebot
	Comment 22 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/733fbadf4fe3

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 23 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/733fbadf4fe3