[Android] Delete old crash reports

chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java

Index: chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java b/chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java
index a1e5ac05161471449e7d326e31e628bc28a2e7a2..8162f95a57c3cdda5a0a43706b4cd7ce3dcb9f12 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java
@@ -12,6 +12,10 @@ import java.io.FilenameFilter;
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.Comparator;
+import java.util.Date;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
@@ -28,7 +32,7 @@ public class CrashFileManager {
 
     // This should mirror the C++ CrashUploadList::kReporterLogFilename variable.
     @VisibleForTesting
-    static final String CRASH_DUMP_LOGFILE = CRASH_DUMP_DIR + "/uploads.log";
+    static final String CRASH_DUMP_LOGFILE = "uploads.log";
 
     private static final Pattern MINIDUMP_FIRST_TRY_PATTERN =
             Pattern.compile("\\.dmp([0-9]*)$\\z");
@@ -50,6 +54,17 @@ public class CrashFileManager {
 
     private static final Pattern TMP_PATTERN = Pattern.compile("\\.tmp\\z");
 
+    // The maximum number of non-uploaded crashes that may be kept in the crash reports directory.
+    // Chosen to attempt to balance between keeping a generous number of crashes, and not using up
+    // too much filesystem storage space for obsolete crash reports.
+    @VisibleForTesting
+    protected static final int MAX_CRASH_REPORTS_TO_KEEP = 10;
+
+    // The maximum age, in days, considered acceptable for a crash report. Reports older than this
+    // age will be removed. The constant is chosen to be quite conservative, while still allowing
+    // users to eventually reclaim filesystem storage space from obsolete crash reports.
+    private static final int MAX_CRASH_REPORT_AGE_IN_DAYS = 30;
+
     /**
      * Comparator used for sorting files by modification
      * Note that the behavior is undecided if the files are created at the same time
@@ -154,6 +169,18 @@ public class CrashFileManager {
         return minidumps;
     }
 
+    @VisibleForTesting
+    protected File[] getAllFilesSorted() {

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

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 ?

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

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

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

I would imagine smth like getAllAvailableMinidumpSorted but this gets too long

+        File crashDir = getCrashDirectoryIfExists();
+        if (crashDir == null) {
+            return new File[] {};
+        }
+
+        File[] files = crashDir.listFiles();
+        Arrays.sort(files, sFileComparator);
+        return files;
+    }
+
     public void cleanOutAllNonFreshMinidumpFiles() {
         for (File f : getAllUploadedFiles()) {
             deleteFile(f);
@@ -161,20 +188,42 @@ public class CrashFileManager {
         for (File f : getAllTempFiles()) {
             deleteFile(f);
         }
+
+        Set<String> recentCrashes = new HashSet<String>();
+        for (File f : getAllFilesSorted()) {
+            // The uploads.log file should always be preserved, as it stores the metadata that
+            // powers the chrome://crashes UI.
+            if (f.getName().equals(CRASH_DUMP_LOGFILE)) {
+                continue;
+            }
+
+            // Delete the oldest crash reports that exceed the cap on the number of allowed reports.
+            // Each crash typically has two files associated with it: a .dmp file and a .logcat
+            // file. These have the same filename other than the file extension.
+            String fileNameSansExtension = f.getName().split("\\.")[0];

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

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?

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

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?

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

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

+            if (recentCrashes.size() < MAX_CRASH_REPORTS_TO_KEEP) {
+                recentCrashes.add(fileNameSansExtension);
+            } else if (!recentCrashes.contains(fileNameSansExtension)) {
+                deleteFile(f);
+                continue;
+            }
+
+            // Delete any crash reports that are especially old.
+            long ageInMillis = new Date().getTime() - f.lastModified();
+            long ageInDays = TimeUnit.DAYS.convert(ageInMillis, TimeUnit.MILLISECONDS);
+            if (ageInDays > MAX_CRASH_REPORT_AGE_IN_DAYS) {

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

Shouldn't this be before we decide to add the report to recentCrashes ?

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

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

+                deleteFile(f);
+            }
+        }
     }
 
     @VisibleForTesting
     File[] getMatchingFiles(final Pattern pattern) {
-        // Get dump dir and get all files with specified suffix.. The path
+        // Get dump dir and get all files with specified suffix. The path
         // constructed here must match chrome_paths.cc (see case
         // chrome::DIR_CRASH_DUMPS).
-        File crashDir = getCrashDirectory();
-        if (!crashDir.exists()) {
-            Log.w(TAG, crashDir.getAbsolutePath() + " does not exist!");
-            return new File[] {};
-        }
-        if (!crashDir.isDirectory()) {
-            Log.w(TAG, crashDir.getAbsolutePath() + " is not a directory!");
+        File crashDir = getCrashDirectoryIfExists();
+        if (crashDir == null) {
             return new File[] {};
         }
         File[] minidumps = crashDir.listFiles(new FilenameFilter() {
@@ -198,6 +247,20 @@ public class CrashFileManager {
         return new File(mCacheDir, CRASH_DUMP_DIR);
     }
 
+    @VisibleForTesting
+    File getCrashDirectoryIfExists() {
+        File crashDirectory = getCrashDirectory();
+        if (!crashDirectory.exists()) {
+            Log.w(TAG, crashDirectory.getAbsolutePath() + " does not exist!");
+            return null;
+        }
+        if (!crashDirectory.isDirectory()) {
+            Log.w(TAG, crashDirectory.getAbsolutePath() + " is not a directory!");
+            return null;
+        }
+        return crashDirectory;
+    }
+
     public File createNewTempFile(String name) throws IOException {
         File f = new File(getCrashDirectory(), name);
         if (f.exists()) {
@@ -216,7 +279,7 @@ public class CrashFileManager {
     }
 
     File getCrashUploadLogFile() {
-        return new File(mCacheDir, CRASH_DUMP_LOGFILE);
+        return new File(getCrashDirectory(), CRASH_DUMP_LOGFILE);
     }
 
     private File[] getAllTempFiles() {