Closed
Bug 1012917
Opened 11 years ago
Closed 10 years ago
e10s: ensure we instantiate content decoder(s) as needed for diverted channels
Categories
(Core :: Networking: HTTP, defect)
Core
Networking: HTTP
Tracking
()
RESOLVED
FIXED
mozilla36
Tracking | Status | |
---|---|---|
e10s | m6+ | --- |
People
(Reporter: jduell.mcbugs, Assigned: sworkman)
References
Details
Attachments
(2 files, 2 obsolete files)
5.95 KB,
patch
|
reuben
:
review+
reuben
:
feedback+
hiro
:
feedback+
|
Details | Diff | Splinter Review |
15.00 KB,
patch
|
mayhemer
:
review+
|
Details | Diff | Splinter Review |
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)
Updated•11 years ago
|
Assignee | ||
Comment 1•11 years ago
|
||
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)
Assignee | ||
Comment 2•11 years ago
|
||
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)
Assignee | ||
Comment 3•11 years ago
|
||
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
Updated•11 years ago
|
Attachment #8426722 -
Flags: feedback?(hiikezoe) → feedback+
Comment 4•11 years ago
|
||
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.
Updated•11 years ago
|
Attachment #8426722 -
Flags: feedback?(reuben.bmo) → feedback+
Updated•11 years ago
|
Depends on: old-e10s-m2
Assignee | ||
Comment 5•11 years ago
|
||
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)
Comment 6•10 years ago
|
||
Hey Steve, any progress on this in the past couple weeks? :)
Flags: needinfo?(sworkman)
Assignee | ||
Comment 7•10 years ago
|
||
Hey Ryan, just waiting on Jason's feedback. He's a busy man :)
Flags: needinfo?(sworkman)
Comment 8•10 years ago
|
||
OK. comm-central's been perma-failing xpcshell for nearly 3 months now is why I'm asking.
Comment 9•10 years ago
|
||
Ping request on the feedback patch? It's really embarrassing that this problem has gone unresolved for so long.
Reporter | ||
Comment 10•10 years ago
|
||
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•10 years ago
|
||
(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.
Reporter | ||
Updated•10 years ago
|
Attachment #8427448 -
Flags: feedback?(jduell.mcbugs) → feedback?(honzab.moz)
Comment 12•10 years ago
|
||
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+
Comment hidden (obsolete) |
Assignee | ||
Comment 14•10 years ago
|
||
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."
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8427448 -
Attachment is obsolete: true
Attachment #8458945 -
Flags: review?(honzab.moz)
Assignee | ||
Updated•10 years ago
|
Attachment #8426722 -
Attachment description: WIP 1.0 SetApplyConversion before channel diversion → v1.0 SetApplyConversion before channel diversion
Attachment #8426722 -
Flags: review?(reuben.bmo)
Comment 16•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8458945 -
Flags: review?(honzab.moz) → review+
Assignee | ||
Comment 17•10 years ago
|
||
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
Assignee | ||
Comment 18•10 years ago
|
||
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
Assignee | ||
Comment 19•10 years ago
|
||
Comment 20•10 years ago
|
||
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
Assignee | ||
Comment 21•10 years ago
|
||
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
Updated•10 years ago
|
Blocks: old-e10s-m2
No longer depends on: old-e10s-m2
Assignee | ||
Comment 23•10 years ago
|
||
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•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1983e1fb3858
https://hg.mozilla.org/mozilla-central/rev/301272cb55b6
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•