Asynchronous creation of Android UI nsThread was potentially racey.

RESOLVED FIXED in Firefox 53

Status

defect
RESOLVED FIXED
3 years ago
7 months ago

People

(Reporter: rbarker, Assigned: rbarker)

Tracking

Trunk
mozilla53
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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: nobody → rbarker
Attachment #8826725 - Flags: review?(nchen)
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-
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+
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
https://hg.mozilla.org/mozilla-central/rev/0a9fef37eeae
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 53 → mozilla53
You need to log in before you can comment on or make changes to this bug.