1334165 - Crash in mozilla::dom::workers::WorkerPrivateParent<T>::DispatchPrivate

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

This bug was filed from the Socorro interface and is 
report bp-eaff2e19-48e4-44df-8b2b-5939c2170125.
=============================================================

This appears to be a crash in WebSocket usage in workers.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Don't allow event dispatch after WebSocket allows worker thread to exit cleanly. r=baku

I believe this will fix the problem.  The Dispatch() method checks mWorkerShuttingDown but we aren't always setting that.  In particular, if the WebSocket drops its WorkerHolder before the Notify() is called then the mWorkerPrivate is cleared without setting mWorkerShuttingDown.  This leaves a race where Dispatch() can nullptr deref.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cd05c8a487b4e77365c0e8c8d492bfee9371fb3

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

I believe this is a case missed in bug 1105194.  As I noted in the previous comment, though, it should only result in a nullptr deref; not a UAF.  So this is not a sec bug AFAICT.

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 8830771 [details] [diff] [review]
Don't allow event dispatch after WebSocket allows worker thread to exit cleanly. r=baku

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

Yes, we set this boolean to true only if the holder is notified. We need to do it also when Disconnect() is called.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3267ca391018
Don't allow event dispatch after WebSocket allows worker thread to exit cleanly. r=baku

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8830771 [details] [diff] [review]
Don't allow event dispatch after WebSocket allows worker thread to exit cleanly. r=baku

Approval Request Comment
[Feature/Bug causing the regression]: Web sockets and missed case from bug 1105194
[User impact if declined]: Low frequency crashes
[Is this code covered by automated tests?]: This is a race condition that our automated tests do not catch
[Has the fix been verified in Nightly?]: Its a race and we don't have STR, so we cna't manually verify.
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Minimal risk
[Why is the change risky/not risky?]: We simply set a flag that forces us to take an existing error path.  Very low chance of regression.
[String changes made/needed]: None

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3267ca391018

Comment 7

[Julien Cristau [:jcristau]]

Comment on attachment 8830771 [details] [diff] [review]
Don't allow event dispatch after WebSocket allows worker thread to exit cleanly. r=baku

websocket/worker crash fix for aurora53 and beta52

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9520740de782

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4271550cd0b4