[Android] Delete old crash reports

[Android] Delete old crash reports

Unsent crash reports can currently accumulate indefinitely, costing
users storage space.  This CL limits (1) the maximum number of unsent
reports that will be saved, and (2) the maximum age of a report.

BUG=641628
TEST=CrashFileManagerTest.*

Committed: https://crrev.com/f31ae5c0f5cc1f2337e16c68fdc3048336eff666
Cr-Commit-Position: refs/heads/master@{#415838}

[Ilya Sherman, 2016-08-27 00:26:45]

The CQ bit was checked by isherman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-27 00:27:17]

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

[Ilya Sherman, 2016-08-27 00:29:56]

isherman@chromium.org changed reviewers:
+ dfalcantara@chromium.org, gayane@chromium.org

[Ilya Sherman, 2016-08-27 00:29:56]

Gayane, PTAL from the metrics/crash reporting team perspective.  (Jesse, I
listed you as FYI, but feel free to review as well if you'd like to.)

Dan, PTAL from an OWNERS perspective.  I rarely code in Java, so please don't
hesitate to nitpick =)  (The nearest owners for this file that I found were from
//chrome/android/OWNERS.  Is that normal?  Not that I mind -- just don't want to
overload y'all with code reviews.)

[commit-bot: I haz the power, 2016-08-27 01:40:12]

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

[commit-bot: I haz the power, 2016-08-27 01:40:13]

Dry run: 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-08-27 05:55:22]

The CQ bit was checked by isherman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-27 05:55:45]

Dry run: 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-08-27 07:12:11]

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

[commit-bot: I haz the power, 2016-08-27 07:12:12]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[gayane -on leave until 09-2017, 2016-08-29 18:56:47]

Thanks Ilya.

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

https://codereview.chromium.org/2280313002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:173:
protected File[] getAllFilesSorted() {
Should we make getAllFilesSorted function to return all the minidumps and rename
getAllMinidumpFilesSorted function to return all the minidump files that are
available for upload ?

https://codereview.chromium.org/2280313002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:203:
String fileNameSansExtension = f.getName().split("\\.")[0];
According to the filenameWithIncrementedAttemptNumber I can see that it build
the suffix as .dmp.try1 in which case this code here will work as expected.
but for some reason in tests minidumps can have .dmp.try1 or .try1.dmp
suffixies. I am not sure this is needed, but is this considered here? would it
match to logcat filename in both cases?

https://codereview.chromium.org/2280313002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:214:
if (ageInDays > MAX_CRASH_REPORT_AGE_IN_DAYS) {
Shouldn't this be before we decide to add the report to recentCrashes ?

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

https://codereview.chromium.org/2280313002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadCallableTest.java:381:
File logfile = new File(mCacheDir, CrashFileManager.CRASH_DUMP_LOGFILE);
mCrashDir

[Ilya Sherman, 2016-08-29 19:14:19]

Description was changed from

==========
[Android] Delete old crash reports

Unsent crash reports can currently accumulate indefinitely, costing
users storage space.  This CL limits (1) the maximum number of unsent
reports that will be saved, and (2) the maximum age of a report.

BUG=641628
TEST=CrashFileManagerTest.*
==========

to

==========
[Android] Delete old crash reports

Unsent crash reports can currently accumulate indefinitely, costing
users storage space.  This CL limits (1) the maximum number of unsent
reports that will be saved, and (2) the maximum age of a report.

BUG=641628
TEST=CrashFileManagerTest.*
==========

[Ilya Sherman, 2016-08-29 22:33:01]

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

https://codereview.chromium.org/2280313002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:173:
protected File[] getAllFilesSorted() {
On 2016/08/29 18:56:47, gayane wrote:
> Should we make getAllFilesSorted function to return all the minidumps and
rename
> getAllMinidumpFilesSorted function to return all the minidump files that are
> available for upload ?

The intention is for getAllFilesSorted() to return *all* files -- originally
named minidump files, renamed minidump files, logcat files, temporary files, and
anything else that might have somehow snuck into this directory.

I'm happy to clear up other function names, though would probably prefer to do
so in a separate CL.  What name would you suggest to replace
getAllMinidumpFilesSorted()?  It's a bit weird currently because "foo.dmp" and
"foo.up0" are both minidump files in terms of their contents, but only "foo.dmp"
is considered a "minidump file" by the method, because the method is really
looking at filename extensions, rather than contents.  But, that's kind of how
most of this class works :/

https://codereview.chromium.org/2280313002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:203:
String fileNameSansExtension = f.getName().split("\\.")[0];
On 2016/08/29 18:56:47, gayane wrote:
> According to the filenameWithIncrementedAttemptNumber I can see that it build
> the suffix as .dmp.try1 in which case this code here will work as expected.
> but for some reason in tests minidumps can have .dmp.try1 or .try1.dmp
> suffixies. I am not sure this is needed, but is this considered here? would it
> match to logcat filename in both cases?

Yeah, I'm not sure why tests allow ".try1.dmp" as a suffix.  But, I believe this
code is robust to that -- "foo.try1.dmp" and "foo.dmp.try1" will both have the
base name "foo", and the corresponding logcat file should be "foo.logcat".  I
haven't seen any code that might change the "foo" part of the name.

Is there any particular input case that you're concerned about, that I might be
overlooking?

https://codereview.chromium.org/2280313002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:214:
if (ageInDays > MAX_CRASH_REPORT_AGE_IN_DAYS) {
On 2016/08/29 18:56:47, gayane wrote:
> Shouldn't this be before we decide to add the report to recentCrashes ?

The behavior should be equivalent, but I agree it might be clearer as you
suggest.  Rearranged.

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

https://codereview.chromium.org/2280313002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadCallableTest.java:381:
File logfile = new File(mCacheDir, CrashFileManager.CRASH_DUMP_LOGFILE);
On 2016/08/29 18:56:47, gayane wrote:
> mCrashDir

Done.

[Ilya Sherman, 2016-08-29 22:33:40]

The CQ bit was checked by isherman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 23:25:00]

Dry run: 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-08-30 03:35:46]

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

[commit-bot: I haz the power, 2016-08-30 03:35:47]

Dry run: This issue passed the CQ dry run.

[gayane -on leave until 09-2017, 2016-08-30 15:37:12]

lgtm

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

https://codereview.chromium.org/2280313002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:173:
protected File[] getAllFilesSorted() {
On 2016/08/29 22:33:01, Ilya Sherman wrote:
> On 2016/08/29 18:56:47, gayane wrote:
> > Should we make getAllFilesSorted function to return all the minidumps and
> rename
> > getAllMinidumpFilesSorted function to return all the minidump files that are
> > available for upload ?
> 
> The intention is for getAllFilesSorted() to return *all* files -- originally
> named minidump files, renamed minidump files, logcat files, temporary files,
and
> anything else that might have somehow snuck into this directory.
> 
> I'm happy to clear up other function names, though would probably prefer to do
> so in a separate CL.  What name would you suggest to replace
> getAllMinidumpFilesSorted()?  It's a bit weird currently because "foo.dmp" and
> "foo.up0" are both minidump files in terms of their contents, but only
"foo.dmp"
> is considered a "minidump file" by the method, because the method is really
> looking at filename extensions, rather than contents.  But, that's kind of how
> most of this class works :/

I would imagine smth like getAllAvailableMinidumpSorted but this gets too long

https://codereview.chromium.org/2280313002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:203:
String fileNameSansExtension = f.getName().split("\\.")[0];
On 2016/08/29 22:33:01, Ilya Sherman wrote:
> On 2016/08/29 18:56:47, gayane wrote:
> > According to the filenameWithIncrementedAttemptNumber I can see that it
build
> > the suffix as .dmp.try1 in which case this code here will work as expected.
> > but for some reason in tests minidumps can have .dmp.try1 or .try1.dmp
> > suffixies. I am not sure this is needed, but is this considered here? would
it
> > match to logcat filename in both cases?
> 
> Yeah, I'm not sure why tests allow ".try1.dmp" as a suffix.  But, I believe
this
> code is robust to that -- "foo.try1.dmp" and "foo.dmp.try1" will both have the
> base name "foo", and the corresponding logcat file should be "foo.logcat".  I
> haven't seen any code that might change the "foo" part of the name.
> 
> Is there any particular input case that you're concerned about, that I might
be
> overlooking?

no, particular input, just misunderstood the split part. Didn't think it picks
"foo" as in your example.

[Ilya Sherman, 2016-08-30 15:55:47]

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

[Ilya Sherman, 2016-08-30 15:55:47]

Dan or Maria, could one of you PTAL for OWNERS approval?  Thanks!

[gone, 2016-08-30 16:57:17]

lgtm

Sorry about the delay; did 8 or 9 yesterday and my brain got a bit fried.

[Maria, 2016-08-30 16:58:41]

lgtm

https://codereview.chromium.org/2280313002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/crash/CrashFileManagerTest.java
(right):

https://codereview.chromium.org/2280313002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/crash/CrashFileManagerTest.java:203:
String prefix = "chromium-renderer-minidump-deadbeef" + Integer.toString(i);
nit: any reason not to just do "chromium..." + i; here?

https://codereview.chromium.org/2280313002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/crash/CrashFileManagerTest.java:252:
// are deleted, which would require setting fake timestamps.
You can set last modified date through File api:
https://docs.oracle.com/javase/7/docs/api/java/io/File.html#setLastModified(l....

[Ilya Sherman, 2016-08-31 04:59:59]

https://codereview.chromium.org/2280313002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/crash/CrashFileManagerTest.java
(right):

https://codereview.chromium.org/2280313002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/crash/CrashFileManagerTest.java:203:
String prefix = "chromium-renderer-minidump-deadbeef" + Integer.toString(i);
On 2016/08/30 16:58:40, Maria wrote:
> nit: any reason not to just do "chromium..." + i; here?

Done.  I had assumed it was poor style or something :P

https://codereview.chromium.org/2280313002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/crash/CrashFileManagerTest.java:252:
// are deleted, which would require setting fake timestamps.
On 2016/08/30 16:58:40, Maria wrote:
> You can set last modified date through File api:
>
https://docs.oracle.com/javase/7/docs/api/java/io/File.html#setLastModified(l....

Ah, nice -- thanks!  Done.

[Ilya Sherman, 2016-08-31 05:01:26]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2016-08-31 05:01:27]

The patchset sent to the CQ was uploaded after l-g-t-m from gayane@chromium.org,
dfalcantara@chromium.org, mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/2280313002/#ps60001
(title: "Test cleanup of old files as well")

[commit-bot: I haz the power, 2016-08-31 05:02:04]

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-08-31 05:04:53]

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

[commit-bot: I haz the power, 2016-08-31 05:04:54]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Ilya Sherman, 2016-08-31 21:42:59]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2016-08-31 21:42:59]

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

[commit-bot: I haz the power, 2016-08-31 21:43:53]

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-08-31 23:04:21]

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

[commit-bot: I haz the power, 2016-08-31 23:04:22]

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-01 00:54:34]

The CQ bit was checked by isherman@chromium.org

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

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

[commit-bot: I haz the power, 2016-09-01 00:54:59]

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-01 01:36:41]

Description was changed from

==========
[Android] Delete old crash reports

Unsent crash reports can currently accumulate indefinitely, costing
users storage space.  This CL limits (1) the maximum number of unsent
reports that will be saved, and (2) the maximum age of a report.

BUG=641628
TEST=CrashFileManagerTest.*
==========

to

==========
[Android] Delete old crash reports

Unsent crash reports can currently accumulate indefinitely, costing
users storage space.  This CL limits (1) the maximum number of unsent
reports that will be saved, and (2) the maximum age of a report.

BUG=641628
TEST=CrashFileManagerTest.*
==========

[commit-bot: I haz the power, 2016-09-01 01:36:42]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-01 01:41:50]

Description was changed from

==========
[Android] Delete old crash reports

Unsent crash reports can currently accumulate indefinitely, costing
users storage space.  This CL limits (1) the maximum number of unsent
reports that will be saved, and (2) the maximum age of a report.

BUG=641628
TEST=CrashFileManagerTest.*
==========

to

==========
[Android] Delete old crash reports

Unsent crash reports can currently accumulate indefinitely, costing
users storage space.  This CL limits (1) the maximum number of unsent
reports that will be saved, and (2) the maximum age of a report.

BUG=641628
TEST=CrashFileManagerTest.*

Committed: https://crrev.com/f31ae5c0f5cc1f2337e16c68fdc3048336eff666
Cr-Commit-Position: refs/heads/master@{#415838}
==========

[commit-bot: I haz the power, 2016-09-01 01:41:51]

Patchset 6 (id:??) landed as
https://crrev.com/f31ae5c0f5cc1f2337e16c68fdc3048336eff666
Cr-Commit-Position: refs/heads/master@{#415838}