[Android] Delete old crash reports

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

 // Copyright 2015 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.chrome.browser.crash;
 
 import org.chromium.base.Log;
 import org.chromium.base.VisibleForTesting;
 
 import java.io.File;
 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;
 
 /**
  * Responsible for the Crash Report directory. It routinely scans the directory
  * for new Minidump files and takes appropriate actions by either uploading new
  * crash dumps or deleting old ones.
  */
 public class CrashFileManager {
     private static final String TAG = "CrashFileManager";
 
     @VisibleForTesting
     static final String CRASH_DUMP_DIR = "Crash Reports";
 
     // 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");
 
     private static final Pattern MINIDUMP_MIME_FIRST_TRY_PATTERN =
             Pattern.compile("\\.dmp([0-9]+)$\\z");
 
     private static final Pattern MINIDUMP_PATTERN =
             Pattern.compile("\\.dmp([0-9]*)(\\.try[0-9])?\\z");
 
     private static final Pattern UPLOADED_MINIDUMP_PATTERN = Pattern.compile("\\.up([0-9]*)\\z");
 
     private static final String UPLOADED_MINIDUMP_SUFFIX = ".up";
 
     private static final String UPLOAD_ATTEMPT_DELIMITER = ".try";
 
     @VisibleForTesting
     protected static final String TMP_SUFFIX = ".tmp";
 
     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
      * @return Comparator for prioritizing the more recently modified file
      */
     @VisibleForTesting
     protected static final Comparator<File> sFileComparator =  new Comparator<File>() {
         @Override
         public int compare(File lhs, File rhs) {
             if (lhs == rhs) {
@@ skipping 84 matching lines @@
     public File[] getAllMinidumpFiles() {
         return getMatchingFiles(MINIDUMP_PATTERN);
     }
 
     public File[] getAllMinidumpFilesSorted() {
         File[] minidumps = getAllMinidumpFiles();
         Arrays.sort(minidumps, sFileComparator);
         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);
         }
         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() {
             @Override
             public boolean accept(File dir, String filename) {
                 Matcher match = pattern.matcher(filename);
                 int tries = readAttemptNumber(filename);
                 return match.find() && tries < MinidumpUploadService.MAX_TRIES_ALLOWED;
             }
         });
         return minidumps;
     }
 
     @VisibleForTesting
     File[] getAllUploadedFiles() {
         return getMatchingFiles(UPLOADED_MINIDUMP_PATTERN);
     }
 
     @VisibleForTesting
     File getCrashDirectory() {
         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()) {
             if (f.delete()) {
                 f = new File(getCrashDirectory(), name);
             } else {
                 Log.w(TAG, "Unable to delete previous logfile"
                         + f.getAbsolutePath());
             }
         }
         return f;
     }
 
     File getCrashFile(String filename) {
         return new File(getCrashDirectory(), filename);
     }
 
     File getCrashUploadLogFile() {
-        return new File(mCacheDir, CRASH_DUMP_LOGFILE);
+        return new File(getCrashDirectory(), CRASH_DUMP_LOGFILE);
     }
 
     private File[] getAllTempFiles() {
         return getMatchingFiles(TMP_PATTERN);
     }
 }