Closed Bug 987294 Opened 6 years ago Closed 6 years ago

Unsafe access to mTabsChangedListeners in Tabs

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

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

People

(Reporter: rnewman, Assigned: rnewman)

Details

Attachments

(1 file)

No description provided.
    E/GeckoAppShell( 9225): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1 ("main")
    E/GeckoAppShell( 9225): java.util.ConcurrentModificationException
    E/GeckoAppShell( 9225):         at java.util.ArrayList$ArrayListIterator.next(ArrayList.java:573)
    E/GeckoAppShell( 9225):         at org.mozilla.gecko.Tabs$5.run(Tabs.java:588)
    E/GeckoAppShell( 9225):         at android.os.Handler.handleCallback(Handler.java:733)
    E/GeckoAppShell( 9225):         at android.os.Handler.dispatchMessage(Handler.java:95)
    E/GeckoAppShell( 9225):         at android.os.Looper.loop(Looper.java:136)
    E/GeckoAppShell( 9225):         at android.app.ActivityThread.main(ActivityThread.java:5017)
    E/GeckoAppShell( 9225):         at java.lang.reflect.Method.invokeNative(Native Method)
    E/GeckoAppShell( 9225):         at java.lang.reflect.Method.invoke(Method.java:515)
    E/GeckoAppShell( 9225):         at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:779)
    E/GeckoAppShell( 9225):         at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:595)
    E/GeckoAppShell( 9225):         at dalvik.system.NativeStart.main(Native Method)
Rather than err if we're not on the UI thread, I simply opted to remove synchronization and thread nonsense, and just make the data structure safe.

Most usage should still occur on the UI thread, but it's no longer required.

Any idea how we test this?
Attachment #8395884 - Flags: review?(bnicholson)
Comment on attachment 8395884 [details] [diff] [review]
Unsafe access to mTabsChangedListeners in Tabs. v1

Review of attachment 8395884 [details] [diff] [review]:
-----------------------------------------------------------------

You can also get rid of the Runnable at http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#228 (that comment may be obsolete anyway; I don't see showTabs making any Tabs mutations).

(In reply to Richard Newman [:rnewman] from comment #2)
> Any idea how we test this?

I'm not sure about FilePickerResultHandler, but TwoLinePageRow is used for all of the home panel list entries. All of the onDetachedFromWindow listeners *should* kick in whenever we leave that panel.
Attachment #8395884 - Flags: review?(bnicholson) → review+
https://hg.mozilla.org/integration/fx-team/rev/fa5452578531
Flags: needinfo?(rnewman)
Target Milestone: --- → Firefox 31
Just wondering: why the all-caps member name?
https://hg.mozilla.org/mozilla-central/rev/fa5452578531
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
(In reply to Lucas Rocha (:lucasr) from comment #5)
> Just wondering: why the all-caps member name?

private static final. Yeah, we technically mutate the internal state, but meh. Happy either way.
Comment on attachment 8395884 [details] [diff] [review]
Unsafe access to mTabsChangedListeners in Tabs. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Long-ago coding errors.

User impact if declined: 
  Crashes, particularly when interacting with dialogs.

Testing completed (on m-c, etc.): 
  Baking on m-c for a while.

Risk to taking this patch (and alternatives if risky): 
  Low: changing from a complicated "let's try to get the threads right" approach to a "let's use a safe data structure" approach.

String or IDL/UUID changes made by this patch:
  None.
Attachment #8395884 - Flags: approval-mozilla-beta?
Attachment #8395884 - Flags: approval-mozilla-aurora?
Flags: needinfo?(rnewman)
Attachment #8395884 - Flags: approval-mozilla-beta?
Attachment #8395884 - Flags: approval-mozilla-beta+
Attachment #8395884 - Flags: approval-mozilla-aurora?
Attachment #8395884 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.