Closed Bug 1261585 Opened 4 years ago Closed 2 years ago

nsiTraceableChannel listener not working as expected with e10s enabled

Categories

(Core :: Networking: HTTP, defect, P1)

48 Branch
x86_64
Linux
defect

Tracking

()

RESOLVED WONTFIX
Tracking Status
e10s + ---
firefox50 --- affected
firefox51 --- affected

People

(Reporter: michel.gutierrez, Assigned: dragana)

References

Details

(Whiteboard: [necko-active] triaged)

Attachments

(3 files, 10 obsolete files)

2.64 KB, application/x-xpinstall
Details
4.17 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
43.04 KB, patch
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160304114926

Steps to reproduce:

install a listener (through the nsITraceableChannel interface) on a channel for a gzip-encoded response
look at the data received to the listener


Actual results:

with e10s disabled, data are received in decoded form
with e10s enabled, data are received still gzip-encoded


Expected results:

Both e10s enabled and disabled should have the same behavior and send decoded data to the listener
The attached add-on demonstrates the issue to the console.

E10s disabled:
==============

console.info: e10s-nsitraceable-issue: Intercepted https://skyfire.vimeocdn.com/1459609078-0x639943828cb9c403e7fe1198e382ef3bdb3a8d50/154036942/video/475409723,475409726,475409725,475409722/master.json?base64_init=1
console.info: e10s-nsitraceable-issue: Request {
    "Host": "skyfire.vimeocdn.com",
    "User-Agent": "Mozilla/5.0 (X11; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0",
    "Accept": "text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8",
    "Accept-Language": "en-US,en;q=0.5",
    "Accept-Encoding": "gzip, deflate, br",
    "Connection": "keep-alive"
}
console.info: e10s-nsitraceable-issue: Response {
    "Content-Type": "application/json",
    "Content-Encoding": "gzip",
    "Via": "1.1 varnish, 1.1 varnish",
    "Access-Control-Allow-Headers": "Content-Type, Accept-Encoding, Range",
    "Fastly-Debug-Digest": "7906ddbb1134dcf5c97a561c24f411bd8752b6488b4afa19c14d871197178d6e",
    "Content-Length": "2158",
    "Accept-Ranges": "bytes",
    "Date": "Sat, 02 Apr 2016 14:15:48 GMT",
    "Age": "105010",
    "Connection": "keep-alive",
    "Timing-Allow-Origin": "*",
    "X-Served-By": "cache-iad2150-IAD, cache-lhr6329-LHR",
    "X-Cache": "HIT, HIT",
    "X-Cache-Hits": "1, 1",
    "X-Timer": "S1459606548.014544,VS0,VE0",
    "Vary": "Accept-Encoding"
}
console.info: e10s-nsitraceable-issue: data 6444 bytes 
 7B 22 63 6C 69 70 5F 69 64 22 3A 31 35 34 30 33 {"clip_id":15403 
 36 39 34 32 2C 22 62 61 73 65 5F 75 72 6C 22 3A 6942,"base_url": 
 22 2E 2E 2F 2E 2E 2F 22 2C 22 76 69 64 65 6F 22 "../../","video" 
 3A 5B 7B 22 69 64 22 3A 34 37 35 34 30 39 37 32 :[{"id":47540972 
 33 2C 22 62 61 73 65 5F 75 72 6C 22 3A 22 76 69 3,"base_url":"vi 
...


E10s enabled:
=============

console.info: e10s-nsitraceable-issue: Intercepted https://skyfire.vimeocdn.com/1459609078-0x639943828cb9c403e7fe1198e382ef3bdb3a8d50/154036942/video/475409723,475409726,475409725,475409722/master.json?base64_init=1
console.info: e10s-nsitraceable-issue: Request {
    "Host": "skyfire.vimeocdn.com",
    "User-Agent": "Mozilla/5.0 (X11; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0",
    "Accept": "text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8",
    "Accept-Language": "en-US,en;q=0.5",
    "Accept-Encoding": "gzip, deflate, br",
    "Connection": "keep-alive"
}
console.info: e10s-nsitraceable-issue: Response {
    "Content-Type": "application/json",
    "Content-Encoding": "gzip",
    "Via": "1.1 varnish, 1.1 varnish",
    "Access-Control-Allow-Headers": "Content-Type, Accept-Encoding, Range",
    "Fastly-Debug-Digest": "7906ddbb1134dcf5c97a561c24f411bd8752b6488b4afa19c14d871197178d6e",
    "Content-Length": "2158",
    "Accept-Ranges": "bytes",
    "Date": "Sat, 02 Apr 2016 14:14:53 GMT",
    "Age": "104956",
    "Connection": "keep-alive",
    "Timing-Allow-Origin": "*",
    "X-Served-By": "cache-iad2150-IAD, cache-lhr6335-LHR",
    "X-Cache": "HIT, HIT",
    "X-Cache-Hits": "1, 1",
    "X-Timer": "S1459606493.859166,VS0,VE0",
    "Vary": "Accept-Encoding"
}
console.info: e10s-nsitraceable-issue: data 2158 bytes 
 1F 8B 08 00 00 00 00 00 04 03 EC 99 6B 8F DA 38 ............k..8 
 14 86 FF 4B BE 96 42 EC 24 5C 46 EA 07 87 81 00 ...K..B.$\F..... 
 33 C3 16 DA 02 93 D5 6A 94 84 94 6B 00 01 33 C0 3......j...k..3. 
 54 FB DF F7 1C DB 01 C2 99 ED AA DA 6A 2B AD 88 T...........j+.. 
 DA C1 76 1E 9F F8 F5 71 E2 BC F0 CD 88 E6 93 D5 ..v....q........ 
 D3 64 68 DC 30 C7 36 AD 62 C5 E6 39 23 0C 36 F1 .dh.0.6.b..9#.6. 
...
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
Summary: nsiTraceable does not work as expected with e10s → nsiTraceableChannel listener not working as expected with e10s enabled
Version: 45 Branch → 48 Branch
Blocks: 930788
Component: Untriaged → Networking: HTTP
Product: Firefox → Core
Blocks: e10s-addons
tracking-e10s: --- → +
Flags: needinfo?(amckay)
This sounds like something we might want to shim.
Sounds like it. There are some add-ons that use nsITraceableChannel explicitly, I imagine there's many that also use it through higher level APIs.
Flags: needinfo?(amckay)
For information, Video DownloadHelper 5.5.0 uses nsITraceableChannel. I'm not aware of an add-on SDK API providing the nsITraceableChannel service.
There was a similar bug for devtools (bug 982319).

In e10s we do conversion on the child process and addons access the parent process.
Devtool decided to do conversion in network-monitor.js

Getting converted data will not be that easy.
jason - can you find an owner.. this is a hole that needs filling.
Assignee: nobody → jduell.mcbugs
Whiteboard: [necko-active]
Assignee: jduell.mcbugs → dd.mozilla
Whiteboard: [necko-active] → [necko-next]
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch bug_1261585_v1.patch (obsolete) — Splinter Review
Attachment #8764866 - Flags: review?(jduell.mcbugs)
Blocks: 1282037
Priority: -- → P2
Whiteboard: [necko-next] → [necko-next] triaged
Comment on attachment 8764866 [details] [diff] [review]
bug_1261585_v1.patch

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

This patch confuses me (but perhaps that's because I don't know the converter and external helper app service well enough).

So we generally convert in the child, but we need to in the parent if we've got a traceable listener.  I get that part.  What I don't understand is the patch's code in HttpChannelParent::OnStartRequest: IIUC it will now decode the channel on the parent, *unless* the URI has a file extension, and the external helper says the content-encoding is zip/gzip.  I assume this is so that we don't unzip tarballs that the user is downloading?  If so, why don't I see any other calls to this type of logic in necko? (there's no call to  ApplyDecodingForExtension() anywhere else in necko AFACIT).  At a minimum this could use a comment explaining what's happening.

I'll be out next week, but it's probably a good idea for someone else (probably Honza?) to look at this code too, since I'm clearly not as up on this code as I could be.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +2620,5 @@
>    nsCOMPtr<nsIStreamListener> wrapper = new nsStreamListenerWrapper(mListener);
>  
>    wrapper.forget(_retval);
>    mListener = aListener;
> +  mHaveListenerForTraceableChannel = true;

I don't see mHaveListenerForTraceableChannel being set by default to 'false' anywhere.  I assume we want to initialize it in the basechannel constructor?

::: netwerk/test/unit/test_content_encoding_gzip.js
@@ +74,5 @@
>      if (++index < tests.length) {
>  	startIter();
>      } else {
>          httpserver.stop(do_test_finished);
> +       prefs.setCharPref("network.http.accept-encoding", cePref);

So the only thing you changed in this file is to mis-indent this line? :)
Attachment #8764866 - Flags: review?(jduell.mcbugs) → feedback+
(In reply to Jason Duell [:jduell] (needinfo? me) from comment #9)
> Comment on attachment 8764866 [details] [diff] [review]
> bug_1261585_v1.patch
> 
> Review of attachment 8764866 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch confuses me (but perhaps that's because I don't know the
> converter and external helper app service well enough).
> 
> So we generally convert in the child, but we need to in the parent if we've
> got a traceable listener.  I get that part.  What I don't understand is the
> patch's code in HttpChannelParent::OnStartRequest: IIUC it will now decode
> the channel on the parent, *unless* the URI has a file extension, and the
> external helper says the content-encoding is zip/gzip.  I assume this is so
> that we don't unzip tarballs that the user is downloading?

yes.

> If so, why don't
> I see any other calls to this type of logic in necko? (there's no call to 
> ApplyDecodingForExtension() anywhere else in necko AFACIT).  At a minimum
> this could use a comment explaining what's happening.
> 

ApplyDecodingForExtension() is called by the app external helper which is added as a listener to the Child channel. I cannot add it as a listener here so i have extracted that part.
Attached patch bug_1261585_v1.patch (obsolete) — Splinter Review
Attachment #8764866 - Attachment is obsolete: true
Attachment #8769803 - Flags: review?(honzab.moz)
Whiteboard: [necko-next] triaged → [necko-active] triaged
Comment on attachment 8769803 [details] [diff] [review]
bug_1261585_v1.patch

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

r+ with comments AND conditioned by a green try (I see no link)

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +1036,5 @@
> +    } else {
> +      // We have a traceableChannel listener so we need to do a conversion on
> +      // the parent. But first we need to check with
> +      // external-helper-app-service, e.g. we do not ungzip if url has a gz
> +      // extention.

maybe add a note this code is a copy of nsExternalAppHandler::MaybeApplyDecodingForExtension and should be kept in sync with it.

I don't see a simple way to make that method public/shared.

::: netwerk/test/unit_ipc/child_tracable_listener.js
@@ +12,5 @@
> +  do_timeout(100, function keepAlive() {
> +    if (!shouldQuit) {
> +      do_timeout(100, keepAlive);
> +    }
> +  });

can't we just do do_test_pending?

::: netwerk/test/unit_ipc/test_traceable_channel.js
@@ +34,5 @@
> +
> +  // Replace received response body.
> +  onDataAvailable: function(request, context, inputStream,
> +                           offset, count) {
> +    dump("*** tracing listener onDataAvailable\n");

you will have to remove the dumps (test people don't like them)

@@ +60,5 @@
> +      binaryOutputStream.writeBytes(newBody, newBody.length);
> +
> +      this.listener.onDataAvailable(request, context,
> +                                    storageStream.newInputStream(0), 0,
> +                                    replacedBody.length);

what if it from some reason happens that TracingListener.onDataAvailable is called more than one time?  you should protect your artificial this.listener.onDataAvailable invocation from being called more than once.

@@ +87,5 @@
> +    try {
> +      var newListener = new TracingListener();
> +      newListener.listener = request.setNewListener(newListener);
> +    } catch(e) {
> +      dump("TracingListener.onStartRequest swallowing exception: " + e + "\n");

maybe do_check(true); here?

@@ +107,5 @@
> +      this.streamSink.close();
> +      var input = this.pipe.inputStream;
> +      sin.init(input);
> +      do_check_eq(sin.available(), originalBody.length);
> + 

ws

@@ +160,5 @@
> +  observe: function(subject, topic, data) {
> +    dump("In HttpResponseExaminer.observe\n");
> +
> +    try {
> +      if (testNum == 1 ) {

nit: space after 1

OTOH, would be better to have two tests?  this is hard to track

@@ +166,5 @@
> +        var newListener = new TracingListener();
> +        newListener.listener = subject.setNewListener(newListener);
> +      } else {
> +        subject.QueryInterface(Components.interfaces.nsITraceableChannel);
> +      

nit: ws
Attachment #8769803 - Flags: review?(honzab.moz) → review+
Attached patch bug_1261585_v1.patch (obsolete) — Splinter Review
Attachment #8769803 - Attachment is obsolete: true
Attachment #8772372 - Flags: review+
(In reply to Honza Bambas (:mayhemer) from comment #12)
> Comment on attachment 8769803 [details] [diff] [review]
> bug_1261585_v1.patch
> 
> Review of attachment 8769803 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with comments AND conditioned by a green try (I see no link)
> 
> ::: netwerk/protocol/http/HttpChannelParent.cpp
> @@ +1036,5 @@
> > +    } else {
> > +      // We have a traceableChannel listener so we need to do a conversion on
> > +      // the parent. But first we need to check with
> > +      // external-helper-app-service, e.g. we do not ungzip if url has a gz
> > +      // extention.
> 
> maybe add a note this code is a copy of
> nsExternalAppHandler::MaybeApplyDecodingForExtension and should be kept in
> sync with it.
> 
> I don't see a simple way to make that method public/shared.

I have added a comment.
> 
> ::: netwerk/test/unit_ipc/child_tracable_listener.js
> @@ +12,5 @@
> > +  do_timeout(100, function keepAlive() {
> > +    if (!shouldQuit) {
> > +      do_timeout(100, keepAlive);
> > +    }
> > +  });
> 
> can't we just do do_test_pending?

that does not work.

> 
> ::: netwerk/test/unit_ipc/test_traceable_channel.js
> @@ +34,5 @@
> > +
> > +  // Replace received response body.
> > +  onDataAvailable: function(request, context, inputStream,
> > +                           offset, count) {
> > +    dump("*** tracing listener onDataAvailable\n");
> 
> you will have to remove the dumps (test people don't like them)
> 

removed.

> @@ +160,5 @@
> > +  observe: function(subject, topic, data) {
> > +    dump("In HttpResponseExaminer.observe\n");
> > +
> > +    try {
> > +      if (testNum == 1 ) {
> 
> nit: space after 1
> 
> OTOH, would be better to have two tests?  this is hard to track
> 

I have split the test.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1f3ed984607
Make nsITraceableChannel listener work with content encoding.r=jduell
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e1f3ed984607
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
May I ask you to back this path out because of bug 1292586?
Thanks!
Flags: needinfo?(wkocher)
Kinda scary that automated tests didn't catch that.
Status: RESOLVED → REOPENED
Flags: in-testsuite?
Resolution: FIXED → ---
So, since the changes were reverted back, how the extensions devs should move on?
(In reply to JustOff from comment #22)
> So, since the changes were reverted back, how the extensions devs should
> move on?

I don't know if Dragana was thinking about it more, but my idea was to provide information to the callback implementers if there should be a converter used or not and let them apply it in the callback.  I don't remember tho all the corner cases where this may not be possible or simple to determine.

As I understand, the problem is to find out if the content decoding was applied before the traceable channel callback or not.  If not, you have the response header to find the right converter and do the conversion "manually" on the provided input stream in the traceable channel implementation.

Dragana, is it possible (or already available) to provide info whether the decoding has been applied on the input stream we provide in transfer encoding?

Also remember that if this is too complicated, extension authors should be advised to migrate to WebExtensions.
Flags: needinfo?(dd.mozilla)
(In reply to Honza Bambas (:mayhemer) from comment #23)
> (In reply to JustOff from comment #22)
> > So, since the changes were reverted back, how the extensions devs should
> > move on?
> 
> I don't know if Dragana was thinking about it more, but my idea was to
> provide information to the callback implementers if there should be a
> converter used or not and let them apply it in the callback.  I don't
> remember tho all the corner cases where this may not be possible or simple
> to determine.
> 
> As I understand, the problem is to find out if the content decoding was
> applied before the traceable channel callback or not.  If not, you have the
> response header to find the right converter and do the conversion "manually"
> on the provided input stream in the traceable channel implementation.
> 
> Dragana, is it possible (or already available) to provide info whether the
> decoding has been applied on the input stream we provide in transfer
> encoding?
> 
> Also remember that if this is too complicated, extension authors should be
> advised to migrate to WebExtensions.

I have spent some time trying to make this work. I have not make a patch because I am really afraid of missing corner cases, because the decision is complicated and depends on some prefs as well what is the content-type .... I am afraid that I do not have all information on the parent, probably i will not find out until bugs are filed.  I was suggesting to ask child (there is a patch in bug 1292586), but this need an ipc message from child to parent, the query can be sent with AsyncOpen. 

devtools already do this - bug 982319. That patch is only thing that needs to be done and addons can query if a conversion has been done or not. The addons will have the same problem that this patch(backed out patch) has, e.g. css with content encoding gzip and ending will .gz will not be decoded but firefox would (furthermore some of this decision depends on prefs as well).
Flags: needinfo?(dd.mozilla)
Attached patch bug_1261585_v2.patch (obsolete) — Splinter Review
In HttpChannelParent.h there is an explanation how all of this work.
Attachment #8772372 - Attachment is obsolete: true
Attachment #8820259 - Flags: review?(honzab.moz)
this is reverting patch from bug 982319 and only keeping tests. Now conversion will be done by necko for e10s mode as well.
Attachment #8820263 - Flags: review?(poirot.alex)
Comment on attachment 8820263 [details] [diff] [review]
bug_1261585_devtools.patch

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

I'm really happy to see that go away!

Thanks Dragana!
Attachment #8820263 - Flags: review?(poirot.alex) → review+
Attached patch bug_1261585_v2.patch (obsolete) — Splinter Review
The patch is rebased. The test that failed on the last ttry run does not fail any more and it is nor failing locally.

(Honza is on pto and does not accept reviews, I will wait)
Attachment #8820259 - Attachment is obsolete: true
Attachment #8820259 - Flags: review?(honzab.moz)
Attachment #8823218 - Flags: review?(honzab.moz)
Comment on attachment 8823218 [details] [diff] [review]
bug_1261585_v2.patch

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

Thanks Dragana for the comments!  This is how it should look :)

Didn't check the tests deeply, but seems OK.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +2807,5 @@
>  
> +  // When a divert to the parent is perform the OnStartRequest of the end
> +  // listener is called on the parent (and of course the end decision about
> +  // a data conversion is made there), therefore just clear
> +  // mContentDecodingWillBeCalledOnParent and do not send SendApplyConversion.

I think this comment needs a wording and grammatical check.  I don't fully follow.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +820,5 @@
>  
> +mozilla::ipc::IPCResult
> +HttpChannelParent::RecvApplyConversion(const bool& aApply)
> +{
> +  LOG(("HttpChannelParent::RecvApplyConversion [this=%p, aApply=%d]", this, aApply));

maybe add mWaitingForApplyConversionResponse to the log too

@@ +1199,5 @@
> +      // the child process during OnStartRequest call, therefore we will
> +      // suspend the channel here and wait for the decision result from the
> +      // child (we are waiting for RecvApplyConversion).
> +      mWaitingForApplyConversionResponse = true;
> +      mChannel->Suspend();

maybe something like:
mWaitingForApplyConversionResponse = NS_SUCCEEDED(mChannel->Suspend()) ?

@@ +1670,5 @@
> +    if (NS_SUCCEEDED(mStatus)) {
> +      mChannel->ApplyContentConversions();
> +    }
> +    mWaitingForApplyConversionResponse = false;
> +    mChannel->Resume();

I'd love to have a test for this if it were simple to write one..  but it's not a condition for r+.

::: netwerk/protocol/http/HttpChannelParent.h
@@ +276,5 @@
> +  //
> +  // If DivertToParent is performed, the OnStartRequest call on the last
> +  // channel listener is performed on the parent and the child process does not
> +  // need to send Send/RecvApplyConversion. ApplyContentConversions will be
> +  // called after the OnStartRequest has been called (This is in the

maybe for completeness say on which process is OSR called for this case.

@@ +278,5 @@
> +  // channel listener is performed on the parent and the child process does not
> +  // need to send Send/RecvApplyConversion. ApplyContentConversions will be
> +  // called after the OnStartRequest has been called (This is in the
> +  // StartDiversion function).
> +  bool mWaitingForApplyConversionResponse;

for correctness sake maybe you should make sure there is always an assertion of (the same) thread when working with this member.  we soon want to move http IPC to off main thread (pbackground) and this could get messy.  

if you feel we should do assertion checking (on not just this new member) as part of the OMT patching, feel free to omit if all the manipulations with this member are obviously on the same (main?) thread.
Attachment #8823218 - Flags: review?(honzab.moz) → review+
I will omit thread checking in this patch, this should be done in OMT patch. I think this flag will always be called on the same thread. If we move OnStart as well then we need to be careful.
Attached patch bug_1261585_v2.patch (obsolete) — Splinter Review
Attachment #8823218 - Attachment is obsolete: true
Attachment #8827501 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6d5e6243d01
Make nsITraceableChannel listener work with content encoding. r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b5321a426ad
Necko does content conversions for TracableChannel so devtool does not need to do it separately. r=ochameau
Keywords: checkin-needed
I'm running on inbound with this patch. going to www.neowin.net I am getting the error below. The regression range seems to point to this patch.

Content Encoding Error

The page you are trying to view cannot be shown because it uses an invalid or unsupported form of compression.

    Please contact the website owners to inform them of this problem.
Confirming...getting same symptoms, except with hg pushloghtml pages.
This may help with STR:
Problem seems to be history/cache-related.  I get the errors when using "back" to return to a gzipped page, or when restoring tabs at startup.
Could you please help to check the issue in comment 35, thanks.
Flags: needinfo?(poirot.alex)
Flags: needinfo?(honzab.moz)
Flags: needinfo?(dd.mozilla)
Backout by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f930d9fbb403
Backed out 2 changesets for causing website bustage.
Following backout, on restore of tabs with gzipped pages, I got the same as described in comment 35 and comment 38. But once those tabs were reloaded, no more problems.  Speculating that while patch was in, it was somehow not caching the content encoding.  Sorry for bugspam, but just hoping this info may help.
(In reply to aja from comment #41)
> Following backout, on restore of tabs with gzipped pages, I got the same as
> described in comment 35 and comment 38. But once those tabs were reloaded,
> no more problems.  Speculating that while patch was in, it was somehow not
> caching the content encoding.  Sorry for bugspam, but just hoping this info
> may help.

Thanks this is useful!

I am looking into this.
Flags: needinfo?(poirot.alex)
Flags: needinfo?(honzab.moz)
Flags: needinfo?(dd.mozilla)
I cannot reproduce it.

Can you give me steps to reproduce it?
Do you have an addon install?
Flags: needinfo?(ajarope)
I managed to reproduce it.
Flags: needinfo?(ajarope)
As I assume you've figured out already, STR was to navigate back to a previously opened gzipped page via "back" button or its dropdown menu, or restart browser with tabs restored...basically accessing any gzipped page via history.
Attached patch bug_1261585_v2.patch (obsolete) — Splinter Review
This is getting more ugly :(

The cache sink is added after conversion listener and in my patch DoApplyConversion is called later, i.e. after cache sink is added which is wrong :)
Attachment #8827501 - Attachment is obsolete: true
Attachment #8828074 - Flags: review?(honzab.moz)
(In reply to Dragana Damjanovic [:dragana] from comment #47)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=2cb16e4662fa38cd4803678ecaebc0c41017802a

doesn't build.

could you please try to find a solution that would rather delay installation of the cache listener tee?  only one you have to delay until you get the conversion answer from child is the call from nsHttpChannel::ContinueProcessNormal.
Flags: needinfo?(dd.mozilla)
Comment on attachment 8828074 [details] [diff] [review]
bug_1261585_v2.patch

dropping r for now.  if solution from the previous comment is found to be too difficult, I'll reconsider this patch.
Attachment #8828074 - Flags: review?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #48)
> (In reply to Dragana Damjanovic [:dragana] from comment #47)
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=2cb16e4662fa38cd4803678ecaebc0c41017802a
> 
> doesn't build.
> 
> could you please try to find a solution that would rather delay installation
> of the cache listener tee?  only one you have to delay until you get the
> conversion answer from child is the call from
> nsHttpChannel::ContinueProcessNormal.

I tried to delay the cache tee installation only but that was crashing because the cache entry was not there.

I will try to delay nsHttpChannel::ContinueProcessNormal.
Flags: needinfo?(dd.mozilla)
Attached patch bug_1261585_v3.patch (obsolete) — Splinter Review
We can not postpone nsHttpChannel::ContinueProcessNormal, because OnStartRequest needs to be called.

I figure out why postponing InstallCacheListener was failing.
Attachment #8828074 - Attachment is obsolete: true
Attachment #8834381 - Flags: review?(honzab.moz)
Comment on attachment 8834381 [details] [diff] [review]
bug_1261585_v3.patch

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

Thanks!

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +276,5 @@
>      , mStronglyFramed(false)
>      , mPushedStream(nullptr)
>      , mLocalBlocklist(false)
>      , mWarningReporter(nullptr)
> +    , mDelayedInstallCacheListenerForTraceableChannel(false)

s/false/0/  ?  I never know...

@@ +2426,5 @@
>  
> +    if (mHasListenerForTraceableChannel) {
> +        mDelayedInstallCacheListenerForTraceableChannel = true;
> +        return NS_OK;
> +    }

one detail: I think this should be inside the |if (mCacheEntry && !mCacheEntryIsReadOnly) {| block.

::: netwerk/protocol/http/nsHttpChannel.h
@@ +28,5 @@
>  #include "nsISupportsPrimitives.h"
>  #include "nsICorsPreflightCallback.h"
>  #include "AlternateServices.h"
>  #include "nsIHstsPrimingCallback.h"
> +#include "nsIStreamListenerTee.h"

probably no longer needed?

@@ +600,5 @@
>      HttpChannelSecurityWarningReporter* mWarningReporter;
>  
>      RefPtr<ADivertableParentChannel> mParentChannel;
> +
> +    // If a cache listener needs to be install, it needs to be install after

installed

@@ +605,5 @@
> +    // DoApplyContentConversions is called (this is because the cache needs to
> +    // receive not converted data). If a channel has a nsITracableListener call
> +    // to DoApplyContentConversions will be delayed so we need to delay the
> +    // InstallCacheListener call as well.
> +    uint32_t mDelayedInstallCacheListenerForTraceableChannel : 1;

I love this name :D

Tho, a bit field has meaning only when added among/after other bit fields (I think).
Attachment #8834381 - Flags: review?(honzab.moz) → review+
Attached patch bug_1261585_v3.patch (obsolete) — Splinter Review
Attachment #8834381 - Attachment is obsolete: true
Attachment #8838433 - Flags: review+
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM] from comment #55)
> Needs rebasing.

I rebased it yesterday :( 

Thanks!
Attached patch bug_1261585_v3.patch (obsolete) — Splinter Review
Attachment #8838433 - Attachment is obsolete: true
Attachment #8839083 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92de31554a9f
Necko does content conversions for TracableChannel so devtool does not need to do it separately. r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/16d6554c9c5b
Make nsITraceableChannel listener work with content encoding. r=mayhemer
Keywords: checkin-needed
Backed out for failing browser_animation_pseudo_elements.js on Linux opt without e10s:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9521a6247d94f225f94cfba58376130599a76b3b
https://hg.mozilla.org/integration/mozilla-inbound/rev/8abd410f96f5412854a8308b3c8d41a1f69698f4

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=16d6554c9c5b073c0a32db7ca72a30fee173794b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=78875499&repo=mozilla-inbound

[task 2017-02-20T20:18:22.569746Z] 20:18:22     INFO - TEST-PASS | devtools/client/debugger/test/mochitest/browser_dbg_post-page.js | The currently shown source contains bacon. Mmm, delicious! - 
[task 2017-02-20T20:18:22.570719Z] 20:18:22     INFO - Waiting for debugger event: 'Debugger:EditorSourceShown' to fire: 1 time(s).
[task 2017-02-20T20:18:22.571869Z] 20:18:22     INFO - Console message: [JavaScript Warning: "A form was submitted in the windows-1252 encoding which cannot encode all Unicode characters, so user input may get corrupted. To avoid this problem, the page should be changed so that the form is submitted in the UTF-8 encoding either by changing the encoding of the page itself to UTF-8 or by specifying accept-charset=utf-8 on the form element." {file: "chrome://mochikit/content/tests/BrowserTestUtils/content-task.js line 52 > eval" line: 4}]
[task 2017-02-20T20:18:22.572953Z] 20:18:22     INFO - Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "http://example.com/browser/devtools/client/debugger/test/mochitest/sjs_post-page.sjs" line: 0}]
[task 2017-02-20T20:18:22.573812Z] 20:18:22     INFO - Console message: [JavaScript Warning: "Use of nsIFile in content process is deprecated." {file: "resource://gre/modules/NetUtil.jsm" line: 304}]
[task 2017-02-20T20:18:22.574955Z] 20:18:22     INFO - Debugger event 'Debugger:EditorSourceShown' fired: 1 time(s).
[task 2017-02-20T20:18:22.575976Z] 20:18:22     INFO - TEST-PASS | devtools/client/debugger/test/mochitest/browser_dbg_post-page.js | Enough 'Debugger:EditorSourceShown' panel events have been fired. - 
[task 2017-02-20T20:18:22.577079Z] 20:18:22     INFO - Source shown: http://example.com/browser/devtools/client/debugger/test/mochitest/sjs_post-page.sjs
[task 2017-02-20T20:18:22.577826Z] 20:18:22     INFO - TEST-PASS | devtools/client/debugger/test/mochitest/browser_dbg_post-page.js | The correct source has been shown. - 
[task 2017-02-20T20:18:22.578857Z] 20:18:22     INFO - TEST-PASS | devtools/client/debugger/test/mochitest/browser_dbg_post-page.js | There should be one source displayed in the view. - 
[task 2017-02-20T20:18:22.579841Z] 20:18:22     INFO - TEST-PASS | devtools/client/debugger/test/mochitest/browser_dbg_post-page.js | The correct source is currently selected in the view. - 
[task 2017-02-20T20:18:22.580768Z] 20:18:22     INFO - Buffered messages finished
[task 2017-02-20T20:18:22.581900Z] 20:18:22     INFO - TEST-UNEXPECTED-FAIL | devtools/client/debugger/test/mochitest/browser_dbg_post-page.js | The currently shown source contains bacon. Mmm, delicious! - Got <script>"GET";</script><form method="POST"><input type="submit"></form>, expected <script>"POST";</script><form method="POST"><input type="submit"></form>
[task 2017-02-20T20:18:22.583277Z] 20:18:22     INFO - Stack trace:
[task 2017-02-20T20:18:22.584175Z] 20:18:22     INFO -     chrome://mochikit/content/browser-test.js:test_is:911
[task 2017-02-20T20:18:22.585091Z] 20:18:22     INFO -     chrome://mochitests/content/browser/devtools/client/debugger/test/mochitest/browser_dbg_post-page.js:null:49
[task 2017-02-20T20:18:22.585959Z] 20:18:22     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:735:9
[task 2017-02-20T20:18:22.586828Z] 20:18:22     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:655:7
[task 2017-02-20T20:18:22.587881Z] 20:18:22     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:791:59
Flags: needinfo?(dd.mozilla)
> Backed out for failing browser_animation_pseudo_elements.js on Linux opt
> without e10s:
This should be: "for failing browser_dbg_post-page.js on Linux x64 opt with e10s"
Ok, the last version does not work when an unknownConverter is involved. Back to the previous version.
Attachment #8839083 - Attachment is obsolete: true
Flags: needinfo?(dd.mozilla)
Attachment #8839584 - Flags: review?(honzab.moz)
(In reply to Dragana Damjanovic [:dragana] from comment #61)
> Created attachment 8839584 [details] [diff] [review]
> bug_1261585_v2.patch
> 
> Ok, the last version does not work when an unknownConverter is involved.
> Back to the previous version.

Is this something I've already reviewed?  If so, just land it.
(In reply to Honza Bambas (:mayhemer) from comment #62)
> (In reply to Dragana Damjanovic [:dragana] from comment #61)
> > Created attachment 8839584 [details] [diff] [review]
> > bug_1261585_v2.patch
> > 
> > Ok, the last version does not work when an unknownConverter is involved.
> > Back to the previous version.
> 
> Is this something I've already reviewed?  If so, just land it.

It is comment 48, I am not you review it in details.
s/I am not you review/I am not sure you have reviewed it in detail.
(In reply to Dragana Damjanovic [:dragana] from comment #61)
> Created attachment 8839584 [details] [diff] [review]
> bug_1261585_v2.patch
> 
> Ok, the last version does not work when an unknownConverter is involved.
> Back to the previous version.

Can you please describe why it fails?  I want to abandon the "yet nice" v3 patch only as the last resort.  Thanks.
Flags: needinfo?(dd.mozilla)
Depends on: 1342386
Flags: needinfo?(dd.mozilla)
Attachment #8839584 - Flags: review?(honzab.moz)
See Also: → 1344561
Honza, I wanted to write you an e-mail (because you miss our last meeting and I was talking about it there):
so we need to allow an addon to modify response body - I looked through our addons and the very first one was modifying the response body :) So I will try to fix all of that, but that includes some reworking of UnknownDecoder and nsURILoader as well.
Oh god :)  OK.  Still I think the comment on bug 1344561 was worth, so that they can think of some better way.

I was also thinking about sending the data back to the parent process, let it be processed (asynchronously) by the traceable channel hook and then sent back to the content process.  In this case I would prefer simplicity of the code changes over a bad performance impact.  But depends, maybe it's crazy and you have already solved this a better way.

Thanks anyway!
Bulk priority update: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: P2 → P1
we found a different way to enable this for webextensions.
Status: REOPENED → RESOLVED
Closed: 3 years ago2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.