982319 - [e10s] Network monitor and webconsole need to decode gzipped response bodies in the client

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P2)

Description

[Mihai Sucan [:msucan]]

If the network request being logged has a gzipped response body we show the raw encoded content in the network monitor and in the webconsole netpanel. This doesnt happen with Firefox for desktops because the content is decoded before being logged. I've seen this happen only with b2g. See bug 917227.

We need to detect encoded content and decode it in the client.

Comment 1

[Chris Peterson [:cpeterson]]

Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".

Comment 5

[Chris Peterson [:cpeterson]]

These DevTools bugs should probably block e10s from riding to Aurora.

Comment 6

[Chris Peterson [:cpeterson]]

devtools bugs will block the e10s merge to Aurora, but blassey would like them to be tracked by dte10s meta bug 875871, not the tracking-e10s=m6+ flag.

Comment 8

[Gabor Krizsanits (INACTIVE)]

Hey Jason, I've seen your name on related bugs so I though you might be the best person to needinfo about this issue...

So the problem is that network monitor gets the zipped data with e10s and
the unzipped without, here is what's going on under the hood:

When we get the callback for the ACTIVITY_SUBTYPE_RESPONSE_HEADER case from our nsIHttpActivityObserver we set up a nsIStreamListenerTee to listen to the channel we recieve. Problem is that for the the e10s case the channel is the original channel and not the decoded one (so the data is still zipped)

The code that fires the mentioned callback is: http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpTransaction.cpp#330

A stack for the e10s case is at the bottom of this comment.

There are so many layers in netwerk that I've got confused (like proxies...), I guess we call DoApplyContentConversions to late in the e10s case, not sure why. Either HttpChannelParent that creates the actual channel, does not do the conversions, except it just send the raw data to HttpChannelChild which does the coversion, but the callback is called by the parent. Or HttpChannelParent does the coversion, only it does it too late, after the transaction already did the callback. I need help from someone who is expert in this area.

So I would like to know which is the case and how should we fix it. Probably there is a quick and dirty hack around to just use nsIStreamConverterService from the network monitor and do the coversion from js if e10s on, but I doubt that would be the right way to fix this, and would like to avoid adding even more hack to this already overcomplicated setup.

The above mentioned stack:
#0  mozilla::net::nsHttpTransaction::Init (this=0x7fffc2dd8300, caps=41, 
    cinfo=0x7fffc1e85d00, requestHead=0x7fffc3ac3120, requestBody=0x0, 
    requestBodyHasHeaders=false, target=0x7fffe447f400, 
    callbacks=0x7fffbf775d40, eventsink=0x7fffc3ac3420, 
    responseBody=0x7fffffffae80)
    at /home/gabor/development/mozilla-central/netwerk/protocol/http/nsHttpTransaction.cpp:330
#1  0x00007fffee10b920 in mozilla::net::nsHttpChannel::SetupTransaction (
    this=0x7fffc3ac3000)
    at /home/gabor/development/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:835
#2  0x00007fffee109dc7 in mozilla::net::nsHttpChannel::ContinueConnect (
    this=0x7fffc3ac3000)
    at /home/gabor/development/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:419
#3  0x00007fffee109b66 in mozilla::net::nsHttpChannel::Connect (
    this=0x7fffc3ac3000)
    at /home/gabor/development/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:371
#4  0x00007fffee11d416 in mozilla::net::nsHttpChannel::ContinueBeginConnect (
    this=0x7fffc3ac3000)
    at /home/gabor/development/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:5050
---Type <return> to continue, or q <return> to quit---
#5  0x00007fffee11cfb3 in mozilla::net::nsHttpChannel::BeginConnect (
    this=0x7fffc3ac3000)
    at /home/gabor/development/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:4985
#6  0x00007fffee11d62b in mozilla::net::nsHttpChannel::OnProxyAvailable (
    this=0x7fffc3ac3000, request=0x7fffc1e85c90, channel=0x7fffc3ac3050, 
    pi=0x0, status=NS_OK)
    at /home/gabor/development/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:5112
#7  0x00007fffedf251ba in nsAsyncResolveRequest::DoCallback (
    this=0x7fffc1e85c80)
    at /home/gabor/development/mozilla-central/netwerk/base/nsProtocolProxyService.cpp:280
#8  0x00007fffedf2486a in nsAsyncResolveRequest::Run (this=0x7fffc1e85c80)
    at /home/gabor/development/mozilla-central/netwerk/base/nsProtocolProxyService.cpp:167
#9  0x00007fffedf11c70 in nsProtocolProxyService::AsyncResolveInternal (
    this=0x7fffcceb0b00, channel=0x7fffc3ac3050, flags=0, 
    callback=0x7fffc3ac3428, result=0x7fffc3ac3478, isSyncOK=true)
    at /home/gabor/development/mozilla-central/netwerk/base/nsProtocolProxyService.cpp:1246
#10 0x00007fffedf11e11 in nsProtocolProxyService::AsyncResolve2 (
    this=0x7fffcceb0b00, channel=0x7fffc3ac3050, flags=0, 
---Type <return> to continue, or q <return> to quit---
    callback=0x7fffc3ac3428, result=0x7fffc3ac3478)
    at /home/gabor/development/mozilla-central/netwerk/base/nsProtocolProxyService.cpp:1270
#11 0x00007fffee11020c in mozilla::net::nsHttpChannel::ResolveProxy (
    this=0x7fffc3ac3000)
    at /home/gabor/development/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:1972
#12 0x00007fffee11bc8b in mozilla::net::nsHttpChannel::AsyncOpen (
    this=0x7fffc3ac3000, listener=0x7fffbb99fed8, context=0x0)
    at /home/gabor/development/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:4770
#13 0x00007fffee0d7b36 in mozilla::net::HttpChannelParent::DoAsyncOpen (
    this=0x7fffbd1c0380, aURI=..., aOriginalURI=..., aDocURI=..., 
    aReferrerURI=..., aReferrerPolicy=@0x7fffffffbd20: 0, 
    aAPIRedirectToURI=..., aTopWindowURI=..., 
    aLoadFlags=@0x7fffffffbd48: 6883328, requestHeaders=..., 
    requestMethod=..., uploadStream=..., 
    uploadStreamHasHeaders=@0x7fffffffbd78: false, 
    priority=@0x7fffffffbd7a: 65526, classOfService=@0x7fffffffbd7c: 0, 
    redirectionLimit=@0x7fffffffbd80: 20 '\024', 
    allowPipelining=@0x7fffffffbd81: true, allowSTS=@0x7fffffffbd82: true, 
    thirdPartyFlags=@0x7fffffffbd84: 4, doResumeAt=@0x7fffffffbd88: false, 
    startPos=@0x7fffffffbd90: 18446744073709551615, entityID=..., 
---Type <return> to continue, or q <return> to quit---
    chooseApplicationCache=@0x7fffffffbda8: true, appCacheClientID=..., 
    allowSpdy=@0x7fffffffbdc0: true, aFds=..., aRequestingPrincipalInfo=..., 
    aTriggeringPrincipalInfo=..., aSecurityFlags=@0x7fffffffbe08: 0, 
    aContentPolicyType=@0x7fffffffbe0c: 6, 
    aInnerWindowID=@0x7fffffffbe10: 2147483655)
    at /home/gabor/development/mozilla-central/netwerk/protocol/http/HttpChannelParent.cpp:376
#14 0x00007fffee0d6581 in mozilla::net::HttpChannelParent::Init (
    this=0x7fffbd1c0380, aArgs=...)
    at /home/gabor/development/mozilla-central/netwerk/protocol/http/HttpChannelParent.cpp:113
#15 0x00007fffee18b1e7 in mozilla::net::NeckoParent::RecvPHttpChannelConstructor (this=0x7fffc5dcdf80, aActor=0x7fffbd1c0380, aBrowser=..., aSerialized=..., 
    aOpenArgs=...)
    at /home/gabor/development/mozilla-central/netwerk/ipc/NeckoParent.cpp:250
#16 0x00007fffee3cffa7 in mozilla::net::PNeckoParent::OnMessageReceived (
    this=0x7fffc5dcdf80, __msg=...)

Comment 9

[Jason Duell]

Dragana has dealt with several bugs around figuring when we should decode content in e10s, so she's probably the best person to take this.

Comment 10

[Dragana Damjanovic [:dragana]]

HttpChannelParent does not do conversion, the conversion is done in the child. In  some cases it is diverted back to parent and converted there. In this case the conversion is done later.

I leave need info to think about what is the best solution here.

Comment 11

[Gabor Krizsanits (INACTIVE)]

(In reply to Dragana Damjanovic [:dragana] from comment #10)
> HttpChannelParent does not do conversion, the conversion is done in the
> child. In  some cases it is diverted back to parent and converted there. In
> this case the conversion is done later.

This bug is a blocker for uplifting e10s to aurora, so moving on with it is very important. If you could point me to some code that does what you've described it here I could try to look into this and come up with something in case you're flooded with other things.

Comment 12

[Dragana Damjanovic [:dragana]]

Sorry for delay.

There are 3 options:
1) get data from httpchannelchild  there it is already converted. I am not sure this is easy and there is the case with diverting back to parent that makes it harder.
2) Let http channel know that it should convert data on parent and not on the child. here you will need to inform child that data are already converted.

  ApplyConversion is set to false at:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelParent.cpp#712
so it would be possible to inform HttpChannelParent to convert data

3) as you suggested do it in network monitoring

Comment 13

[Gabor Krizsanits (INACTIVE)]

So, I've been playing with my options a bit and I'm getting somewhere, but I
think I need some more help...

(In reply to Dragana Damjanovic [:dragana] from comment #12)
> 1) get data from httpchannelchild  there it is already converted. I am not
> sure this is easy and there is the case with diverting back to parent that
> makes it harder.

I considered this hard if possible at all (probably not without a huge refactoring
which I don't have the time for nor the knowledge at this area), so I skipped this approach...

> 2) Let http channel know that it should convert data on parent and not on
> the child. here you will need to inform child that data are already
> converted.
> 
>   ApplyConversion is set to false at:
> http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/
> HttpChannelParent.cpp#712
> so it would be possible to inform HttpChannelParent to convert data
>

I thought that this should be the proper solution. But after reading through
the story of that flag: Bug 567267 I've decided that I don't want to touch that
code. It also does not help that it seems like we do conversion on both the
parent and the child side. I wonder if anyone knows what's truly happening...
 
> 3) as you suggested do it in network monitoring

This is what I'm trying to do right now. As it turns out nsIStreamConverterService::convert is not implemented only the async version, but that's fine.

nsIStreamConverterService::asyncConvertData("gzip", "uncompressed",...) did fix the problem once I managed to hack it into the devtools code. Problem is that I cannot determine when I need this converter. The request we have at hand might be originated from the parent process might be from the child process. Maybe e10s is turned on maybe not. I cannot check these, and if I just apply the converter blindly, then in some cases it just throws and shuts down the channel.

So this approach will only ever work if we can determine if such conversion is needed. Can we?  	nsIStreamConverterService and nsIStreamConverter surely do not offer any method to determine if they can/should do any work or not. So how am I supposed to check if such converter is needed or not for a given request? I could check the response header, but then I should still be able to check where was the request initiated from, since I only want to apply the converter if there is a HttpChannelChild is involved...

Comment 14

[Dragana Damjanovic [:dragana]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #13)
> So, I've been playing with my options a bit and I'm getting somewhere, but I
> think I need some more help...
> 
> (In reply to Dragana Damjanovic [:dragana] from comment #12)
> > 1) get data from httpchannelchild  there it is already converted. I am not
> > sure this is easy and there is the case with diverting back to parent that
> > makes it harder.
> 
> I considered this hard if possible at all (probably not without a huge
> refactoring

I agree with this.

> 
> > 2) Let http channel know that it should convert data on parent and not on
> > the child. here you will need to inform child that data are already
> > converted.
> > 
> >   ApplyConversion is set to false at:
> > http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/
> > HttpChannelParent.cpp#712
> > so it would be possible to inform HttpChannelParent to convert data
> >
> 
> I thought that this should be the proper solution. But after reading through
> the story of that flag: Bug 567267 I've decided that I don't want to touch
> that
> code. It also does not help that it seems like we do conversion on both the
> parent and the child side. I wonder if anyone knows what's truly happening...
>  

Actually i do know this code and how it is all done in nsHttpChannel (Child/Parent) and I could help you here. My only concern here is if it is possible to inform nsChannelParent early enough. It should be possible since network monitoring do connect its listener on time.

> > 3) as you suggested do it in network monitoring
> 
> This is what I'm trying to do right now. As it turns out
> nsIStreamConverterService::convert is not implemented only the async
> version, but that's fine.
> 
> nsIStreamConverterService::asyncConvertData("gzip", "uncompressed",...) did
> fix the problem once I managed to hack it into the devtools code. Problem is
> that I cannot determine when I need this converter. The request we have at
> hand might be originated from the parent process might be from the child
> process. Maybe e10s is turned on maybe not. I cannot check these, and if I
> just apply the converter blindly, then in some cases it just throws and
> shuts down the channel.
> 
> So this approach will only ever work if we can determine if such conversion
> is needed. Can we?  	nsIStreamConverterService and nsIStreamConverter surely
> do not offer any method to determine if they can/should do any work or not.
> So how am I supposed to check if such converter is needed or not for a given
> request? I could check the response header, but then I should still be able
> to check where was the request initiated from, since I only want to apply
> the converter if there is a HttpChannelChild is involved...

So you will need to know whether applyConversion is set to false. 
channel.applyConversion (check if channel is instanceof Ci.nsIEncodedChannel)
applyConversion is valid only after onStartRequest is called for all listeners.

There is one problem. applyConversion can be set to false because e10s is on and we do conversion in the HttpchannelChild or for some other reason we do not do conversion (retrieving a gzip file that we want to store zipped). The real problem for the second case when e10s is on, is that we will know about it only in the child process so network monitor would already start decoding (this is a problem for possible solution number 2). 
nsExternalHelperAppService is used to decide to decode or not (but please search again if there is something else that is changing this flag). You can use the same thing to make a decision (applyConversion is false and nsExternalHelperAppService would decode content).

HttpChannel uses response header to decide which conversion it will need: 
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#663  


if you need help just let me know.

Comment 15

[Gabor Krizsanits (INACTIVE)]

Attachment: WIP do the coversion on parent side

(In reply to Dragana Damjanovic [:dragana] from comment #14)

Thanks a lot for the quick and helpful reply. I'm going to try the approach you suggested for 3) but in the meanwhile I'm trying to understand 2). It might be not as complex as I thought. This patch seems to be fixing the problem, but I'm not sure if this is what you had in mind exactly, I might got you wrong. But I guess it's easier to discuss with some example code...

I also don't know what this might break, that's what I meant with the "I wonder if someone knows what's going on" with all the various edge cases... but if you're confident that we don't break anything with this approach, or that we have full test coverage a proper fix would be cool instead of some more hacking on the network monitor code. The one thing I want to avoid is to fix a network monitoring bug and cause a more serious intermittent http channel issue for e10s or performance problems or whatever the reason was for doing the conversion on the child side :)

(In reply to Dragana Damjanovic [:dragana] from comment #14)
> possible to inform nsChannelParent early enough. It should be possible since
> network monitoring do connect its listener on time.

Actually it connects to it's listener via the nsIHttpActivityDistributor so it's kind of cheating :) But isn't the case that we had to inform not to do the conversion on the parent side and now we simply let it do the conversion without setting the flag to false (like I do it in the patch)?

Comment 16

[Dragana Damjanovic [:dragana]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #15)
> Created attachment 8590273 [details] [diff] [review]
> WIP do the coversion on parent side
> 
> (In reply to Dragana Damjanovic [:dragana] from comment #14)
> 
> Thanks a lot for the quick and helpful reply. I'm going to try the approach
> you suggested for 3) but in the meanwhile I'm trying to understand 2). It
> might be not as complex as I thought. This patch seems to be fixing the
> problem, but I'm not sure if this is what you had in mind exactly, I might
> got you wrong. But I guess it's easier to discuss with some example code...
> 
> I also don't know what this might break, that's what I meant with the "I
> wonder if someone knows what's going on" with all the various edge cases...
> but if you're confident that we don't break anything with this approach, or
> that we have full test coverage a proper fix would be cool instead of some
> more hacking on the network monitor code. The one thing I want to avoid is
> to fix a network monitoring bug and cause a more serious intermittent http
> channel issue for e10s or performance problems or whatever the reason was
> for doing the conversion on the child side :)
> 

I would not move conversion from child to parent always, I had in mind to move it only if networkmonitoring is use. This will degrade firefox performance only if networkmonitoring is use (i am not sure how much but conversion will be done on main thread which can slow down other things and messages that we are sending between parent and child will be larger)
I am not sure if it is desirable to change firefox behavior that much if networkmonitoring is used.

So my idea was to have a flag ItIsNetworkMonitoringDoConversionOnParent and depending on its value set:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelParent.cpp#712

Then we will need to inform child not to do it, maybe as a parameter in:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelParent.cpp#736


This part is easy, but there is a problem: 
To convert data or not is decided by calling nsExternalHelperAppService and this is call in child after RecvOnStartRequest is received and this is too late. I am afraid it its too much hacking to get this working :(

Comment 17

[Dragana Damjanovic [:dragana]]

Comment on attachment 8590273 [details] [diff] [review]
WIP do the coversion on parent side

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

see the comment above

Comment 18

[Gabor Krizsanits (INACTIVE)]

(In reply to Dragana Damjanovic [:dragana] from comment #16)
> I would not move conversion from child to parent always, I had in mind to
> move it only if networkmonitoring is use.

Right... there is my misunderstanding :) I don't think we should do that. Especially since some add-ons might want to do the same what I'm trying to achieve for devtools here... I'm going to try and finish 3) instead then.

Comment 19

[Gabor Krizsanits (INACTIVE)]

Attachment: Unzipping response boddies in network monitor when needed

Comment 20

[Dragana Damjanovic [:dragana]]

Comment on attachment 8590887 [details] [diff] [review]
Unzipping response boddies in network monitor when needed

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

Looks good, just make i more generic.

::: toolkit/devtools/webconsole/network-monitor.js
@@ +398,5 @@
> +
> +    // In the multi-process mode, the coversion happens on the child side while we can
> +    // only monitor the channel on the parent side. If the content is gzipped, we have
> +    // to unzip it ourself. For that we use the stream converter services.
> +    if (!this.needsConversionChecked) {

I do not know network-monitor that well, just asking is it possible to make this check in onStartRequest (it is called just once and it is the first think called before data comes).

@@ +404,5 @@
> +      this.needsConversionChecked = true;
> +      let channel = this.request;
> +      if (channel instanceof Ci.nsIEncodedChannel &&
> +          channel.contentEncodings) {
> +        this.conversionApplied = channel.applyConversion;

you can make an if here: if channel.applyConversion is true you do not need any other checks.

@@ +407,5 @@
> +          channel.contentEncodings) {
> +        this.conversionApplied = channel.applyConversion;
> +        let uri = channel.URI;
> +        // We only care about gzipped content for now.
> +        if (channel.getResponseHeader("Content-Encoding") == "gzip") {

are you planning to make this more generic?

You will need to change converterHelper

Comment 21

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I have implemented a stream convertor (JSON prettifier) with contract id:
"@mozilla.org/streamconv;1?from=application/json&to=*/*";

It doesn't work in e10s mode and this bug sounds related.
Should I expect that my convertor will start working as soon as this is fixed.

Here is my repo:
https://github.com/janodvarko/prototypes/tree/master/json-viewer

And related piece of code
https://github.com/janodvarko/prototypes/blob/master/json-viewer/lib/main.js#L20

Honza

Comment 22

[Gabor Krizsanits (INACTIVE)]

(In reply to Jan Honza Odvarko [:Honza] from comment #21)
> I have implemented a stream convertor (JSON prettifier) with contract id:
> "@mozilla.org/streamconv;1?from=application/json&to=*/*";
> 
> It doesn't work in e10s mode and this bug sounds related.
> Should I expect that my convertor will start working as soon as this is
> fixed.

I'm not a big expert here, but if I read correctly what you're doing you register
a converter on parent side, but since the conversion happens on child side (usually...)
it is ignored (since the child process is not aware of it).

What I would try to do is registering it in the child process (you should use a process
script for that, which I'm not sure is documented yet, see: bug 1133594)

If this is the problem here, we should file a bug for this and add a compatibility shim
for this case. Let's keep this issue separated from this bug: bug 1154662.

Comment 23

[Gabor Krizsanits (INACTIVE)]

(In reply to Dragana Damjanovic [:dragana] from comment #20)
> Looks good, just make i more generic.

We should totally do that. Problem is that I have no idea what other cases there are and what are the ones we should care about here. What do you think?

> I do not know network-monitor that well, just asking is it possible to make
> this check in onStartRequest (it is called just once and it is the first
> think called before data comes).

That makes sense, I will look into it, I don't know this code that much either tbh. In the meanwhile I'll try to find someone who owns this code...

> 
> @@ +404,5 @@
> > +      this.needsConversionChecked = true;
> > +      let channel = this.request;
> > +      if (channel instanceof Ci.nsIEncodedChannel &&
> > +          channel.contentEncodings) {
> > +        this.conversionApplied = channel.applyConversion;
> 
> you can make an if here: if channel.applyConversion is true you do not need
> any other checks.

Hmm... I think I had a problem with that approach, but I can be wrong... I will try it again.

> > +        // We only care about gzipped content for now.
> > +        if (channel.getResponseHeader("Content-Encoding") == "gzip") {
> 
> are you planning to make this more generic?

I'd like to. I'm just lacking of expertise here... 

> 
> You will need to change converterHelper

Yeah in the extended version it should have modes or something for the
different cases.

Comment 24

[Gabor Krizsanits (INACTIVE)]

Hey Alex, would you be OK with reviewing a patch on network-monitor.js?

Comment 25

[Alexandre Poirot [:ochameau]]

Yes. I just have two comments for now on this patch:
 - I hope you can figure out how to simplify the various if conditions with Dragana's help.
 - for the sake of readability, this code is already complex enough, it would be great if there is some way to pull the gzip stream work outside of this NetworkResponseListener helper?

Otherwise, now that we have some C++ experts on board... with some knowledges on both netwerk and actors...
Couldn't we just get rid of this whole mess? I mean the piping from parent to child?
Wouldn't we have valid nsIHttpChannel objects being created in the child process?

We are currently using @mozilla.org/network/http-activity-distributor;1 and http-on-examine-response to get references to them. I think both are not working in child processes. See bug 806753. This bug tends to say that the channels we might retrieve in the child may not work as expected. And especially we may not be able to modify the requests. But I'm not sure we really care. I don't think we modify request live. We eventually replay them, but when we do that it looks like we just do a new XHR from the child.

I think it would be worth looking into that in order to understand why we are doing that and better define how best doing this horrible pipe work. I can help on the actor side for removing/tweaking the piping if we can get rid of it!!!

Comment 26

[Gabor Krizsanits (INACTIVE)]

(In reply to Alexandre Poirot [:ochameau] from comment #25)
> Yes. I just have two comments for now on this patch:
>  - I hope you can figure out how to simplify the various if conditions with
> Dragana's help.

Probably.

>  - for the sake of readability, this code is already complex enough, it
> would be great if there is some way to pull the gzip stream work outside of
> this NetworkResponseListener helper?

I wanted to do that for fixing one of Draganas requests anyway, so sure.

> 
> Otherwise, now that we have some C++ experts on board... with some
> knowledges on both netwerk and actors...
> Couldn't we just get rid of this whole mess? I mean the piping from parent
> to child?

This whole file is... overcomplicated for sure. I'm willing to help cleaning it up
thought I'm quite packed with e10s work. If you can do the heavy lifting of it I'm
sure we can find a way to make this whole network monitoring a lot nicer. What I
don't want to do is starting a huge refactoring that blocks e10s uplift to aurora,
so let's do that work in a separated bug as a follow-up.

> Wouldn't we have valid nsIHttpChannel objects being created in the child
> process?

The channel is valid, but does not fire some events as you mention it as well...
Also it might be tricky to hook up for it, but I'm sure we can do it somehow,
and probably that would be the right way to tackle this problem on the long run.
Probably this will also need some platform work...

> I think it would be worth looking into that in order to understand why we
> are doing that and better define how best doing this horrible pipe work. I
> can help on the actor side for removing/tweaking the piping if we can get
> rid of it!!!

+1
Bug 1154709

Comment 27

[Alexandre Poirot [:ochameau]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #26)
> This whole file is... overcomplicated for sure. I'm willing to help cleaning
> it up
> thought I'm quite packed with e10s work. If you can do the heavy lifting of
> it I'm
> sure we can find a way to make this whole network monitoring a lot nicer.
> What I
> don't want to do is starting a huge refactoring that blocks e10s uplift to
> aurora,
> so let's do that work in a separated bug as a follow-up.

What I have in mind isn't really a refactoring, we would just drop the piping codepaths.
It may drop a bunch of code, but shouldn't involve much code modification.

> 
> > Wouldn't we have valid nsIHttpChannel objects being created in the child
> > process?
> 
> The channel is valid, but does not fire some events as you mention it as
> well...
> Also it might be tricky to hook up for it, but I'm sure we can do it somehow,
> and probably that would be the right way to tackle this problem on the long
> run.
> Probably this will also need some platform work...

I mentioned it here in case doing that is easier than handling gzip content,
or if there is some other similar issues that wouldn't happen if we were tracking channels from the child.

If you have ideas on how to hook up channels in childs, I can test any draft platform patch to evaluate if that works.
I think http-on-examine-response is explicitely disabled because it used to work in child,
that one would be easy to revive.
But I don't know about the http-activity-distributor. I would imagine it doesn't work?
We need it just to measure various timings:
  http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/webconsole/network-monitor.js#457

Comment 28

[Dragana Damjanovic [:dragana]]

(In reply to Alexandre Poirot [:ochameau] from comment #27)
> 
> I mentioned it here in case doing that is easier than handling gzip content,
> or if there is some other similar issues that wouldn't happen if we were
> tracking channels from the child.
> 
> If you have ideas on how to hook up channels in childs, I can test any draft
> platform patch to evaluate if that works.
> I think http-on-examine-response is explicitely disabled because it used to
> work in child,
> that one would be easy to revive.
> But I don't know about the http-activity-distributor. I would imagine it
> doesn't work?
> We need it just to measure various timings:
>  
> http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/webconsole/
> network-monitor.js#457

We could make a new "http-on-data-coming" (or something better) that will fire just once - in e10s on child process and in not e10s in original channel (I think now we do not know in the original httpchannel whether there is a child or not, but i think it is possible to make this work) and you could connect listener on that event, probably need refactoring(splitting) it a bit . We should document this not to have similar bugs as he one that led to removing http-on-examine-response at the first place.

Comment 29

[Gabor Krizsanits (INACTIVE)]

(In reply to Alexandre Poirot [:ochameau] from comment #27)
> What I have in mind isn't really a refactoring, we would just drop the
> piping codepaths.
> It may drop a bunch of code, but shouldn't involve much code modification.

Dropping a channel would mean that we have to figure out how to do the stream-listener-tee
work without pipe. As it seems like that thing needs the blocking pipe, and if we don't
use stream-listener-tee that would mean we unzip the original child channel, which would
screw things up. Do you have an idea how to do that?

And I still think the proper solution here needs refactoring, as some part of the code
should run _sometimes_ (not e10s / some channels when e10s is on) in the same process
sometimes not.

(In reply to Dragana Damjanovic [:dragana] from comment #28)
> (In reply to Alexandre Poirot [:ochameau] from comment #27)
> > If you have ideas on how to hook up channels in childs, I can test any draft
> > platform patch to evaluate if that works.
> > I think http-on-examine-response is explicitely disabled because it used to
> > work in child,
> > that one would be easy to revive.
> > But I don't know about the http-activity-distributor. I would imagine it
> > doesn't work?
> > We need it just to measure various timings:
> >  
> > http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/webconsole/
> > network-monitor.js#457
> 
> We could make a new "http-on-data-coming" (or something better) that will
> fire just once - in e10s on child process and in not e10s in original
> channel (I think now we do not know in the original httpchannel whether
> there is a child or not, but i think it is possible to make this work) and
> you could connect listener on that event, probably need
> refactoring(splitting) it a bit . We should document this not to have
> similar bugs as he one that led to removing http-on-examine-response at the
> first place.

So what is the imagined workflow here? We have a channel and we have to decide
if it has(/ will have) a child or not and if it does we have to spawn some task
in the child process (and hope it's not too late) if not then do the work on parent
side? It's not ideal to write js code for an API like that to say the least...

We probably will need something like this. I just think we should do this as a
follow-up as it will take some time to get this right. I could totally work on
this after the aurora uplift. So is anyone against landing a quick fix and then
keep working on this in the second half of Q2?

Comment 30

[Dragana Damjanovic [:dragana]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #29)
> 
> So what is the imagined workflow here? We have a channel and we have to
> decide
> if it has(/ will have) a child or not and if it does we have to spawn some
> task
> in the child process (and hope it's not too late) if not then do the work on
> parent
> side? It's not ideal to write js code for an API like that to say the
> least...


My idea was to fire this event just once and the logic for that will be in necko. So in js you have just one event that is coming from the original channel if it is non e10s or from child if it is e10s.

Comment 31

[Gabor Krizsanits (INACTIVE)]

(In reply to Dragana Damjanovic [:dragana] from comment #30)
> My idea was to fire this event just once and the logic for that will be in
> necko. So in js you have just one event that is coming from the original
> channel if it is non e10s or from child if it is e10s.

Sorry, I think I've got confused earlier. Let's try again.

Currently we use nsIHttpActivityObserver to get a notification when some traffic is going on.
We will get the parent side of the channel from there for the e10s case. This whole code is running
in the parent process actually. So how could I hook up a listener for that event? I don't have
a reference to the child channel. Sorry, if I'm missing something obvious...

We could try to solve this logic in necko, and let's say fire the event from the child and then propagate it to the parent, so if one hooks up a listener on the parent, then it will get the event,
but then we still have only reference to the channel on the parent side, so we cannot get the data
from the child after it did all the conversions on it.

My other concern is that there is some ongoing work on defining what to do with http-on events
in general in Bug 1108827, and usually these events are fired on parent side I think...

Comment 32

[Dragana Damjanovic [:dragana]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #31)
> (In reply to Dragana Damjanovic [:dragana] from comment #30)
> > My idea was to fire this event just once and the logic for that will be in
> > necko. So in js you have just one event that is coming from the original
> > channel if it is non e10s or from child if it is e10s.
> 
> Sorry, I think I've got confused earlier. Let's try again.
> 
> Currently we use nsIHttpActivityObserver to get a notification when some
> traffic is going on.
> We will get the parent side of the channel from there for the e10s case.
> This whole code is running
> in the parent process actually. So how could I hook up a listener for that
> event? I don't have
> a reference to the child channel. Sorry, if I'm missing something obvious...
> 
> We could try to solve this logic in necko, and let's say fire the event from
> the child and then propagate it to the parent, so if one hooks up a listener
> on the parent, then it will get the event,
> but then we still have only reference to the channel on the parent side, so
> we cannot get the data
> from the child after it did all the conversions on it.
> 
> My other concern is that there is some ongoing work on defining what to do
> with http-on events
> in general in Bug 1108827, and usually these events are fired on parent side
> I think...

I have not look at this deeply. Before bug 806753, we did fire this events on child probably over parent (i have not look at it). We do not do this any more so probably needs some investigation if it is possible revert it and how.
And streams need to be proxied...etc.

So easier solution is to continue what you already did and just make it more readable.

Comment 33

[Dragana Damjanovic [:dragana]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #23)
> 
> > > +        // We only care about gzipped content for now.
> > > +        if (channel.getResponseHeader("Content-Encoding") == "gzip") {
> > 
> > are you planning to make this more generic?
> 
> I'd like to. I'm just lacking of expertise here... 

Hint: here is how necko decides what is acceptable encoding:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpHandler.cpp#471


or look here, how decision about conversion is done and take care about the order of applying conversions:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#676

Comment 34

[Gabor Krizsanits (INACTIVE)]

Attachment: Unzipping response boddies in network monitor when needed. v2

I cleaned up the code and added some generalization to it. I think this should be it for now. Thanks for all the help Dragana!

Comment 36

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Adding the devedition-40 whiteboard so this is on my radar.

Comment 37

[Alexandre Poirot [:ochameau]]

Comment on attachment 8593968 [details] [diff] [review]
Unzipping response boddies in network monitor when needed. v2

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

Just reading at the code, something looks suspicious if you involve more than one converter.
Did you tested such scenario?
Do you think you could write a test involving a compressed request? (not necessary all scenario, but just a simple usecase that is currently broken)

The following test could be a good candidate, you may just augment it to request a gziped request:
  http://mxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/test/browser_net_content-type.js

Mochitests are using .sjs files to expose some http server for tests, using httpd.js, see this one exposing a gziped request:
  http://mxr.mozilla.org/mozilla-central/source/dom/base/test/send_gzip_content.sjs

Netmonitor test I refered to is already using one .sjs file:
  http://mxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/test/sjs_content-type-test-server.sjs

::: toolkit/devtools/webconsole/network-monitor.js
@@ +17,5 @@
>                           "nsIHttpActivityDistributor");
>  
> +loader.lazyServiceGetter(this, "gExternalHelperAppService",
> +                         "@mozilla.org/uriloader/external-helper-app-service;1",
> +                          Ci.nsIExternalHelperAppService);

It looks like you are no longer using this, which is great!

@@ +216,5 @@
> +    // We need to track the offset for the onDataAvailable calls where we pass the data
> +    // from our pipe to the coverter.
> +    this.offset = 0;
> +
> +    // In the multi-process mode, the coversion happens on the child side while we can

s/coversion/conversion/

@@ +223,5 @@
> +    this.needsConversionChecked = true;
> +    let channel = this.request;
> +    if (channel instanceof Ci.nsIEncodedChannel &&
> +        channel.contentEncodings &&
> +        !channel.applyConversion) {

Could you write in comment a reference (mxr/dxr link) to the code you looked at for this particular condition, if there is something easy to refer to?
May be also for acceptedEncodings.

@@ +226,5 @@
> +        channel.contentEncodings &&
> +        !channel.applyConversion) {
> +      let encodingHeader = channel.getResponseHeader("Content-Encoding");
> +      var scs = Cc["@mozilla.org/streamConverters;1"].
> +        getService(Ci.nsIStreamConverterService);

s/var/let/

@@ +237,5 @@
> +        if (acceptedEncodings.indexOf(enc) > -1) {
> +          this.converter = scs.asyncConvertData(enc, "uncompressed", nextListener, null);
> +          this.converter.onStartRequest(this.request, null);
> +          nextListener = this.converter;
> +        }

Does that really work with multiple converters?
Isn't it full asynchronous so that we are going to mangle converter all together?
Or is `this.converter.onStartRequest` synchronous?
I imagine that `this.converter` will just be the last converter instanciated in this for loop, when used from onInputStreamReady.

@@ +427,5 @@
>        if (available != 0) {
> +        if (this.converter)
> +          this.converter.onDataAvailable(this.request, null, aStream, this.offset, available);
> +        else
> +          this.onDataAvailable(this.request, null, aStream, this.offset, available);

Please use braces, even for one line statements.

Comment 38

[Dragana Damjanovic [:dragana]]

Comment on attachment 8593968 [details] [diff] [review]
Unzipping response boddies in network monitor when needed. v2

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

::: toolkit/devtools/webconsole/network-monitor.js
@@ +235,5 @@
> +        // There can be multiple conversions applied
> +        let enc = encodings[i].toLowerCase();
> +        if (acceptedEncodings.indexOf(enc) > -1) {
> +          this.converter = scs.asyncConvertData(enc, "uncompressed", nextListener, null);
> +          this.converter.onStartRequest(this.request, null);

this will probably go each time through the hole chain.
You should call this outside of for loop.

I agree, some test would be good.

Comment 39

[Gabor Krizsanits (INACTIVE)]

(In reply to Alexandre Poirot [:ochameau] from comment #37)
> Could you write in comment a reference (mxr/dxr link) to the code you looked
> at for this particular condition, if there is something easy to refer to?
> May be also for acceptedEncodings.

I don't really like mxr/dxr links in comment, since those will be outdated soon.
Other problem is that there is no such place really, I read various code, and
got a lot of help from Dragana in this thread...

Fixed the rest of nits/suggestions also added a test with double gzip.

Comment 40

[Gabor Krizsanits (INACTIVE)]

Attachment: Unzipping response boddies in network monitor when needed. v3

Comment 41

[Gabor Krizsanits (INACTIVE)]

(In reply to Dragana Damjanovic [:dragana] from comment #38)
> this will probably go each time through the hole chain.
> You should call this outside of for loop.

Whoops, I thought I fixed this one, thanks!

> 
> I agree, some test would be good.

I thought there are a bunch of devtools tests turned off because of this bug,
and I just wanted to turned them back on, but it seems I was wrong...

I flagged only Alex for this round since I think one review is enough and don't
want to hold you up any longer with this bug, but of course you are very welcome
to read it through and add your suggestions or flag it r- if you feel necessary!

Comment 43

[Alexandre Poirot [:ochameau]]

Comment on attachment 8596503 [details] [diff] [review]
Unzipping response boddies in network monitor when needed. v3

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

See my comment about this.converter, which still looks suspicious.
But otherwise, that looks good if you confirm there is nothing bad about it.

::: browser/devtools/netmonitor/test/sjs_content-type-test-server.sjs
@@ +197,5 @@
>        }
> +      case "gzip": {
> +        response.setStatusLine(request.httpVersion, status, "OK");
> +        response.setHeader("Content-Type", "text/plain", false);
> +        response.setHeader("Content-Encoding", "gzip\t ,gzip", false);

Could you add a comment saying that you are testing double gzip compression, that's not obvious and this line looks like a typo without any context.

@@ +212,5 @@
> +          onStreamComplete: function(loader, context, status, length, result) {
> +            let buffer = String.fromCharCode.apply(this, result);
> +            gzipCompressString(buffer, observer);
> +          }
> +        };

nit: I would have put observer2 in gzipCompressString,
and just accept a callback for when `buffer` is ready.

::: toolkit/devtools/webconsole/network-monitor.js
@@ +216,5 @@
> +
> +    // In the multi-process mode, the conversion happens on the child side while we can
> +    // only monitor the channel on the parent side. If the content is gzipped, we have
> +    // to unzip it ourself. For that we use the stream converter services.
> +    this.needsConversionChecked = true;

`needsConversionChecked` seems to be a leftover?

@@ +223,5 @@
> +        channel.contentEncodings &&
> +        !channel.applyConversion) {
> +      let encodingHeader = channel.getResponseHeader("Content-Encoding");
> +      var scs = Cc["@mozilla.org/streamConverters;1"].
> +        getService(Ci.nsIStreamConverterService);

var -> let

@@ +231,5 @@
> +      for (let i in encodings) {
> +        // There can be multiple conversions applied
> +        let enc = encodings[i].toLowerCase();
> +        if (acceptedEncodings.indexOf(enc) > -1) {
> +          this.converter = scs.asyncConvertData(enc, "uncompressed", nextListener, null);

Is onInputStreamReady going to be called only once?
Just for the last converter on which you call this.converter.onStartRequest?
Otherwise, this.converter may not always be the expected one in onInputStreamReady.
It may work now as both converters are gzip... I just don't know.

Comment 44

[Gabor Krizsanits (INACTIVE)]

(In reply to Alexandre Poirot [:ochameau] from comment #43)
> Is onInputStreamReady going to be called only once?
> Just for the last converter on which you call this.converter.onStartRequest?
> Otherwise, this.converter may not always be the expected one in
> onInputStreamReady.
> It may work now as both converters are gzip... I just don't know.


(In reply to Alexandre Poirot [:ochameau] from comment #37)
> > +          nextListener = this.converter;
> > +        }
> 
> Does that really work with multiple converters?

It seems so :)

> Isn't it full asynchronous so that we are going to mangle converter all
> together?

What do you mean by fully async? I set up a chain of converters then whenever there is data
on the pipe onInputStreamReady is called which calls the first converter and then the event
loop spins again. When the converter is done it calls the next converter, then the event loop
spins again, etc. There might be a new onInputStreamReady call before all the converters are done
with the first chunk, but that's not a problem, converters are ready to handle that.

The last converter will call NetworkResponseListener's onDataAvailable every time it finished some
chunk and it's granted that the chunk went through all the converters and that it won't leave out any chunks. What will be the length of each chunks and how much delay there will be compared to the no converter case it's hard to tell, but doesn't matter much.

> Or is `this.converter.onStartRequest` synchronous?

onStartRequest is usually only an init call, it just tells the converter to get ready for data. It
only has to be called on the first converter in the chain and it will propagate the call to the next
one.

> I imagine that `this.converter` will just be the last converter instanciated
> in this for loop, when used from onInputStreamReady.

Converters are in reverse order in the enumeration, so the last one has to be decoded first. As they are
chained we only need to reference this last one and when we call it passes the converted data to the next one when it's ready.

Thanks for the review! Let me know if my explanation is not clear.

Comment 45

[Pulsebot]

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

Comment 46

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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