Asynchronous creation of Android UI nsThread was potentially racey.

RESOLVED FIXED in Firefox 53

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: rbarker, Assigned: rbarker)

Tracking

Trunk
Firefox 53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
The asynchronous creation of the Android UI nsThread was potentially racey because the thread pointer could be potentially accessed before the thread was initialized.
(Assignee)

Updated

2 years ago
Assignee: nobody → rbarker
(Assignee)

Comment 1

2 years ago
Created attachment 8826725 [details] [diff] [review]
0001-Bug-1331055-Ensure-the-Android-UI-nsThread-has-been-initialized-before-making-it-accessible-17011311-8ac9bfb.patch
(Assignee)

Updated

2 years ago
Attachment #8826725 - Flags: review?(nchen)
(Assignee)

Updated

2 years ago
Depends on: 1328747
Comment on attachment 8826725 [details] [diff] [review]
0001-Bug-1331055-Ensure-the-Android-UI-nsThread-has-been-initialized-before-making-it-accessible-17011311-8ac9bfb.patch

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

::: widget/android/AndroidUiThread.cpp
@@ +124,5 @@
> +    RefPtr<AndroidUiThread> thread = new AndroidUiThread();
> +    thread->InitCurrentThread();
> +    thread->SetObserver(new ThreadObserver());
> +    sMessageLoop = new MessageLoop(MessageLoop::TYPE_MOZILLA_ANDROID_UI, thread.get());
> +    sThread = thread;

So this can still be racy because to other cores/processors, `sThread = thread;` can appear to execute _before_ the previous lines. It's best to either 1) rid of double-checked locking in `GetAndroidUiThread()`, 2) add memory barriers here and in `GetAndroidUiThread()` around sThread accesses, or 3) use something that adds memory barriers for you, like Atomic<> from mfbt/Atomics.h.

If you really don't want to take the lock in `GetAndroidUiThread()`, I think the next best choice is to wrap sThread in an Atomic<> object.
Attachment #8826725 - Flags: review?(nchen) → review-
(Assignee)

Comment 3

2 years ago
Created attachment 8827610 [details] [diff] [review]
0001-Bug-1331055-Ensure-the-Android-UI-nsThread-has-been-initialized-before-making-it-accessible.-r-jchen-17011712-6339446.patch
Attachment #8826725 - Attachment is obsolete: true
(Assignee)

Comment 4

2 years ago
Created attachment 8827612 [details] [diff] [review]
0001-Bug-1331055-Ensure-the-Android-UI-nsThread-has-been-initialized-before-making-it-accessible.-r-jchen-17011712-ee96df4.patch
Attachment #8827610 - Attachment is obsolete: true
(Assignee)

Comment 5

2 years ago
Comment on attachment 8827612 [details] [diff] [review]
0001-Bug-1331055-Ensure-the-Android-UI-nsThread-has-been-initialized-before-making-it-accessible.-r-jchen-17011712-ee96df4.patch

I hope this address the race condition in initialization.
Attachment #8827612 - Flags: review?(nchen)
Attachment #8827612 - Flags: review?(nchen) → review+

Comment 6

2 years ago
Pushed by rbarker@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a9fef37eeae
Ensure the Android UI nsThread has been initialized before making it accessible. r=jchen

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0a9fef37eeae
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.