Bug 844407 - Make Tabs thread-safe. r=rnewman,bnicholson
authorBrian Nicholson <bnicholson@mozilla.com>
Wed, 13 Mar 2013 21:56:50 -0700
changeset 124780 e38d0a3fe054f8e55b900c48d9801237258ebc5f
parent 124779 6bbd7ce8308e4aeaeca73b2540f557bab3905681
child 124781 06f85e10a40c8c094ae3c029472cf4e8fd4cf84d
push id24433
push useremorley@mozilla.com
push dateThu, 14 Mar 2013 12:21:10 +0000
treeherdermozilla-central@96af92fa87fd [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrnewman, bnicholson
bugs844407
milestone22.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 844407 - Make Tabs thread-safe. r=rnewman,bnicholson
mobile/android/base/Tabs.java
--- a/mobile/android/base/Tabs.java
+++ b/mobile/android/base/Tabs.java
@@ -15,28 +15,35 @@ import android.accounts.Account;
 import android.accounts.AccountManager;
 import android.accounts.OnAccountsUpdateListener;
 import android.content.ContentResolver;
 import android.database.ContentObserver;
 import android.graphics.Color;
 import android.net.Uri;
 import android.util.Log;
 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;
 
 public class Tabs implements GeckoEventListener {
     private static final String LOGTAG = "GeckoTabs";
 
-    private Tab mSelectedTab;
+    // mOrder and mTabs are always of the same cardinality, and contain the same values.
+    private final CopyOnWriteArrayList<Tab> mOrder = new CopyOnWriteArrayList<Tab>();
+
+    // All writes to mSelectedTab must be synchronized on the Tabs instance.
+    // In general, it's preferred to always use selectTab()).
+    private volatile Tab mSelectedTab;
+
+    // All accesses to mTabs must be synchronized on the Tabs instance.
     private final HashMap<Integer, Tab> mTabs = new HashMap<Integer, Tab>();
-    private final CopyOnWriteArrayList<Tab> mOrder = new CopyOnWriteArrayList<Tab>();
-    private volatile boolean mInitialTabsAdded;
 
     // Keeps track of how much has happened since we last updated our persistent tab store.
     private volatile int mScore = 0;
 
     private AccountManager mAccountManager;
     private OnAccountsUpdateListener mAccountListener = null;
 
     public static final int LOADURL_NONE = 0;
@@ -47,16 +54,17 @@ public class Tabs implements GeckoEventL
     public static final int LOADURL_DELAY_LOAD = 16;
     public static final int LOADURL_DESKTOP = 32;
 
     private static final int SCORE_INCREMENT_TAB_LOCATION_CHANGE = 5;
     private static final int SCORE_INCREMENT_TAB_SELECTED = 10;
     private static final int SCORE_THRESHOLD = 30;
 
     private static AtomicInteger sTabId = new AtomicInteger(0);
+    private volatile boolean mInitialTabsAdded;
 
     private GeckoApp mActivity;
     private ContentObserver mContentObserver;
 
     private Tabs() {
         registerEventListener("Session:RestoreEnd");
         registerEventListener("SessionHistory:New");
         registerEventListener("SessionHistory:Back");
@@ -73,116 +81,142 @@ public class Tabs implements GeckoEventL
         registerEventListener("Content:LoadError");
         registerEventListener("Content:PageShow");
         registerEventListener("DOMContentLoaded");
         registerEventListener("DOMTitleChanged");
         registerEventListener("DOMLinkAdded");
         registerEventListener("DesktopMode:Changed");
     }
 
-    public void attachToActivity(GeckoApp activity) {
+    public synchronized void attachToActivity(GeckoApp activity) {
+        if (mActivity == activity) {
+            return;
+        }
+
+        if (mActivity != null) {
+            detachFromActivity(mActivity);
+        }
+
         mActivity = activity;
         mAccountManager = AccountManager.get(mActivity);
 
-        // The listener will run on the background thread (see 2nd argument)
-        mAccountManager.addOnAccountsUpdatedListener(mAccountListener = new OnAccountsUpdateListener() {
+        mAccountListener = new OnAccountsUpdateListener() {
             @Override
             public void onAccountsUpdated(Account[] accounts) {
                 persistAllTabs();
             }
-        }, GeckoAppShell.getHandler(), false);
+        };
+
+        // The listener will run on the background thread (see 2nd argument).
+        mAccountManager.addOnAccountsUpdatedListener(mAccountListener, GeckoAppShell.getHandler(), false);
+
         if (mContentObserver != null) {
             BrowserDB.registerBookmarkObserver(getContentResolver(), mContentObserver);
         }
     }
 
-    public void detachFromActivity(GeckoApp activity) {
+    // Ideally, this would remove the reference to the activity once it's
+    // detached; however, we have lifecycle issues with GeckoApp and Tabs that
+    // requires us to keep it around (see
+    // https://bugzilla.mozilla.org/show_bug.cgi?id=844407).
+    public synchronized void detachFromActivity(GeckoApp activity) {
+        if (mContentObserver != null) {
+            BrowserDB.unregisterContentObserver(getContentResolver(), mContentObserver);
+        }
+
         if (mAccountListener != null) {
             mAccountManager.removeOnAccountsUpdatedListener(mAccountListener);
             mAccountListener = null;
         }
-        if (mContentObserver != null) {
-            BrowserDB.unregisterContentObserver(getContentResolver(), mContentObserver);
-        }
     }
 
-    public int getDisplayCount() {
+    /**
+     * Gets the tab count corresponding to the private state of the selected
+     * tab.
+     *
+     * If the selected tab is a non-private tab, this will return the number of
+     * non-private tabs; likewise, if this is a private tab, this will return
+     * the number of private tabs.
+     *
+     * @return the number of tabs in the current private state
+     */
+    public synchronized int getDisplayCount() {
+        // Once mSelectedTab is non-null, it cannot be null for the remainder
+        // of the object's lifetime.
         boolean getPrivate = mSelectedTab != null && mSelectedTab.isPrivate();
         int count = 0;
-        for (Tab tab : mTabs.values()) {
+        for (Tab tab : mOrder) {
             if (tab.isPrivate() == getPrivate) {
                 count++;
             }
         }
         return count;
     }
 
+    // Must be synchronized to avoid racing on mContentObserver.
     private void lazyRegisterBookmarkObserver() {
         if (mContentObserver == null) {
             mContentObserver = new ContentObserver(null) {
                 @Override
                 public void onChange(boolean selfChange) {
-                    for (Tab tab : mTabs.values()) {
+                    for (Tab tab : mOrder) {
                         tab.updateBookmark();
                     }
                 }
             };
             BrowserDB.registerBookmarkObserver(getContentResolver(), mContentObserver);
         }
     }
 
     private Tab addTab(int id, String url, boolean external, int parentId, String title, boolean isPrivate) {
-        lazyRegisterBookmarkObserver();
-
         final Tab tab = isPrivate ? new PrivateTab(id, url, external, parentId, title) :
                                     new Tab(id, url, external, parentId, title);
-        mTabs.put(id, tab);
-        mOrder.add(tab);
+        synchronized (this) {
+            lazyRegisterBookmarkObserver();
+            mTabs.put(id, tab);
+            mOrder.add(tab);
+        }
 
-        // Suppress the ADDED event to prevent animation of tabs created via session restore
+        // Suppress the ADDED event to prevent animation of tabs created via session restore.
         if (mInitialTabsAdded) {
             notifyListeners(tab, TabEvents.ADDED);
         }
 
         return tab;
     }
 
-    public void removeTab(int id) {
+    public synchronized void removeTab(int id) {
         if (mTabs.containsKey(id)) {
             Tab tab = getTab(id);
             mOrder.remove(tab);
             mTabs.remove(id);
         }
     }
 
-    public Tab selectTab(int id) {
+    public synchronized Tab selectTab(int id) {
         if (!mTabs.containsKey(id))
             return null;
 
         final Tab oldTab = getSelectedTab();
         final Tab tab = mTabs.get(id);
+
         // This avoids a NPE below, but callers need to be careful to
-        // handle this case
-        if (tab == null || oldTab == tab)
+        // handle this case.
+        if (tab == null || oldTab == tab) {
             return null;
+        }
 
         mSelectedTab = tab;
-        mActivity.runOnUiThread(new Runnable() { 
-            @Override
-            public void run() {
-                if (isSelectedTab(tab)) {
-                    notifyListeners(tab, TabEvents.SELECTED);
+        notifyListeners(tab, TabEvents.SELECTED);
 
-                    if (oldTab != null)
-                        notifyListeners(oldTab, TabEvents.UNSELECTED);
-                }
-            }
-        });
+        if (oldTab != null) {
+            notifyListeners(oldTab, TabEvents.UNSELECTED);
+        }
 
-        // Pass a message to Gecko to update tab state in BrowserApp
+        // Pass a message to Gecko to update tab state in BrowserApp.
         GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Tab:Selected", String.valueOf(tab.getId())));
         return tab;
     }
 
     private int getIndexOf(Tab tab) {
         return mOrder.lastIndexOf(tab);
     }
 
@@ -218,39 +252,36 @@ public class Tabs implements GeckoEventL
      *
      * @return the selected tab, or null if no tabs exist
      */
     public Tab getSelectedTab() {
         return mSelectedTab;
     }
 
     public boolean isSelectedTab(Tab tab) {
-        if (mSelectedTab == null)
-            return false;
-
-        return tab == mSelectedTab;
+        return tab != null && tab == mSelectedTab;
     }
 
-    public Tab getTab(int id) {
+    public synchronized Tab getTab(int id) {
         if (mTabs.size() == 0)
             return null;
 
         if (!mTabs.containsKey(id))
            return null;
 
         return mTabs.get(id);
     }
 
     /** Close tab and then select the default next tab */
-    public void closeTab(Tab tab) {
+    public synchronized void closeTab(Tab tab) {
         closeTab(tab, getNextTab(tab));
     }
 
     /** Close tab and then select nextTab */
-    public void closeTab(final Tab tab, Tab nextTab) {
+    public synchronized void closeTab(final Tab tab, Tab nextTab) {
         if (tab == null)
             return;
 
         if (nextTab == null)
             nextTab = loadUrl("about:home", LOADURL_NEW_TAB);
 
         selectTab(nextTab.getId());
 
@@ -289,21 +320,32 @@ public class Tabs implements GeckoEventL
         }
         return nextTab;
     }
 
     public Iterable<Tab> getTabsInOrder() {
         return mOrder;
     }
 
-    public ContentResolver getContentResolver() {
-        return mActivity.getContentResolver();
+    /**
+     * @return the current GeckoApp instance, or throws if
+     *         we aren't correctly initialized.
+     */
+    private synchronized GeckoApp getActivity() {
+        if (mActivity == null) {
+            throw new IllegalStateException("Tabs not initialized with a GeckoApp instance.");
+        }
+        return mActivity;
     }
 
-    //Making Tabs a singleton class
+    public ContentResolver getContentResolver() {
+        return getActivity().getContentResolver();
+    }
+
+    // Make Tabs a singleton class.
     private static class TabsInstanceHolder {
         private static final Tabs INSTANCE = new Tabs();
     }
 
     public static Tabs getInstance() {
        return Tabs.TabsInstanceHolder.INSTANCE;
     }
 
@@ -395,52 +437,45 @@ public class Tabs implements GeckoEventL
                 tab.updateTitle(message.getString("title"));
             } else if (event.equals("DOMLinkAdded")) {
                 tab.updateFaviconURL(message.getString("href"), message.getInt("size"));
                 notifyListeners(tab, TabEvents.LINK_ADDED);
             } else if (event.equals("DesktopMode:Changed")) {
                 tab.setDesktopMode(message.getBoolean("desktopMode"));
                 notifyListeners(tab, TabEvents.DESKTOP_MODE_CHANGE);
             }
-        } catch (Exception e) { 
+        } catch (Exception e) {
             Log.w(LOGTAG, "handleMessage threw for " + event, e);
         }
     }
 
     public void refreshThumbnails() {
         final ThumbnailHelper helper = ThumbnailHelper.getInstance();
-        Iterator<Tab> iterator = mTabs.values().iterator();
-        while (iterator.hasNext()) {
-            final Tab tab = iterator.next();
-            GeckoAppShell.getHandler().post(new Runnable() {
-                @Override
-                public void run() {
+        GeckoAppShell.getHandler().post(new Runnable() {
+            @Override
+            public void run() {
+                for (final Tab tab : mOrder) {
                     helper.getAndProcessThumbnailFor(tab);
                 }
-            });
-        }
+            }
+        });
     }
 
     public interface OnTabsChangedListener {
         public void onTabChanged(Tab tab, TabEvents msg, Object data);
     }
-    
-    private static ArrayList<OnTabsChangedListener> mTabsChangedListeners;
+
+    private static List<OnTabsChangedListener> mTabsChangedListeners =
+        Collections.synchronizedList(new ArrayList<OnTabsChangedListener>());
 
     public static void registerOnTabsChangedListener(OnTabsChangedListener listener) {
-        if (mTabsChangedListeners == null)
-            mTabsChangedListeners = new ArrayList<OnTabsChangedListener>();
-        
         mTabsChangedListeners.add(listener);
     }
 
     public static void unregisterOnTabsChangedListener(OnTabsChangedListener listener) {
-        if (mTabsChangedListeners == null)
-            return;
-        
         mTabsChangedListeners.remove(listener);
     }
 
     public enum TabEvents {
         CLOSED,
         START,
         LOADED,
         LOAD_ERROR,
@@ -460,68 +495,76 @@ public class Tabs implements GeckoEventL
         READER_ENABLED,
         DESKTOP_MODE_CHANGE
     }
 
     public void notifyListeners(Tab tab, TabEvents msg) {
         notifyListeners(tab, msg, "");
     }
 
+    // Throws if not initialized.
     public void notifyListeners(final Tab tab, final TabEvents msg, final Object data) {
-        mActivity.runOnUiThread(new Runnable() {
+        getActivity().runOnUiThread(new Runnable() {
             @Override
             public void run() {
                 onTabChanged(tab, msg, data);
 
-                if (mTabsChangedListeners == null)
-                    return;
+                synchronized (mTabsChangedListeners) {
+                    if (mTabsChangedListeners.isEmpty()) {
+                        return;
+                    }
 
-                Iterator<OnTabsChangedListener> items = mTabsChangedListeners.iterator();
-                while (items.hasNext()) {
-                    items.next().onTabChanged(tab, msg, data);
+                    Iterator<OnTabsChangedListener> items = mTabsChangedListeners.iterator();
+                    while (items.hasNext()) {
+                        items.next().onTabChanged(tab, msg, data);
+                    }
                 }
             }
         });
     }
 
     private void onTabChanged(Tab tab, Tabs.TabEvents msg, Object data) {
-        switch(msg) {
+        switch (msg) {
             case LOCATION_CHANGE:
                 mScore += SCORE_INCREMENT_TAB_LOCATION_CHANGE;
                 break;
             case RESTORED:
                 mInitialTabsAdded = true;
                 break;
 
             // When one tab is deselected, another one is always selected, so only
             // increment the score once. When tabs are added/closed, they are also
             // selected/unselected, so it would be redundant to also listen
             // for ADDED/CLOSED events.
             case SELECTED:
                 mScore += SCORE_INCREMENT_TAB_SELECTED;
             case UNSELECTED:
                 tab.onChange();
                 break;
+            default:
+                break;
         }
 
         if (mScore > SCORE_THRESHOLD) {
             persistAllTabs();
             mScore = 0;
         }
     }
 
     // This method persists the current ordered list of tabs in our tabs content provider.
     public void persistAllTabs() {
+        final GeckoApp activity = getActivity();
         final Iterable<Tab> tabs = getTabsInOrder();
         GeckoAppShell.getHandler().post(new Runnable() {
             @Override
             public void run() {
-                boolean syncIsSetup = SyncAccounts.syncAccountsExist(mActivity);
-                if (syncIsSetup)
+                boolean syncIsSetup = SyncAccounts.syncAccountsExist(activity);
+                if (syncIsSetup) {
                     TabsAccessor.persistLocalTabs(getContentResolver(), tabs);
+                }
             }
         });
     }
 
     private void registerEventListener(String event) {
         GeckoAppShell.getEventDispatcher().registerEventListener(event, this);
     }