[Android] Provide an API for manually triggering a crash report upload.

[Android] Provide an API for manually triggering a crash report upload.

BUG=620762
TEST=CrashFileMangerTest.*:MinidumpUploadServiceTest.*:MinidumpUploadCallableTest.*
R=gayane@chromium.org

Committed: https://crrev.com/8ca4d1a66de6fcfdfe29011b5f9c8d1ff4c191b1
Cr-Commit-Position: refs/heads/master@{#417787}

[Ilya Sherman, 2016-09-02 00:43:15]

Hi Gayane, PTAL.  I realized that it didn't make a lot of sense to try to hook
this up to anything on the C++ side until your CL [
https://codereview.chromium.org/2268783002/ ] lands, so feel free to take a look
into it tomorrow if you get to it before I do =)  (It's the first thing I'm
planning to work on once your CL lands, so feel free to defer to me if you have
other tasks to work on instead.)

[gayane -on leave until 09-2017, 2016-09-02 15:16:15]

Thanks Ilya, 
I think we have a need for a new state because simply resetting the attempt
number will not work. Next time the crash will be tried to be uploaded in
MinidumpUploadCallable.java it will meet the same conditions (e.g. crash
reporting disabled, user sampled out) and would be rejected again.
If we have a new state then in MinidumpUploadCallable.java we could skip the
checks for that crash report.

[Ilya Sherman, 2016-09-02 22:56:31]

Description was changed from

==========
[Android] Provide an API for manually triggering a crash report upload.

BUG=620762
TEST=CrashFileMangerTest.*, MinidumpUploadServiceTest.*
R=gayane@chromium.org
==========

to

==========
[Android] Provide an API for manually triggering a crash report upload.

BUG=620762
TEST=CrashFileMangerTest.*:MinidumpUploadServiceTest.*:MinidumpUploadCallableTest.*
R=gayane@chromium.org
==========

[Ilya Sherman, 2016-09-02 22:56:50]

On 2016/09/02 15:16:15, gayane wrote:
> Thanks Ilya, 
> I think we have a need for a new state because simply resetting the attempt
> number will not work. Next time the crash will be tried to be uploaded in
> MinidumpUploadCallable.java it will meet the same conditions (e.g. crash
> reporting disabled, user sampled out) and would be rejected again.
> If we have a new state then in MinidumpUploadCallable.java we could skip the
> checks for that crash report.

That... is a really good point.  Whoops!  Updated -- PTAL.

[gayane -on leave until 09-2017, 2016-09-06 17:00:01]

Thanks. Looks good. There is just a question about the limited condition which I
will try to clarify and get back to you.

https://codereview.chromium.org/2307713002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java
(right):

https://codereview.chromium.org/2307713002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:131:
* Attempts to rename the given file to clear any state from previous upload
attempts,
The comment needs an update

https://codereview.chromium.org/2307713002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java
(right):

https://codereview.chromium.org/2307713002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java:117:
if (isLimited || !mPermManager.isUploadPermitted()) {
I am not sure whether we need to respect this condition or not for manual
uploads. The most common case of this condition is when user is on cellular
connection where we limit crash report uploads

[Ilya Sherman, 2016-09-07 00:54:01]

Thanks, Gayane =)

https://codereview.chromium.org/2307713002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java
(right):

https://codereview.chromium.org/2307713002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:131:
* Attempts to rename the given file to clear any state from previous upload
attempts,
On 2016/09/06 17:00:00, gayane wrote:
> The comment needs an update

Done.

https://codereview.chromium.org/2307713002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java
(right):

https://codereview.chromium.org/2307713002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java:117:
if (isLimited || !mPermManager.isUploadPermitted()) {
On 2016/09/06 17:00:00, gayane wrote:
> I am not sure whether we need to respect this condition or not for manual
> uploads. The most common case of this condition is when user is on cellular
> connection where we limit crash report uploads

Okay, I see.  Thanks for noting that.  It seemed like everyone on the email
conversation agreed that it would be appropriate to upload in this case, so I've
updated the code accordingly.

FWIW, the naming of these functions is *really* quite confusing -- there are a
lot of very similarly named functions, and the subtle differences among them are
not entirely clear.  I'm hoping things will become much clearer after your
cleanup of the preference-handling code.

[gayane -on leave until 09-2017, 2016-09-07 19:13:20]

Thanks. A few more minor comments.

https://codereview.chromium.org/2307713002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java
(right):

https://codereview.chromium.org/2307713002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:132:
* users to manual initiate previously skipped uploads.
to manual -> to manually

https://codereview.chromium.org/2307713002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:154:
* @return The filename to rename to so as to clear any upload attempt history.
also adds the forced state.

https://codereview.chromium.org/2307713002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:360:
* Returns the minidump file with the given local ID, or null if no previously
minidump file has
... if no previously minidump file has ...

this seems broken. Do you mean "... if no previously saved (or recorded, etc)
but not uploaded minidump file has ..."

https://codereview.chromium.org/2307713002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadCallableTest.java
(right):

https://codereview.chromium.org/2307713002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadCallableTest.java:180:
mTestUpload = renamed;
why is this line needed?

https://codereview.chromium.org/2307713002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadCallableTest.java:229:
mIsPermitted = true;
why is this change? it shouldn't change the behavior as mIsUserPermitted is
checked before mIsPermitted, but still logically it cannot be true is
mIsUserPermitted is false.

https://codereview.chromium.org/2307713002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadCallableTest.java:433:
setForcedUpload();
this test should be successful now, right?

[gayane -on leave until 09-2017, 2016-09-07 20:23:33]

https://codereview.chromium.org/2307713002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadCallableTest.java
(right):

https://codereview.chromium.org/2307713002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadCallableTest.java:433:
setForcedUpload();
On 2016/09/07 19:13:20, gayane wrote:
> this test should be successful now, right?

Ah. This is confusing. I noticed after that commandline condition is checked
separately even though it is also included in the isPermitted. So disregard this
comment.

[Ilya Sherman, 2016-09-08 00:07:57]

Thanks, Gayane =)

https://codereview.chromium.org/2307713002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java
(right):

https://codereview.chromium.org/2307713002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:132:
* users to manual initiate previously skipped uploads.
On 2016/09/07 19:13:20, gayane wrote:
> to manual -> to manually

Done.

https://codereview.chromium.org/2307713002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:154:
* @return The filename to rename to so as to clear any upload attempt history.
On 2016/09/07 19:13:20, gayane wrote:
> also adds the forced state.

Done.

https://codereview.chromium.org/2307713002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:360:
* Returns the minidump file with the given local ID, or null if no previously
minidump file has
On 2016/09/07 19:13:20, gayane wrote:
> ... if no previously minidump file has ...
> 
> this seems broken. Do you mean "... if no previously saved (or recorded, etc)
> but not uploaded minidump file has ..."

Whoops, definitely missed a word in that sentence!  Rephrased; hopefully clearer
now.

https://codereview.chromium.org/2307713002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadCallableTest.java
(right):

https://codereview.chromium.org/2307713002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadCallableTest.java:180:
mTestUpload = renamed;
On 2016/09/07 19:13:20, gayane wrote:
> why is this line needed?

Added a comment -- hopefully clearer that way?

https://codereview.chromium.org/2307713002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadCallableTest.java:229:
mIsPermitted = true;
On 2016/09/07 19:13:20, gayane wrote:
> why is this change? it shouldn't change the behavior as mIsUserPermitted is
> checked before mIsPermitted, but still logically it cannot be true is
> mIsUserPermitted is false.

You're right, reverted.

[Ilya Sherman, 2016-09-08 00:08:29]

isherman@chromium.org changed reviewers:
+ mariakhomenko@chromium.org

[Ilya Sherman, 2016-09-08 00:08:30]

+Maria for OWNERS review -- thanks!

[gayane -on leave until 09-2017, 2016-09-08 15:50:57]

LGTM

https://codereview.chromium.org/2307713002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadCallableTest.java
(right):

https://codereview.chromium.org/2307713002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadCallableTest.java:180:
mTestUpload = renamed;
On 2016/09/08 00:07:57, Ilya Sherman (Away Sep 9-20) wrote:
> On 2016/09/07 19:13:20, gayane wrote:
> > why is this line needed?
> 
> Added a comment -- hopefully clearer that way?

thanks, it is confusing I was actually thinking that renameTo will take care of
that

[Maria, 2016-09-09 01:05:45]

lgtm

https://codereview.chromium.org/2307713002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java
(right):

https://codereview.chromium.org/2307713002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java:100:
if (!CrashFileManager.isForcedUpload(mFileToUpload)) {
Make this an else if()

[Ilya Sherman, 2016-09-09 19:37:44]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2016-09-09 19:37:44]

The patchset sent to the CQ was uploaded after l-g-t-m from gayane@chromium.org,
mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/2307713002/#ps100001
(title: "collapse else if")

[Ilya Sherman, 2016-09-09 19:37:57]

https://codereview.chromium.org/2307713002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java
(right):

https://codereview.chromium.org/2307713002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java:100:
if (!CrashFileManager.isForcedUpload(mFileToUpload)) {
On 2016/09/09 01:05:44, Maria wrote:
> Make this an else if()

Ah, good catch.  Done.

[commit-bot: I haz the power, 2016-09-09 19:38:25]

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

[Ilya Sherman, 2016-09-09 19:42:59]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2016-09-09 19:42:59]

The patchset sent to the CQ was uploaded after l-g-t-m from gayane@chromium.org,
mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/2307713002/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2016-09-09 19:43:42]

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, 2016-09-09 21:17:30]

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

[commit-bot: I haz the power, 2016-09-09 21:17:31]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Ilya Sherman, 2016-09-09 22:32:58]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2016-09-09 22:32:59]

The patchset sent to the CQ was uploaded after l-g-t-m from gayane@chromium.org,
mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/2307713002/#ps140001
(title: "Fix null ptr dereference in log stmt, and fix test")

[commit-bot: I haz the power, 2016-09-09 22:33:25]

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

[Ilya Sherman, 2016-09-09 22:49:34]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2016-09-09 22:49:34]

The patchset sent to the CQ was uploaded after l-g-t-m from gayane@chromium.org,
mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/2307713002/#ps160001
(title: "Annotate tests to suppress warnings")

[commit-bot: I haz the power, 2016-09-09 22:49:46]

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, 2016-09-10 00:05:33]

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

[commit-bot: I haz the power, 2016-09-10 00:05:34]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Ilya Sherman, 2016-09-10 00:18:54]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2016-09-10 00:18:55]

The patchset sent to the CQ was uploaded after l-g-t-m from gayane@chromium.org,
mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/2307713002/#ps180001
(title: "More test suppressions")

[commit-bot: I haz the power, 2016-09-10 00:19:25]

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, 2016-09-10 01:07:01]

Description was changed from

==========
[Android] Provide an API for manually triggering a crash report upload.

BUG=620762
TEST=CrashFileMangerTest.*:MinidumpUploadServiceTest.*:MinidumpUploadCallableTest.*
R=gayane@chromium.org
==========

to

==========
[Android] Provide an API for manually triggering a crash report upload.

BUG=620762
TEST=CrashFileMangerTest.*:MinidumpUploadServiceTest.*:MinidumpUploadCallableTest.*
R=gayane@chromium.org
==========

[commit-bot: I haz the power, 2016-09-10 01:07:02]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-09-10 01:09:16]

Description was changed from

==========
[Android] Provide an API for manually triggering a crash report upload.

BUG=620762
TEST=CrashFileMangerTest.*:MinidumpUploadServiceTest.*:MinidumpUploadCallableTest.*
R=gayane@chromium.org
==========

to

==========
[Android] Provide an API for manually triggering a crash report upload.

BUG=620762
TEST=CrashFileMangerTest.*:MinidumpUploadServiceTest.*:MinidumpUploadCallableTest.*
R=gayane@chromium.org

Committed: https://crrev.com/8ca4d1a66de6fcfdfe29011b5f9c8d1ff4c191b1
Cr-Commit-Position: refs/heads/master@{#417787}
==========

[commit-bot: I haz the power, 2016-09-10 01:09:18]

Patchset 10 (id:??) landed as
https://crrev.com/8ca4d1a66de6fcfdfe29011b5f9c8d1ff4c191b1
Cr-Commit-Position: refs/heads/master@{#417787}