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

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: bug1329716_updatecrash.patch

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 2

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8825121 [details] [diff] [review]
bug1329716_updatecrash.patch

Andrew pointed out nsMainThreadPtrHandle doesn't work anything like I thought.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Try to ensure we don't dispatch a SW event if the registration has been removed. r=asuth

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.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
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 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4097854c271e

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

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 7

[Gerry Chang [:gchang]]

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.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4b07f1c2f325a658df284da46377ec1b39e9c28f

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

https://hg.mozilla.org/releases/mozilla-beta/rev/32ef31f34139dcbcae6299a47b5bb637fb117535