1137287 - Redirection does not work with Service Worker interception

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

If a ServiceWorker responds with a redirect request, it is not respected.

// window
use XHR to fetch a file intercepted by the service worker
Expected: xhr.status == 200, assuming the redirect location is valid
Actual: xhr.status == 302

// SW
e.respondWith(Promise.resolve(Response.redirect("other-location")))

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: tests and partial fix

This does not fix the real problem (jdm will do that), but it adds tests and makes ResolvedCallback continue even on empty body.

Comment 2

[Josh Matthews [:jdm]]

Ok, for the non-e10s case, I suspect the problem lies in http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#5338. It's not yet clear to me whether it's safe to take that path if we're reading from a synthesized cache entry.

For the e10s case, this is going to be harder. Honza, do you have any ideas about how to deal with synthesized redirects in content processes?

Comment 3

[Josh Matthews [:jdm]]

Attachment: Non-e10s proof-of-concept fix

This makes the test pass in non-e10s. I'm not really pleased with it, though.

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Josh Matthews [:jdm] from comment #2)
> Ok, for the non-e10s case, I suspect the problem lies in
> http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/
> nsHttpChannel.cpp#5338. It's not yet clear to me whether it's safe to take
> that path if we're reading from a synthesized cache entry.
> 
> For the e10s case, this is going to be harder. Honza, do you have any ideas
> about how to deal with synthesized redirects in content processes?

Josh, how is that you don't get to nsHttpChannel::ReadFromCache where we process the redirection (AsyncCall(&nsHttpChannel::HandleAsyncRedirect);)?

For e10s this needs a bit of thinking.  Redirs are handled fully on the parent only..

Comment 5

[Josh Matthews [:jdm]]

Aha! You're right, it works now in non-e10s since bug 1142124 was fixed.

Comment 6

[Honza Bambas (:mayhemer)]

Is this then fixed or invalid or what? :)

Comment 7

[Josh Matthews [:jdm]]

e10s is still broken.

Comment 8

[Honza Bambas (:mayhemer)]

Ah, "it works now in non-e10s".

Yep, so for e10s we need to special case the redirect response and interception, I'm afraid.  It also means that we need to call and wait for the redirect callbacks on the parent.  There is no code for this now, since all the redirects (from net or cache) are originated fully on the parent.  

Easiest may be to send a notification to the parent (HttpChannelParent) that there is a synthesized redir response and let it handle everything for you (callbacks, setup of a new channel which also transfers mListener on the child automatically - see all the RecvRedirect[1-3].* methods on HttpChannelParent and Child)

Comment 9

[Josh Matthews [:jdm]]

Attachment: tests and partial fix

Comment 10

[Josh Matthews [:jdm]]

Attachment: Part 1: Shift some code from InterceptedChannelContent to HttpChannelChild

Comment 11

[Josh Matthews [:jdm]]

Attachment: Part 2: Send non-200/404 synthesized responses via the parent HTTP implementation for proper processing

Comment 12

[Honza Bambas (:mayhemer)]

(In reply to Josh Matthews [:jdm] from comment #11)
> Created attachment 8582194 [details] [diff] [review]
> Part 2: Send non-200/404 synthesized responses via the parent HTTP
> implementation for proper processing

I need some explanation on this one.  From staring at it I don't think I like the idea very much.

Comment 13

[Josh Matthews [:jdm]]

(In reply to Honza Bambas (:mayhemer) from comment #12)
> (In reply to Josh Matthews [:jdm] from comment #11)
> > Created attachment 8582194 [details] [diff] [review]
> > Part 2: Send non-200/404 synthesized responses via the parent HTTP
> > implementation for proper processing
> 
> I need some explanation on this one.  From staring at it I don't think I
> like the idea very much.

If the synthesized response code is anything except 200/404, we send the synthesized input to the parent as we create the regular cross-process HTTP channel. The parent actor determines whether or not to synthesize a response for the new channel based on the presence of the synthesized input stream. It copies the synthesized input stream into the output buffer provided when the channel is successfully intercepted (using the same code as http://hg.mozilla.org/mozilla-central/file/9dbb2d41bb2c/dom/workers/ServiceWorkerEvents.cpp#l174). I'm not sure what you don't like here, as this is basically the same thing that happens in non-e10s.

Comment 14

[Honza Bambas (:mayhemer)]

(In reply to Josh Matthews [:jdm] from comment #13)
> (In reply to Honza Bambas (:mayhemer) from comment #12)
> > (In reply to Josh Matthews [:jdm] from comment #11)
> > > Created attachment 8582194 [details] [diff] [review]
> > > Part 2: Send non-200/404 synthesized responses via the parent HTTP
> > > implementation for proper processing
> > 
> > I need some explanation on this one.  From staring at it I don't think I
> > like the idea very much.
> 
> If the synthesized response code is anything except 200/404, we send the
> synthesized input to the parent as we create the regular cross-process HTTP
> channel. The parent actor determines whether or not to synthesize a response
> for the new channel based on the presence of the synthesized input stream.

Sorry, still not getting this.  Why do you need to send the content to the parent?  Aren't headers enough?

> It copies the synthesized input stream into the output buffer provided when
> the channel is successfully intercepted (using the same code as
> http://hg.mozilla.org/mozilla-central/file/9dbb2d41bb2c/dom/workers/
> ServiceWorkerEvents.cpp#l174). I'm not sure what you don't like here, as
> this is basically the same thing that happens in non-e10s.

I don't like sending data across IPC when I don't follow the reason to do so.

Comment 15

[Josh Matthews [:jdm]]

Oh, I didn't think of that. If we can ignore the body for the responses, that will definitely be easier.

Comment 16

[Honza Bambas (:mayhemer)]

(In reply to Josh Matthews [:jdm] from comment #15)
> Oh, I didn't think of that. If we can ignore the body for the responses,
> that will definitely be easier.

We definitely don't.  Headers and response code are enough.

Josh, please next time you submit any patch and asking me for review write few notes/points about what the patch roughly does and why.  It makes my hard work of a reviewer much easier.  Thanks.

Comment 17

[Honza Bambas (:mayhemer)]

Comment on attachment 8582193 [details] [diff] [review]
Part 1: Shift some code from InterceptedChannelContent to HttpChannelChild

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

So, this one will obsolete, right?

Comment 18

[Honza Bambas (:mayhemer)]

Comment on attachment 8582194 [details] [diff] [review]
Part 2: Send non-200/404 synthesized responses via the parent HTTP implementation for proper processing

And this one as well I think.

Comment 19

[Josh Matthews [:jdm]]

Attachment: Part 0: Test for synthesized redirects

Comment 20

[Josh Matthews [:jdm]]

Attachment: Part 1: Shift some code from InterceptedChannelContent to HttpChannelChild

This is just a simple code reorganization to make part 2 easier to read.

Comment 21

[Josh Matthews [:jdm]]

Attachment: Part 2: Send non-200/404 synthesized responses via the parent HTTP implementation for proper processing

Send the synthesized response head from the child to the parent for nontrivial response codes. The various operator== changes are required to support the new IPDL type, unfortunately.

Comment 22

[Honza Bambas (:mayhemer)]

Josh, please (and I'm not asking the first time) add some description of how the patch works.  I don't quite follow changes in HttpChannelChild::OverrideWithSynthesizedResponse and HttpChannelParent::ChannelIntercepted.  Thanks.

Comment 23

[Josh Matthews [:jdm]]

I'm sorry, I assumed that we had covered all the necessary detail previously. OverrideWithSynthesizedResponse stores the synthesized response head; if we have a nontrivial status code we choose to continue with the original IPC request by completing the AsyncOpen code that was interrupted by the channel being intercepted. ChannelIntercepted is called when the synthesized cache entry is ready - we then instruct the channel object to store the synthesized status and headers that were sent from the child, and finally FinishSynthesizedResponse causes the channel to actually make use of the synthesized cache entry. Does that clarify the changes?

Comment 24

[Honza Bambas (:mayhemer)]

Comment on attachment 8587678 [details] [diff] [review]
Part 1: Shift some code from InterceptedChannelContent to HttpChannelChild

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

Thanks

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +2104,5 @@
> +  // In our current implementation, the FetchEvent handler will copy the
> +  // response stream completely into the pipe backing the input stream so we
> +  // can treat the available as the length of the stream.
> +  uint64_t available;
> +  rv = aSynthesizedInput->Available(&available);

nit: maybe (for safety reasons) call this before you give the stream to the pump.

Comment 25

[Honza Bambas (:mayhemer)]

Comment on attachment 8587680 [details] [diff] [review]
Part 2: Send non-200/404 synthesized responses via the parent HTTP implementation for proper processing

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

::: netwerk/protocol/http/nsHttpResponseHead.h
@@ +123,5 @@
>  
> +    bool operator==(const nsHttpResponseHead& aOther) const
> +    {
> +        return mHeaders == aOther.mHeaders &&
> +                mVersion == aOther.mVersion &&

nit: indent

Comment 26

[Josh Matthews [:jdm]]

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

Comment 27

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0f6c66c6cfe
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d9b00f78e59
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c6bb7dae971

Comment 28

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/61fe4fad0533

Comment 29

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c0f6c66c6cfe
https://hg.mozilla.org/mozilla-central/rev/1d9b00f78e59
https://hg.mozilla.org/mozilla-central/rev/5c6bb7dae971
https://hg.mozilla.org/mozilla-central/rev/61fe4fad0533