1438935 - Serialize source (old) channel's load info from parent to child via SendRedirect1Begin

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement, P2)

Description

[Honza Bambas (:mayhemer)]

Reason: when the old channel's load info is modified between it's creation and redirection, _on child_ we still use the load info as it was seen at the time of creation to clone it for the redirect target channel.

Other option is to carry the new channel load info and to not clone it on the child (just use the one we have created on the parent).

The update of load info is needed to pass some redirect security checks.


(Side note: I'm not a big fan of doing this, but if I can't think of any better and more consistent approach, let's do it)

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Here is some code getting individually sent back in OnStartRequest... it could be replaced with something like this:

https://searchfox.org/mozilla-central/rev/74b7ffee403c7ffd05b8b476c411cbf11d134eb9/netwerk/protocol/http/HttpChannelParent.cpp#1537-1552

https://searchfox.org/mozilla-central/rev/74b7ffee403c7ffd05b8b476c411cbf11d134eb9/netwerk/protocol/http/HttpChannelChild.cpp#622-627

Comment 2

[Honza Bambas (:mayhemer)]

We may also do the same thing once more through OnStartRequest (Ben is already passing some args to affect load info)

Comment 3

[Honza Bambas (:mayhemer)]

(In reply to Ben Kelly [:bkelly] from comment #1)
> Here is some code getting individually sent back in OnStartRequest... it
> could be replaced with something like this:
> 
> https://searchfox.org/mozilla-central/rev/
> 74b7ffee403c7ffd05b8b476c411cbf11d134eb9/netwerk/protocol/http/
> HttpChannelParent.cpp#1537-1552
> 
> https://searchfox.org/mozilla-central/rev/
> 74b7ffee403c7ffd05b8b476c411cbf11d134eb9/netwerk/protocol/http/
> HttpChannelChild.cpp#622-627

If the descriptor can be serialized inside background utils for load info, then yes, this could be removed from here and just serialize the whole LI object down.

Comment 4

[Valentin Gosu [:valentin] (he/him)]

Do you want to take the bug, or can you suggest an assignee?

Comment 5

[Honza Bambas (:mayhemer)]

I'm not sure about a priority here.... probably P2, since from time to time I can see new code added being dependent on some flags taken from loadinfo that are not found on child, leading to expanding the args of onstart/redirectbegin in ipdlh.

I can work on this bug, but I would like not to right now...

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Honza Bambas (:mayhemer) from comment #5)
> I'm not sure about a priority here.... probably P2, since from time to time
> I can see new code added being dependent on some flags taken from loadinfo
> that are not found on child, leading to expanding the args of
> onstart/redirectbegin in ipdlh.

I think fixing this bug will bubble up some problems we are facing. I would like to see that happening. If Necko does not have the resources, I can find someone in sec-eng to fix that. Let me know.

Comment 7

[Honza Bambas (:mayhemer)]

As this is work for just few hours, I can take it and do it.

Comment 8

[Honza Bambas (:mayhemer)]

So, I had to realize this was not a work for just few hours...  The patch is simple, yes, but figure out all the issue it may bring is another thing..

First issue I found: when I update load info on the "old" child http channel during a redirect (@HttpChannelChild::Redirect1Begin) the LoadingNode() on is rewritten (actually nullified).  That apparently breaks number of things.

Hence, we will probably need to copy only some of the properties instead of doing plain mLoadInfo = aLoadInfo;

Comment 9

[Honza Bambas (:mayhemer)]

I'll go even more conservative way.  Trying to copy everything from parent load info but few properties would probably be an endless fight.  I rather decided to go the opposite way - build a list of which properties should be taken from parent when channel's send down their data to child processes.

I so far found out following properties can't be copied just that:

* mLoadingContext + mContextForTopLevelLoad, both are constructed and existing solely on the child process, parent load info usually has these null
* mController, bug 1439879
* mPerformanceStorage, collected on child, parent's is null, if there is anything to merge, then probably items that are missing on child (not implemented in the patch)
* mReservedClientSource
* mLoadingPrincipal + mTriggeringPrincipal, since those can update on child for data: channels while parent keeps them as NullPrincipals

The list is probably not complete...

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: v1

see comment 9 for the rational.  this is basically just a foundation of something we can easily extend on.  the patch otherwise doesn't change the behavior of the current code base at all.

Comment 11

[Honza Bambas (:mayhemer)]

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

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8955214 [details] [diff] [review]
v1

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

Seems ok.  Just a couple thoughts:

1) Comment 9 suggested moving mReservedClientSource.  That should never be necessary.  In e10s mode this attribute should never be set in the parent.
2) Your copying the entire LoadInfo across the wire only to save a small portion of it.  That seems a bit wasteful, but also probably not critical.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8955214 [details] [diff] [review]
v1

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

Thanks for taking on the work. That looks good to me, besides the only question I marked inline.

::: netwerk/protocol/http/PHttpChannel.ipdl
@@ +96,5 @@
>    async OnStartRequest(nsresult            channelStatus,
>                         nsHttpResponseHead  responseHead,
>                         bool                useResponseHead,
>                         nsHttpHeaderArray   requestHeaders,
> +                       OptionalLoadInfoArgs loadInfo,

Why does that need to be optional? Don't we have a loadinfo on all channels by now? If not, please let me know and I'll take a closer look at that. Every channel should have a loadinfo by now.

Comment 14

[Honza Bambas (:mayhemer)]

(In reply to Ben Kelly [:bkelly] from comment #12)
> Comment on attachment 8955214 [details] [diff] [review]
> v1
> 
> Review of attachment 8955214 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems ok.  Just a couple thoughts:
> 
> 1) Comment 9 suggested moving mReservedClientSource.  That should never be
> necessary.  In e10s mode this attribute should never be set in the parent.

It doesn't suggest anything like that.  It's listed there as something that _cannot be copied_ from parent to child.  (I just don't remember the reason why.)

> 2) Your copying the entire LoadInfo across the wire only to save a small
> portion of it.  That seems a bit wasteful, but also probably not critical.

Good point.  We may have a new object like LoadInfoChildForwarder that would have it's own serialization code and pick only those properties that we want to carry down.  That it better performing (important) but puts a burden to update the serialization code side by the helper class.  How many times have you missed to update BackgroundUtils to carry new properties of load info?  Me at least once :)

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #13)
> Comment on attachment 8955214 [details] [diff] [review]
> v1
> 
> Review of attachment 8955214 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for taking on the work. That looks good to me, besides the only
> question I marked inline.
> 
> ::: netwerk/protocol/http/PHttpChannel.ipdl
> @@ +96,5 @@
> >    async OnStartRequest(nsresult            channelStatus,
> >                         nsHttpResponseHead  responseHead,
> >                         bool                useResponseHead,
> >                         nsHttpHeaderArray   requestHeaders,
> > +                       OptionalLoadInfoArgs loadInfo,
> 
> Why does that need to be optional? Don't we have a loadinfo on all channels
> by now? If not, please let me know and I'll take a closer look at that.
> Every channel should have a loadinfo by now.

Don't know about any non-opt version for LI IPC args.  If LI is ensured everywhere, please file a new bug to maybe remove/change the IPC args class (if there is not one already).


Thanks both for your feedback!

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Honza Bambas (:mayhemer) from comment #14)
> > 1) Comment 9 suggested moving mReservedClientSource.  That should never be
> > necessary.  In e10s mode this attribute should never be set in the parent.
> 
> It doesn't suggest anything like that.  It's listed there as something that
> _cannot be copied_ from parent to child.  (I just don't remember the reason
> why.)

Sorry.  I misread comment 9.

Just FYI, mClientInfo, mReservedClientInfo, and mInitialClientInfo are all "one way" from child-to-parent.  They should never change in the parent.

The mReservedClientSource, when present, is related to the mReservedClientInfo.  It should never exist in the parent process when the channel was initiated in the child process, though.

Comment 16

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #14)
> Don't know about any non-opt version for LI IPC args.  If LI is ensured

ah, we have LoadInfoArgs struct!  anyway, going to into a brand new IPC struct

Comment 17

[Honza Bambas (:mayhemer)]

Attachment: v2

- introduces ParentLoadInfoForwarderArgs keeping a subset of properties we want to  carry down
- introduces BackgroundUtils!LoadInfoToParentLoadInfoForwarder that fills it on the parent process from a parent channel load info
- introduces BackgroundUtils!MergeParentLoadInfoForwarder that replaces properties from ParentLoadInfoForwarderArgs on child channel's load info

otherwise as the v1 patch; doesn't change the current functionality

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

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8955549 [details] [diff] [review]
v2

Sorry, I'm going on parental leave a bit early.  Can you find another reviewer?  My assertions are landed for service workers, so as long as try is green I think this should be fine in regards to the controller.

Comment 19

[Honza Bambas (:mayhemer)]

Comment on attachment 8955549 [details] [diff] [review]
v2

Andrew, can you review this please?  see comment 17 for details.

Comment 20

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8955549 [details] [diff] [review]
v2

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

This makes sense to me and satisfies bkelly's concerns about redundant propagation of data; this is absolutely the minimal set of things to propagate! :)

::: netwerk/ipc/NeckoChannelParams.ipdlh
@@ +120,5 @@
> + * too huge and we only care about small subpart of properties anyway.
> + */
> +struct ParentLoadInfoForwarderArgs
> +{
> +  bool                        allowInsecureRedirectToDataURI;

Suggest adding an explanatory comment to provide context here like:
WebExtextensions' WebRequest API allows extensions to intercept and redirect a channel to a data URI.  This modifications happens in the parent and needs to be mirrored to the child so that security checks can pass.

@@ +124,5 @@
> +  bool                        allowInsecureRedirectToDataURI;
> +
> +  // IMPORTANT: when you add new properites here you must also update
> +  // LoadInfoToParentLoadInfoForwarder and MergeParentLoadInfoForwarder
> +  // in BackgroundUtils.ccp/.h!

typo nit: s/ccp/cpp/

Comment 21

[Honza Bambas (:mayhemer)]

Attachment: v2.1

Comment 22

[Honza Bambas (:mayhemer)]

thanks for a quick r!

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/501e74dd33da
Serialize selected LoadInfo properties from HTTPChannelParent to HTTPChannelChild through OnStartRequest and Redirect1Begin, r=asuth

Comment 24

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/501e74dd33da