1465587 - propagate internal redirects initiated by an InterceptedHttpChannel back to the child process

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, enhancement, P2)

Description

[Ben Kelly [:bkelly, not reviewing]]

In bug 1391693 comment 62 I added some code that "hid" the internal redirect from nsHttpChannel to InterceptedHttpChannel from the child process's HttpChannelChild.  This was necessary to avoid confusing the complicated legacy service worker interception code that operates in the child process.

Unfortunately it seems that the code I added was a bit to aggressive in hiding internal redirects.  When a service worker intercepts with an opaque response we need to do an internal redirect to the new URL.  This goes from InterceptedHttpChannel to InterceptedHttpChannel.  In this case we *do* want to allow the redirect through to the child process so downstream listeners can act appropriately.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Only hide the initial internal redirect to an InterceptedHttpChannel and not internal redirects initiated from IHC. r=mayhemer

Honza, please see comment 0 for some background here.  Basically we have an ugly work around to make parent-side intercept work while still keeping the complicated child-side intercept code around.  This work around involves hiding an internal redirect from nsHttpChannel to InterceptedHttpChannel.  This code was also accidentally blocking internal redirects from InterceptedHttpChannel to another InterceptedHttpChannel.  This patch corrects that issue.

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 8982009 [details] [diff] [review]
Only hide the initial internal redirect to an InterceptedHttpChannel and not internal redirects initiated from IHC. r=mayhemer

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

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +1773,5 @@
> +    // We only want to hide the initial internal redirect from nsHttpChannel
> +    // to InterceptedHttpChannel.  We want to allow through internal redirects
> +    // initiated from the InterceptedHttpChannel even if they are to another
> +    // InterceptedHttpChannel.
> +    if (!oldIntercepted && newIntercepted) {

what has to happen when the redirects go as (if even possible) httpchannel -> interceptchannel -> httpchannel -> interceptchannel?

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Honza Bambas (:mayhemer) from comment #2)
> what has to happen when the redirects go as (if even possible) httpchannel
> -> interceptchannel -> httpchannel -> interceptchannel?

This can happen.  For example:

1. Navigation A creates nsHttpChannel
2. Navigation A is intercepted, causing a hidden internal redirect to InterceptedHttpChannel
3. The service worker returns a 30x redirect response to URL B
4. The InterceptedHttpChannel performs a real redirect back to a new nsHttpChannel for URL B
5. The B nsHttpChannel is intercepted again, causing a hidden internal redirect to InterceptedHttpChannel

So each internal redirect from nsHttpChannel->InterceptedHttpChannel is hidden from the child process.  Redirects produced by the InterceptedHttpChannel are never hidden.

Hope that explains.

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Ben Kelly [:bkelly] from comment #3)
> (In reply to Honza Bambas (:mayhemer) from comment #2)
> > what has to happen when the redirects go as (if even possible) httpchannel
> > -> interceptchannel -> httpchannel -> interceptchannel?
> 
> This can happen.  For example:
> 
> 1. Navigation A creates nsHttpChannel
> 2. Navigation A is intercepted, causing a hidden internal redirect to
> InterceptedHttpChannel
> 3. The service worker returns a 30x redirect response to URL B
> 4. The InterceptedHttpChannel performs a real redirect back to a new
> nsHttpChannel for URL B
> 5. The B nsHttpChannel is intercepted again, causing a hidden internal
> redirect to InterceptedHttpChannel
> 
> So each internal redirect from nsHttpChannel->InterceptedHttpChannel is
> hidden from the child process.  Redirects produced by the
> InterceptedHttpChannel are never hidden.
> 
> Hope that explains.

Thanks.  Then I think the "the initial internal redirect" words in the comment should be changed.  That was what I was confused with, probably.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Only hide the initial internal redirect to an InterceptedHttpChannel and not internal redirects initiated from IHC. r=mayhemer

Sure.  This does s/initial/special/ in that comment.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e6bcb2cd0d8
Only hide the initial internal redirect to an InterceptedHttpChannel and not internal redirects initiated from IHC. r=mayhemer

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/7e6bcb2cd0d8