Redirection does not work with Service Worker interception

RESOLVED FIXED in Firefox 40

Status

()

Core
DOM: Workers
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: nsm, Assigned: jdm)

Tracking

33 Branch
mozilla40
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(3 attachments, 5 obsolete attachments)

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")))
Created attachment 8569961 [details] [diff] [review]
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.
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
Assignee: nsm.nikhil → nobody
(Assignee)

Comment 2

3 years ago
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?
Flags: needinfo?(honzab.moz)
(Assignee)

Updated

3 years ago
Depends on: 1134462
(Assignee)

Comment 3

3 years ago
Created attachment 8576093 [details] [diff] [review]
Non-e10s proof-of-concept fix

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

Updated

3 years ago
Assignee: nobody → josh
(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..
Flags: needinfo?(honzab.moz) → needinfo?(josh)
(Assignee)

Comment 5

3 years ago
Aha! You're right, it works now in non-e10s since bug 1142124 was fixed.
Flags: needinfo?(josh)
(Assignee)

Updated

3 years ago
Attachment #8576093 - Attachment is obsolete: true
Is this then fixed or invalid or what? :)
(Assignee)

Comment 7

3 years ago
e10s is still broken.
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)

Updated

3 years ago
Depends on: 1093357
(Assignee)

Comment 9

3 years ago
Created attachment 8582192 [details] [diff] [review]
tests and partial fix
(Assignee)

Comment 10

3 years ago
Created attachment 8582193 [details] [diff] [review]
Part 1: Shift some code from InterceptedChannelContent to HttpChannelChild
Attachment #8582193 - Flags: review?(honzab.moz)
(Assignee)

Comment 11

3 years ago
Created attachment 8582194 [details] [diff] [review]
Part 2: Send non-200/404 synthesized responses via the parent HTTP implementation for proper processing
Attachment #8582194 - Flags: review?(honzab.moz)
(Assignee)

Updated

3 years ago
Attachment #8569961 - Attachment is obsolete: true
(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.
Flags: needinfo?(josh)
(Assignee)

Comment 13

3 years ago
(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.
Flags: needinfo?(josh) → needinfo?(honzab.moz)
(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.
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 15

3 years ago
Oh, I didn't think of that. If we can ignore the body for the responses, that will definitely be easier.
(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 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?
Attachment #8582193 - Flags: review?(honzab.moz)
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.
Attachment #8582194 - Flags: review?(honzab.moz)
(Assignee)

Comment 19

3 years ago
Created attachment 8587677 [details] [diff] [review]
Part 0: Test for synthesized redirects
(Assignee)

Updated

3 years ago
Attachment #8582192 - Attachment is obsolete: true
(Assignee)

Comment 20

3 years ago
Created attachment 8587678 [details] [diff] [review]
Part 1: Shift some code from InterceptedChannelContent to HttpChannelChild

This is just a simple code reorganization to make part 2 easier to read.
Attachment #8587678 - Flags: review?(honzab.moz)
(Assignee)

Updated

3 years ago
Attachment #8582193 - Attachment is obsolete: true
(Assignee)

Comment 21

3 years ago
Created attachment 8587680 [details] [diff] [review]
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.
Attachment #8587680 - Flags: review?(honzab.moz)
(Assignee)

Updated

3 years ago
Attachment #8582194 - Attachment is obsolete: true
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.
(Assignee)

Comment 23

3 years ago
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 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.
Attachment #8587678 - Flags: review?(honzab.moz) → review+
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
Attachment #8587680 - Flags: review?(honzab.moz) → review+

Updated

3 years ago
Blocks: 1122161
You need to log in before you can comment on or make changes to this bug.