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

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

When we copy a minidump from an app directory to WebView's directory we
schedule a new minidump-uploading job. Before this CL we would only
schedule such a job if no previous job was running - but doing so means
there are cases where the old job might be finishing up, while we
are in the process of determining whether to run the new job. In such
cases we wouldn't schedule a new job for the new minidump - thus
postponing the upload of the new minidump until the next time a crash
occurs and a new job is scheduled.
Synchronizing the job-starting code and the code running in the jobs
themselves would add complexity to the current code - instead we
schedule a new job unconditionally.
Note that a drawback with this new approach is that if we schedule new
jobs too often we might never run our jobs (since scheduling a new job
cancels the old one). Though as long as a job starts it will try to
upload at least one minidump, ensuring we make progress in that case.

Add a test for the cancellation behaviour, and fix a previous
cancellation test that wasn't waiting for the upload-code to run
before asserting that a file hadn't been uploaded.

BUG=687993
Review-Url: https://codereview.chromium.org/2682913006
Cr-Commit-Position: refs/heads/master@{#450924}
Committed: https://chromium.googlesource.com/chromium/src/+/0699e0240d57d5b557b59c5d4060cf86d2b46f17

[gsennton, 2017-02-09 15:02:53]

gsennton@chromium.org changed reviewers:
+ isherman@chromium.org, tobiasjs@chromium.org, torne@chromium.org

[gsennton, 2017-02-09 15:02:53]

isherman@chromium.org: ptal at minidump_uploader/

Adding both Torne and Toby since we have all been discussing this change.
Toby, ptal in android_webview/

[Tobias Sargeant, 2017-02-09 17:29:24]

On 2017/02/09 15:02:53, gsennton wrote:
> mailto:isherman@chromium.org: ptal at minidump_uploader/
> 
> Adding both Torne and Toby since we have all been discussing this change.
> Toby, ptal in android_webview/

android_webview/ LGTM

[Torne, 2017-02-09 17:36:16]

Yeah, the general approach here LGTM.

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

Oof, the test file is getting pretty hairy.  I don't have any concrete
suggestions on how to simplify it; but it might be worth investing some effort
to do so.

https://codereview.chromium.org/2682913006/diff/1/android_webview/java/src/or...
File
android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java
(right):

https://codereview.chromium.org/2682913006/diff/1/android_webview/java/src/or...
android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java:78:
scheduleNewJob();
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?

https://codereview.chromium.org/2682913006/diff/1/android_webview/java/src/or...
File
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java
(right):

https://codereview.chromium.org/2682913006/diff/1/android_webview/java/src/or...
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java:206:
// make some progress whenever we start a job.
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?

https://codereview.chromium.org/2682913006/diff/1/android_webview/javatests/s...
File
android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java
(right):

https://codereview.chromium.org/2682913006/diff/1/android_webview/javatests/s...
android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java:170:
throw new RuntimeException(e);
What's the reason to catch the exception just to rethrow it again as a different
type of exception?

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

I completely agree that the test-file is getting hairy.

I think one way of making it more easy to use would be to split it in two parts,
one for WebView-specific testing (i.e. ensuring that WebView's
CrashReportingPermissionManager is implemented correctly), and one part for more
generic testing (i.e. mocking CrashReportingPermissionManager, and ensuring the
copying+uploading-logic works).

We probably want this for when Chrome will be using MinidumpUploaderImpl anyway
(so we can componentize most of the logic - including the non-webview-specific
test parts).

https://codereview.chromium.org/2682913006/diff/1/android_webview/java/src/or...
File
android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java
(right):

https://codereview.chromium.org/2682913006/diff/1/android_webview/java/src/or...
android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java:78:
scheduleNewJob();
On 2017/02/11 01:00:47, Ilya Sherman wrote:
> 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?

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 :)

https://codereview.chromium.org/2682913006/diff/1/android_webview/java/src/or...
File
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java
(right):

https://codereview.chromium.org/2682913006/diff/1/android_webview/java/src/or...
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java:206:
// make some progress whenever we start a job.
On 2017/02/11 01:00:47, Ilya Sherman wrote:
> 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?

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.

https://codereview.chromium.org/2682913006/diff/1/android_webview/javatests/s...
File
android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java
(right):

https://codereview.chromium.org/2682913006/diff/1/android_webview/javatests/s...
android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java:170:
throw new RuntimeException(e);
On 2017/02/11 01:00:47, Ilya Sherman wrote:
> What's the reason to catch the exception just to rethrow it again as a
different
> type of exception?

Just to avoid having to declare the Exception type being thrown for each method
using this method :).

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

https://codereview.chromium.org/2682913006/diff/1/android_webview/java/src/or...
File
android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java
(right):

https://codereview.chromium.org/2682913006/diff/1/android_webview/java/src/or...
android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java:78:
scheduleNewJob();
On 2017/02/13 10:46:55, gsennton wrote:
> On 2017/02/11 01:00:47, Ilya Sherman wrote:
> > 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?
> 
> 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 :)

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.

https://codereview.chromium.org/2682913006/diff/1/android_webview/java/src/or...
File
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java
(right):

https://codereview.chromium.org/2682913006/diff/1/android_webview/java/src/or...
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java:206:
// make some progress whenever we start a job.
On 2017/02/13 10:46:55, gsennton wrote:
> On 2017/02/11 01:00:47, Ilya Sherman wrote:
> > 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?
> 
> 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.

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]

https://codereview.chromium.org/2682913006/diff/1/android_webview/java/src/or...
File
android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java
(right):

https://codereview.chromium.org/2682913006/diff/1/android_webview/java/src/or...
android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java:78:
scheduleNewJob();
On 2017/02/13 22:28:15, Ilya Sherman wrote:
> On 2017/02/13 10:46:55, gsennton wrote:
> > On 2017/02/11 01:00:47, Ilya Sherman wrote:
> > > 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?
> > 
> > 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 :)
> 
> 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.

Please see the comment in MinidumpUploaderImpl for an explanation.

https://codereview.chromium.org/2682913006/diff/1/android_webview/java/src/or...
File
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java
(right):

https://codereview.chromium.org/2682913006/diff/1/android_webview/java/src/or...
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java:206:
// make some progress whenever we start a job.
On 2017/02/13 22:28:15, Ilya Sherman wrote:
> On 2017/02/13 10:46:55, gsennton wrote:
> > On 2017/02/11 01:00:47, Ilya Sherman wrote:
> > > 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?
> > 
> > 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.
> 
> 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?

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]

LGTM -- thanks for bearing with me.  I do think it would be good to provide more
inline documentation for this CL, though.

https://codereview.chromium.org/2682913006/diff/1/android_webview/java/src/or...
File
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java
(right):

https://codereview.chromium.org/2682913006/diff/1/android_webview/java/src/or...
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java:206:
// make some progress whenever we start a job.
On 2017/02/14 16:49:41, gsennton wrote:
> On 2017/02/13 22:28:15, Ilya Sherman wrote:
> > On 2017/02/13 10:46:55, gsennton wrote:
> > > On 2017/02/11 01:00:47, Ilya Sherman wrote:
> > > > 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?
> > > 
> > > 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.
> > 
> > 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?
> 
> 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. 

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]

No worries Ilya, thanks for following up on this. It's always good to
double-check this kind of stuff :).

I added a bug to for the clean-up of MinidumpUploaderTest here:
crbug.com/692448. The current CL doesn't introduce much logic that can be
cleaned up easily IMO, so I'm gonna do some cleaning-up in separate CLs.

https://codereview.chromium.org/2682913006/diff/1/android_webview/java/src/or...
File
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java
(right):

https://codereview.chromium.org/2682913006/diff/1/android_webview/java/src/or...
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java:206:
// make some progress whenever we start a job.
On 2017/02/15 00:21:05, Ilya Sherman wrote:
> On 2017/02/14 16:49:41, gsennton wrote:
> > On 2017/02/13 22:28:15, Ilya Sherman wrote:
> > > On 2017/02/13 10:46:55, gsennton wrote:
> > > > On 2017/02/11 01:00:47, Ilya Sherman wrote:
> > > > > 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?
> > > > 
> > > > 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.
> > > 
> > > 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?
> > 
> > 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. 
> 
> 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.

Done (ptal).
 
> 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?

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.

[Torne, 2017-02-15 11:14:35]

On 2017/02/15 10:26:58, gsennton wrote:
> 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.

Yeah, I think this is the key point: the previous logic has a very, very narrow
window for when it will actually "notice" a cancellation before doing the first
upload, and it's just a race. Removing that unpredictable small window is
unlikely to make any actual difference to the timing of job cancellations.

[Ilya Sherman, 2017-02-16 00:42:11]

LGTM, thanks!

[Ilya Sherman, 2017-02-16 00:42:17]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2017-02-16 00:42:18]

The patchset sent to the CQ was uploaded after l-g-t-m from torne@chromium.org,
tobiasjs@chromium.org
Link to the patchset: https://codereview.chromium.org/2682913006/#ps20001
(title: "Added inline comments explaining the new (non-)cancellation
behaviour.")

[Ilya Sherman, 2017-02-16 00:42:20]

The CQ bit was unchecked by isherman@chromium.org

[Ilya Sherman, 2017-02-16 00:42:27]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2017-02-16 00:43:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 02:47:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 02:47:32]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[gsennton, 2017-02-16 10:16:07]

The CQ bit was checked by gsennton@chromium.org

[commit-bot: I haz the power, 2017-02-16 10:16:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 10:27:01]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487240167527880,
"parent_rev": "39a7d963f56666f34a337c87c4ddc8d432b1c39c", "commit_rev":
"0699e0240d57d5b557b59c5d4060cf86d2b46f17"}

[commit-bot: I haz the power, 2017-02-16 10:27:45]

Description was changed from

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

When we copy a minidump from an app directory to WebView's directory we
schedule a new minidump-uploading job. Before this CL we would only
schedule such a job if no previous job was running - but doing so means
there are cases where the old job might be finishing up, while we
are in the process of determining whether to run the new job. In such
cases we wouldn't schedule a new job for the new minidump - thus
postponing the upload of the new minidump until the next time a crash
occurs and a new job is scheduled.
Synchronizing the job-starting code and the code running in the jobs
themselves would add complexity to the current code - instead we
schedule a new job unconditionally.
Note that a drawback with this new approach is that if we schedule new
jobs too often we might never run our jobs (since scheduling a new job
cancels the old one). Though as long as a job starts it will try to
upload at least one minidump, ensuring we make progress in that case.

Add a test for the cancellation behaviour, and fix a previous
cancellation test that wasn't waiting for the upload-code to run
before asserting that a file hadn't been uploaded.

BUG=687993
==========

to

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

When we copy a minidump from an app directory to WebView's directory we
schedule a new minidump-uploading job. Before this CL we would only
schedule such a job if no previous job was running - but doing so means
there are cases where the old job might be finishing up, while we
are in the process of determining whether to run the new job. In such
cases we wouldn't schedule a new job for the new minidump - thus
postponing the upload of the new minidump until the next time a crash
occurs and a new job is scheduled.
Synchronizing the job-starting code and the code running in the jobs
themselves would add complexity to the current code - instead we
schedule a new job unconditionally.
Note that a drawback with this new approach is that if we schedule new
jobs too often we might never run our jobs (since scheduling a new job
cancels the old one). Though as long as a job starts it will try to
upload at least one minidump, ensuring we make progress in that case.

Add a test for the cancellation behaviour, and fix a previous
cancellation test that wasn't waiting for the upload-code to run
before asserting that a file hadn't been uploaded.

BUG=687993

Review-Url: https://codereview.chromium.org/2682913006
Cr-Commit-Position: refs/heads/master@{#450924}
Committed:
https://chromium.googlesource.com/chromium/src/+/0699e0240d57d5b557b59c5d4060...
==========

[commit-bot: I haz the power, 2017-02-16 10:27:46]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/0699e0240d57d5b557b59c5d4060...