1012917 - e10s: ensure we instantiate content decoder(s) as needed for diverted channels

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Jason Duell]

See bug 984194 (this is just a fork so we don't have all the builbot spam).

In both nsHttpChannel/HttpChannelChild we call ApplyContentConversions() right after the Listener's OnStartRequest is called, so we can honor nsIEncodedChannel.applyConversion by setting up decoders as needed (this has to happen before OnData gets called, as the converters insert themselves into the listener chain).

If we divert a channel back to the parent we're failing to set up these converters, so (for instance) the client is getting gzip'd content even when the server has sent the Content-Encoding header.  That's a bug.  I'm not sure how common it is for the Download manager (I'm guessing most dowloads want to keep gzip, etc format so servers don't set Content-Encoding) but it could easily mess up external app handlers (not sure we have any in Firefox OS?  But we definitely will in desktop e10s).

The Fix: we need to set up the needed encoders in the parent, and send both the diverted data and then the remaining nsHttpChannel data to it.

1) Check if mApplyConversion is set in the child when we divert, and send the boolean in the IPDL msg to the parent
2) HttpChannelParent should call HttpBaseChannel::ApplyContentConversions on the original nsHttpChannel.  It needs to be that *after* it has changed the channel's mListener to be the listener that we're diverting to.
3) If I'm scanning the code correctly, we can then simply have HttpChannelParent get the nsHttpChannel.mListener, and set HttpChannelParentListener.mNextListener to it, and things should "just work".  Make sure to update the comment at 

   http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelParentListener.h#46

  // or some other listener that we have been diverted to via
  // nsIDivertableChannel, (or content encoders that sit in front of that diverted-to listener)

Comment 1

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 SetApplyConversion before channel diversion

Base patch to call SetApplyConversion before the channel is diverted back to the child.

Based off of :hiro's work in bug 984194.

Comment 2

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 1.0 Send HttpBaseChannel::mApplyConversion with ChannelDiverter constructor in IPDL

Jason, I think the patch is mostly done; it builds but I haven't tested it yet, so just setting feedback? for now.

Comment 3

[Steve Workman [:sworkman] (INACTIVE)]

Pushed to try to get some builds (for Linux, Win and Mac), and to get some test results.

https://tbpl.mozilla.org/?tree=Try&rev=db7060ca0266

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8426723 [details] [diff] [review]
WIP 1.0 Send HttpBaseChannel::mApplyConversion with ChannelDiverter constructor in IPDL

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

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +919,5 @@
> +  nsCOMPtr<nsIStreamListener> listener = mChannel->GetListener();
> +  if (listener != mParentListener) {
> +    mConverterListener = listener.forget();
> +  }
> +

mListener in HttpBaseChannel is null at this timing so ApplyContentConversions() fails. Then stream converter is not prepared for conversion there.

Comment 5

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 1.1 Send HttpBaseChannel::mApplyConversion with ChannelDiverter constructor in IPDL

Updated based on hiro's point in comment 4.

Changed ApplyContentConversions:
-- Called without params, it will behave as it currently does.
-- Called with params (from HttpChannelParent), it will use HttpBaseChannel::mListener if available; otherwise it will use the passed in listener - HttpChannelParent will pass in its mParentListener for this alternate case. It will build the listener/decoder chain as normal, update HttpBaseChannel::mListener, and return a pointer so that HttpChannelParent can update mParentListener.

I'm not 100% happy with the API here, but it's a little more focused than adding a Get~ and SetListener.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=d6ce8b99a928

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Hey Steve, any progress on this in the past couple weeks? :)

Comment 7

[Steve Workman [:sworkman] (INACTIVE)]

Hey Ryan, just waiting on Jason's feedback. He's a busy man :)

Comment 8

[Ryan VanderMeulen [:RyanVM]]

OK. comm-central's been perma-failing xpcshell for nearly 3 months now is why I'm asking.

Comment 9

[Joshua Cranmer [:jcranmer]]

Ping request on the feedback patch? It's really embarrassing that this problem has gone unresolved for so long.

Comment 10

[Jason Duell]

Yes, I'm going to review this...  No it doesn't break anything but a thunderbird test case right now (and not Thunderbird itself, which doesn't do e10s and certainly not Divert().)

We should figure out as part of this bug why test_encoding.js is only failing under the thunderbird e10s test, and not mozilla-central.

Comment 11

[Joshua Cranmer [:jcranmer]]

(In reply to Jason Duell (:jduell) from comment #10)
> We should figure out as part of this bug why test_encoding.js is only
> failing under the thunderbird e10s test, and not mozilla-central.

It's only enabled if MOZ_JSDOWNLOADS isn't set, i.e., not Firefox or B2G. The test is specifically disabled for Android, so that only leaves comm-central projects running it.

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 8427448 [details] [diff] [review]
WIP 1.1 Send HttpBaseChannel::mApplyConversion with ChannelDiverter constructor in IPDL

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

I think the approach is good.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +618,5 @@
>    // order. This is accomplished because the converter chain ends up
>    // being a stack with the last converter created being the first one
>    // to accept the raw network data.
>  
> +  char *cePtr, *val;

what's this for?

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1650,5 @@
>    mDivertingToParent = true;
>  
> +  HttpChannelDiverterArgs args;
> +  args.mChannelChild() = this;
> +  args.mApplyConversion() = mApplyConversion;

only concern is - are we here past the point of call of nsExternalAppHandler::OnStartRequest that sets (actually drops in some cases) mApplyConversion?

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +929,5 @@
> +  // Create a content conversion chain based on mParentListener.
> +  // Get ptr to final listener and set that in mContentConverter.
> +
> +  nsCOMPtr<nsIStreamListener> converterListener;
> +  mChannel->ApplyContentConversions(mParentListener,

Isn't mListener on mChannel already set here?  I just don't much follow rational for the first arg of this method.

::: netwerk/protocol/http/HttpChannelParent.h
@@ +154,5 @@
>    nsCOMPtr<nsILoadContext> mLoadContext;
>    nsRefPtr<nsHttpHandler>  mHttpHandler;
>  
>    nsRefPtr<HttpChannelParentListener> mParentListener;
> +  // Set if the nsHttpChannel has been requested to apply content conversion.

some richer comment would be good

Comment 13

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Honza Bambas (:mayhemer) from comment #12)
> Comment on attachment 8427448 [details] [diff] [review]
> WIP 1.1 Send HttpBaseChannel::mApplyConversion with ChannelDiverter
> constructor in IPDL
> 
> Review of attachment 8427448 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think the approach is good.
> 
> ::: netwerk/protocol/http/HttpBaseChannel.cpp
> @@ +618,5 @@
> >    // order. This is accomplished because the converter chain ends up
> >    // being a stack with the last converter created being the first one
> >    // to accept the raw network data.
> >  
> > +  char *cePtr, *val;
> 
> what's this for?

I just moved these closer to where they were first used.
> 
> ::: netwerk/protocol/http/HttpChannelChild.cpp
> @@ +1650,5 @@
> >    mDivertingToParent = true;
> >  
> > +  HttpChannelDiverterArgs args;
> > +  args.mChannelChild() = this;
> > +  args.mApplyConversion() = mApplyConversion;
> 
> only concern is - are we here past the point of call of
> nsExternalAppHandler::OnStartRequest that sets (actually drops in some
> cases) mApplyConversion?
> 
> ::: netwerk/protocol/http/HttpChannelParent.cpp
> @@ +929,5 @@
> > +  // Create a content conversion chain based on mParentListener.
> > +  // Get ptr to final listener and set that in mContentConverter.
> > +
> > +  nsCOMPtr<nsIStreamListener> converterListener;
> > +  mChannel->ApplyContentConversions(mParentListener,
> 
> Isn't mListener on mChannel already set here?  I just don't much follow
> rational for the first arg of this method.
> 
> ::: netwerk/protocol/http/HttpChannelParent.h
> @@ +154,5 @@
> >    nsCOMPtr<nsILoadContext> mLoadContext;
> >    nsRefPtr<nsHttpHandler>  mHttpHandler;
> >  
> >    nsRefPtr<HttpChannelParentListener> mParentListener;
> > +  // Set if the nsHttpChannel has been requested to apply content conversion.
> 
> some richer comment would be good

Comment 14

[Steve Workman [:sworkman] (INACTIVE)]

Uh oops. I hit "Save Changes" way too early on comment 13, sorry. I tagged it as obsolete.

(In reply to Honza Bambas (:mayhemer) from comment #12)
> Comment on attachment 8427448 [details] [diff] [review]
> WIP 1.1 Send HttpBaseChannel::mApplyConversion with ChannelDiverter
> constructor in IPDL
> 
> Review of attachment 8427448 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think the approach is good.
> 
> ::: netwerk/protocol/http/HttpBaseChannel.cpp
> @@ +618,5 @@
> >    // order. This is accomplished because the converter chain ends up
> >    // being a stack with the last converter created being the first one
> >    // to accept the raw network data.
> >  
> > +  char *cePtr, *val;
> 
> what's this for?

I just moved these to be closer to where they're first used.

> ::: netwerk/protocol/http/HttpChannelChild.cpp
> @@ +1650,5 @@
> >    mDivertingToParent = true;
> >  
> > +  HttpChannelDiverterArgs args;
> > +  args.mChannelChild() = this;
> > +  args.mApplyConversion() = mApplyConversion;
> 
> only concern is - are we here past the point of call of
> nsExternalAppHandler::OnStartRequest that sets (actually drops in some
> cases) mApplyConversion?

This should still be in the context of ExternalAppHelperChild::OnStartRequest, and called just after MaybeApplyDecodingForExtension.

> ::: netwerk/protocol/http/HttpChannelParent.cpp
> @@ +929,5 @@
> > +  // Create a content conversion chain based on mParentListener.
> > +  // Get ptr to final listener and set that in mContentConverter.
> > +
> > +  nsCOMPtr<nsIStreamListener> converterListener;
> > +  mChannel->ApplyContentConversions(mParentListener,
> 
> Isn't mListener on mChannel already set here?  I just don't much follow
> rational for the first arg of this method.

mListener may already have been released at this stage if OnStop was already sent and enqueued on the child. So we pass in mParentListener to ensure that we're start the listener/decoder chain from HttpChannelParentListener. Make sense?

> ::: netwerk/protocol/http/HttpChannelParent.h
> @@ +154,5 @@
> >    nsCOMPtr<nsILoadContext> mLoadContext;
> >    nsRefPtr<nsHttpHandler>  mHttpHandler;
> >  
> >    nsRefPtr<HttpChannelParentListener> mParentListener;
> > +  // Set if the nsHttpChannel has been requested to apply content conversion.
> 
> some richer comment would be good

How about: "The first listener in the decode chain if channel decoding is applied."

Comment 15

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.2 Send HttpBaseChannel::mApplyConversion with ChannelDiverter constructor in IPDL

Comment 16

[Reuben Morais [:reuben]]

Comment on attachment 8426722 [details] [diff] [review]
v1.0 SetApplyConversion before channel diversion

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

Thanks for fixing this, Steve.

::: uriloader/exthandler/ExternalHelperAppChild.cpp
@@ +98,2 @@
>  {
> +  // nsIDivertable must know about content conversions before before being

Shouldn't this say nsIRequest (or nsIEncodedChannel)? And "before" is duplicated.

::: uriloader/exthandler/nsExternalHelperAppService.cpp
@@ +1493,5 @@
> +                                                   &applyConversion);
> +          }
> +        }
> +      }
> +    }    

nit: whitespace

@@ +1564,5 @@
>    // Con: Uncompressed data means more IPC overhead.
>    // Pros: ExternalHelperAppParent doesn't need to implement nsIEncodedChannel.
>    //       Parent process doesn't need to expect CPU time on decompression.
> +  MaybeApplyDecodingForExtension(aChannel);
> +  

nit: whitespace

Comment 17

[Steve Workman [:sworkman] (INACTIVE)]

Had to remove some bitrot here; pushing to try to make sure no regressions:

https://tbpl.mozilla.org/?tree=Try&rev=3f3f7c91c3d8
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3f3f7c91c3d8

Comment 18

[Steve Workman [:sworkman] (INACTIVE)]

Ugh - sorry, wrong try statement. New push:

https://tbpl.mozilla.org/?tree=Try&rev=eacc9a66d527
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=eacc9a66d527

Comment 19

[Steve Workman [:sworkman] (INACTIVE)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f94e060bac3

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Backed out for apparently breaking B2G marionette-webapi.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a25309077ac7

https://tbpl.mozilla.org/php/getParsedLog.php?id=46959067&tree=Mozilla-Inbound

Comment 21

[Steve Workman [:sworkman] (INACTIVE)]

Re-pushed to try with a full unit test run for all default platforms.

https://tbpl.mozilla.org/?tree=Try&rev=8d6bc4095ac6
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8d6bc4095ac6

Comment 22

[Chris Peterson [:cpeterson]]

Move old M2's low-priority bugs to M6 milestone.

Comment 23

[Steve Workman [:sworkman] (INACTIVE)]

Sorry for the delay on this!

try run from today:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7b33ce05ac91
Some intermittents, but they all look to be unrelated ...

So, I landed it on m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1983e1fb3858
https://hg.mozilla.org/integration/mozilla-inbound/rev/301272cb55b6

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1983e1fb3858
https://hg.mozilla.org/mozilla-central/rev/301272cb55b6