Closed
Bug 1331055
Opened 7 years ago
Closed 7 years ago
Asynchronous creation of Android UI nsThread was potentially racey.
Categories
(GeckoView :: General, defect)
GeckoView
General
Tracking
(firefox53 fixed)
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: rbarker, Assigned: rbarker)
References
Details
Attachments
(1 file, 2 obsolete files)
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•7 years ago
|
Assignee: nobody → rbarker
Assignee | ||
Comment 1•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8826725 -
Flags: review?(nchen)
Comment 2•7 years ago
|
||
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•7 years ago
|
||
Attachment #8826725 -
Attachment is obsolete: true
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8827610 -
Attachment is obsolete: true
Assignee | ||
Comment 5•7 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)
Updated•7 years ago
|
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
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0a9fef37eeae
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Updated•5 years ago
|
Product: Firefox for Android → GeckoView
Updated•5 years ago
|
Target Milestone: Firefox 53 → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•