Closed Bug 987415 Opened 8 years ago Closed 8 years ago

GeckoAppShell.gPendingEvents is not thread safe

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox30 fixed, firefox31 fixed)

RESOLVED FIXED
Firefox 31
Tracking Status
firefox30 --- fixed
firefox31 --- fixed

People

(Reporter: rnewman, Assigned: rnewman)

Details

Attachments

(2 files)

Accessed from multiple threads, no synchronization, not a thread-safe data structure. This is an early-runtime race condition waiting to happen.
gThe mMisuse sOf prefixes makes me crylaugh.
Attachment #8396041 - Flags: review?(bnicholson)
Switched to using concurrent data structures for these two members.
Attachment #8396043 - Flags: review?(bnicholson)
Attachment #8396041 - Flags: review?(bnicholson) → review+
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+
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)
https://hg.mozilla.org/mozilla-central/rev/54e12e510264
https://hg.mozilla.org/mozilla-central/rev/b4fce211dbde
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
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)
Attachment #8396043 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.