Move Minidump Uploading IO operations off the UI thread.

Before this CL we would create a CrashFileManager in
MinidumpUploaderImpl on the UI thread. CrashFileManager requires a
directory in which to store minidumps, so creating that directory, or
ensuring that it exists, requires disk operations.
In this CL we move the creation of the CrashFileManager onto the worker
thread where the actual minidump uploading happens.

Also change the rescheduling logic for minidump uploading slightly,
Previously we would check whether there are any minidumps left, at the
time when the current job is cancelled, from the UI thread. But that
requires a disk operation, so now we instead update whether to
reschedule uploads, from the worker thread, for the UI thread to read in
a thread-safe way (without disk reads).

Note that this change affects the minidump uploading of both WebView and
Chrome.

BUG=714138

Review-Url: https://codereview.chromium.org/2838383002
Cr-Commit-Position: refs/heads/master@{#468955}

android_webview/java/src/org/chromium/android_webview/crash/AwMinidumpUploaderDelegate.java

@@ -37,7 +37,7 @@ public class AwMinidumpUploaderDelegate implements MinidumpUploaderDelegate {
 
     @Override
     public File getCrashParentDir() {
-        return CrashReceiverService.getOrCreateWebViewCrashDir();
+        return CrashReceiverService.getWebViewCrashDir();
     }
 
     @Override

android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java

@@ -140,20 +140,9 @@ public class MinidumpUploaderTest extends CrashTestCase {
                     }
                 });
 
-        File firstFile = createMinidumpFileInCrashDir("1_abc.dmp0");
-        File secondFile = createMinidumpFileInCrashDir("12_abcd.dmp0");
-        File expectedFirstUploadFile =
-                new File(mCrashDir, firstFile.getName().replace(".dmp", ".up"));
-        File expectedSecondUploadFile =
-                new File(mCrashDir, secondFile.getName().replace(".dmp", ".up"));
-
+        // Ensure that we don't crash when trying to upload minidumps without a crash directory.
         MinidumpUploadTestUtility.uploadMinidumpsSync(
                 minidumpUploader, false /* expectReschedule */);
-
-        assertFalse(firstFile.exists());
-        assertTrue(expectedFirstUploadFile.exists());
-        assertFalse(secondFile.exists());
-        assertTrue(expectedSecondUploadFile.exists());
     }
 
     private interface MinidumpUploadCallableCreator {
@@ -258,6 +247,31 @@ public class MinidumpUploaderTest extends CrashTestCase {
         }
     }
 
+    /**
+     * Minidump uploader implementation that stalls minidump-uploading until a given CountDownLatch
+     * counts down.
+     */
+    private static class StallingMinidumpUploaderImpl extends TestMinidumpUploaderImpl {
+        CountDownLatch mStopStallingLatch;
+        boolean mSuccessfulUpload;
+
+        public StallingMinidumpUploaderImpl(File cacheDir,
+                CrashReportingPermissionManager permissionManager, CountDownLatch stopStallingLatch,
+                boolean successfulUpload) {
+            super(cacheDir, permissionManager);
+            mStopStallingLatch = stopStallingLatch;
+            mSuccessfulUpload = successfulUpload;
+        }
+
+        @Override
+        public MinidumpUploadCallable createMinidumpUploadCallable(
+                File minidumpFile, File logfile) {
+            return new MinidumpUploadCallable(minidumpFile, logfile,
+                    new StallingHttpUrlConnectionFactory(mStopStallingLatch, mSuccessfulUpload),
+                    mDelegate.createCrashReportingPermissionManager());
+        }
+    }
+
     private static class FailingHttpUrlConnectionFactory implements HttpURLConnectionFactory {
         public HttpURLConnection createHttpURLConnection(String url) {
             return null;
@@ -286,16 +300,8 @@ public class MinidumpUploaderTest extends CrashTestCase {
                     { mIsEnabledForTests = true; }
                 };
         final CountDownLatch stopStallingLatch = new CountDownLatch(1);
-        MinidumpUploaderImpl minidumpUploader = new TestMinidumpUploaderImpl(
-                getExistingCacheDir(), permManager) {
-            @Override
-            public MinidumpUploadCallable createMinidumpUploadCallable(
-                    File minidumpFile, File logfile) {
-                return new MinidumpUploadCallable(minidumpFile, logfile,
-                        new StallingHttpUrlConnectionFactory(stopStallingLatch, successfulUpload),
-                        permManager);
-            }
-        };
+        MinidumpUploaderImpl minidumpUploader = new StallingMinidumpUploaderImpl(
+                getExistingCacheDir(), permManager, stopStallingLatch, successfulUpload);
 
         File firstFile = createMinidumpFileInCrashDir("123_abc.dmp0");
         File expectedFirstUploadFile =
@@ -339,6 +345,40 @@ public class MinidumpUploaderTest extends CrashTestCase {
         }
     }
 
+    /**
+     * Ensure that canceling an upload that fails causes a reschedule.
+     */
+    @MediumTest
+    public void testCancelFailedUploadCausesReschedule() throws IOException {
+        final CrashReportingPermissionManager permManager =
+                new MockCrashReportingPermissionManager() {
+                    { mIsEnabledForTests = true; }
+                };
+        final CountDownLatch stopStallingLatch = new CountDownLatch(1);
+        MinidumpUploaderImpl minidumpUploader =
+                new StallingMinidumpUploaderImpl(getExistingCacheDir(), permManager,
+                        stopStallingLatch, false /* successfulUpload */);
+
+        createMinidumpFileInCrashDir("123_abc.dmp0");
+
+        MinidumpUploader.UploadsFinishedCallback crashingCallback =
+                new MinidumpUploader.UploadsFinishedCallback() {
+                    @Override
+                    public void uploadsFinished(boolean reschedule) {
+                        // We don't guarantee whether uploadsFinished is called after a job has been
+                        // cancelled, but if it is, it should indicate that we want to reschedule
+                        // the job.
+                        assertTrue(reschedule);
+                    }
+                };
+
+        // This is run on the UI thread to avoid failing any assertOnUiThread assertions.
+        MinidumpUploadTestUtility.uploadAllMinidumpsOnUiThread(minidumpUploader, crashingCallback);
+        // Ensure we tell JobScheduler to reschedule the job.
+        assertTrue(minidumpUploader.cancelUploads());
+        stopStallingLatch.countDown();
+    }
+
     /**
      * Ensures that the minidump copying works together with the minidump uploading.
      */

components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java

@@ -296,13 +296,20 @@ public class CrashFileManager {
      * Create the crash directory for this file manager unless it exists already.
      * @return true iff the crash directory exists when this method returns.
      */
-    public boolean ensureCrashDirExists() {
+    private boolean ensureCrashDirExists() {
         File crashDir = getCrashDirectory();
         // Call mkdir before isDirectory to ensure that if another thread created the directory
         // just before the call to mkdir, the current thread fails mkdir, but passes isDirectory.
         return crashDir.mkdir() || crashDir.isDirectory();
     }
 
+    /**
+     * @return whether the crash directory already exists.
+     */
+    public boolean crashDirectoryExists() {
+        return getCrashDirectory().isDirectory();
+    }
+
     /**
      * Returns all minidump files that could still be uploaded, sorted by modification time stamp.
      * Only returns files that we have tried to upload less than {@param maxTries} number of times.

components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java

@@ -27,11 +27,6 @@ public class MinidumpUploaderImpl implements MinidumpUploader {
     @VisibleForTesting
     protected final MinidumpUploaderDelegate mDelegate;
 
-    /**
-     * Manages the set of pending and failed local minidump files.
-     */
-    private final CrashFileManager mFileManager;
-
     /**
      * Whether the current job has been canceled. This is written to from the main thread, and read
      * from the worker thread.
@@ -49,10 +44,6 @@ public class MinidumpUploaderImpl implements MinidumpUploader {
     @VisibleForTesting
     public MinidumpUploaderImpl(MinidumpUploaderDelegate delegate) {
         mDelegate = delegate;
-        mFileManager = createCrashFileManager(mDelegate.getCrashParentDir());
-        if (!mFileManager.ensureCrashDirExists()) {
-            Log.e(TAG, "Crash directory doesn't exist!");
-        }
     }
 
     /**
@@ -89,12 +80,29 @@ public class MinidumpUploaderImpl implements MinidumpUploader {
 
         @Override
         public void run() {
-            File[] minidumps = mFileManager.getAllMinidumpFiles(MAX_UPLOAD_TRIES_ALLOWED);
+            // If the directory in where we store minidumps doesn't exist - then early out because
+            // there are no minidumps to upload.
+            File crashParentDir = mDelegate.getCrashParentDir();
+            if (!crashParentDir.isDirectory()) {
+                Log.e(TAG, "Parent crash directory doesn't exist!");
+                mUploadsFinishedCallback.uploadsFinished(false /* reschedule */);
+                return;
+            }
+
+            final CrashFileManager fileManager = createCrashFileManager(crashParentDir);
+            if (!fileManager.crashDirectoryExists()) {
+                Log.e(TAG, "Crash directory doesn't exist!");
+                mUploadsFinishedCallback.uploadsFinished(false /* reschedule */);
+                return;
+            }
+
+            File[] minidumps = fileManager.getAllMinidumpFiles(MAX_UPLOAD_TRIES_ALLOWED);
+
             Log.i(TAG, "Attempting to upload %d minidumps.", minidumps.length);
             for (File minidump : minidumps) {
                 Log.i(TAG, "Attempting to upload " + minidump.getName());
-                MinidumpUploadCallable uploadCallable = createMinidumpUploadCallable(
-                        minidump, mFileManager.getCrashUploadLogFile());
+                MinidumpUploadCallable uploadCallable =
+                        createMinidumpUploadCallable(minidump, fileManager.getCrashUploadLogFile());
                 int uploadResult = uploadCallable.call();
 
                 // Record metrics about the upload.
@@ -137,11 +145,11 @@ public class MinidumpUploaderImpl implements MinidumpUploader {
 
             // Clean out old/uploaded minidumps. Note that this clean-up method is more strict than
             // our copying mechanism in the sense that it keeps fewer minidumps.
-            mFileManager.cleanOutAllNonFreshMinidumpFiles();
+            fileManager.cleanOutAllNonFreshMinidumpFiles();
 
             // Reschedule if there are still minidumps to upload.
             boolean reschedule =
-                    mFileManager.getAllMinidumpFiles(MAX_UPLOAD_TRIES_ALLOWED).length > 0;
+                    fileManager.getAllMinidumpFiles(MAX_UPLOAD_TRIES_ALLOWED).length > 0;
             mUploadsFinishedCallback.uploadsFinished(reschedule);
         }
     }
@@ -182,8 +190,13 @@ public class MinidumpUploaderImpl implements MinidumpUploader {
     public boolean cancelUploads() {
         mCancelUpload = true;
 
-        // Reschedule if there are still minidumps to upload.
-        return mFileManager.getAllMinidumpFiles(MAX_UPLOAD_TRIES_ALLOWED).length > 0;
+        // We always return true here to reschedule the job even in cases where the are no minidumps
+        // left to upload. We choose to allow this minor inconsistency to avoid blocking the
+        // UI-thread on IO operations. The unnecessary rescheduling only happens if we cancel the
+        // job after it has attempted to upload all minidumps but before the job finishes.
+        // If a job is rescheduled unnecessarily, the next time it starts it will have no minidumps
+        // to upload and thus finish without yet another rescheduling.
+        return true;
     }
 
     @VisibleForTesting