1157283 - Make synthesized redirections open the redirected channel in the child process

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Josh Matthews [:jdm]]

From bug 1122161 comment 11:
>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?

Comment 1

[Josh Matthews [:jdm]]

Attachment: Recreate IPC redirected HTTP channels as necessary after intercepting the request in the child

Comment 2

[Josh Matthews [:jdm]]

Comment on attachment 8601778 [details] [diff] [review]
Recreate IPC redirected HTTP channels as necessary after intercepting the request in the child

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

First, here are the tests that pass with these changes:
1) synthesizing a 302 to a real file
2) synthesizing a 302 to another synthesized response
3) synthesizing a 302 to #1
4) synthesizing a 302 to #2
5) fetching a 302 to #2, which yields a 404 since it should not be intercepted

With these changes, when a redirect is initiated in the parent from a synthesized response (eg. a synthesized 302) we intercept the redirected channel and suspend any further processing. The redirection completion notification in the child then destroys the IPC actor and performs a real AsyncOpen, which runs the usual SW interception - this will only end up recreating the IPC actor if a nontrivial response is synthesized (ie. one that requires processing in by the parent, such as a redirection). Additionally, these changes ensure that parent channels are either always forced to intercept (when a synthesized redirection occurred, or the parent is processing a synthesized response) or always forced to skip an interception check. This ensures that we follow the spec, which says that after a real network request occurs there must be no more opportunities to intercept redirections.

Comment 3

[Honza Bambas (:mayhemer)]

Comment on attachment 8601778 [details] [diff] [review]
Recreate IPC redirected HTTP channels as necessary after intercepting the request in the child

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

Not tested locally.

r=honzab

::: netwerk/protocol/http/HttpChannelParent.h
@@ +205,5 @@
>  
> +  // Set if this channel should be intercepted before it sets up the HTTP transaction.
> +  bool mShouldIntercept;
> +  // Set if this channel should suspend on interception.
> +  bool mShouldSuspendIntercept;

when you are here, could you convert these to bit fields?

::: netwerk/protocol/http/nsIHttpChannelChild.idl
@@ +13,5 @@
>    void addCookiesToRequest();
>  
> +  // Mark this channel as requiring an interception; this will propagate
> +  // to the corresponding parent channel when a redirect occurs.
> +  void forceIntercepted();

change iid!

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

Josh, which versions of Firefox are affected by this?

Comment 5

[Josh Matthews [:jdm]]

Any version in which ServiceWorkers are enabled by default. That usually means 40+.

Comment 6

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

Ehsan flipped serviceworkers off again (on nightly builds too), so this doesn't affect anything right now unless users flip the pref

Comment 7

[Josh Matthews [:jdm]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7a3796f457a

Comment 8

[Josh Matthews [:jdm]]

Attachment: Avoid leaking the world when tearing down an intercepted redirected channel

Running this through try showed that we leak the world for redirected, intercepted channels. This can be avoided by ensuring that the InterceptedChannelChrome is notified when the channel is torn down, but we can't do it any earlier because cancelling the channel will cause IPC notifications to be sent and confuse the child. This is the only solution I've been able to come up with.

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 8611186 [details] [diff] [review]
Avoid leaking the world when tearing down an intercepted redirected channel

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

Sounds good to me.

Comment 10

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2c2577ac49c

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b2c2577ac49c