Closed Bug 1329716 Opened 3 years ago Closed 3 years ago

Crash in mozilla::dom::workers::ServiceWorkerRegistrationInfo::MaybeScheduleTimeCheckAndUpdate


(Core :: DOM: Service Workers, defect, critical)

Windows 10
Not set



Tracking Status
firefox-esr45 --- wontfix
firefox50 --- wontfix
firefox51 --- fixed
firefox52 --- fixed
firefox53 --- fixed


(Reporter: bkelly, Assigned: bkelly)


(Blocks 1 open bug)


(Keywords: crash)

Crash Data


(1 file, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-ad9c3775-7eca-410e-81c1-e513b2170106.

This is crashing due to a nullptr mRegistration here:

This happens because ExtendableFunctionalEventWorkerRunnable::PostRun() has a nullptr mRegistration:

This is nullptr because mRegistration is actually an nsMainThreadPtrHandle.  This can be consumed in the main WorkerRun().  For example, like here:

We should check for nullptr instead of crashing.
Attached patch bug1329716_updatecrash.patch (obsolete) — Splinter Review
This patch makes us skip the automatic post-event-dispatch update if something else in the WorkerRun() method consumed the mRegistration.

AFAICT, this only happens when CancelChannelRunnable takes it.  Fortunately that runnable also triggers an update:
Attachment #8825121 - Flags: review?(bugmail)
Comment on attachment 8825121 [details] [diff] [review]

Andrew pointed out nsMainThreadPtrHandle doesn't work anything like I thought.
Attachment #8825121 - Flags: review?(bugmail)
As we discussed on irc this avoids the event dispatch if the registration went away.  It also strengthens the assertions that if the event runnable is created it must have a registration.
Attachment #8825121 - Attachment is obsolete: true
Attachment #8825166 - Flags: review?(bugmail)
Attachment #8825166 - Flags: review?(bugmail) → review+
Pushed by
Try to ensure we don't dispatch a SW event if the registration has been removed. r=asuth
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8825166 [details] [diff] [review]
Try to ensure we don't dispatch a SW event if the registration has been removed. r=asuth

Approval Request Comment
[Feature/Bug causing the regression]: Service workers
[User impact if declined]: About 20 crashes a week in release/beta channels
[Is this code covered by automated tests?]: Existing automated tests, but none of them caught this corner case.
[Has the fix been verified in Nightly?]: Its landed in nightly but we don't have steps to reproduce.  The fix was made based on crash-stats and code inspection.
[Needs manual test from QE? If yes, steps to reproduce]: None
[List of other uplifts needed for the feature/fix]:None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Its basically just an extra nullptr check and some diagnostic assertions.
[String changes made/needed]: None
Attachment #8825166 - Flags: approval-mozilla-beta?
Attachment #8825166 - Flags: approval-mozilla-aurora?
Comment on attachment 8825166 [details] [diff] [review]
Try to ensure we don't dispatch a SW event if the registration has been removed. r=asuth

Fix a crash. Beta51+ & Aurora52+. Should be in 51 beta 14.
Attachment #8825166 - Flags: approval-mozilla-beta?
Attachment #8825166 - Flags: approval-mozilla-beta+
Attachment #8825166 - Flags: approval-mozilla-aurora?
Attachment #8825166 - Flags: approval-mozilla-aurora+
Blocks: 1334480
You need to log in before you can comment on or make changes to this bug.