Bug 844407 - Make Tabs thread-safe. r=rnewman,bnicholson

--HG--
extra : rebase_source : 1b11fe51c46f9ab61a7fadd8a61bec5bf43c49b6

mobile/android/base/Tabs.java

@@ -20,18 +20,25 @@ 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;
-    private final HashMap<Integer, Tab> mTabs = new HashMap<Integer, Tab>();
+    // mOrder and mTabs are always of the same cardinality, and contain the same values.
     private final CopyOnWriteArrayList<Tab> mOrder = new CopyOnWriteArrayList<Tab>();
-    private volatile boolean mInitialTabsAdded;
+
+    // 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>();
 
     // Keeps track of how much has happened since we last updated our persistent tab store.
     private volatile int mScore = 0;
@@ -52,6 +59,7 @@ public class Tabs implements GeckoEventListener {
     private static final int SCORE_THRESHOLD = 30;
 
     private static AtomicInteger sTabId = new AtomicInteger(0);
+    private volatile boolean mInitialTabsAdded;
 
     private GeckoApp mActivity;
     private ContentObserver mContentObserver;
@@ -78,36 +86,64 @@ public class Tabs implements GeckoEventListener {
         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++;
             }
@@ -115,12 +151,13 @@ public class Tabs implements GeckoEventListener {
         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();
                     }
                 }
@@ -130,14 +167,15 @@ public class Tabs implements GeckoEventListener {
     }
 
     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);
         }
@@ -145,7 +183,7 @@ public class Tabs implements GeckoEventListener {
         return tab;
     }
 
-    public void removeTab(int id) {
+    public synchronized void removeTab(int id) {
         if (mTabs.containsKey(id)) {
             Tab tab = getTab(id);
             mOrder.remove(tab);
@@ -153,31 +191,27 @@ public class Tabs implements GeckoEventListener {
         }
     }
 
-    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;
     }
@@ -223,13 +257,10 @@ public class Tabs implements GeckoEventListener {
     }
 
     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;
 
@@ -240,12 +271,12 @@ public class Tabs implements GeckoEventListener {
     }
 
     /** 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;
 
@@ -294,11 +325,22 @@ public class Tabs implements GeckoEventListener {
         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();
     }
@@ -400,42 +442,35 @@ public class Tabs implements GeckoEventListener {
                 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);
     }
 
@@ -465,25 +500,29 @@ public class Tabs implements GeckoEventListener {
         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;
@@ -500,6 +539,8 @@ public class Tabs implements GeckoEventListener {
             case UNSELECTED:
                 tab.onChange();
                 break;
+            default:
+                break;
         }
 
         if (mScore > SCORE_THRESHOLD) {
@@ -510,13 +551,15 @@ public class Tabs implements GeckoEventListener {
 
     // 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);
+                }
             }
         });
     }