1445475 - Crash in mozilla::dom::ServiceWorkerPrivate::StoreISupports

Status: RESOLVED FIXED

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

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-64ffbe71-a83d-42c9-be45-c676c0180313.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll mozilla::dom::ServiceWorkerPrivate::StoreISupports dom/serviceworkers/ServiceWorkerPrivate.cpp:1959
1 xul.dll mozilla::dom::`anonymous namespace'::SWRUpdateRunnable::Run dom/serviceworkers/ServiceWorkerRegistrationImpl.cpp:386
2 xul.dll mozilla::ThrottledEventQueue::Inner::ExecuteRunnable xpcom/threads/ThrottledEventQueue.cpp:193
3 xul.dll mozilla::ThrottledEventQueue::Inner::Executor::Run xpcom/threads/ThrottledEventQueue.cpp:79
4 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1040
5 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:97
6 xul.dll mozilla::ipc::MessagePumpForChildProcess::Run ipc/glue/MessagePump.cpp:301
7 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:319
8 xul.dll MessageLoop::Run ipc/chromium/src/base/message_loop.cc:299
9 xul.dll nsBaseAppShell::Run widget/nsBaseAppShell.cpp:157

=============================================================

this is a low volume crash signature starting to show up at the start of the 60.0b cycle with MOZ_RELEASE_ASSERT(mWorkerPrivate).

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

This is almost assuredly fallout from bug 1432846.  The mWorkerPrivate in the SWP is cleared when TerminateWorker is called, even if the service worker thread is still held alive.  So the check for if mPromiseProxy has cleaned up is not adequate to ensure that StoreISupports() is safe to call.

Probably the easiest fix is to make a MaybeStoreISupports() that does nothing if the mWorkerPrivate is nullptr.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

I'm going to steal this.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Make service worker self-update fallibly store its timer on the private and cleanup if that fails. r=catalinb

Catalin, this patch makes StoreISupports fallible.  If we can't store the timer we just cancel it immediately and let the update destruct due to de-ref.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b222a8304589a29b95c3f214962ec9835a21c96

Comment 5

[Cătălin Badea (:catalinb)]

Comment on attachment 8961391 [details] [diff] [review]
Make service worker self-update fallibly store its timer on the private and cleanup if that fails. r=catalinb

Review of attachment 8961391 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d0bff47a499
Make service worker self-update fallibly store its timer on the private and cleanup if that fails. r=catalinb

Comment 7

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/8d0bff47a499

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Is this worth a backport to Beta?

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Sure.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8961391 [details] [diff] [review]
Make service worker self-update fallibly store its timer on the private and cleanup if that fails. r=catalinb

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1432846
[User impact if declined]: Low frequency assertion.  In beta/release the user may see other crashes or leaks if this condition is hit.
[Is this code covered by automated tests?]: Its a race condition, so we don't really have tests for it.  We have overall service worker regression tests.
[Has the fix been verified in Nightly?]: Its landed in nightly.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: I don't think so.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: This patch makes a method that expects a certain state instead perform a runtime check for that state.  This should be relatively safe.
[String changes made/needed]: None

Comment 11

[Julien Cristau [:jcristau]]

Comment on attachment 8961391 [details] [diff] [review]
Make service worker self-update fallibly store its timer on the private and cleanup if that fails. r=catalinb

service worker fix, beta60+, should be in 60.0b8

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a93e20c50c4b