1331055 - Asynchronous creation of Android UI nsThread was potentially racey.

Status: RESOLVED FIXED

(GeckoView :: General, defect)

Description

[Randall Barker [:rbarker]]

The asynchronous creation of the Android UI nsThread was potentially racey because the thread pointer could be potentially accessed before the thread was initialized.

Comment 1

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1331055-Ensure-the-Android-UI-nsThread-has-been-initialized-before-making-it-accessible-17011311-8ac9bfb.patch

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 3

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1331055-Ensure-the-Android-UI-nsThread-has-been-initialized-before-making-it-accessible.-r-jchen-17011712-6339446.patch

Comment 4

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1331055-Ensure-the-Android-UI-nsThread-has-been-initialized-before-making-it-accessible.-r-jchen-17011712-ee96df4.patch

Comment 5

[Randall Barker [:rbarker]]

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.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0a9fef37eeae