745540 - ConcurrentModificationException thrown when unregistering a gecko event listener while the listener is running

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: Make mEventListeners access thread-safe

I added a Preferences:Data listener that unregisters itself when it gets triggered, and to my amazement it threw a ConcurrentModificationException!

Turns out this is not very amazing once you look at the code, because there is no multithreaded access consideration given to the mEventListeners structure that these are stored in. Since we normally only unregister listeners in onDestroy() methods, this hasn't cropped up yet, but really all the code that deals with mEventListeners only works by accident.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 615146 [details] [diff] [review]
Make mEventListeners access thread-safe

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

::: mobile/android/base/GeckoAppShell.java
@@ +1684,5 @@
> +            } else {
> +                // we should not modify "listeners" directly here, because this
> +                // method could be called from a GeckoEventListener implementation
> +                // that is executing right now. Modifying the "listeners" object
> +                // that may be being iterated through in handleGeckoMessage

would it easier to synchronize on listeners here and in handleGeckoMessage?

Comment 2

[Kartikaya Gupta (email:[e-mail])]

(In reply to Brad Lassey [:blassey] from comment #1)
> Comment on attachment 615146 [details] [diff] [review]
> would it easier to synchronize on listeners here and in handleGeckoMessage?

No, because they might be on the same thread, and so the synchronization will not block anything. i.e.:

1. handleGeckoMessage does synchronize(listeners)
2. handleGeckoMessage calls handleMessage(event)
3. handleMessage calls unregisterGeckoEventListener(this)
4. unregisterGeckoEventListener does synchronize(listeners) // this works fine, it's the same thread
5. listeners array is mutated
...
n. handleGeckoMessage blows up

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Make mEventListeners access thread-safe (v2)

:rnewman suggested using CopyOnWriteArrayList instead to avoid reinventing it, so updated the patch to do that.

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 615383 [details] [diff] [review]
Make mEventListeners access thread-safe (v2)

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

::: mobile/android/base/GeckoAppShell.java
@@ +115,5 @@
>      private static Boolean sNSSLibsLoaded = false;
>      private static Boolean sLibsSetup = false;
>      private static File sGREDir = null;
>  
> +    private static Map<String, CopyOnWriteArrayList<GeckoEventListener>> mEventListeners

This can probably be a less specific type...

@@ +1719,5 @@
> +            }
> +            listeners.remove(listener);
> +            if (listeners.size() == 0) {
> +                mEventListeners.remove(event);
> +            }

I'd consider whether this cleanup is worthwhile, compared to just leaving the empty list around and sometimes doing less work in the register case... considering the case of frequently registering and unregistering listeners for the same event.

@@ +1767,5 @@
>                  } catch (Exception ex) { }
>                  return ret.toString();
>              }
>  
> +            CopyOnWriteArrayList<GeckoEventListener> listeners;

This can be less specific; will make your life easier if you ever switch internal representations.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

(In reply to Richard Newman [:rnewman] from comment #4)
> 
> I'd consider whether this cleanup is worthwhile, compared to just leaving
> the empty list around and sometimes doing less work in the register case...
> considering the case of frequently registering and unregistering listeners
> for the same event.
> 

Yeah, I wasn't really sure which case I should optimize for there. In practice we don't unregister things until shutdown anyway so it doesn't really matter.

> 
> This can be less specific; will make your life easier if you ever switch
> internal representations.

The initial version of the patch used just List<GeckoEventListener> but with the change we are pretty much forced to use a CopyOnWriteArrayList; changing to a regular ArrayList or LinkedList will un-fix the original bug. Since we *are* constrained to a specific internal representation I figured it would be better to make that explicit in the type.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/55f527f49d54

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/55f527f49d54