Closed Bug 1329716 Opened 3 years ago Closed 3 years ago
Crash in mozilla::dom::workers::Service
Worker Registration Info::Maybe Schedule Time Check And Update
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.
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
Comment on attachment 8825121 [details] [diff] [review] bug1329716_updatecrash.patch Andrew pointed out nsMainThreadPtrHandle doesn't work anything like I thought.
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 #8825166 - Flags: review?(bugmail) → review+
Pushed by firstname.lastname@example.org: 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
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
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.
You need to log in before you can comment on or make changes to this bug.