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

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jduell.mcbugs, Assigned: sworkman)

Tracking

unspecified
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm6+)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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)
Blocks: 984194
Blocks: core-e10s
tracking-e10s: --- → ?
OS: Linux → All
Hardware: x86_64 → All
Base patch to call SetApplyConversion before the channel is diverted back to the child.

Based off of :hiro's work in bug 984194.
Assignee: nobody → sworkman
Status: NEW → ASSIGNED
Attachment #8426722 - Flags: feedback?(reuben.bmo)
Attachment #8426722 - Flags: feedback?(hiikezoe)
Jason, I think the patch is mostly done; it builds but I haven't tested it yet, so just setting feedback? for now.
Attachment #8426723 - Flags: feedback?(jduell.mcbugs)
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
Attachment #8426722 - Flags: feedback?(hiikezoe) → feedback+
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.
Attachment #8426722 - Flags: feedback?(reuben.bmo) → feedback+
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
Attachment #8426723 - Attachment is obsolete: true
Attachment #8426723 - Flags: feedback?(jduell.mcbugs)
Attachment #8427448 - Flags: feedback?(jduell.mcbugs)
Hey Steve, any progress on this in the past couple weeks? :)
Flags: needinfo?(sworkman)
Hey Ryan, just waiting on Jason's feedback. He's a busy man :)
Flags: needinfo?(sworkman)
OK. comm-central's been perma-failing xpcshell for nearly 3 months now is why I'm asking.
Ping request on the feedback patch? It's really embarrassing that this problem has gone unresolved for so long.
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.
(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.
Attachment #8427448 - Flags: feedback?(jduell.mcbugs) → feedback?(honzab.moz)
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
Attachment #8427448 - Flags: feedback?(honzab.moz) → feedback+
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."
Attachment #8426722 - Attachment description: WIP 1.0 SetApplyConversion before channel diversion → v1.0 SetApplyConversion before channel diversion
Attachment #8426722 - Flags: review?(reuben.bmo)
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
Attachment #8426722 - Flags: review?(reuben.bmo) → review+
Attachment #8458945 - Flags: review?(honzab.moz) → review+
Blocks: old-e10s-m2
No longer depends on: old-e10s-m2
Move old M2's low-priority bugs to M6 milestone.
Blocks: 1084997
https://hg.mozilla.org/mozilla-central/rev/1983e1fb3858
https://hg.mozilla.org/mozilla-central/rev/301272cb55b6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.