1313740 - [e10s] Crash in mozilla::net::HttpChannelChild::OverrideWithSynthesizedResponse with add-on Decentraleyes

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[6lobe]

This bug was filed from the Socorro interface and is 
report bp-42d5dd1a-da14-4a6c-9b8c-b20422161028.
=============================================================

STR:

1. Install the addon Decentraleyes from here: https://addons.mozilla.org/en-US/firefox/addon/decentraleyes/

2. Navigate to: https://www.bytemark.co.uk/

3. Refresh the page (you may have to do this a few times).

4. Crash.


Here is the regression range:

22:53.79 INFO: Narrowed nightly regression window from [2015-06-05, 2015-06-08] (3 days) to [2015-06-07, 2015-06-08] (1 days) (~0 steps left)
22:53.79 INFO: Got as far as we can go bisecting nightlies...
22:53.79 INFO: Last good revision: 7d4ab4a9febd (2015-06-07)
22:53.79 INFO: First bad revision: 4700d1cdf489 (2015-06-08)
22:53.79 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7d4ab4a9febd&tochange=4700d1cdf489

I couldnt bisect further because I got some errors.

Comment 1

[Loic]

I can't reproduce the crash with FF49 or 52 on Win 7. I installed the add-on, refreshed the page 20x, no crash.


Is the bug only reproducible on Linux?

Comment 2

[6lobe]

Sorry I forgot to mention this only happens with e10s enabled.

I can only test on Linux. The crash never happens on first page load. Most of the time it happens on second page load.

I guess this could be the source of the issue? https://bugzilla.mozilla.org/show_bug.cgi?id=1164397

Comment 3

[Loic]

Well, just forget my previous comment, I can reproduce it now, but only with e10s enabled like you.
CR FF52:
https://crash-stats.mozilla.com/report/index/55c2304f-1897-407d-9e33-d25042161029

And I confirm the regression range too.

Comment 4

[Valentin Gosu [:valentin] (he/him)]

The problem seems to be that in HttpChannelChild::OnRedirectVerifyCallback we QI mRedirectChannelChild to nsIHttpChannelChild - but in this case mRedirectChannelChild is a DataChannelChild instead.
Not sure exactly what we should do in this case, but it should be easy to avoid the crash.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Josh, any idea what we should do with a DataChannelChild? I assume we should make sure we call CompleteRedirectSetup on it, right?

Comment 6

[Josh Matthews [:jdm]]

Yeah, looks like https://dxr.mozilla.org/mozilla-central/rev/e3279760cd977aac30bd9e8032d3ee71f55d2a67/netwerk/protocol/http/HttpChannelChild.cpp#1532 should be QIing to nsIChildChannel to ensure we deal properly with data channels.

Comment 7

[Valentin Gosu [:valentin] (he/him)]

Attachment: Handle null mNewChannel in OverrideWithSynthesizedResponse

MozReview-Commit-ID: 702H0eJKdx1

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 8806186 [details] [diff] [review]
Handle null mNewChannel in OverrideWithSynthesizedResponse

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

I really can't review this, sorry :/

Comment 9

[Valentin Gosu [:valentin] (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/af46e53372abd9a03188e580dbe76ac54f209ed6
Bug 1313740 - Handle null mNewChannel in OverrideWithSynthesizedResponse r=jdm

Comment 10

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

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

Comment 11

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8806186 [details] [diff] [review]
Handle null mNewChannel in OverrideWithSynthesizedResponse

Approval Request Comment
[Feature/regressing bug #]: bug 1164397
[User impact if declined]: Crash with the decentraleyes extension installed under e10s when a redirect to a data:// URI occurs
[Describe test coverage new/current, TreeHerder]: currently working on a test.
[Risks and why]: Very low. Just a avoiding a null pointer deref.
[String/UUID change made/needed]: None.

Comment 12

[Gerry Chang [:gchang]]

Comment on attachment 8806186 [details] [diff] [review]
Handle null mNewChannel in OverrideWithSynthesizedResponse

Fix a crash. Take it in 51 aurora.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8cc3ea80f82c

Comment 14

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8806186 [details] [diff] [review]
Handle null mNewChannel in OverrideWithSynthesizedResponse

This is in 51, updating 50 uplift request from beta to release

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8806186 [details] [diff] [review]
Handle null mNewChannel in OverrideWithSynthesizedResponse

Low risk crash fix, let's include it in 50.1.0

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/b977ed05722e