Bug 844407 - Make Tabs thread-safe. r=rnewman,bnicholson a=bajaj
authorBrian Nicholson <bnicholson@mozilla.com>
Wed, 13 Mar 2013 21:56:50 -0700
changeset 132399 65e5af7ac41b4febb6c04f2e0a65125f228a3886
parent 132398 93755f9fbbc10afa7b74fb37580619a3fc813a17
child 132400 cd1d1f737528efab06be15186881f2ae46f000d1
push id2323
push userbbajaj@mozilla.com
push dateMon, 01 Apr 2013 19:47:02 +0000
treeherdermozilla-beta@7712be144d91 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrnewman, bnicholson, bajaj
bugs844407
milestone21.0a2
Bug 844407 - Make Tabs thread-safe. r=rnewman,bnicholson a=bajaj
mobile/android/base/Tabs.java
--- a/mobile/android/base/Tabs.java
+++ b/mobile/android/base/Tabs.java
@@ -17,28 +17,35 @@ import android.accounts.OnAccountsUpdate
 import android.content.ContentResolver;
 import android.content.Intent;
 import android.database.ContentObserver;
 import android.net.Uri;
 import android.util.Log;
 import android.widget.Toast;
 
 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;
@@ -49,16 +56,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("SessionHistory:New");
         registerEventListener("SessionHistory:Back");
         registerEventListener("SessionHistory:Forward");
@@ -69,113 +77,140 @@ public class Tabs implements GeckoEventL
         registerEventListener("Tab:Select");
         registerEventListener("Content:LocationChange");
         registerEventListener("Session:RestoreEnd");
         registerEventListener("Reader:Added");
         registerEventListener("Reader:Removed");
         registerEventListener("Reader:Share");
     }
 
-    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() {
             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) {
                 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() { 
-            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);
     }
 
@@ -211,39 +246,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());
 
@@ -282,21 +314,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;
     }
 
@@ -388,44 +431,38 @@ public class Tabs implements GeckoEventL
                 BrowserDB.removeReadingListItemWithURL(getContentResolver(), url);
                 mActivity.showToast(R.string.reading_list_removed, Toast.LENGTH_SHORT);
             }
         });
     }
 
     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() {
-                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,
@@ -440,66 +477,74 @@ public class Tabs implements GeckoEventL
         LOCATION_CHANGE,
         MENU_UPDATED
     }
 
     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() {
             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() {
             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);
     }