Fix up _some_web app directory StrictMode violations

chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java

Index: chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java b/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java
index db8d63297c52aa8182d19c8f5890b80ab91a8144..471ec075f8ad12c932be91ee7c8d9b7ed711678c 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java
@@ -15,114 +15,96 @@ import android.os.AsyncTask;
 import android.os.Build;
 import android.os.StrictMode;
 import android.text.TextUtils;
-import android.util.Log;
 
 import org.chromium.base.ApplicationStatus;
+import org.chromium.base.FileUtils;
+import org.chromium.base.Log;
 import org.chromium.chrome.browser.document.DocumentUtils;
 
 import java.io.File;
 import java.util.HashSet;
+import java.util.Set;
 import java.util.concurrent.atomic.AtomicBoolean;
 
 /**
- * Manages directories created to store data for webapps.
+ * Manages directories created to store data for web apps.
+ *
+ * Running this task should be called by WebappActivities after they have restored all the data

[Yaron, 2015/09/24 18:31:01]

This seems out of place. What "task"? Move to member variable/function?

[gone, 2015/09/24 19:37:11]

Moved it downward.  Make more sense?

[Yaron, 2015/09/24 19:45:21]

yes, thanks

+ * they need from their directory.
  *
  * Directories managed by this class are all subdirectories of the app_WebappActivity/ directory,
  * which each WebappActivity using a directory named either for its Webapp's ID in Document mode,
  * or the index of the WebappActivity if it is a subclass of the WebappManagedActivity class (which
  * are used in pre-L devices to allow multiple WebappActivities launching).
  */
-public class WebappDirectoryManager extends AsyncTask<Void, Void, Void> {
-    private static final String TAG = "WebappDirectoryCleaner";
-    private static final String WEBAPP_DIRECTORY_NAME = "WebappActivity";
+public class WebappDirectoryManager {
+    protected static final String WEBAPP_DIRECTORY_NAME = "WebappActivity";
+    private static final String TAG = "WebappDirectoryManag";
 
     /** Whether or not the class has already started trying to clean up obsolete directories. */
     private static final AtomicBoolean sMustCleanUpOldDirectories = new AtomicBoolean(true);
 
-    /** Scheme used for Intents fired for WebappActivity instances. */
-    private final String mWebappScheme;
-
-    /** Directories that will be deleted. */
-    private final HashSet<File> mDirectoriesToDelete;
+    /** AsyncTask that is used to clean up the web app directories. */
+    private AsyncTask<Void, Void, Void> mCleanupTask;
 
     /**
-     * Constructs a WebappDirectoryManager, which will manage the deletion of directories
-     * corresponding to webapps that no longer need their data.
-     *
-     * Should be called by WebappActivities after they have restored all the data they need from
-     * their directory.
-     *
-     * @param directory Directory that must be deleted.  Corresponds to the current webapp.
-     * @param webappScheme Scheme used for WebappActivities when building their Intent data URIs.
-     * @param deleteOldDirectories Whether directories for old WebappActivities should be purged.
+     * Deletes web app directories with stale data, including the directory for the current
+     * {@link WebappActivity}, which should have already loaded its {@link TabState}.
+     * @param context         Context to pull info and Files from.
+     * @param currentWebappId ID for the currently running web app.
+     * @return                AsyncTask doing the cleaning.
      */
-    public WebappDirectoryManager(
-            final File directory, String webappScheme, boolean deleteOldDirectories) {
-        mWebappScheme = webappScheme;
-
-        mDirectoriesToDelete = new HashSet<File>();
-        mDirectoriesToDelete.add(directory);
-
-        if (deleteOldDirectories && sMustCleanUpOldDirectories.getAndSet(false)) {
-            assert Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP;
-            Context context = ApplicationStatus.getApplicationContext();
-            cleanUpOldWebappDirectories(
-                    mDirectoriesToDelete, context.getApplicationInfo().dataDir);
-        }
-    }
-
-    @Override
-    protected Void doInBackground(Void... params) {
-        for (File directory : mDirectoriesToDelete) {
-            if (isCancelled()) return null;
+    public AsyncTask<Void, Void, Void> cleanUpDirectories(
+            final Context context, final String currentWebappId) {
+        if (mCleanupTask != null) return mCleanupTask;

[Peter Wen, 2015/09/24 16:17:56]

Do we need to deal with multiple concurrent cleanUpDirectories calls by creating
a list of cleanup tasks?

[gone, 2015/09/24 17:31:34]

I think we're alright:
1) Only one cleanup task will try to clean up old directories because of the
static AtomicBoolean
2) Any cleanup task after the first will just try to delete one particular
directory (the one for the WebappActivity that owns it).  The task never
explicitly tries to keep directories alive, unless the task is canceled.

Worst case is that the very first cleanup task deletes the directory for another
WebappActivity that starts up before the task finishes.  All that means is that
the web app will lose the TabState for the other web app, resulting in either:
1) The WebappActivity sends the user back to the web app's original URL
2) The WebappActivity is still running when its directory was deleted (which it
was going to do anyway), and saves out its TabState again when it is
backgrounded.

Easiest remedy is to make it a SERIAL_EXECUTOR task instead, but I don't think
it's really worth the hassle.

+
+        mCleanupTask = new AsyncTask<Void, Void, Void>() {
+            @Override
+            protected final Void doInBackground(Void... params) {
+                Set<File> directoriesToDelete = new HashSet<File>();
+                directoriesToDelete.add(getWebappDirectory(context, currentWebappId));
+
+                boolean shouldDeleteOldDirectories =
+                        Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP;
+                if (shouldDeleteOldDirectories && sMustCleanUpOldDirectories.getAndSet(false)) {
+                    findStaleWebappDirectories(context, directoriesToDelete);
+                }
 
-            File[] files = directory.listFiles();
-            if (files != null) {
-                // Delete all the files in the directory.
-                for (File file : files) {
-                    if (!file.delete()) Log.e(TAG, "Failed to delete file: " + file.getPath());
+                for (File directory : directoriesToDelete) {
+                    if (isCancelled()) return null;
+                    FileUtils.recursivelyDeleteFile(directory);
                 }
-            }
 
-            // Delete the directory itself.
-            if (!directory.delete()) {
-                Log.e(TAG, "Failed to delete directory: " + directory.getPath());
+                return null;
             }
-        }
-        return null;
+        };
+        mCleanupTask.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
+        return mCleanupTask;
+    }
+
+    /** Cancels the cleanup task, if one exists. */
+    public void cancelCleanup() {
+        if (mCleanupTask != null) mCleanupTask.cancel(true);
     }
 
     /**
-     * Removes all directories using the old pre-K directory structure, which used directories named
-     * app_WebappActivity*.  Also deletes directories corresponding to WebappActivities that are no
-     * longer listed in Android's recents, since these will be unable to restore their data, anyway.
+     * Finds all directories for web apps containing stale data.
+     *
+     * This includes all directories using the old pre-L directory structure, which used directories
+     * named * app_WebappActivity*, as well as directories corresponding to WebappActivities that
+     * are no longer listed in Android's recents, since these will be unable to restore their data.
+     *
      * @param directoriesToDelete Set to append directory names to.
-     * @param baseDirectory Base directory of all of Chrome's persisted files.
      */
-    @TargetApi(Build.VERSION_CODES.LOLLIPOP)
-    private void cleanUpOldWebappDirectories(
-            HashSet<File> directoriesToDelete, String baseDirectory) {
-        Context context = ApplicationStatus.getApplicationContext();
-        File webappBaseDirectory = null;
-
-        // Temporarily allowing disk access while fixing. TODO: http://crbug.com/525781
-        StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskReads();
-        try {
-            webappBaseDirectory = context.getDir(WEBAPP_DIRECTORY_NAME, Context.MODE_PRIVATE);
-        } finally {
-            StrictMode.setThreadPolicy(oldPolicy);
-        }
+    private void findStaleWebappDirectories(Context context, Set<File> directoriesToDelete) {
+        File webappBaseDirectory = getBaseWebappDirectory(context);
 
         // Figure out what WebappActivities are still listed in Android's recents menu.
-        HashSet<String> liveWebapps = new HashSet<String>();
-        ActivityManager manager =
-                (ActivityManager) context.getSystemService(Context.ACTIVITY_SERVICE);
-        for (AppTask task : manager.getAppTasks()) {
-            Intent intent = DocumentUtils.getBaseIntentFromTask(task);
-            if (intent == null) continue;
-
+        Set<String> liveWebapps = new HashSet<String>();
+        Set<Intent> baseIntents = getBaseIntentsForAllTasks();
+        for (Intent intent : baseIntents) {
             Uri data = intent.getData();
-            if (data != null && TextUtils.equals(mWebappScheme, data.getScheme())) {
+            if (data != null && TextUtils.equals(WebappActivity.WEBAPP_SCHEME, data.getScheme())) {
                 liveWebapps.add(data.getHost());
             }
 
@@ -143,10 +125,10 @@ public class WebappDirectoryManager extends AsyncTask<Void, Void, Void> {
         }
 
         if (webappBaseDirectory != null) {
-            // Delete all webapp directories in the main directory.
+            // Delete all web app directories in the main directory, which were for pre-L web apps.
+            File appDirectory = new File(context.getApplicationInfo().dataDir);
             String webappDirectoryAppBaseName = webappBaseDirectory.getName();
-            File dataDirectory = new File(baseDirectory);
-            File[] files = dataDirectory.listFiles();
+            File[] files = appDirectory.listFiles();
             if (files != null) {
                 for (File file : files) {
                     String filename = file.getName();
@@ -156,7 +138,7 @@ public class WebappDirectoryManager extends AsyncTask<Void, Void, Void> {
                 }
             }
 
-            // Clean out webapp directories no longer corresponding to tasks in Recents.
+            // Clean out web app directories no longer corresponding to tasks in Recents.
             if (webappBaseDirectory.exists()) {
                 files = webappBaseDirectory.listFiles();
                 if (files != null) {
@@ -169,23 +151,42 @@ public class WebappDirectoryManager extends AsyncTask<Void, Void, Void> {
     }
 
     /**
-     * Returns the name of the directory for this webapp.
-     * @param identifier ID for the webapp.  Used as a subdirectory name.
-     * @return File for storing information about the webapp.
+     * Returns the directory for a web app, creating it if necessary.
+     * @param webappId ID for the web app.  Used as a subdirectory name.
+     * @return File for storing information about the web app.
      */
-    public static File getWebappDirectory(String identifier) {
-        Context context = ApplicationStatus.getApplicationContext();
+    File getWebappDirectory(Context context, String webappId) {
         // Temporarily allowing disk access while fixing. TODO: http://crbug.com/525781

[Yaron, 2015/09/24 18:31:01]

Wait, wasn't the point of this to get rid of StrictMode violations. It's still
here. Is that the next CL?

[Yaron, 2015/09/24 19:30:45]

Got it - one fixed, one more to go.

[gone, 2015/09/24 19:37:11]

Got rid of one; can't get rid of this one at the moment.

         StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskReads();
         try {
-            File baseDirectory = context.getDir(WEBAPP_DIRECTORY_NAME, Context.MODE_PRIVATE);
-            File webappDirectory = new File(baseDirectory, identifier);
+            File webappDirectory = new File(getBaseWebappDirectory(context), webappId);
             if (!webappDirectory.exists() && !webappDirectory.mkdir()) {
-                Log.e(TAG, "Failed to create webapp directory.");
+                Log.e(TAG, "Failed to create web app directory.");
             }
             return webappDirectory;
         } finally {
             StrictMode.setThreadPolicy(oldPolicy);
         }
     }
+
+    /** Returns the directory containing all of Chrome's web app data, creating it if needed. */
+    final File getBaseWebappDirectory(Context context) {
+        return context.getDir(WEBAPP_DIRECTORY_NAME, Context.MODE_PRIVATE);
+    }
+
+    /** Returns a Set of Intents for all Chrome tasks currently known by the ActivityManager. */
+    @TargetApi(Build.VERSION_CODES.LOLLIPOP)
+    protected Set<Intent> getBaseIntentsForAllTasks() {
+        Set<Intent> baseIntents = new HashSet<Intent>();
+
+        Context context = ApplicationStatus.getApplicationContext();
+        ActivityManager manager =
+                (ActivityManager) context.getSystemService(Context.ACTIVITY_SERVICE);
+        for (AppTask task : manager.getAppTasks()) {
+            Intent intent = DocumentUtils.getBaseIntentFromTask(task);
+            if (intent != null) baseIntents.add(intent);
+        }
+
+        return baseIntents;
+    }
 }