Bug 987294 - Unsafe access to mTabsChangedListeners in Tabs. r=bnicholson, a=sylvestre
authorRichard Newman <rnewman@mozilla.com>
Mon, 24 Mar 2014 14:40:54 -0700
changeset 192379 dd0849d5322844945ec36ac877eceb7081eb8656
parent 192378 810352e2835133e97f30d8e735799089f5fdac98
child 192380 c60a64a29bf485de0fd2030cd30eca4a7e9e8946
push id474
push userasasaki@mozilla.com
push dateMon, 02 Jun 2014 21:01:02 +0000
treeherdermozilla-release@967f4cf1b31c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbnicholson, sylvestre
bugs987294
milestone30.0a2
Bug 987294 - Unsafe access to mTabsChangedListeners in Tabs. r=bnicholson, a=sylvestre
mobile/android/base/BrowserApp.java
mobile/android/base/FilePickerResultHandler.java
mobile/android/base/Tabs.java
mobile/android/base/home/TwoLinePageRow.java
--- a/mobile/android/base/BrowserApp.java
+++ b/mobile/android/base/BrowserApp.java
@@ -220,25 +220,20 @@ abstract public class BrowserApp extends
                 // fall through
             case SELECTED:
                 if (Tabs.getInstance().isSelectedTab(tab)) {
                     updateHomePagerForTab(tab);
 
                     final TabsPanel.Panel panel = tab.isPrivate()
                                                 ? TabsPanel.Panel.PRIVATE_TABS
                                                 : TabsPanel.Panel.NORMAL_TABS;
-                    // Delay calling showTabs so that it does not modify the mTabsChangedListeners
-                    // array while we are still iterating through the array.
-                    ThreadUtils.postToUiThread(new Runnable() {
-                        @Override
-                        public void run() {
-                            if (areTabsShown() && mTabsPanel.getCurrentPanel() != panel)
-                                showTabs(panel);
-                        }
-                    });
+
+                    if (areTabsShown() && mTabsPanel.getCurrentPanel() != panel) {
+                        showTabs(panel);
+                    }
                 }
                 break;
             case START:
                 if (Tabs.getInstance().isSelectedTab(tab)) {
                     invalidateOptionsMenu();
 
                     if (mDynamicToolbar.isEnabled()) {
                         mDynamicToolbar.setVisible(true, VisibilityTransition.ANIMATE);
--- a/mobile/android/base/FilePickerResultHandler.java
+++ b/mobile/android/base/FilePickerResultHandler.java
@@ -239,22 +239,17 @@ class FilePickerResultHandler implements
                 ThreadUtils.postToBackgroundThread(new Runnable() {
                     @Override
                     public void run() {
                         File f = new File(tempFile);
                         f.delete();
                     }
                 });
 
-                // We're already on the UIThread, but we have to post this back to the uithread to avoid
-                // modifying the listener array while its being iterated through.
-                ThreadUtils.postToUiThread(new Runnable() {
-                    @Override
-                    public void run() {
-                        Tabs.unregisterOnTabsChangedListener(FileLoaderCallbacks.this);
-                    }
-                });
+                // Tabs' listener array is safe to modify during use: its
+                // iteration pattern is based on snapshots.
+                Tabs.unregisterOnTabsChangedListener(this);
             }
         }
     }
 
 }
 
--- a/mobile/android/base/Tabs.java
+++ b/mobile/android/base/Tabs.java
@@ -1,17 +1,15 @@
 /* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 package org.mozilla.gecko;
 
-import java.util.ArrayList;
-import java.util.Collections;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
 import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.atomic.AtomicInteger;
 
 import org.json.JSONObject;
 import org.mozilla.gecko.db.BrowserDB;
@@ -523,25 +521,24 @@ public class Tabs implements GeckoEventL
             }
         });
     }
 
     public interface OnTabsChangedListener {
         public void onTabChanged(Tab tab, TabEvents msg, Object data);
     }
 
-    private static List<OnTabsChangedListener> mTabsChangedListeners =
-        Collections.synchronizedList(new ArrayList<OnTabsChangedListener>());
+    private static final List<OnTabsChangedListener> TABS_CHANGED_LISTENERS = new CopyOnWriteArrayList<OnTabsChangedListener>();
 
     public static void registerOnTabsChangedListener(OnTabsChangedListener listener) {
-        mTabsChangedListeners.add(listener);
+        TABS_CHANGED_LISTENERS.add(listener);
     }
 
     public static void unregisterOnTabsChangedListener(OnTabsChangedListener listener) {
-        mTabsChangedListeners.remove(listener);
+        TABS_CHANGED_LISTENERS.remove(listener);
     }
 
     public enum TabEvents {
         CLOSED,
         START,
         LOADED,
         LOAD_ERROR,
         STOP,
@@ -573,25 +570,23 @@ public class Tabs implements GeckoEventL
             throw new IllegalArgumentException("onTabChanged:" + msg + " must specify a tab.");
         }
 
         ThreadUtils.postToUiThread(new Runnable() {
             @Override
             public void run() {
                 onTabChanged(tab, msg, data);
 
-                synchronized (mTabsChangedListeners) {
-                    if (mTabsChangedListeners.isEmpty()) {
-                        return;
-                    }
+                if (TABS_CHANGED_LISTENERS.isEmpty()) {
+                    return;
+                }
 
-                    Iterator<OnTabsChangedListener> items = mTabsChangedListeners.iterator();
-                    while (items.hasNext()) {
-                        items.next().onTabChanged(tab, msg, data);
-                    }
+                Iterator<OnTabsChangedListener> items = TABS_CHANGED_LISTENERS.iterator();
+                while (items.hasNext()) {
+                    items.next().onTabChanged(tab, msg, data);
                 }
             }
         });
     }
 
     private void onTabChanged(Tab tab, Tabs.TabEvents msg, Object data) {
         switch (msg) {
             case LOCATION_CHANGE:
--- a/mobile/android/base/home/TwoLinePageRow.java
+++ b/mobile/android/base/home/TwoLinePageRow.java
@@ -97,24 +97,19 @@ public class TwoLinePageRow extends Line
 
     @Override
     protected void onAttachedToWindow() {
         Tabs.registerOnTabsChangedListener(this);
     }
 
     @Override
     protected void onDetachedFromWindow() {
-        // Delay removing the listener to avoid modifying mTabsChangedListeners
-        // while notifyListeners is iterating through the array.
-        ThreadUtils.postToUiThread(new Runnable() {
-            @Override
-            public void run() {
-                Tabs.unregisterOnTabsChangedListener(TwoLinePageRow.this);
-            }
-        });
+        // Tabs' listener array is safe to modify during use: its
+        // iteration pattern is based on snapshots.
+        Tabs.unregisterOnTabsChangedListener(this);
     }
 
     @Override
     public void onTabChanged(final Tab tab, final Tabs.TabEvents msg, final Object data) {
         switch(msg) {
             case ADDED:
             case CLOSED:
             case LOCATION_CHANGE: