1319606 - Crash in mozilla::net::nsStreamListenerWrapper::OnStopRequest

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Nicholas Nethercote [inactive]]

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?

Comment 1

[Nicholas Nethercote [inactive]]

Sorry, wrong Honza.

Comment 2

[Nicholas Nethercote [inactive]]

It's the #5 topcrash on Aurora on Mac.

Comment 3

[Josh Matthews [:jdm]]

Looks like the same crash as bug 1319461.

Comment 5

[Honza Bambas (:mayhemer)]

Possibly a regression from bug 1313595.

Comment 6

[Honza Bambas (:mayhemer)]

I suggested to back bug 1313595 completely out for now.

Comment 7

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Bug 1319606 - Missing nsITimerCallback interface in HSTSPrimerListener

r?mayhemer

Review commit: https://reviewboard.mozilla.org/r/95568/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95568/

Comment 8

[Nicholas Nethercote [inactive]]

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.

Comment 9

[Honza Bambas (:mayhemer)]

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?

Comment 10

[Nicholas Nethercote [inactive]]

[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.

Comment 11

[Julien Cristau [:jcristau]]

tracking this topcrash for 52/53

Comment 12

[Nicholas Nethercote [inactive]]

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.

Comment 13

[Honza Bambas (:mayhemer)]

This likely is NOT FIXED with the patch for bug 1318759.

Stealing from Kate for now.

Comment 14

[Honza Bambas (:mayhemer)]

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.

Comment 15

[Honza Bambas (:mayhemer)]

Attachment: v1 (null check in SetNewListener, safe forward in stream listener wrapper)

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

Comment 17

[Honza Bambas (:mayhemer)]

Thanks Jason!

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18470be9bf00
Prevent nsStreamListenerWrapper instance w/o mListener. r=jduell

Comment 19

[Guilherme Lima]

https://hg.mozilla.org/mozilla-central/rev/18470be9bf00

Landed 17 hours ago.

Comment 20

[Julien Cristau [:jcristau]]

Honza, can this be uplifted to aurora?  If so can you fill in the uplift request?  Thanks!

Comment 21

[Honza Bambas (:mayhemer)]

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

Comment 22

[Nicholas Nethercote [inactive]]

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.

Comment 23

[Julien Cristau [:jcristau]]

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

Comment 24

[Carsten Book [:Tomcat]]

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