[Android Crash Reporting] Allow uploading minidumps via the JobScheduler

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

Index: components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java
diff --git a/components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java b/components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java
index c275030846f7b104471f8a6dc2c0397644de1848..24fa3f5c3b42c583a7b540a7a85e139a55c03e6c 100644
--- a/components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java
+++ b/components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java
@@ -4,18 +4,59 @@
 package org.chromium.components.minidump_uploader;
 
 import android.annotation.TargetApi;
+import android.app.job.JobInfo;
 import android.app.job.JobParameters;
+import android.app.job.JobScheduler;
 import android.app.job.JobService;
+import android.content.Context;
 import android.os.Build;
+import android.os.PersistableBundle;
+
+import org.chromium.base.Log;
 
 /**
  * Class that interacts with the Android JobScheduler to upload Minidumps at appropriate times.
  */
 @TargetApi(Build.VERSION_CODES.LOLLIPOP)

[Maria, 2017/03/16 04:10:43]

switch to Marshmallow?

[Ilya Sherman, 2017/03/16 05:21:03]

This class is shared between Chrome and Android Webview, so I left it at
Lollipop, which is what Webview is targeting.

 public abstract class MinidumpUploadJobService extends JobService {

[nyquist, 2017/03/16 14:52:18]

Would it be possible to use BackgroundTask here instead?

[Ilya Sherman, 2017/03/16 17:46:29]

This code is shared with the Android Webview implementation, so I'd prefer not
to switch over to BackgroundTask for M58 (which this code is destined to be
merged to).  For M59 (i.e. in a follow-up CL), possibly.  It depends: does
BackgroundTask support Android L?

[nyquist, 2017/03/16 18:50:02]

BackgroundTask does not support Android L yet.

Also; for pre-M it will have to use GCM for now. Does WebView also roll in GMS
core?

[Ilya Sherman, 2017/03/16 18:56:08]

Okay.  In that case it sounds like it wouldn't make sense to switch this over
yet -- agreed?
I'm not sure whether Webview rolls in GMS core.  Gustav (gsennton@) would be the
best person to discuss with -- he's OOO until next week though.

[Tobias Sargeant, 2017/03/16 19:01:36]

Also WebView has different requirements to Chrome here; this code runs in a
separate service in the WebView package, and so we need to be able to schedule
the work for a later point when network/power conditions are favourable, and
still be able to have the service exit. I don't think BackgroundTask would give
us that ability.

We do link the GMSCore client library, but it's only a recent development.

[nyquist, 2017/03/16 20:06:26]

I'm not sure what you mean? For now BackgroundTask is mostly a wrapper around
JobService. It has the same ability to run later. It still requires an entry in
the manifest though.

See:
https://chromium.googlesource.com/chromium/src/+/master/components/background...

[Tobias Sargeant, 2017/03/16 20:51:30]

Sorry, I spoke too quickly without looking into how BackgroundTask was
implemented. Thanks for the docs pointer.

-    Object mRunningLock = new Object();
-    boolean mRunningJob = false;
-    MinidumpUploader mMinidumpUploader;
+    private static final String TAG = "MinidumpJobService";
+
+    // Initial back-off time for upload-job, i.e. the minimum delay when a job is retried. A retry
+    // will happen when there are minidumps left after trying to upload all minidumps. This could
+    // happen if an upload attempt fails, or if more minidumps are added at the same time as
+    // uploading old ones. The initial backoff is set to a fairly high number (30 minutes) to
+    // increase the chance of performing uploads in batches if the initial upload fails.
+    private static final int JOB_INITIAL_BACKOFF_TIME_IN_MS = 1000 * 60 * 30;
+
+    // Back-off policy for upload-job.
+    private static final int JOB_BACKOFF_POLICY = JobInfo.BACKOFF_POLICY_EXPONENTIAL;
+
+    private MinidumpUploader mMinidumpUploader;
+
+    // Used in Debug builds to assert that this job service never attempts to run more than one job
+    // at a time:
+    private final Object mRunningLock = new Object();
+    private boolean mRunningJob = false;
+
+    /**
+     * Schedules uploading of all pending minidumps.
+     * @param context The application context, in which to schedule the crash report uploads.
+     * @param jobInfoBuilder A job info builder that has been initialized with any embedder-specific
+     *     requriements. This builder will be extended to include shared requirements, and then used
+     *     to build an upload job for scheduling.
+     */
+    public static void scheduleUpload(Context context, JobInfo.Builder jobInfoBuilder) {
+        Log.i(TAG, "Scheduling upload of all pending minidumps.");
+        JobScheduler scheduler =
+                (JobScheduler) context.getSystemService(Context.JOB_SCHEDULER_SERVICE);
+        JobInfo uploadJob =
+                jobInfoBuilder
+                        .setBackoffCriteria(JOB_INITIAL_BACKOFF_TIME_IN_MS, JOB_BACKOFF_POLICY)
+                        .build();
+        if (scheduler.schedule(uploadJob) == JobScheduler.RESULT_FAILURE) {
+            throw new RuntimeException("Couldn't schedule " + uploadJob);

[Maria, 2017/03/16 04:10:43]

hmm, when can this happen? If I am reading it correctly, this is the function
that gets called from the deferred handler. So throwing an exception here will
crash chrome. That seems like a weird thing to do if we couldn't schedule
uploads for crash reports...

[Ilya Sherman, 2017/03/16 05:21:03]

It can only happen if the job is misconfigured, i.e if the builder is given an
invalid set of parameters.  Would you prefer this to be handled somehow other
than by throwing an exception?  This should only ever fail deterministically.  I
agree that a crash loop would be bad, though -- perhaps logging an error makes
more sense?  (This is code that I moved from the Android Webview implementation,
fwiw.)

[Maria, 2017/03/16 05:45:19]

Hmm, how about an assert instead? It will alert the developers early, but won't
get triggered in production builds.

[Ilya Sherman, 2017/03/16 17:46:29]

Okay, I'll check later today whether an assert would have triggered in my build
configuration.  I'm a little concerned that asserts are going to tend to be
disabled in the near-Official-Release build configuration that's needed for the
crash reporting code to actually run.  If an assert doesn't trigger in that
configuration, do you have any preferences for a backup option?

[Ilya Sherman, 2017/03/16 18:35:19]

Actually, I suppose we use asserts in various other places in the crash
reporting code as well, and if my build configuration doesn't trigger assertion
failures, I should probably fix up my build configuration somehow.  So, I went
ahead and switched to using an assert here.  Done.

+        }
+    }
 
     @Override
     public boolean onStartJob(JobParameters params) {
@@ -24,13 +65,14 @@ public abstract class MinidumpUploadJobService extends JobService {
             assert !mRunningJob;
             mRunningJob = true;
         }
-        mMinidumpUploader = createMinidumpUploader();
+        mMinidumpUploader = createMinidumpUploader(params.getExtras());
         mMinidumpUploader.uploadAllMinidumps(createJobFinishedCallback(params));
         return true; // true = processing work on a separate thread, false = done already.
     }
 
     @Override
     public boolean onStopJob(JobParameters params) {
+        Log.i(TAG, "Canceling pending uploads due to change in networking status.");
         boolean reschedule = mMinidumpUploader.cancelUploads();
         synchronized (mRunningLock) {
             mRunningJob = false;
@@ -49,6 +91,9 @@ public abstract class MinidumpUploadJobService extends JobService {
         return new MinidumpUploader.UploadsFinishedCallback() {
             @Override
             public void uploadsFinished(boolean reschedule) {
+                if (reschedule) {
+                    Log.i(TAG, "Some minidumps remain un-uploaded; rescheduling.");
+                }
                 synchronized (mRunningLock) {
                     mRunningJob = false;
                 }
@@ -58,7 +103,8 @@ public abstract class MinidumpUploadJobService extends JobService {
     }
 
     /**
+     * @param extras Any extra data persisted for this job.
      * @return The minidump uploader that jobs should use.
      */
-    protected abstract MinidumpUploader createMinidumpUploader();
+    protected abstract MinidumpUploader createMinidumpUploader(PersistableBundle extras);
 }