Closed Bug 1271527 Opened 5 years ago Closed 5 years ago

Crash in mozilla::net::WebSocketChannel::Notify

Categories

(Core :: Networking, defect)

Unspecified
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- fixed
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: njn, Assigned: michal)

Details

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

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-c101a497-35f5-40cf-9f2d-dadd72160508.
=============================================================

This crash has been around for a long time but appears to have never had a bug report filed for it.

Since May 1st, there have been 216 occurrences across all versions, as far back as FF36:

https://crash-stats.mozilla.com/signature/?product=Firefox&platform=Windows&date=%3E%3D2016-05-01&signature=mozilla%3A%3Anet%3A%3AWebSocketChannel%3A%3ANotify&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&page=2

Every crash address is 0x0, so it looks like a null pointer dereference. I checked crash reports for numerous versions, in all of them the crashing line was this one in WebSocketChannel.cpp:

> mPingTimer->InitWithCallback(this, mPingResponseTimeout,
>                              nsITimer::TYPE_ONE_SHOT);

Perhaps mPingTimer is null?
michal, I know you didn't write this code but you have been modifying nearby code recently. Could you please take a look?
Flags: needinfo?(michal.novotny)
Assignee: nobody → michal.novotny
Flags: needinfo?(michal.novotny)
Whiteboard: [necko-active]
Attached patch fixSplinter Review
We have a non-null mPingTimer few lines above in the if check, so what happened here is that sending a ping packet in OnOutputStreamReady fails and StopSession is called which nulls out mPingTimer.
Attachment #8751697 - Flags: review?(mcmanus)
Comment on attachment 8751697 [details] [diff] [review]
fix

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

makes sense thanks!
Attachment #8751697 - Flags: review?(mcmanus) → review+
https://hg.mozilla.org/mozilla-central/rev/6f5d73b492f8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment on attachment 8751697 [details] [diff] [review]
fix

Approval Request Comment
[Feature/regressing bug #]: 640003
[User impact if declined]: Crash under specific network conditions 
[Describe test coverage new/current, TreeHerder]: This part of the code is probably not covered by automated tests
[Risks and why]: Very low, it is a simple fix of an obvious bug
[String/UUID change made/needed]: none
Attachment #8751697 - Flags: approval-mozilla-beta?
Attachment #8751697 - Flags: approval-mozilla-aurora?
Comment on attachment 8751697 [details] [diff] [review]
fix

Crash fix, Aurora48+, Beta47+
Attachment #8751697 - Flags: approval-mozilla-beta?
Attachment #8751697 - Flags: approval-mozilla-beta+
Attachment #8751697 - Flags: approval-mozilla-aurora?
Attachment #8751697 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.