[Android Crash Reporting] Allow uploading minidumps via the JobScheduler

chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java

Index: chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java b/chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java
index 148b8a6097a18854e69ec18e84ee524609ea4d42..85bc9b697561c30955c420849c22076968178074 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java
@@ -4,9 +4,14 @@
 
 package org.chromium.chrome.browser.crash;
 
+import android.annotation.SuppressLint;
 import android.app.IntentService;
+import android.app.job.JobInfo;
+import android.content.ComponentName;
 import android.content.Context;
 import android.content.Intent;
+import android.os.Build;
+import android.os.PersistableBundle;
 import android.support.annotation.StringDef;
 
 import org.chromium.base.Log;
@@ -14,10 +19,12 @@ import org.chromium.base.StreamUtil;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.metrics.RecordHistogram;
+import org.chromium.chrome.browser.ChromeFeatureList;
 import org.chromium.chrome.browser.preferences.ChromePreferenceManager;
 import org.chromium.chrome.browser.preferences.privacy.PrivacyPreferencesManager;
 import org.chromium.components.minidump_uploader.CrashFileManager;
 import org.chromium.components.minidump_uploader.MinidumpUploadCallable;
+import org.chromium.components.minidump_uploader.MinidumpUploadJobService;
 import org.chromium.components.minidump_uploader.util.CrashReportingPermissionManager;
 
 import java.io.BufferedReader;
@@ -41,6 +48,12 @@ public class MinidumpUploadService extends IntentService {
     static final String UPLOAD_LOG_KEY = "upload_log";
 
     /**
+     * The job id for uploading minidumps. For more info on this constant, see
+     * https://developer.android.com/reference/android/app/job/JobInfo.Builder.html#JobInfo.Builder(int,%20android.content.ComponentName)
+     */
+    private static final int MINIDUMP_UPLOADING_JOB_ID = 142;

[gsennton, 2017/03/14 18:17:28]

I just realized it might be good to look for the IDs we are already using ;).
It looks like the different values for JobIds in Chrome are being set in
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/TaskIds.java

I think we should at least add a comment there describing which IDs we are using
for WebView+Chrome Minidump uploading.
(in the long run we might want to just use all the utility in
components/background_task_scheduler/ to implement our job-scheduler).

[Ilya Sherman, 2017/03/15 02:13:34]

Good call!  Moved the constants into that component.

And, yeah, I think after merging things to M58 I'll update the code to use that
component.  Looks much nicer than maintaining two similar-but-not-the-same code
paths =D

[gsennton, 2017/03/15 12:57:43]

Indeed, I wonder if there will be anything stopping us from using that code-path
for WebView as well. Will have to take a look ;)

+
+    /**
      * The number of times we will try to upload a crash.
      */
     public static final int MAX_TRIES_ALLOWED = 3;
@@ -68,6 +81,47 @@ public class MinidumpUploadService extends IntentService {
     }
 
     /**
+     * @return Whether to use the JobSchduler API to upload crash reports, rather than directly
+     *     creating a service for uploading.
+     */
+    public static boolean shouldUseJobSchedulerForUploads() {
+        // The JobScheduler API is only available as of Android L.
+        if (Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP) {
+            return false;
+        }
+
+        return ChromeFeatureList.isEnabled(
+                ChromeFeatureList.UPLOAD_CRASH_REPORTS_USING_JOB_SCHEDULER);
+    }
+
+    /**
+     * Schedules uploading of all pending minidumps, using the JobScheduler API.
+     */
+    @SuppressLint("NewApi")
+    public static void scheduleUploadJob(Context context) {
+        assert shouldUseJobSchedulerForUploads();
+
+        CrashReportingPermissionManager permissionManager = PrivacyPreferencesManager.getInstance();
+        PersistableBundle permissions = new PersistableBundle();
+        // Note: putBoolean is only available in API level 22, so massage the data into ints
+        // instead.
+        permissions.putInt(ChromeMinidumpUploaderDelegate.IS_CLIENT_IN_METRICS_SAMPLE,
+                permissionManager.isClientInMetricsSample() ? 1 : 0);
+        permissions.putInt(ChromeMinidumpUploaderDelegate.IS_CRASH_UPLOAD_DISABLED_BY_COMMAND_LINE,
+                permissionManager.isCrashUploadDisabledByCommandLine() ? 1 : 0);
+        permissions.putInt(ChromeMinidumpUploaderDelegate.IS_UPLOAD_ENABLED_FOR_TESTS,
+                permissionManager.isUploadEnabledForTests() ? 1 : 0);
+
+        JobInfo.Builder builder =
+                new JobInfo
+                        .Builder(MINIDUMP_UPLOADING_JOB_ID,
+                                new ComponentName(context, ChromeMinidumpUploadJobService.class))
+                        .setRequiredNetworkType(JobInfo.NETWORK_TYPE_ANY)
+                        .setExtras(permissions);
+        MinidumpUploadJobService.scheduleUpload(context, builder);
+    }
+
+    /**
      * Stores the successes and failures from uploading crash to UMA,
      */
     public static void storeBreakpadUploadStatsInUma(ChromePreferenceManager pref) {
@@ -106,10 +160,6 @@ public class MinidumpUploadService extends IntentService {
             return;
         }
         int tries = CrashFileManager.readAttemptNumber(minidumpFileName);
-        // -1 means no attempt number was read.
-        if (tries == -1) {
-            tries = 0;
-        }
 
         // Since we do not rename a file after reaching max number of tries,
         // files that have maxed out tries will NOT reach this.
@@ -132,7 +182,7 @@ public class MinidumpUploadService extends IntentService {
 
         if (uploadStatus == MinidumpUploadCallable.UPLOAD_SUCCESS) {
             // Only update UMA stats if an intended and successful upload.
-            incrementCrashSuccessUploadCount(getNewNameAfterSuccessfulUpload(minidumpFileName));
+            incrementCrashSuccessUploadCount(minidumpFileName);
         } else if (uploadStatus == MinidumpUploadCallable.UPLOAD_FAILURE) {
             // Unable to upload minidump. Incrementing try number and restarting.
 
@@ -212,12 +262,12 @@ public class MinidumpUploadService extends IntentService {
      * crashes by looking into the file.
      * @param fileName is the name of a minidump file that contains the type of crash.
      */
-    private void incrementCrashSuccessUploadCount(String fileName) {
+    public static void incrementCrashSuccessUploadCount(String fileName) {
         ChromePreferenceManager.getInstance().incrementCrashSuccessUploadCount(
-                getCrashType(fileName));
+                getCrashType(getNewNameAfterSuccessfulUpload(fileName)));

[gsennton, 2017/03/14 18:17:28]

Hiding this call in here seems like an implementation detail that could easily
be missed/overseen? (I might be wrong though ;))

[Ilya Sherman, 2017/03/15 02:13:34]

That's a very good point, and I actually had a bug for incrementing the failure
count.  The current state is awkward because MinidumpUploadCallable renames
successful crashes, but not failed ones.  I added thorough documentation,
though, in hopes that it will help (a) clarify and (b) alert the reader to be
careful.  Do you think that's sufficient for now?

[gsennton, 2017/03/15 12:57:43]

Yeah, that looks fine ;).

     }
 
-    private void incrementCrashFailureUploadCount(String fileName) {
+    public static void incrementCrashFailureUploadCount(String fileName) {
         ChromePreferenceManager.getInstance().incrementCrashFailureUploadCount(
                 getCrashType(fileName));
     }
@@ -249,6 +299,7 @@ public class MinidumpUploadService extends IntentService {
      */
     public static void tryUploadCrashDump(Context context, File minidumpFile)
             throws SecurityException {
+        assert !shouldUseJobSchedulerForUploads();
         CrashFileManager fileManager = new CrashFileManager(context.getCacheDir());
         Intent intent = new Intent(context, MinidumpUploadService.class);
         intent.setAction(ACTION_UPLOAD);
@@ -268,11 +319,12 @@ public class MinidumpUploadService extends IntentService {
      * @param context Context of the application.
      */
     public static void tryUploadAllCrashDumps(Context context) {
+        assert !shouldUseJobSchedulerForUploads();
         CrashFileManager fileManager = new CrashFileManager(context.getCacheDir());
         File[] minidumps = fileManager.getAllMinidumpFiles(MAX_TRIES_ALLOWED);
         Log.i(TAG, "Attempting to upload accumulated crash dumps.");
         for (File minidump : minidumps) {
-            MinidumpUploadService.tryUploadCrashDump(context, minidump);
+            tryUploadCrashDump(context, minidump);
         }
     }
 
@@ -305,6 +357,11 @@ public class MinidumpUploadService extends IntentService {
             Log.w(TAG, "Could not rename the file " + minidumpFile.getName() + " for re-upload");
             return;
         }
-        MinidumpUploadService.tryUploadCrashDump(context, renamedMinidumpFile);
+
+        if (shouldUseJobSchedulerForUploads()) {
+            scheduleUploadJob(context);
+        } else {
+            tryUploadCrashDump(context, renamedMinidumpFile);
+        }
     }
 }