Closed Bug 1329716 Opened 3 years ago Closed 3 years ago

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

Categories

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

x86
Windows 10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox-esr45 --- wontfix
firefox50 --- wontfix
firefox51 --- fixed
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Crash Data

Attachments

(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:

https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerPrivate.cpp#402

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

https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerPrivate.cpp#602

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

https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerPrivate.cpp#1633

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:

https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerEvents.cpp#112
Attachment #8825121 - Flags: review?(bugmail)
Comment on attachment 8825121 [details] [diff] [review]
bug1329716_updatecrash.patch

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 bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4097854c271e
Try to ensure we don't dispatch a SW event if the registration has been removed. r=asuth
https://hg.mozilla.org/mozilla-central/rev/4097854c271e
Status: ASSIGNED → RESOLVED
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.