1509585 - Crash in mozilla::dom::WorkerPrivate::Notify

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect, P1)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-2e06cd13-3fa6-471f-9db7-e1d460181123.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll mozilla::dom::WorkerPrivate::Notify dom/workers/WorkerPrivate.cpp:1699
1 xul.dll mozilla::dom::RemoteWorkerChild::CloseWorkerOnMainThread dom/workers/remoteworkers/RemoteWorkerChild.cpp:515
2 xul.dll mozilla::dom::workerinternals::RuntimeService::UnregisterWorker dom/workers/RuntimeService.cpp:1536
3 xul.dll ?Run@TopLevelWorkerFinishedRunnable@?A0x30C6BC27@dom@mozilla@@EEAA?AW4nsresult@@XZ$3a0bfecfb3064f89e01e6f0b017f7aa0 dom/workers/WorkerPrivate.cpp:295
4 xul.dll mozilla::ThrottledEventQueue::Inner::Executor::Run xpcom/threads/ThrottledEventQueue.cpp:84
5 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1244
6 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:530
7 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:97
8 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:318
9 xul.dll MessageLoop::Run ipc/chromium/src/base/message_loop.cc:298

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

this content crash signature is regressing on 65.0a1 since build 20181120100045, so timing wise that looks related to 1438945. so far the crash is only reported from installations on windows 7.

Comment 1

[Yaron Tausky [:ytausky]]

This looks like another use-after-free, at least from a quick look. :mrbkap, could you please have a look to see if something pops up?

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

All of the crashes are at 0x10 (or 0xc), which suggests a null pointer somewhere. I'll dig in more here.

Comment 3

[Daniel Veditz [:dveditz]]

This looks like a null ptr deref, not a use-after-free.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cb139da8f47b52109a8d0198663373e695a4e4e

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1509585 - Remove some redundant worker shutdown code. r=asuth

This code races with the WeakWorkerRef shutdown code that sets both `mWorkerState` and `mWorkerPrivate` (though on different threads). This patch is based on the observation that except for failure cases, we can't get to `RuntimeServiceWorker::UnregisterWorker` without having already notified the `WorkerRef`s.

Comment 6

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/cbfa24a98e21

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 9030530 [details]
Bug 1509585 - Remove some redundant worker shutdown code. r=asuth

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1438945

User impact if declined: Random crashes on websites using shared workers.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): We have audited the code and the patch simply removes a call that isn't needed any more.

String changes made/needed:

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9030530 [details]
Bug 1509585 - Remove some redundant worker shutdown code. r=asuth

[Triage Comment]
Removes some redundant code to fix random crashes on websites using shared workers. Approved for 65.0b7.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6e7f4acecc9f