Componentize parts of minidump uploading for use from WebView.

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;
@@ skipping 16 matching lines @@
 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 = "uploads.log";
 
-    private static final Pattern MINIDUMP_FIRST_TRY_PATTERN =
-            Pattern.compile("\\.dmp([0-9]*)$\\z");
+    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 NOT_YET_UPLOADED_MINIDUMP_SUFFIX = ".dmp";
@@ skipping 20 matching lines @@
     // 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 date.
      * @return Comparator for prioritizing the more recently modified file
      */
     @VisibleForTesting
-    protected static final Comparator<File> sFileComparator =  new Comparator<File>() {
+    protected static final Comparator<File> sFileComparator = new Comparator<File>() {
         @Override
         public int compare(File lhs, File rhs) {
             if (lhs.lastModified() == rhs.lastModified()) {
                 return lhs.compareTo(rhs);
             } else if (lhs.lastModified() < rhs.lastModified()) {
                 return 1;
             } else {
                 return -1;
             }
         }
@@ skipping 75 matching lines @@
     }
 
     @VisibleForTesting
     public static int readAttemptNumber(String filename) {
         int tryIndex = filename.lastIndexOf(UPLOAD_ATTEMPT_DELIMITER);
         if (tryIndex >= 0) {
             tryIndex += UPLOAD_ATTEMPT_DELIMITER.length();
             // To avoid out of bound exceptions
             if (tryIndex < filename.length()) {
                 // We don't try more than 3 times.
-                String numTriesString = filename.substring(
-                        tryIndex, tryIndex + 1);
+                String numTriesString = filename.substring(tryIndex, tryIndex + 1);
                 try {
                     return Integer.parseInt(numTriesString);
                 } catch (NumberFormatException ignored) {
                     return 0;
                 }
             }
         }
         return 0;
     }
 
@@ skipping 32 matching lines @@
         boolean renamed = crashDumpFile.renameTo(new File(newName));
         if (!renamed) {
             Log.w(TAG, "Failed to rename " + crashDumpFile);
             if (!crashDumpFile.delete()) {
                 Log.w(TAG, "Failed to delete " + crashDumpFile);
             }
         }
     }
 
     private final File mCacheDir;
+    /**
+     * The number of times we will try to upload a crash.
+     */
+    @VisibleForTesting
+    public static final int MAX_TRIES_ALLOWED = 3;

[Ilya Sherman, 2016/10/07 07:08:39]

This concept seems more appropriate for the minidump upload code.  What's the
reason for moving it to the crash file manager?

Also, if you do move it here, please move it to the top of the file, with the
other constants.

[gsennton, 2016/10/07 09:58:13]

Alright, so I chatted a bit with Toby (tobiasjs@) about this and we think it
makes more sense to not use the MAX_TRIES_ALLOWED in this file at all. The
reason I moved it here because we use it in one place (getMatchingFiles) to
ensure files that have been retried too many times aren't returned in minidump
queries.

We think it makes more sense to - instead of incrementing the tries for a file
that we have tried to upload MAX_TRIES_ALLOWED number of times - rename the file
so that it won't be included in the queries for files to upload. In this way the
CrashFileManager doesn't need to know MAX_TRIES_ALLOWED, and any retry limit can
instead be handled by the classes using CrashFileManager (in a way
CrashFileManager shouldn't depend on the rest of the services in this folder
IMO, so removing the dependency on MinidumpUploadService.MAX_TRIES_ALLOWED is
good for the overall code health here :)).

 
     public CrashFileManager(File cacheDir) {
         if (cacheDir == null) {
             throw new NullPointerException("Specified context cannot be null.");
         } else if (!cacheDir.isDirectory()) {
-            throw new IllegalArgumentException(cacheDir.getAbsolutePath()
-                    + " is not a directory.");
+            throw new IllegalArgumentException(cacheDir.getAbsolutePath() + " is not a directory.");
         }
         mCacheDir = cacheDir;
     }
 
     public File[] getAllMinidumpFiles() {
         return getMatchingFiles(MINIDUMP_PATTERN);
     }
 
     public File[] getAllMinidumpFilesSorted() {
         File[] minidumps = getAllMinidumpFiles();
@@ skipping 56 matching lines @@
         // chrome::DIR_CRASH_DUMPS).
         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 match.find() && tries < MAX_TRIES_ALLOWED;
             }
         });
         return minidumps;
     }
 
     @VisibleForTesting
     File[] getAllUploadedFiles() {
         return getMatchingFiles(UPLOADED_MINIDUMP_PATTERN);
     }
 
@@ skipping 15 matching lines @@
         }
         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());
+                Log.w(TAG, "Unable to delete previous logfile" + f.getAbsolutePath());
             }
         }
         return f;
     }
 
     File getCrashFile(String filename) {
         return new File(getCrashDirectory(), filename);
     }
 
     /**
@@ skipping 24 matching lines @@
     }
 
     File getCrashUploadLogFile() {
         return new File(getCrashDirectory(), CRASH_DUMP_LOGFILE);
     }
 
     private File[] getAllTempFiles() {
         return getMatchingFiles(TMP_PATTERN);
     }
 }