Closed
Bug 987415
Opened 10 years ago
Closed 10 years ago
GeckoAppShell.gPendingEvents is not thread safe
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox30 fixed, firefox31 fixed)
RESOLVED
FIXED
Firefox 31
People
(Reporter: rnewman, Assigned: rnewman)
Details
Attachments
(2 files)
16.45 KB,
patch
|
bnicholson
:
review+
|
Details | Diff | Splinter Review |
3.85 KB,
patch
|
bnicholson
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Accessed from multiple threads, no synchronization, not a thread-safe data structure. This is an early-runtime race condition waiting to happen.
Assignee | ||
Comment 1•10 years ago
|
||
gThe mMisuse sOf prefixes makes me crylaugh.
Attachment #8396041 -
Flags: review?(bnicholson)
Assignee | ||
Comment 2•10 years ago
|
||
Switched to using concurrent data structures for these two members.
Attachment #8396043 -
Flags: review?(bnicholson)
Updated•10 years ago
|
Attachment #8396041 -
Flags: review?(bnicholson) → review+
Comment 3•10 years ago
|
||
Comment on attachment 8396043 [details] [diff] [review] Part 1: GeckoAppShell thread-safety. v1 Review of attachment 8396043 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/GeckoAppShell.java @@ +374,5 @@ > + } > + > + boolean offered = PENDING_EVENTS.offer(e); > + if (!offered) { > + Log.e(LOGTAG, "Unable to queue event for Gecko: " + e); We have no idea what event got skipped here, so it might be better to use add() and fail fast -- though hopefully we'll never hit this to begin with.
Attachment #8396043 -
Flags: review?(bnicholson) → review+
Assignee | ||
Comment 4•10 years ago
|
||
Using `add`, documentation added to note exceptions. https://hg.mozilla.org/integration/fx-team/rev/54e12e510264 https://hg.mozilla.org/integration/fx-team/rev/b4fce211dbde We might consider uplifting Part 1.
Flags: needinfo?(rnewman)
Comment 5•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/54e12e510264 https://hg.mozilla.org/mozilla-central/rev/b4fce211dbde
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Assignee | ||
Comment 6•10 years ago
|
||
Comment on attachment 8396043 [details] [diff] [review] Part 1: GeckoAppShell thread-safety. v1 [Approval Request Comment] Bug caused by (feature/regressing bug #): Long-ago coding errors. User impact if declined: Potential for arbitrary crashes or corruption due to thread-unsafe operations, particularly early in startup. Testing completed (on m-c, etc.): Baking happily on Nightly for a while. Risk to taking this patch (and alternatives if risky): Low: switching implementations for two simple data structures. String or IDL/UUID changes made by this patch: None.
Attachment #8396043 -
Flags: approval-mozilla-aurora?
Flags: needinfo?(rnewman)
Updated•10 years ago
|
Attachment #8396043 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 7•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/6e508a3526e6 https://hg.mozilla.org/releases/mozilla-aurora/rev/ec30bfc1c545
status-firefox30:
--- → fixed
status-firefox31:
--- → fixed
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•