1432882 - Crash in RtlRaiseStatus | RtlpUnWaitCriticalSection | RtlLeaveCriticalSection | mozilla::dom::workers::WorkerPrivateParent<T>::EventTarget::Dispatch

Status: RESOLVED WORKSFORME

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

Description

[Nathan Froyd [:froydnj]]

This bug was filed from the Socorro interface and is
report bp-239d8878-5c36-47e0-a707-df5dd0180123.
=============================================================

Top 10 frames of crashing thread:

0 ntdll.dll RtlRaiseStatus 
1 ntdll.dll RtlpUnWaitCriticalSection 
2 ntdll.dll RtlLeaveCriticalSection 
3 xul.dll mozilla::dom::workers::WorkerPrivateParent<mozilla::dom::workers::WorkerPrivate>::EventTarget::Dispatch dom/workers/WorkerPrivate.cpp:7153
4 xul.dll mozilla::dom::workers::MainThreadStopSyncLoopRunnable::DispatchInternal dom/workers/WorkerRunnable.cpp:513
5 xul.dll mozilla::dom::workers::WorkerRunnable::Dispatch dom/workers/WorkerRunnable.cpp:95
6 xul.dll mozilla::dom::workers::WorkerMainThreadRunnable::Run dom/workers/WorkerRunnable.cpp:617
7 xul.dll mozilla::ThrottledEventQueue::Inner::ExecuteRunnable xpcom/threads/ThrottledEventQueue.cpp:193
8 xul.dll mozilla::ThrottledEventQueue::Inner::Executor::Run xpcom/threads/ThrottledEventQueue.cpp:79
9 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1039

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

Filing this bug because the first couple of frames on the stack look very similar to bug 1426067/bug 1425904, and those bugs have UAF potential.

Are we sure that something will hold the WorkerParentPrivate::EventTarget alive across the mWorkerPrivate->DispatchPrivate call?

Comment 1

[Andrea Marchesini [:baku]]

> Are we sure that something will hold the WorkerParentPrivate::EventTarget
> alive across the mWorkerPrivate->DispatchPrivate call?

If seems that we are failing here: https://hg.mozilla.org/releases/mozilla-release/annotate/afa87f9be3a8/dom/workers/WorkerPrivate.cpp#l7153 and not in DispatchPrivate, but yes, it could be an UAF.

Here we have a worker sync event loop. This can be turned down only by calling WorkerPrivate::StopSyncLoop().
This stack shows a failure when dispatching MainThreadStopSyncLoopRunnable, which should call StopSyncLoop().
Only when StopSyncLoop() is called, the EventTarget nullifies mWorkerPrivate.

But, to answer your question: the EventTarget is kept alive by WorkerPrivate via a SyncLoopInfo struct which has a RefPtr<EventTarget> mEventTarget.

Note that a worker cannot shutdown if there is an active sync loop.

Comment 2

[Daniel Veditz [:dveditz]]

Assigning unowned critical/high security bugs to triage owner. Please find an appropriate assignee for this bug.

Comment 3

[Marion Daly [:mdaly]]

:baku, all yours.

Comment 4

[Marion Daly [:mdaly]]

:baku: update?

Comment 5

[Marion Daly [:mdaly]]

:baku update please.

Comment 6

[Marion Daly [:mdaly]]

:bkelly would you want to take this over from baku?

Comment 7

[Andrea Marchesini [:baku]]

All the crashes show that we are in the middle of a XHR request on workers. It can be a SendInternal or a setter runnable.
I would start investing the use of such runnables. The worker is kept alive because of the sync event loop, but it seems that the event target is not available when the main-thread wants to dispatch something.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

I would prefer not to take this right now.  I'm already working on a service worker crash issue and need to get back to the e10s stuff soon.

Andrea, is this a case where the code should be using a control runnable instead of a normal worker runnable to handle the cases where the worker has been notified of shutdown, but is still alive?

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

BTW, what is the new signature since removing WorkerPrivate and the WorkerRef stuff?  This hasn't triggered in 60+ AFAICT.

Comment 10

[Marion Daly [:mdaly]]

:catalin can you take a look?

Comment 11

[Marion Daly [:mdaly]]

Hey ben, sorry that I had to reassign this your way.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

This has still never triggered in 60 or later.  Lets just watch to see if the crash shows up after 60 releases.  If it does not, then we can probably just close this as WFM.

Comment 13

[Andrea Marchesini [:baku]]

No crashes after 60 releases. Let's mark this bug as WFM.