[Android WebView] Always schedule new upload job after copying minidump.

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

Index: android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java
diff --git a/android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java b/android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java
index 53d6897441e1ee15605cc3730a034793a76b5bd2..ecd8c0a607c5a08adf8cb70a40801887a1bcc6a1 100644
--- a/android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java
+++ b/android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java
@@ -22,7 +22,6 @@ import org.chromium.components.minidump_uploader.CrashFileManager;
 
 import java.io.File;
 import java.io.IOException;
-import java.util.List;
 
 /**
  * Service that is responsible for receiving crash dumps from an application, for upload.
@@ -76,7 +75,7 @@ public class CrashReceiverService extends Service {
             boolean copySucceeded = copyMinidumps(context, uid, fileDescriptors);
             if (copySucceeded && scheduleUploads) {
                 // Only schedule a new job if there actually are any files to upload.
-                scheduleNewJobIfNoJobsActive();
+                scheduleNewJob();

[Ilya Sherman, 2017/02/11 01:00:47]

Hmm, you mentioned that a crash loop could actually leave us blind to the
crashes, because we'd be scheduling job after job without letting any of them
run (did I understand that correctly?).  Is it worth adding some complexity to
protect against that case?

[gsennton, 2017/02/13 10:46:55]

I added some discussion on this in crbug.com/687993 (and showed how we should be
able to solve this with synchronization). We would have to synchronize
CrashReceiverService with MinidumpUploaderImpl to ensure that we know whether
the currently running upload-job is canceling.
That synchronization shouldn't be super-complex by itself, but given that this
implementation is already fairly complex I don't think we want to add (that was
the conclusion we = me, Torne, Toby reached anyway).

If you disagree please do tell :)

[Ilya Sherman, 2017/02/13 22:28:15]

Okay, if I understand correctly, a crash loop wouldn't cause us to never upload
any crash reports, because once there are two crashes MinidumpUploaderJob would
at least see the first crash.  Is that correct?  If so, I agree with your
conclusion.  It would probably be good to (briefly) document the possible
failure mode in the code somewhere, and link to the bug for context/discussion.

[gsennton, 2017/02/14 16:49:41]

Please see the comment in MinidumpUploaderImpl for an explanation.

             }
         } finally {
             synchronized (mCopyingLock) {
@@ -105,23 +104,8 @@ public class CrashReceiverService extends Service {
         }
     }
 
-    /**
-     * @return the currently pending job with ID MINIDUMP_UPLOADING_JOB_ID, or null if no such job
-     * exists.
-     */
-    private static JobInfo getCurrentPendingJob(JobScheduler jobScheduler) {
-        List<JobInfo> pendingJobs = jobScheduler.getAllPendingJobs();
-        for (JobInfo job : pendingJobs) {
-            if (job.getId() == MINIDUMP_UPLOADING_JOB_ID) return job;
-        }
-        return null;
-    }
-
-    private void scheduleNewJobIfNoJobsActive() {
+    private void scheduleNewJob() {
         JobScheduler jobScheduler = (JobScheduler) getSystemService(Context.JOB_SCHEDULER_SERVICE);
-        if (getCurrentPendingJob(jobScheduler) != null) {
-            return;
-        }
         JobInfo newJob = new JobInfo
                 .Builder(MINIDUMP_UPLOADING_JOB_ID /* jobId */,
                     new ComponentName(this, MinidumpUploadJobService.class))