[Android] Do not immediately delete skipped crash dump uploads.

This will give users a chance to decide to manually upload any skipped
crashes, which can be useful when reporting bugs.

This CL pairs nicely with [ https://codereview.chromium.org/2280313002/ ],
which adds some heuristics to clean up old/obsolete crash reports.

BUG=641628
TEST=CrashFileManagerTest.*, MinidumpUploadCallableTest.*
R=gayane@chromium.org

Review-Url: https://codereview.chromium.org/2281373002
Cr-Commit-Position: refs/heads/master@{#415103}

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

@@ -43,6 +43,8 @@ public class CrashFileManager {
 
     private static final String UPLOADED_MINIDUMP_SUFFIX = ".up";
 
+    private static final String UPLOAD_SKIPPED_MINIDUMP_SUFFIX = ".skipped";
+
     private static final String UPLOAD_ATTEMPT_DELIMITER = ".try";
 
     @VisibleForTesting
@@ -126,10 +128,42 @@ public class CrashFileManager {
         return 0;
     }
 
-    public static boolean tryMarkAsUploaded(File mFileToUpload) {
-        return mFileToUpload.renameTo(
-                new File(mFileToUpload.getPath().replaceAll(
-                        "\\.dmp", UPLOADED_MINIDUMP_SUFFIX)));
+    /**
+     * Marks a crash dump file as successfully uploaded, by renaming the file.
+     *
+     * Does not immediately delete the file, for testing reasons. However, if renaming fails,
+     * attempts to delete the file immediately.
+     */
+    public static void markUploadSuccess(File crashDumpFile) {
+        CrashFileManager.renameCrashDumpFollowingUpload(crashDumpFile, UPLOADED_MINIDUMP_SUFFIX);
+    }
+
+    /**
+     * Marks a crash dump file's upload being skipped. An upload might be skipped due to lack of
+     * user consent, or due to this client being excluded from the sample of clients reporting
+     * crashes.
+     *
+     * Renames the file rather than deleting it, so that the user can manually upload the file later
+     * (via chrome://crashes). However, if renaming fails, attempts to delete the file immediately.
+     */
+    public static void markUploadSkipped(File crashDumpFile) {
+        CrashFileManager.renameCrashDumpFollowingUpload(
+                crashDumpFile, UPLOAD_SKIPPED_MINIDUMP_SUFFIX);
+    }
+
+    /**
+     * Renames a crash dump file. However, if renaming fails, attempts to delete the file
+     * immediately.
+     */
+    private static void renameCrashDumpFollowingUpload(File crashDumpFile, String suffix) {
+        boolean renamed = crashDumpFile.renameTo(
+                new File(crashDumpFile.getPath().replaceAll("\\.dmp", suffix)));
+        if (!renamed) {
+            Log.w(TAG, "Failed to rename " + crashDumpFile);
+            if (!crashDumpFile.delete()) {
+                Log.w(TAG, "Failed to delete " + crashDumpFile);
+            }
+        }
     }
 
     private final File mCacheDir;

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

@@ -98,16 +98,16 @@ public class MinidumpUploadCallable implements Callable<Integer> {
             Log.i(TAG, "Minidump upload enabled for tests, skipping other checks.");
         } else {
             if (!mPermManager.isUploadUserPermitted()) {
-                Log.i(TAG, "Minidump upload is not permitted by user. Marking file as uploaded for "
-                        + "cleanup to prevent future uploads.");
-                cleanupMinidumpFile();
+                Log.i(TAG, "Minidump upload is not permitted by user. Marking file as skipped for "
+                                + "cleanup to prevent future uploads.");
+                CrashFileManager.markUploadSkipped(mFileToUpload);
                 return UPLOAD_USER_DISABLED;
             }
 
             if (!mPermManager.isClientInMetricsSample()) {
-                Log.i(TAG, "Minidump upload skipped due to sampling.  Marking file as uploaded for "
+                Log.i(TAG, "Minidump upload skipped due to sampling.  Marking file as skipped for "
                                 + "cleanup to prevent future uploads.");
-                cleanupMinidumpFile();
+                CrashFileManager.markUploadSkipped(mFileToUpload);
                 return UPLOAD_DISABLED_BY_SAMPLING;
             }
 
@@ -189,7 +189,7 @@ public class MinidumpUploadCallable implements Callable<Integer> {
             // TODO(acleung): MinidumpUploadService is in charge of renaming while this class is
             // in charge of deleting. We should move all the file system operations into
             // MinidumpUploadService instead.
-            cleanupMinidumpFile();
+            CrashFileManager.markUploadSuccess(mFileToUpload);
 
             try {
                 appendUploadedEntryToLog(id);
@@ -259,21 +259,6 @@ public class MinidumpUploadCallable implements Callable<Integer> {
         return boundary;
     }
 
-    /**
-     * Mark file we just uploaded for cleanup later.
-     *
-     * We do not immediately delete the file for testing reasons,
-     * but if marking the file fails, we do delete it right away.
-     */
-    private void cleanupMinidumpFile() {
-        if (!CrashFileManager.tryMarkAsUploaded(mFileToUpload)) {
-            Log.w(TAG, "Unable to mark " + mFileToUpload + " as uploaded.");
-            if (!mFileToUpload.delete()) {
-                Log.w(TAG, "Cannot delete " + mFileToUpload);
-            }
-        }
-    }
-
     /**
      * Returns whether the response code indicates a successful HTTP request.
      *

chrome/android/javatests/src/org/chromium/chrome/browser/crash/CrashFileManagerTest.java

@@ -174,4 +174,20 @@ public class CrashFileManagerTest extends CrashTestCase {
         assertEquals("f.tryN.dmp.try1",
                 CrashFileManager.filenameWithIncrementedAttemptNumber("f.tryN.dmp"));
     }
+
+    @SmallTest
+    @Feature({"Android-AppBase"})
+    public void testMarkUploadSuccess() {
+        CrashFileManager.markUploadSuccess(mDmpFile1);
+        assertFalse(mDmpFile1.exists());
+        assertTrue(new File(mCrashDir, "123_abc.up0").exists());
+    }
+
+    @SmallTest
+    @Feature({"Android-AppBase"})
+    public void testMarkUploadSkipped() {
+        CrashFileManager.markUploadSkipped(mDmpFile1);
+        assertFalse(mDmpFile1.exists());
+        assertTrue(new File(mCrashDir, "123_abc.skipped0").exists());
+    }
 }

chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadCallableTest.java

@@ -241,7 +241,11 @@ public class MinidumpUploadCallableTest extends CrashTestCase {
                 new MockMinidumpUploadCallable(httpURLConnectionFactory, testPermManager);
         assertEquals(MinidumpUploadCallable.UPLOAD_USER_DISABLED,
                 minidumpUploadCallable.call().intValue());
-        assertTrue(mExpectedFileAfterUpload.exists());
+
+        File expectedSkippedFileAfterUpload =
+                new File(mCrashDir, mTestUpload.getName().replaceFirst("\\.dmp", ".skipped"));
+        assertTrue(expectedSkippedFileAfterUpload.exists());
+        assertFalse(mExpectedFileAfterUpload.exists());
     }
 
     @SmallTest
@@ -289,7 +293,11 @@ public class MinidumpUploadCallableTest extends CrashTestCase {
                 new MockMinidumpUploadCallable(httpURLConnectionFactory, testPermManager);
         assertEquals(MinidumpUploadCallable.UPLOAD_DISABLED_BY_SAMPLING,
                 minidumpUploadCallable.call().intValue());
-        assertTrue(mExpectedFileAfterUpload.exists());
+
+        File expectedSkippedFileAfterUpload =
+                new File(mCrashDir, mTestUpload.getName().replaceFirst("\\.dmp", ".skipped"));
+        assertTrue(expectedSkippedFileAfterUpload.exists());
+        assertFalse(mExpectedFileAfterUpload.exists());
     }
 
     @SmallTest