[Android Crash Reporting] Simplify crash report upload code.

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

Index: chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionRunnable.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java b/chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionRunnable.java
similarity index 56%
rename from chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java
rename to chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionRunnable.java
index b5bcc7f6f31955ce16b76b0b81287084948f51ea..a56533a66c213dfbbcbf44f2bfde49449e4e3d4c 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionRunnable.java
@@ -5,7 +5,7 @@
 package org.chromium.chrome.browser.crash;
 
 import android.content.Context;
-import android.content.Intent;
+import android.os.Build;
 import android.util.Patterns;
 
 import org.chromium.base.Log;
@@ -14,37 +14,24 @@ import org.chromium.components.minidump_uploader.CrashFileManager;
 
 import java.io.BufferedReader;
 import java.io.File;
-import java.io.FileInputStream;
-import java.io.FileOutputStream;
-import java.io.FileWriter;
 import java.io.IOException;
-import java.io.InputStream;
 import java.io.InputStreamReader;
-import java.io.OutputStream;
-import java.io.PrintWriter;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collections;
 import java.util.LinkedList;
 import java.util.List;
-import java.util.concurrent.Callable;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
 /**
- * Extracts logcat out of Android devices and elide PII sensitive info from it.
+ * Extracts the recent logcat output from an Android device, elides PII sensitive info from it,
+ * prepends the logcat data to the caller-provided minidump file, and initiates upload for the crash
+ * report.
  *
  * Elided information includes: Emails, IP address, MAC address, URL/domains as well as Javascript
  * console messages.
- *
- * Caller provides a list of minidump files as well as an intent. This Callable will then extract
- * the most recent logcat and save a copy for each minidump.
- *
- * Upon completion, each minidump + logcat pairs will be passed to the MinidumpPreparationService
- * along with the intent provided here.
  */
-public class LogcatExtractionCallable implements Callable<Boolean> {
-
+public class LogcatExtractionRunnable implements Runnable {
     private static final String TAG = "LogcatExtraction";
     private static final long HALF_SECOND = 500;
 
@@ -55,8 +42,7 @@ public class LogcatExtractionCallable implements Callable<Boolean> {
     @VisibleForTesting
     protected static final String URL_ELISION = "HTTP://WEBADDRESS.ELIDED";
 
-    private static final String GOOD_IRI_CHAR =
-            "a-zA-Z0-9\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF";
+    private static final String GOOD_IRI_CHAR = "a-zA-Z0-9\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF";

[gsennton, 2017/03/07 18:32:04]

Not sure if you want to make these format changes in this CL?

[Ilya Sherman, 2017/03/08 02:32:44]

You're right, probably not.  Oh, the joys of clang autoformatting...  I split
off https://codereview.chromium.org/2735133003/ to format ALL the code.

[gsennton, 2017/03/08 15:18:41]

Nice, thanks!

 
     private static final Pattern IP_ADDRESS = Pattern.compile(
             "((25[0-5]|2[0-4][0-9]|[0-1][0-9]{2}|[1-9][0-9]|[1-9])\\.(25[0-5]|2[0-4]"
@@ -65,27 +51,25 @@ public class LogcatExtractionCallable implements Callable<Boolean> {
             + "|[1-9][0-9]|[0-9]))");
 
     private static final String IRI =
-            "[" + GOOD_IRI_CHAR + "]([" + GOOD_IRI_CHAR + "\\-]{0,61}["
-            + GOOD_IRI_CHAR + "]){0,1}";
+            "[" + GOOD_IRI_CHAR + "]([" + GOOD_IRI_CHAR + "\\-]{0,61}[" + GOOD_IRI_CHAR + "]){0,1}";
 
-    private static final String GOOD_GTLD_CHAR =
-            "a-zA-Z\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF";
+    private static final String GOOD_GTLD_CHAR = "a-zA-Z\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF";
     private static final String GTLD = "[" + GOOD_GTLD_CHAR + "]{2,63}";
     private static final String HOST_NAME = "(" + IRI + "\\.)+" + GTLD;
 
     private static final Pattern DOMAIN_NAME =
             Pattern.compile("(" + HOST_NAME + "|" + IP_ADDRESS + ")");
 
-    private static final Pattern WEB_URL = Pattern.compile(
-            "(?:\\b|^)((?:(http|https|Http|Https|rtsp|Rtsp):"
-            + "\\/\\/(?:(?:[a-zA-Z0-9\\$\\-\\_\\.\\+\\!\\*\\'\\(\\)"
-            + "\\,\\;\\?\\&\\=]|(?:\\%[a-fA-F0-9]{2})){1,64}(?:\\:(?:[a-zA-Z0-9\\$\\-\\_"
-            + "\\.\\+\\!\\*\\'\\(\\)\\,\\;\\?\\&\\=]|(?:\\%[a-fA-F0-9]{2})){1,25})?\\@)?)?"
-            + "(?:" + DOMAIN_NAME + ")"
-            + "(?:\\:\\d{1,5})?)"
-            + "(\\/(?:(?:[" + GOOD_IRI_CHAR + "\\;\\/\\?\\:\\@\\&\\=\\#\\~"
-            + "\\-\\.\\+\\!\\*\\'\\(\\)\\,\\_])|(?:\\%[a-fA-F0-9]{2}))*)?"
-            + "(?:\\b|$)");
+    private static final Pattern WEB_URL =
+            Pattern.compile("(?:\\b|^)((?:(http|https|Http|Https|rtsp|Rtsp):"
+                    + "\\/\\/(?:(?:[a-zA-Z0-9\\$\\-\\_\\.\\+\\!\\*\\'\\(\\)"
+                    + "\\,\\;\\?\\&\\=]|(?:\\%[a-fA-F0-9]{2})){1,64}(?:\\:(?:[a-zA-Z0-9\\$\\-\\_"
+                    + "\\.\\+\\!\\*\\'\\(\\)\\,\\;\\?\\&\\=]|(?:\\%[a-fA-F0-9]{2})){1,25})?\\@)?)?"
+                    + "(?:" + DOMAIN_NAME + ")"
+                    + "(?:\\:\\d{1,5})?)"
+                    + "(\\/(?:(?:[" + GOOD_IRI_CHAR + "\\;\\/\\?\\:\\@\\&\\=\\#\\~"
+                    + "\\-\\.\\+\\!\\*\\'\\(\\)\\,\\_])|(?:\\%[a-fA-F0-9]{2}))*)?"
+                    + "(?:\\b|$)");
 
     @VisibleForTesting
     protected static final String BEGIN_MICRODUMP = "-----BEGIN BREAKPAD MICRODUMP-----";
@@ -101,135 +85,72 @@ public class LogcatExtractionCallable implements Callable<Boolean> {
     @VisibleForTesting
     protected static final String MAC_ELISION = "01:23:45:67:89:AB";
 
-    private static final String LOGCAT_EXTENSION = ".logcat";
-
     @VisibleForTesting
-    protected static final String CONSOLE_ELISION =
-            "[ELIDED:CONSOLE(0)] ELIDED CONSOLE MESSAGE";
+    protected static final String CONSOLE_ELISION = "[ELIDED:CONSOLE(0)] ELIDED CONSOLE MESSAGE";
 
     private static final Pattern MAC_ADDRESS =
             Pattern.compile("([0-9a-fA-F]{2}[-:]+){5}[0-9a-fA-F]{2}");
 
-    private static final Pattern CONSOLE_MSG =
-            Pattern.compile("\\[\\w*:CONSOLE.*\\].*");
-
-    private static final Pattern MINIDUMP_EXTENSION = Pattern.compile("\\.dmp");
-
-    private static final String[] CHROME_NAMESPACE = new String[] {
-            "org.chromium.", "com.google."
-    };
-
-    private static final String[] SYSTEM_NAMESPACE = new String[] {
-            "android.accessibilityservice", "android.accounts", "android.animation",
-            "android.annotation", "android.app", "android.appwidget", "android.bluetooth",
-            "android.content", "android.database", "android.databinding", "android.drm",
-            "android.gesture", "android.graphics", "android.hardware",
-            "android.inputmethodservice", "android.location", "android.media", "android.mtp",
-            "android.net", "android.nfc", "android.opengl", "android.os", "android.preference",
-            "android.print", "android.printservice", "android.provider", "android.renderscript",
-            "android.sax", "android.security", "android.service", "android.speech",
-            "android.support", "android.system", "android.telecom", "android.telephony",
-            "android.test", "android.text", "android.transition", "android.util", "android.view",
-            "android.webkit", "android.widget", "com.android.", "dalvik.", "java.", "javax.",
-            "org.apache.", "org.json.", "org.w3c.dom.", "org.xml.", "org.xmlpull."
-    };
+    private static final Pattern CONSOLE_MSG = Pattern.compile("\\[\\w*:CONSOLE.*\\].*");
+
+    private static final String[] CHROME_NAMESPACE = new String[] {"org.chromium.", "com.google."};
+
+    private static final String[] SYSTEM_NAMESPACE = new String[] {"android.accessibilityservice",
+            "android.accounts", "android.animation", "android.annotation", "android.app",
+            "android.appwidget", "android.bluetooth", "android.content", "android.database",
+            "android.databinding", "android.drm", "android.gesture", "android.graphics",
+            "android.hardware", "android.inputmethodservice", "android.location", "android.media",
+            "android.mtp", "android.net", "android.nfc", "android.opengl", "android.os",
+            "android.preference", "android.print", "android.printservice", "android.provider",
+            "android.renderscript", "android.sax", "android.security", "android.service",
+            "android.speech", "android.support", "android.system", "android.telecom",
+            "android.telephony", "android.test", "android.text", "android.transition",
+            "android.util", "android.view", "android.webkit", "android.widget", "com.android.",
+            "dalvik.", "java.", "javax.", "org.apache.", "org.json.", "org.w3c.dom.", "org.xml.",
+            "org.xmlpull."};
 
     private final Context mContext;
-    private final String[] mMinidumpFilenames;
-    private final Intent mRedirectIntent;
+    private final File mMinidumpFile;
 
     /**
-     * @param filenames List of minidump filenames that need logcat to be attached.
-     * @param redirectIntent The intent to be triggered once upon completion.
+     * @param context The application context for accessing the cache directory and firing intents.
+     * @param minidump The minidump file that needs logcat output to be attached.
      */
-    public LogcatExtractionCallable(Context context, String[] filenames, Intent redirectIntent) {
-        if (context == null) {

[gsennton, 2017/03/07 18:32:04]

Is this check unnecessary?

[Ilya Sherman, 2017/03/08 02:32:44]

I think so?  We pass the application context into this method.  Is it accurate
to assume that the application context is always non-null?

[Maria, 2017/03/08 06:18:47]

I don't think it can be null anymore -- I think there are cases where it can be
null within a service, but we are no longer calling this from a service. If
anything, this should have been assert context != null;

[gsennton, 2017/03/08 15:18:41]

The application context should never be null.

-            throw new NullPointerException("Context cannot be null.");
-        }
+    public LogcatExtractionRunnable(Context context, File minidump) {

[Maria, 2017/03/07 06:36:39]

So I know we are getting rid of the services here, but what's the reason we
can't just attach the logcat to all minidumps like we used to do? Prepend all
logcats here and then have the job scheduler take care of uploads?

[Ilya Sherman, 2017/03/08 02:32:44]

I chose to attach the logcat to just the most recent crash for a couple of
reasons: (1) We always grab the most recent logcat output, so it seems most
likely to be relevant to the most recent crash; and (2) honestly, I'd expect it
to almost never happen that we have multiple pending crash reports that need
logcat output attached.  Given that, I didn't want to support the extra
complexity needed to attach logcat output to all the pending minidumps.

That said, if y'all think it's worth a bit of extra complexity, we certainly
could cover all of the minidumps.

(Gustav asked the same thing in a comment on DeferredStartupHandler.java, so I'm
including this reply twice)

[Maria, 2017/03/08 06:18:47]

What do you think about adding a histogram in the calling code to record how
many pending crashes we are uploading? I would be cool with just adding logcat
to one crash right now and checking that histogram. If we see the non-1 case
often enough, we could consider re-attaching logcats to everything later on (I
don't think it's a blocker for M-58 impl). Though I agree, the value of it is
still pretty questionable. It would probably need to be paired with some code to
find the right dump in the logcat and grab logcat preceding it.

[gsennton, 2017/03/08 15:18:41]

Ah, good idea to add a histogram! It would be great to be able to make an
informed decision on this (without blocking this CL for 58).

[Ilya Sherman, 2017/03/09 01:08:45]

Done.  I agree that this is a great idea -- thanks! =)

         mContext = context;
-        mMinidumpFilenames = Arrays.copyOf(filenames, filenames.length);
-        mRedirectIntent = redirectIntent;
+        mMinidumpFile = minidump;
     }
 
     @Override
-    public Boolean call() {
-        Log.i(TAG, "Trying to extract logcat for minidump");
+    public void run() {
+        Log.i(TAG, "Trying to extract logcat for minidump " + mMinidumpFile.getName());
+        CrashFileManager fileManager = new CrashFileManager(mContext.getCacheDir());
+        File fileToUpload = mMinidumpFile;
         try {
-            // Step 1: Extract a single logcat file.
-            File logcatFile = getElidedLogcat();
-
-            // Step 2: Make copies of logcat file for each  minidump then invoke
-            // MinidumpPreparationService on each file pair.
-            int len = mMinidumpFilenames.length;
-            CrashFileManager fileManager = new CrashFileManager(mContext.getCacheDir());
-            for (int i = 0; i < len; i++) {
-                // Output crash dump file path to logcat so non-browser crashes appear too.
-                Log.i(TAG, "Output crash dump:");
-                Log.i(TAG, fileManager.getCrashFile(mMinidumpFilenames[i]).getAbsolutePath());
-                processMinidump(logcatFile, mMinidumpFilenames[i], fileManager, i == len - 1);
-            }
-            return true;
+            List<String> logcat = getElidedLogcat();
+            fileToUpload = new MinidumpLogcatPrepender(fileManager, mMinidumpFile, logcat).run();
         } catch (IOException | InterruptedException e) {
             Log.w(TAG, e.toString());
-            return false;
         }
-    }
 
-    private void processMinidump(File logcatFile, String name,
-            CrashFileManager manager, boolean isLast) throws IOException {
-        String toPath = MINIDUMP_EXTENSION.matcher(name).replaceAll(LOGCAT_EXTENSION);
-        File toFile = manager.createNewTempFile(toPath);
-
-        // For the last file, we don't need to make extra copies. We'll use the original
-        // logcat file but we would pass down the redirect intent so it can be triggered
-        // upon completion.
-        Intent intent = null;
-        if (isLast) {
-            move(logcatFile, toFile);
-            intent = MinidumpPreparationService.createMinidumpPreparationIntent(mContext,
-                    manager.getCrashFile(name), toFile, mRedirectIntent);
-        } else {
-            copy(logcatFile, toFile);
-            intent = MinidumpPreparationService.createMinidumpPreparationIntent(mContext,
-                    manager.getCrashFile(name), toFile, null);
-        }
-        mContext.startService(intent);
-    }
-
-    private File getElidedLogcat() throws IOException, InterruptedException {
-        List<String> rawLogcat = getLogcat();
-        List<String> elidedLogcat =
-                Collections.unmodifiableList(processLogcat(rawLogcat));
-        return writeLogcat(elidedLogcat);
-    }
-
-    private static void copy(File src, File dst) throws IOException {
-        InputStream in = null;
-        OutputStream out = null;
+        // Regardless of success, initiate the upload. That way, even if there are errors augmenting
+        // the minidump with logcat data, the service can still upload the unaugmented minidump.
         try {
-            in = new FileInputStream(src);
-            out = new FileOutputStream(dst);
-            // Transfer bytes from in to out
-            byte[] buf = new byte[1024];
-            int len;
-            while ((len = in.read(buf)) > 0) {
-                out.write(buf, 0, len);
-            }
-        } catch (IOException e) {
-            Log.w(TAG, e.toString());
-        } finally {
-            try {
-                if (in != null) in.close();
-            } finally {
-                if (out != null) out.close();
+            MinidumpUploadService.tryUploadCrashDump(mContext, fileToUpload);
+        } catch (SecurityException e) {
+            // For KitKat and below, there was a framework bug which causes us to not be able to
+            // find our own crash uploading service. Ignore a SecurityException here on older
+            // OS versions since the crash will eventually get uploaded on next start.
+            // crbug/542533
+            if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) {
+                throw e;
             }
         }
     }
 
-    private static void move(File from, File to) {
-        if (!from.renameTo(to)) {
-            Log.w(TAG, "Fail to rename logcat file");
-        }
+    private List<String> getElidedLogcat() throws IOException, InterruptedException {
+        List<String> rawLogcat = getLogcat();
+        return Collections.unmodifiableList(elideLogcat(rawLogcat));
     }
 
     @VisibleForTesting
@@ -306,35 +227,18 @@ public class LogcatExtractionCallable implements Callable<Boolean> {
         return rawLogcat;
     }
 
-    private File writeLogcat(List<String> elidedLogcat) throws IOException {
-        CrashFileManager fileManager = new CrashFileManager(mContext.getCacheDir());
-        File logcatFile = fileManager.createNewTempFile("logcat.txt");
-        PrintWriter pWriter = null;
-        try {
-            pWriter = new PrintWriter(new FileWriter(logcatFile));
-            for (String ln : elidedLogcat) {
-                pWriter.println(ln);
-            }
-            return logcatFile;
-        } finally {
-            if (pWriter != null) {
-                pWriter.close();
-            }
-        }
-    }
-
     @VisibleForTesting
-    protected static List<String> processLogcat(List<String> rawLogcat) {
-        List<String> out = new ArrayList<String>(rawLogcat.size());
+    protected static List<String> elideLogcat(List<String> rawLogcat) {
+        List<String> elided = new ArrayList<String>(rawLogcat.size());
         for (String ln : rawLogcat) {
             ln = elideEmail(ln);
             ln = elideUrl(ln);
             ln = elideIp(ln);
             ln = elideMac(ln);
             ln = elideConsole(ln);
-            out.add(ln);
+            elided.add(ln);
         }
-        return out;
+        return elided;
     }
 
     /**