[Android Crash Reporting] Componentize MinidumpUploadImpl.java

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

Index: components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java
diff --git a/android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java b/components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java
similarity index 37%
rename from android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java
rename to components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java
index 14227a5ec489dbb18d8ae6ca430dccac6796c31b..fc11949407b60009f33115e754465377e819782e 100644
--- a/android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java
+++ b/components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java
@@ -2,28 +2,16 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-package org.chromium.android_webview.crash;
+package org.chromium.components.minidump_uploader;
 
-import android.content.Context;
-import android.net.ConnectivityManager;
-import android.net.NetworkInfo;
-import android.webkit.ValueCallback;
-
-import org.chromium.android_webview.PlatformServiceBridge;
-import org.chromium.android_webview.command_line.CommandLineUtil;
-import org.chromium.base.CommandLine;
 import org.chromium.base.Log;
 import org.chromium.base.ThreadUtils;
 import org.chromium.base.VisibleForTesting;
-import org.chromium.components.minidump_uploader.CrashFileManager;
-import org.chromium.components.minidump_uploader.MinidumpUploadCallable;
-import org.chromium.components.minidump_uploader.MinidumpUploader;
-import org.chromium.components.minidump_uploader.util.CrashReportingPermissionManager;
 
 import java.io.File;
 
 /**
- * Class in charge of uploading Minidumps from WebView's data directory.
+ * Class in charge of uploading minidumps from their local data directory.
  * This class gets invoked from a JobScheduler job and posts the operation of uploading minidumps to
  * a privately defined worker thread.
  * Note that this implementation is state-less in the sense that it doesn't keep track of whether it
@@ -32,111 +20,56 @@ import java.io.File;
  */
 public class MinidumpUploaderImpl implements MinidumpUploader {
     private static final String TAG = "MinidumpUploaderImpl";
-    private Context mContext;
-    private Thread mWorkerThread;
-    private final ConnectivityManager mConnectivityManager;
-    private final CrashFileManager mFileManager;
-
-    private boolean mCancelUpload = false;
 
-    private final boolean mCleanOutMinidumps;
-    private boolean mPermittedByUser = false;
+    /**
+     * The delegate that performs embedder-specific behavior.
+     */
+    private final MinidumpUploaderDelegate mDelegate;
 
-    @VisibleForTesting
-    public static final int MAX_UPLOAD_TRIES_ALLOWED = 3;
+    /**
+     * Manages the set of pending and failed local minidump files.
+     */
+    private final CrashFileManager mFileManager;
 
     /**
-     * Notify the worker thread that the current job has been canceled - so we shouldn't upload any
-     * more minidumps.
+     * Whether the current job has been canceled. This is written to from the main thread, and read
+     * from the worker thread.
      */
-    private void setCancelUpload(boolean cancel) {
-        mCancelUpload = cancel;
-    }
+    private volatile boolean mCancelUpload = false;
 
     /**
-     * Check whether the current job has been canceled.
+     * The thread used for the actual work of uploading minidumps.
      */
-    private boolean getCancelUpload() {
-        return mCancelUpload;
-    }
+    private Thread mWorkerThread;
 
     @VisibleForTesting
-    public MinidumpUploaderImpl(Context context, boolean cleanOutMinidumps) {
-        mConnectivityManager =
-                (ConnectivityManager) context.getSystemService(Context.CONNECTIVITY_SERVICE);
-        mContext = context;
-        File webviewCrashDir = CrashReceiverService.createWebViewCrashDir(context);
-        mFileManager = new CrashFileManager(webviewCrashDir);
+    public static final int MAX_UPLOAD_TRIES_ALLOWED = 3;
+
+    @VisibleForTesting
+    public MinidumpUploaderImpl(MinidumpUploaderDelegate delegate) {
+        mDelegate = delegate;
+        mFileManager = createCrashFileManager();
         if (!mFileManager.ensureCrashDirExists()) {
             Log.e(TAG, "Crash directory doesn't exist!");
         }
-        mCleanOutMinidumps = cleanOutMinidumps;
     }
 
     /**
-     * Utility method to allow us to test the logic of this class by injecting
-     * test-MinidumpUploadCallables.
+     * Utility method to allow tests to customize the behavior of the crash file manager.
      */
     @VisibleForTesting
-    public MinidumpUploadCallable createMinidumpUploadCallable(File minidumpFile, File logfile) {
-        return new MinidumpUploadCallable(
-                minidumpFile, logfile, createWebViewCrashReportingManager());
+    public CrashFileManager createCrashFileManager() {
+        return new CrashFileManager(mDelegate.createCrashDir());
     }
 
     /**
      * Utility method to allow us to test the logic of this class by injecting
-     * a test-PlatformServiceBridge.
+     * test-specific MinidumpUploadCallables.
      */
     @VisibleForTesting
-    public PlatformServiceBridge createPlatformServiceBridge() {
-        return PlatformServiceBridge.getInstance(mContext);
-    }
-
-    @VisibleForTesting
-    protected CrashReportingPermissionManager createWebViewCrashReportingManager() {
-        return new CrashReportingPermissionManager() {
-            @Override
-            public boolean isClientInMetricsSample() {
-                // We will check whether the client is in the metrics sample before
-                // generating a minidump - so if no minidump is generated this code will
-                // never run and we don't need to check whether we are in the sample.
-                // TODO(gsennton): when we switch to using Finch for this value we should use the
-                // Finch value here as well.
-                return true;
-            }
-            @Override
-            public boolean isNetworkAvailableForCrashUploads() {
-                // JobScheduler will call onStopJob causing our upload to be interrupted when our
-                // network requirements no longer hold.
-                NetworkInfo networkInfo = mConnectivityManager.getActiveNetworkInfo();
-                if (networkInfo == null || !networkInfo.isConnected()) return false;
-                return !mConnectivityManager.isActiveNetworkMetered();
-            }
-            @Override
-            public boolean isCrashUploadDisabledByCommandLine() {
-                return false;
-            }
-            /**
-             * This method is already represented by isClientInMetricsSample() and
-             * isNetworkAvailableForCrashUploads().
-             */
-            @Override
-            public boolean isMetricsUploadPermitted() {
-                return true;
-            }
-            @Override
-            public boolean isUsageAndCrashReportingPermittedByUser() {
-                return mPermittedByUser;
-            }
-            @Override
-            public boolean isUploadEnabledForTests() {
-                // Note that CommandLine/CommandLineUtil are not thread safe. They are initialized
-                // on the main thread, but before the current worker thread started - so this thread
-                // will have seen the initialization of the CommandLine.
-                return CommandLine.getInstance().hasSwitch(
-                        CommandLineUtil.CRASH_UPLOADS_ENABLED_FOR_TESTING_SWITCH);
-            }
-        };
+    public MinidumpUploadCallable createMinidumpUploadCallable(File minidumpFile, File logfile) {
+        return new MinidumpUploadCallable(
+                minidumpFile, logfile, mDelegate.createCrashReportingPermissionManager());
     }
 
     /**
@@ -155,19 +88,27 @@ public class MinidumpUploaderImpl implements MinidumpUploader {
         public void run() {
             File[] minidumps = mFileManager.getAllMinidumpFiles(MAX_UPLOAD_TRIES_ALLOWED);
             for (File minidump : minidumps) {
-                // Store the name of the current minidump so that we can mark it as a failure from
-                // the main thread if JobScheduler tells us to stop.
                 MinidumpUploadCallable uploadCallable = createMinidumpUploadCallable(
                         minidump, mFileManager.getCrashUploadLogFile());
                 int uploadResult = uploadCallable.call();
 
-                // Job was canceled -> early out: any clean-up will be performed in cancelUploads().
-                // Note that we check whether we are canceled AFTER trying to upload a minidump -
-                // this is to allow the uploading of at least one minidump per job (to deal with
-                // cases where we reschedule jobs over and over again and would never upload any
-                // minidumps because old jobs are canceled when scheduling new jobs).
-                if (getCancelUpload()) return;
-
+                // Bail if the job was canceled. Note that the cancelation status is checked AFTER
+                // trying to upload a minidump. This is to ensure that the scheduler attempts to
+                // upload at least one minidump per job. Otherwise, it's possible for a crash loop
+                // to continually write files to the crash directory; each such write would
+                // reschedule the job, and therefore cancel any pending jobs. In such a scenario,
+                // it's important to make at least *some* progress uploading minidumps.
+                // Note that other likely cancelation reasons are not a concern, because the upload
+                // callable checks relevant state prior to uploading. For example, if the job is
+                // canceled because the network connection is lost, or because the user switches
+                // over to a metered connection, the callable will detect the changed network state,
+                // and not attempt an upload.
+                if (mCancelUpload) return;
+
+                // Note that if the job was canceled midway through, the attempt number is not
+                // incremented, even if the upload failed. This is because a common reason for
+                // cancelation is loss of network connectivity, which does result in a failure, but
+                // it's a transient failure rather than something non-recoverable.
                 if (uploadResult == MinidumpUploadCallable.UPLOAD_FAILURE) {
                     String newName = CrashFileManager.tryIncrementAttemptNumber(minidump);
                     if (newName == null) {
@@ -178,9 +119,7 @@ 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 less minidumps.
-            if (mCleanOutMinidumps) {
-                mFileManager.cleanOutAllNonFreshMinidumpFiles();
-            }
+            mFileManager.cleanOutAllNonFreshMinidumpFiles();
 
             // Reschedule if there are still minidumps to upload.
             boolean reschedule =
@@ -192,50 +131,53 @@ public class MinidumpUploaderImpl implements MinidumpUploader {
     @Override
     public void uploadAllMinidumps(
             final MinidumpUploader.UploadsFinishedCallback uploadsFinishedCallback) {
-        // This call should happen on the main thread
         ThreadUtils.assertOnUiThread();
         if (mWorkerThread != null) {
-            throw new RuntimeException("Only one upload-job should be active at a time");
+            throw new RuntimeException(
+                    "A given minidump uploader instance should never be launched more than once.");
         }
-        mWorkerThread = new Thread(new UploadRunnable(uploadsFinishedCallback), "mWorkerThread");
-        setCancelUpload(false);
+        mWorkerThread = new Thread(
+                new UploadRunnable(uploadsFinishedCallback), "MinidumpUploader-WorkerThread");
+        mCancelUpload = false;
 
-        createPlatformServiceBridge().queryMetricsSetting(new ValueCallback<Boolean>() {
-            public void onReceiveValue(Boolean enabled) {
-                // This callback should be received on the main thread (e.g. mWorkerThread
-                // is not thread-safe).
+        mDelegate.prepareToUploadMinidumps(new Runnable() {
+            @Override
+            public void run() {
                 ThreadUtils.assertOnUiThread();
 
-                mPermittedByUser = enabled;
-                // Note that our job might have been cancelled by this time - however, we do start
-                // our worker thread anyway to try to make some progress towards uploading
-                // minidumps.
-                // This is to ensure that in the case where an app is crashing over and over again
-                // - and we are rescheduling jobs over and over again - we are still uploading at
-                // least one minidump per job, as long as that job starts before it is canceled by
-                // the next job.
-                // For cases where the job is cancelled because the network connection is lost, or
-                // because we switch over to a metered connection, we won't try to upload any
-                // minidumps anyway since we check the network connection just before the upload of
-                // each minidump.
+                // Note that the upload job might have been canceled by this time. However, it's
+                // important to start the worker thread anyway to try to make some progress towards
+                // uploading minidumps. This is to ensure that in the case where an app is crashing
+                // over and over again, resulting in rescheduling jobs over and over again, there's
+                // still a chance to upload at least one minidump per job, as long as that job
+                // starts before it is canceled by the next job. See the UploadRunnable
+                // implementation for more details.
                 mWorkerThread.start();
             }
         });
     }
 
-    @VisibleForTesting
-    public void joinWorkerThreadForTesting() throws InterruptedException {
-        mWorkerThread.join();
-    }
-
     /**
-     * @return whether to reschedule the uploads.
+     * @return Whether to reschedule the uploads.
      */
     @Override
     public boolean cancelUploads() {
-        setCancelUpload(true);
+        mCancelUpload = true;
 
         // Reschedule if there are still minidumps to upload.
         return mFileManager.getAllMinidumpFiles(MAX_UPLOAD_TRIES_ALLOWED).length > 0;
     }
+
+    /**
+     * Utility method to allow tests to access the delegate.
+     */
+    @VisibleForTesting
+    public MinidumpUploaderDelegate getDelegateForTesting() {
+        return mDelegate;
+    }
+
+    @VisibleForTesting
+    public void joinWorkerThreadForTesting() throws InterruptedException {
+        mWorkerThread.join();
+    }
 }