Crash in @0x0 | nsTimerImpl::Fire

RESOLVED FIXED

Status

()

Core
XPCOM
P1
critical
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mccr8, Unassigned)

Tracking

({crash, regression})

unspecified
x86
Windows 10
crash, regression
Points:
---

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox50 unaffected, firefox51 unaffected, firefox52+ fixed, firefox53 fixed, firefox54 fixed)

Details

(crash signature)

(Reporter)

Description

a year ago
This bug was filed from the Socorro interface and is 
report bp-d47f3944-ce02-4eec-9284-b61c62161215.
=============================================================

I only see a few hundred crashes like this across all branches, but it looks like we sometimes try to run mCallback.c when it is null. It seems like we should null check that.
I don't see how mCallback.c can reasonably be nullptr while mCallbackType is CallbackType::Function.
It could be null if a timer was cross-thread cancelled while it was in the midst of firing. :(  Would be interesting to know if the crash was more or less common after bug 1157323 landed.
In the past 7 days there have been 228 crashes with this signature in Aurora 52, and 0 in Nightly 53, which suggests that something may have landed that fixed it. froydnj, any idea what it might have been? If it has been fixed, it would be nice to uplift the fix to Aurora.
Flags: needinfo?(nfroyd)
I think Ehsan has been refactoring some nsGlobalWindow code in FF53 that uses nsITimer.  I don't have any reason to believe it would cause this, but perhaps its related.

There may also be some changes related to requestIdleCallback that go through timers.
status-firefox50: --- → unaffected
status-firefox51: --- → unaffected
status-firefox52: --- → affected
status-firefox53: --- → unaffected
status-firefox-esr45: --- → unaffected

Comment 5

a year ago
the crash first appeared in 52.0a2 build 20161121004022 - this is the pushlog to the day before: https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=e59ed8b3ba13fd33435eea75da37ea6029c9af40&tochange=a99c9599b45835479e6e18da0614b0652d42435a
Maybe we should consider uplifting bug 132864 to beta if it lands in early enough in the release cycle.  It adds locking to the timer cancelation.
Blocks: 1328643

Updated

a year ago
Crash Signature: [@ @0x0 | nsTimerImpl::Fire] → [@ @0x0 | nsTimerImpl::Fire] [@ nsTimerImpl::Fire ]
Keywords: regression
status-firefox53: unaffected → affected
status-firefox54: --- → affected
See Also: → bug 1313822
[Tracking Requested - why for this release]: #17 top crash for 52.0b in last 7 days.
tracking-firefox52: --- → ?
Priority: -- → P1

Comment 8

a year ago
this crash seems to have turned into [@0x0 | nsTimerEvent::Run] in 52.0b2
Crash Signature: [@ @0x0 | nsTimerImpl::Fire] [@ nsTimerImpl::Fire ] → [@ @0x0 | nsTimerImpl::Fire] [@ nsTimerImpl::Fire ] [@ @0x0 | nsTimerEvent::Run]
Tracking as regression and top crash in 52.
tracking-firefox52: ? → +
I would like to see what the patch from bug 1328643 does to this crash rate on Nightly before doing anything here.
Flags: needinfo?(nfroyd)
Looks like bug 1328643's patch helped the crash rate a lot! Should we uplift that?

Given it's no longer a super-top-crasher, should we un-track for 52?
Flags: needinfo?(nfroyd)
Flags: needinfo?(jcristau)
Got this backward...
No longer blocks: 1328643
Depends on: 1328643

Updated

a year ago
See Also: → bug 1306733
(In reply to Andrew Overholt [:overholt] from comment #11)
> Looks like bug 1328643's patch helped the crash rate a lot! Should we uplift
> that?

I would be OK with uplifting that.
Flags: needinfo?(nfroyd)
Bug 1328643 seems to have fixed this.
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Agreed, the rate seems to have fallen off a cliff starting with 52b5 as well. \m/
status-firefox52: affected → fixed
status-firefox53: affected → fixed
status-firefox54: affected → fixed
Flags: needinfo?(jcristau)
You need to log in before you can comment on or make changes to this bug.