Closed
Bug 1273043
Opened 8 years ago
Closed 8 years ago
Crash in mozilla::net::HttpChannelChild::DoOnStartRequest
Categories
(Core :: Networking, defect)
Tracking
()
RESOLVED
FIXED
mozilla50
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
(Keywords: crash, Whiteboard: [necko-active])
Crash Data
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
michal
:
review+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
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?
Attachment #8752657 -
Flags: feedback?(michal.novotny)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•8 years ago
|
||
Here's a version that compiles :)
Attachment #8753110 -
Flags: review?(michal.novotny)
Assignee | ||
Updated•8 years ago
|
Attachment #8752657 -
Attachment is obsolete: true
Attachment #8752657 -
Flags: feedback?(michal.novotny)
Updated•8 years ago
|
Whiteboard: [necko-active]
Comment 5•8 years ago
|
||
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.
Attachment #8753110 -
Flags: review?(michal.novotny) → review?(honzab.moz)
Comment 6•8 years ago
|
||
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.
Attachment #8753110 -
Flags: review?(honzab.moz) → feedback?(jduell.mcbugs)
Comment 8•8 years ago
|
||
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()).
Attachment #8753110 -
Flags: feedback?(jduell.mcbugs) → feedback+
Assignee | ||
Comment 9•8 years ago
|
||
michal, jduell is on PTO and not accepting review requests ATM. Can you review, taking into account comment 8? Thank you.
Attachment #8769989 -
Flags: review?(michal.novotny)
Assignee | ||
Updated•8 years ago
|
Attachment #8753110 -
Attachment is obsolete: true
Comment 10•8 years ago
|
||
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)
Attachment #8769989 -
Flags: review?(michal.novotny) → feedback+
Assignee | ||
Comment 11•8 years ago
|
||
Apologies for the crapness of the previous patch.
Attachment #8770353 -
Flags: review?(michal.novotny)
Assignee | ||
Updated•8 years ago
|
Attachment #8769989 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8770353 -
Flags: review?(michal.novotny) → review+
Assignee | ||
Comment 12•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/df35aa6dffb62d73cbc404094e2cf5ee147bf587 Bug 1273043 - Add a missing null check to HttpChannelChild::DoOnStartRequest(). r=michal.
Comment 13•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/df35aa6dffb6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 14•8 years ago
|
||
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
status-firefox48:
--- → affected
Assignee | ||
Comment 16•8 years ago
|
||
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]:
Flags: needinfo?(n.nethercote)
Attachment #8770353 -
Flags: approval-mozilla-beta?
Comment 17•8 years ago
|
||
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.
Attachment #8770353 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 18•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/73b76f705ac7
You need to log in
before you can comment on or make changes to this bug.
Description
•