Crash in mozilla::dom::ServiceWorkerPrivate::StoreISupports

RESOLVED FIXED in Firefox 60

Status

()

defect
P3
critical
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: philipp, Assigned: bkelly)

Tracking

(Blocks 1 bug, {crash, regression})

60 Branch
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox59 unaffected, firefox60 fixed, firefox61 fixed)

Details

(crash signature)

Attachments

(1 attachment)

Reporter

Description

a year ago
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).
Assignee: nobody → bugmail
Priority: -- → P2
Priority: P2 → P3
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.
I'm going to steal this.
Assignee: bugmail → bkelly
Status: NEW → ASSIGNED
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.
Attachment #8961391 - Flags: review?(catalin.badea392)
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.
Attachment #8961391 - Flags: review?(catalin.badea392) → review+

Comment 6

a year ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8d0bff47a499
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Is this worth a backport to Beta?
Flags: needinfo?(bkelly)
Sure.
Flags: needinfo?(bkelly)
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
Attachment #8961391 - Flags: approval-mozilla-beta?
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
Attachment #8961391 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.