Closed
Bug 1261585
Opened 8 years ago
Closed 7 years ago
nsiTraceableChannel listener not working as expected with e10s enabled
Categories
(Core :: Networking: HTTP, defect, P1)
Tracking
()
RESOLVED
WONTFIX
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
Reporter | ||
Comment 1•8 years ago
|
||
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
Updated•8 years ago
|
Component: Untriaged → Networking: HTTP
Product: Firefox → Core
Updated•8 years ago
|
Comment 2•8 years ago
|
||
This sounds like something we might want to shim.
Comment 3•8 years ago
|
||
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)
Reporter | ||
Comment 4•8 years ago
|
||
For information, Video DownloadHelper 5.5.0 uses nsITraceableChannel. I'm not aware of an add-on SDK API providing the nsITraceableChannel service.
Assignee | ||
Comment 5•8 years ago
|
||
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.
Comment 6•8 years ago
|
||
jason - can you find an owner.. this is a hole that needs filling.
Assignee: nobody → jduell.mcbugs
Whiteboard: [necko-active]
Updated•8 years ago
|
Assignee: jduell.mcbugs → dd.mozilla
Whiteboard: [necko-active] → [necko-next]
Assignee | ||
Updated•8 years ago
|
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 7•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=10a51625ad3e
Assignee | ||
Comment 8•8 years ago
|
||
Attachment #8764866 -
Flags: review?(jduell.mcbugs)
Updated•8 years ago
|
Priority: -- → P2
Whiteboard: [necko-next] → [necko-next] triaged
Comment 9•8 years ago
|
||
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+
Assignee | ||
Comment 10•8 years ago
|
||
(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.
Assignee | ||
Comment 11•8 years ago
|
||
Attachment #8764866 -
Attachment is obsolete: true
Attachment #8769803 -
Flags: review?(honzab.moz)
Updated•8 years ago
|
Whiteboard: [necko-next] triaged → [necko-active] triaged
Comment 12•8 years ago
|
||
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+
Assignee | ||
Comment 13•8 years ago
|
||
the try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cf28a4c89d2
Assignee | ||
Comment 14•8 years ago
|
||
Attachment #8769803 -
Attachment is obsolete: true
Attachment #8772372 -
Flags: review+
Assignee | ||
Comment 15•8 years ago
|
||
(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.
Assignee | ||
Comment 16•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3fa65fada88
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 17•8 years ago
|
||
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
Comment 18•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e1f3ed984607
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Assignee | ||
Comment 19•8 years ago
|
||
May I ask you to back this path out because of bug 1292586? Thanks!
Flags: needinfo?(wkocher)
Backouts: https://hg.mozilla.org/mozilla-central/rev/3345f0e8ed3b https://hg.mozilla.org/releases/mozilla-aurora/rev/7b44f5b3b0bb
Comment 21•8 years ago
|
||
Kinda scary that automated tests didn't catch that.
Status: RESOLVED → REOPENED
Flags: in-testsuite?
Resolution: FIXED → ---
Comment 22•7 years ago
|
||
So, since the changes were reverted back, how the extensions devs should move on?
Comment 23•7 years ago
|
||
(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)
Assignee | ||
Comment 24•7 years ago
|
||
(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)
Assignee | ||
Comment 25•7 years ago
|
||
In HttpChannelParent.h there is an explanation how all of this work.
Attachment #8772372 -
Attachment is obsolete: true
Attachment #8820259 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 26•7 years ago
|
||
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)
Assignee | ||
Comment 27•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f46965420ebbeb4c44c672fc1370cbbda60d109
Comment 28•7 years ago
|
||
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+
Assignee | ||
Comment 29•7 years ago
|
||
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)
Assignee | ||
Comment 30•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=90e9c125bcf4d9c4e5038016d88ae9d4566cc81f
Updated•7 years ago
|
Attachment #8823218 -
Flags: review?(honzab.moz)
Comment 31•7 years ago
|
||
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+
Assignee | ||
Comment 32•7 years ago
|
||
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.
Assignee | ||
Comment 33•7 years ago
|
||
Attachment #8823218 -
Attachment is obsolete: true
Attachment #8827501 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 34•7 years ago
|
||
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
Comment 35•7 years ago
|
||
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.
Comment 36•7 years ago
|
||
Regression Range: Bad... https://hg.mozilla.org/integration/mozilla-inbound/rev/9157dfae3c3e8e451295bf2c6c196dcd5f14ef55 Good... https://hg.mozilla.org/integration/mozilla-inbound/rev/74790f4d30de03105799064332f44d045e40723e
Comment 37•7 years ago
|
||
Confirming...getting same symptoms, except with hg pushloghtml pages.
Comment 38•7 years ago
|
||
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.
Comment 39•7 years ago
|
||
Could you please help to check the issue in comment 35, thanks.
Flags: needinfo?(poirot.alex)
Flags: needinfo?(honzab.moz)
Flags: needinfo?(dd.mozilla)
Comment 40•7 years ago
|
||
Backout by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f930d9fbb403 Backed out 2 changesets for causing website bustage.
Comment 41•7 years ago
|
||
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.
Assignee | ||
Comment 42•7 years ago
|
||
(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)
Assignee | ||
Comment 43•7 years ago
|
||
I cannot reproduce it. Can you give me steps to reproduce it? Do you have an addon install?
Flags: needinfo?(ajarope)
Comment 45•7 years ago
|
||
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.
Assignee | ||
Comment 46•7 years ago
|
||
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)
Assignee | ||
Comment 47•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cb16e4662fa38cd4803678ecaebc0c41017802a
Comment 48•7 years ago
|
||
(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 49•7 years ago
|
||
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)
Assignee | ||
Comment 50•7 years ago
|
||
(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)
Assignee | ||
Comment 51•7 years ago
|
||
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)
Assignee | ||
Comment 52•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=33689d4dea38cea805daa714668eabc009ddd7f5
Comment 53•7 years ago
|
||
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+
Assignee | ||
Comment 54•7 years ago
|
||
Attachment #8834381 -
Attachment is obsolete: true
Attachment #8838433 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 56•7 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #55) > Needs rebasing. I rebased it yesterday :( Thanks!
Assignee | ||
Comment 57•7 years ago
|
||
Attachment #8838433 -
Attachment is obsolete: true
Attachment #8839083 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 58•7 years ago
|
||
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
Comment 59•7 years ago
|
||
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)
Comment 60•7 years ago
|
||
> 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"
Assignee | ||
Comment 61•7 years ago
|
||
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)
Comment 62•7 years ago
|
||
(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.
Assignee | ||
Comment 63•7 years ago
|
||
(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.
Assignee | ||
Comment 64•7 years ago
|
||
s/I am not you review/I am not sure you have reviewed it in detail.
Comment 65•7 years ago
|
||
(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)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(dd.mozilla)
Attachment #8839584 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 66•7 years ago
|
||
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.
Comment 67•7 years ago
|
||
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!
Comment 68•7 years ago
|
||
Bulk priority update: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: P2 → P1
Assignee | ||
Comment 69•7 years ago
|
||
we found a different way to enable this for webextensions.
Status: REOPENED → RESOLVED
Closed: 8 years ago → 7 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•