Closed Bug 1319606 Opened 4 years ago Closed 4 years ago

Crash in mozilla::net::nsStreamListenerWrapper::OnStopRequest

Categories

(Core :: Networking, defect)

Unspecified
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox52 + fixed
firefox53 + verified disabled

People

(Reporter: n.nethercote, Assigned: mayhemer)

References

Details

(Keywords: crash, Whiteboard: [necko-active])

Crash Data

Attachments

(1 file, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-9c4ae09a-df95-49d4-a50e-f70352161122.
=============================================================

Bug 1274279 existed for this crash signature and it was fixed back in Firefox 49. But there has been a new spike that began slowly Nov 14 and has increased. (There are a couple of crash reports before that but they look different, e.g. the crash address is different.) So this new problem likely has a different root cause to bug 1274279.

This is showing up on both Nightly (53.0a1, #22 topcrash for the past day) and Aurora (52.0a2, #16 topcrash for the past day).

The last merge date was 2016-11-14 so it looks like this crash just slipped in before then.

It's always a null pointer crash. Perhaps mListener is null?

Here's the regression range for Nov 14:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b37be3d705d929ee52280051d58cedc70a47626f&tochange=a516c754042c438a5c1499171ca525a980ecb911

honza, kmckinley, this is the same regression window as bug 1317517. Could it be related?
Flags: needinfo?(odvarko)
Flags: needinfo?(kmckinley)
Sorry, wrong Honza.
Flags: needinfo?(odvarko) → needinfo?(honzab.moz)
It's the #5 topcrash on Aurora on Mac.
Looks like the same crash as bug 1319461.
Duplicate of this bug: 1319461
Crash Signature: [@ mozilla::net::nsStreamListenerWrapper::OnStopRequest] → [@ mozilla::net::nsStreamListenerWrapper::OnStopRequest] [@ mozilla::net::nsStreamListenerWrapper::OnStartRequest]
Possibly a regression from bug 1313595.
Blocks: 1313595
Flags: needinfo?(honzab.moz)
I suggested to back bug 1313595 completely out for now.
Attachment #8814305 - Flags: review?(honzab.moz)
Assignee: nobody → kmckinley
Flags: needinfo?(kmckinley)
Attachment #8814305 - Flags: review?(honzab.moz)
Comment on attachment 8814305 [details]
Bug 1319606 - Missing nsITimerCallback interface in HSTSPrimerListener

https://reviewboard.mozilla.org/r/95568/#review95620

::: netwerk/protocol/http/HSTSPrimerListener.cpp:84
(Diff revision 1)
>      mHSTSPrimingTimer = nullptr;
>    }
>  
>    // if callback is null, we have already canceled this request and reported
>    // the failure
> -  NS_ENSURE_STATE(callback);
> +  if(!callback) {

Space after 'if', please.
Attachment #8814305 - Attachment is obsolete: true
Ah!  Missing nsITimerCallback impl, yes, I missed that.

The tests look good.  Kate, just a nit - it's a good practice to put description to each test what it does (in reasonable details) so that one doesn't need to read the code to understand what steps the test takes.

Question: when you run the test w/o the C changes, do you make it crash?
[Tracking Requested - why for this release]: top 10 browser crash in both Nightly 53 and Aurora 52, and may have the same root cause as bug 1317517, which has also been nominated for tracking.
Whiteboard: [necko-active]
tracking this topcrash for 52/53
Summary: Crash in mozilla::net::nsStreamListenerWrapper::OnStopRequest → Crash in mozilla::net::nsStreamListenerWrapper::OnStopRequest [hsts priming]
We have this correlation:

> (100.0% in signature vs 03.25% overall) Addon "Ghostery" = true

Likewise for the signatures in bug 1317517 and bug 1318759.
This likely is NOT FIXED with the patch for bug 1318759.

Stealing from Kate for now.
Assignee: kmckinley → honzab.moz
Correlation: (100.0% in signature vs 05.54% overall) Addon "Ghostery" = true, this crash spike is around the November 28 which is the last update date to Ghostery.
No longer blocks: 1313595
Status: NEW → ASSIGNED
Jason, lately I don't know who else should review my patches :D

- switches to NS_FORWARD_SAFE_* in nsStreamListenerWrapper
- HttpBaseChannel::SetNewListener (only place that may add listener being null to nsStreamListenerWrapper) was added a null-check -> return ERROR

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfa396bc16e1c00ed19fcb3d4ce1a766b8bfd49b
Attachment #8817620 - Flags: review?(jduell.mcbugs)
Attachment #8817620 - Flags: review?(jduell.mcbugs) → review+
Duplicate of this bug: 1322693
Thanks Jason!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18470be9bf00
Prevent nsStreamListenerWrapper instance w/o mListener. r=jduell
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/18470be9bf00

Landed 17 hours ago.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Honza, can this be uplifted to aurora?  If so can you fill in the uplift request?  Thanks!
Flags: needinfo?(honzab.moz)
Comment on attachment 8817620 [details] [diff] [review]
v1 (null check in SetNewListener, safe forward in stream listener wrapper)

Approval Request Comment
[Feature/Bug causing the regression]: unknown
[User impact if declined]: null deref crash during normal browsing with some widely used extensions
[Is this code covered by automated tests?]: I think it is (devtools, xhr tests exercise it)
[Has the fix been verified in Nightly?]: i never reproduced the crash locally nor on try.  crash stats must be consulted
[Needs manual test from QE? If yes, steps to reproduce]: combination of ghostery and noscript addons may trigger this during heavy sites browsing
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not at all
[Why is the change risky/not risky?]: -
[String changes made/needed]: none
Flags: needinfo?(honzab.moz)
Attachment #8817620 - Flags: approval-mozilla-aurora?
I can confirm this is appears to be working: we haven't had crashes with either signature since Nightly 20161211030209, which is the last Nightly prior to the fix landing. Thank you, Honza.
Status: RESOLVED → VERIFIED
Comment on attachment 8817620 [details] [diff] [review]
v1 (null check in SetNewListener, safe forward in stream listener wrapper)

fix top crash in aurora52, verified in nightly
Attachment #8817620 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Summary: Crash in mozilla::net::nsStreamListenerWrapper::OnStopRequest [hsts priming] → Crash in mozilla::net::nsStreamListenerWrapper::OnStopRequest
Duplicate of this bug: 1320031
You need to log in before you can comment on or make changes to this bug.