Fix strict mode violation in TabPersistentStore.cleanupPersistentData()

chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java

Index: chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java b/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java
index fd870da029d87051ff1206444a8f04a97e6bb582..530dac947dcbe59e975a5bebaa09781304e62cc5 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java
@@ -361,7 +361,7 @@ public class TabPersistentStore extends TabPersister {
         // pointing to old files and not deleted fast enough. This makes sure to delete everything
         // that we are sure not to use.
         // This assumes that the app will only create tab with id at and above nextId.
-        cleanupPersistentDataAtAndAboveId(nextId);
+        cleanUpPersistentDataAtAndAboveId(nextId);
 
         if (mObserver != null) mObserver.onInitialized(mTabsToRestore.size());
         return nextId;
@@ -509,7 +509,7 @@ public class TabPersistentStore extends TabPersister {
 
     public void clearState() {
         deleteFileAsync(SAVED_STATE_FILE);
-        cleanupPersistentData();
+        cleanUpPersistentData();
         onStateLoaded();
     }
 
@@ -943,7 +943,7 @@ public class TabPersistentStore extends TabPersister {
         if (mTabsToRestore.isEmpty()) {
             mNormalTabsRestored = null;
             mIncognitoTabsRestored = null;
-            cleanupPersistentData();
+            cleanUpPersistentData();
             onStateLoaded();
             mLoadTabTask = null;
             Log.d(TAG, "Loaded tab lists; counts: " + mTabModelSelector.getModel(false).getCount()
@@ -955,37 +955,17 @@ public class TabPersistentStore extends TabPersister {
         }
     }
 
-    private void cleanupPersistentData() {
-        String[] files = getStateDirectory().list();
-        if (files != null) {
-            for (String file : files) {
-                Pair<Integer, Boolean> data = TabState.parseInfoFromFilename(file);
-                if (data != null) {
-                    TabModel model = mTabModelSelector.getModel(data.second);
-                    if (TabModelUtils.getTabById(model, data.first) == null) {
-                        deleteFileAsync(file);
-                    }
-                }
-            }
-        }
-
-        if (mTabContentManager != null) {
-            mTabContentManager.cleanupPersistentData(mTabModelSelector);
-        }
+    private void cleanUpPersistentData() {
+        cleanUpPersistentDataAtAndAboveId(-1);
     }
 
-    private void cleanupPersistentDataAtAndAboveId(int minForbiddenId)  {
-        String[] files = getStateDirectory().list();
-        if (files != null) {
-            for (String file : files) {
-                Pair<Integer, Boolean> data = TabState.parseInfoFromFilename(file);
-                if (data != null && data.first >= minForbiddenId) {
-                    deleteFileAsync(file);
-                }
-            }
-        }
+    private void cleanUpPersistentDataAtAndAboveId(int minForbiddenId) {
+        new CleanUpPersistentDataTask(minForbiddenId)
+                .executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
 
-        if (mTabContentManager != null) {
+        if (mTabContentManager != null && minForbiddenId < 0) {
+            mTabContentManager.cleanupPersistentData(mTabModelSelector);
+        } else if (mTabContentManager != null) {
             mTabContentManager.cleanupPersistentDataAtAndAboveId(minForbiddenId);

[gone, 2016/02/18 17:45:35]

Can't tell if it's cleaner to pull the common null check out.  It makes it
slightly more readable, though.

[agrieve, 2016/02/23 21:28:38]

Done.

         }
     }
@@ -1024,6 +1004,39 @@ public class TabPersistentStore extends TabPersister {
         // Explicitly serializing file mutations (save & delete) to ensure they occur in order.
     }
 
+    private class CleanUpPersistentDataTask extends AsyncTask<Void, Void, String[]> {

[gone, 2016/02/18 17:45:35]

Can you rename this CleanUpTabStateDataTask, instead?  Spent too much time
trying to understand the difference between this and the other
cleanUpPersistentData() method.

[agrieve, 2016/02/23 21:28:38]

Done.

+        private final int mAtAndAboveId;
+
+        public CleanUpPersistentDataTask(int atAndAboveId) {
+            mAtAndAboveId = atAndAboveId;
+        }
+
+        @Override
+        protected String[] doInBackground(Void... voids) {
+            if (mDestroyed) {
+                return null;
+            }
+            return getStateDirectory().list();
+        }
+
+        @Override
+        protected void onPostExecute(String[] fileNames) {
+            if (!mDestroyed && fileNames != null) {

[gone, 2016/02/18 17:45:35]

nit: early exit instead to avoid indentation?

[agrieve, 2016/02/23 21:28:38]

Done.

+                for (String fileName : fileNames) {
+                    Pair<Integer, Boolean> data = TabState.parseInfoFromFilename(fileName);
+                    if (data != null && data.first >= mAtAndAboveId) {
+                        TabModel model = mTabModelSelector.getModel(data.second);
+                        if (TabModelUtils.getTabById(model, data.first) == null) {

[gone, 2016/02/18 17:45:35]

There's a behavioral change here, right?  Before, any TabState files >= the
given ID would be deleted by cleanupPersistentDataAtAndAboveId, regardless of
whether there was a Tab with that ID in the given TabModel.  I believe this
clause was (originally) put in to prevent cases where a new tab could be created
with the same ID as a previously closed tab, which could potentially result in
loading the old TabState for the new tab.

I don't know if this is still possible now that we're storing the maximum ID
ever used since some M4X milestone (in addition to using the original
backwards-compatible calculation in TabPersistentStore#readSaveStateFile), but
users upgrading to M50 from an old version of Clank could hit it.

Do you think the benefit of the unified codepath outweigh that possibility?  If
so, this change should really be documented in the commit message.

[agrieve, 2016/02/19 01:43:26]

Yikes! I stared and stared at this code and didn't see that it didn't call
getTabById() in the old version :(

It seems like even in the old code though, it would have been possible for the
bad case to happen if a tab got loaded before the scheduled asynchronous delete
happened?

Perhaps we could just delete this code path and always call cleanUp(-1)? 

We've already read the state file by this time, so we know the list of tabs IDs
that are reachable. If we want to protect against a new tab being created with a
stale tab data file existing, we should probably add a call to deleteAsync
whenever a new tab id is created? wdyt?

[gone, 2016/02/19 23:54:04]

I think we'd be fine just with deleting TabState files for Tabs that aren't in a
TabModel.  I was worried that this would be problematic for multi-instance
Clank, but each TabModelSelector uses a different directory for its tabs.

I'm not sure if it'd be good, performance-wise, to keep firing (even async)
tasks to delete TabState files immediately after the Tab closed, though we're
probably mostly safe because we don't create tabs with the same ID nowadays
(keeping a global max tab ID is a relatively new thing).

[agrieve, 2016/02/23 21:28:38]

Done.

+                            // It might be more efficient to use a single task for all files, but
+                            // the number of files is expected to be very small.
+                            deleteFileAsync(fileName);
+                        }
+                    }
+                }
+            }
+        }
+    }
+
     private class LoadTabTask extends AsyncTask<Void, Void, TabState> {
 
         public final TabRestoreDetails mTabToRestore;