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

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

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // 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;
 
 import android.content.Context;
 import android.net.ConnectivityManager;
 import android.net.NetworkInfo;
 import android.webkit.ValueCallback;
@@ skipping 184 matching lines @@
         mWorkerThread = new Thread(new UploadRunnable(uploadsFinishedCallback), "mWorkerThread");
         setCancelUpload(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).
                 ThreadUtils.assertOnUiThread();
 
                 mPermittedByUser = enabled;
-                // Our job might have been cancelled by now - make sure we honour this.
-                if (!getCancelUpload()) {
-                    mWorkerThread.start();
-                }
+                // Start our job even if it might have been cancelled already - we want to try to
+                // make some progress whenever we start a job.

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

This is a bit unclear to me -- are we simply ignoring the cancelation?  Or is it
respected somewhere further along in the sequence of operations?

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

We are indeed ignoring the initial cancellation here, i.e. we are removing step
3 below:

1. ask gmsCore - do we have user approval?
2. wait for response from GmsCore
3. have we cancelled yet? if so, bail/return
4. for each minidump:
   5. if on unmetered connection and we have user consent: upload minidump
   6. have we cancelled yet? if so, bail/return.

so what this means is that a started job will always try to upload one minidump
before cancelling (this seems right because cancelling a job that just started
doesn't make sense anyway).
Given that we make sure that we are on an unmetered connection before performing
any uploads, I believe this should be fine. I think the worst-case scenario here
is that we happen to upload a minidump twice because the new job runs before the
old one has renamed the uploaded file.

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

Could we simply swap steps 5 and 6, thereby getting the benefit of early
cancelation (if for some reason it does get triggered) and also simpler code? 
Or, is early cancelation actively undesirable?  If the latter, what's a context
in which the task might get canceled early, where we wouldn't want it to be?

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

So the deal is this:
When you schedule a new JobScheduler job with the same ID as an existing job,
the existing job will be cancelled and the new job will be scheduled instead.
If the existing job hasn't started yet it will just be removed from the
JobScheduler data structure, while if it has already started JobScheduler will
tell the job to stop by calling JobService.onStopJob() (which is the method that
will call setCancelUpload(true) in our implementation).

So, in the case where we are crashing and copying minidumps very often we will
end up scheduling our job very often - thus canceling the previous job.
We thought it would be better to not allow a cancellation of the job before the
upload of the first minidump - since by not allowing such a cancellation we
always make progress as long as each job is started before the next job
cancels/removes it.

Also, not allowing a cancellation before the first minidump-upload doesn't seem
harmful since we are still checking our network connection before each upload,
and since normally we shouldn't be cancelled just after being started anyway. 

[Ilya Sherman, 2017/02/15 00:21:05]

Okay, I see, I think I understand.  I think it would be helpful to include more
of this context in the comment within this file.

Also, are you sure that there aren't any other reasons that the job might be
cancelled almost immediately?  I understand why you don't want the job to be
cancelled for *this* reason, but maybe there are other valid reasons?

[gsennton, 2017/02/15 10:26:58]

Done (ptal).
 
I am not a 100% sure that there aren't any other reasons the job might be
cancelled almost immediately - there might be cases where we are cancelled
because of some device-state change (like going into doze-mode or something).
I still don't think this is a major problem - we are only finishing one upload
anyway - and the case we are the most interested in (when the network connection
goes away / switches to a metered connection) is checked just before each
upload.
Also, we could just as well be cancelled just after starting a minidump upload -
and then there isn't a way for us to cancel it. 

+                mWorkerThread.start();
             }
         });
     }
 
+    @VisibleForTesting
+    public void joinWorkerThreadForTesting() throws InterruptedException {
+        mWorkerThread.join();
+    }
+
     /**
      * @return whether to reschedule the uploads.
      */
     @Override
     public boolean cancelUploads() {
         setCancelUpload(true);
 
         // Reschedule if there are still minidumps to upload.
         return mFileManager.getAllMinidumpFiles(MAX_UPLOAD_TRIES_ALLOWED).length > 0;
     }
 }