987294 - Unsafe access to mTabsChangedListeners in Tabs

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Comment 1

[Richard Newman [:rnewman]]

    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)

Comment 2

[Richard Newman [:rnewman]]

Attachment: Unsafe access to mTabsChangedListeners in Tabs. v1

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?

Comment 3

[Brian Nicholson (: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.

Comment 4

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/fa5452578531

Comment 5

[Lucas Rocha (:lucasr)]

Just wondering: why the all-caps member name?

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fa5452578531

Comment 7

[Richard Newman [:rnewman]]

(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 8

[Richard Newman [:rnewman]]

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.

Comment 9

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dd0849d53228

Comment 10

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e066003ed6f1