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

RESOLVED FIXED in Firefox 52

Status

()

defect
--
critical
RESOLVED FIXED
3 years ago
5 months ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

({crash})

unspecified
mozilla54
Unspecified
macOS
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr45 wontfix, firefox51 wontfix, firefox52 fixed, firefox53 fixed, firefox54 fixed)

Details

(crash signature)

Attachments

(1 attachment)

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.
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
Attachment #8830771 - Flags: review?(amarchesini)
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.
Blocks: 1105194
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.
Attachment #8830771 - Flags: review?(amarchesini) → review+
Pushed by bkelly@mozilla.com:
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 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
Attachment #8830771 - Flags: approval-mozilla-beta?
Attachment #8830771 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/3267ca391018
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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
Attachment #8830771 - Flags: approval-mozilla-beta?
Attachment #8830771 - Flags: approval-mozilla-beta+
Attachment #8830771 - Flags: approval-mozilla-aurora?
Attachment #8830771 - Flags: approval-mozilla-aurora+
See Also: → 1402731
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.