1273043 - Crash in mozilla::net::HttpChannelChild::DoOnStartRequest

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Nicholas Nethercote [inactive]]

This bug was filed from the Socorro interface and is 
report bp-00127344-3a9a-4506-8280-f0b752160515.
=============================================================

This is a low-volume, longstanding crash for which no file has previously been filed. In the past 7 days it's occurred 90 times, in Firefox versions ranging from 44 to 49.

The crash address is always 0x0, so it's a null deref. I looked at a couple of minidumps and I *think* the problem is that mListener is null, but I'm not 100% certain.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Add a missing null check to HttpChannelChild::DoOnStartRequest()

I'm not sure about this. HttpBaseChannel has lots of null checks for mListener,
but HttpChannelChild has none. I'm not even sure if this null check I've added
will fix the crash... but if it does, perhaps other functions in
HttpChannelChild should also have null checks?

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Add a missing null check to HttpChannelChild::DoOnStartRequest()

Here's a version that compiles :)

Comment 3

[Dragana Damjanovic [:dragana]]

we already have a bug for this - bug 1272611. I will close the other one :)

Comment 5

[Michal Novotny [:michal]]

Comment on attachment 8753110 [details] [diff] [review]
Add a missing null check to HttpChannelChild::DoOnStartRequest()

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

I think Honza or Patrick will be a better reviewer here. Anyway, it seems to me like a wrong fix. If mListener is null in HttpChannelChild::DoOnStartRequest then the channel was not set up correctly or OnStart is called after OnStop. In both cases we should find the cause.

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 8753110 [details] [diff] [review]
Add a missing null check to HttpChannelChild::DoOnStartRequest()

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

This is pretty much unexpected.  I think the best one to look at this is jason that wrote most of this code originally and best understands the queuing logic here.  As michal said, we probably call some spot that releases the listener sooner than we get to onstart.  That is not a correct code path.

I'd rather spend some time to figure the true cause here than just add a non-null check.

Comment 7

[Nicholas Nethercote [inactive]]

jduell: 22 day feedback ping.

Comment 8

[Jason Duell]

Comment on attachment 8753110 [details] [diff] [review]
Add a missing null check to HttpChannelChild::DoOnStartRequest()

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

Sorry for review delay.

I agree with Honza and Michal here--if we get to this point, we've hit a serious programming error.  So I want to add in an assert to see if we can get useful stack traces from buildbots, etc.

While we track that down, it does make sense to see if we can recover from the error (by canceling the channel, as you've done).

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +522,5 @@
>  HttpChannelChild::DoOnStartRequest(nsIRequest* aRequest, nsISupports* aContext)
>  {
>    LOG(("HttpChannelChild::DoOnStartRequest [this=%p]\n", this));
> +  if (!mListener) {
> +    Cancel(NS_ERROR_FAILURE);

Let's also add a MOZ_ASSERT(mListener) above the check for !mListener here.

We'll also need to add a check for !mListener in DoOnStopRequest (otherwise we'll crash when we blindly call mListener->OnStopRequest()).

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: Add a missing null check to HttpChannelChild::DoOnStartRequest()

michal, jduell is on PTO and not accepting review requests ATM. Can you review,
taking into account comment 8? Thank you.

Comment 10

[Michal Novotny [:michal]]

Comment on attachment 8769989 [details] [diff] [review]
Add a missing null check to HttpChannelChild::DoOnStartRequest()

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

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +952,5 @@
>    MOZ_ASSERT(!mOnStopRequestCalled,
>               "We should not call OnStopRequest twice");
> +
> +  // In theory mListener should not be null, but in practice sometimes it is.
> +  MOZ_ASSERT(mListener)

semicolon is missing

@@ +953,5 @@
>               "We should not call OnStopRequest twice");
> +
> +  // In theory mListener should not be null, but in practice sometimes it is.
> +  MOZ_ASSERT(mListener)
> +  if (!mListener) {

this should be if (mListener)

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: Add a missing null check to HttpChannelChild::DoOnStartRequest()

Apologies for the crapness of the previous patch.

Comment 12

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/df35aa6dffb62d73cbc404094e2cf5ee147bf587
Bug 1273043 - Add a missing null check to HttpChannelChild::DoOnStartRequest(). r=michal.

Comment 13

[Carsten Book [:Tomcat]]

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

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

Crash volume for signature 'mozilla::net::HttpChannelChild::DoOnStartRequest':
 - nightly (version 50): 75 crashes from 2016-06-06.
 - aurora  (version 49): 229 crashes from 2016-06-07.
 - beta    (version 48): 502 crashes from 2016-06-06.
 - release (version 47): 0 crashes from 2016-05-31.
 - esr     (version 45): 0 crashes from 2016-04-07.

Crash volume on the last weeks:
            W. N-1  W. N-2  W. N-3  W. N-4  W. N-5  W. N-6  W. N-7
 - nightly       3       9      13       9      10      13      13
 - aurora       21      29      34      36      37      36      27
 - beta         53      85      54      50      95      60      68
 - release       0       0       0       0       0       0       0
 - esr           0       0       0       0       0       0       0

Affected platforms: Windows, Mac OS X, Linux

Comment 15

[[:philipp]]

hi, can we uplift the fix to 49?

Comment 16

[Nicholas Nethercote [inactive]]

Comment on attachment 8770353 [details] [diff] [review]
Add a missing null check to HttpChannelChild::DoOnStartRequest()

Approval Request Comment

[Feature/regressing bug #]: Unknown.

[User impact if declined]: Crashes.

[Describe test coverage new/current, TreeHerder]: No specific tests that I know of. Patch has been in 50 for over a month now without problem.

[Risks and why]: very low. Patch is almost trivial, just adding a couple of null pointer checks.

[String/UUID change made/needed]:

Comment 17

[Gerry Chang [:gchang]]

Comment on attachment 8770353 [details] [diff] [review]
Add a missing null check to HttpChannelChild::DoOnStartRequest()

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

This patch fixes a crash. Take it in 49 beta. Should be in 49 beta 8.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/73b76f705ac7