[Android] Read the *last* N lines of a logcat to associate with a crash.

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

Index: chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.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/LogcatExtractionCallable.java
index 281708669fbd1ac10d7091decfcc20c0b49207d7..220300ecf24921103ddc5da1b5b9a8b7731b9345 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java
@@ -230,23 +230,23 @@ public class LogcatExtractionCallable implements Callable<Boolean> {
 
     @VisibleForTesting
     protected List<String> getLogcat() throws IOException, InterruptedException {
-        return getLogcatInternal();
-    }
+        List<String> rawLogcat = new LinkedList<>();
+        boolean inMicrodump = false;
 
-    private static List<String> getLogcatInternal() throws IOException, InterruptedException {
-        List<String> rawLogcat = null;
         Integer exitValue = null;
-        // In the absence of the android.permission.READ_LOGS permission the
-        // the logcat call will just hang.
+        // In the absence of the android.permission.READ_LOGS permission, the logcat call will just
+        // hang.

[Ilya Sherman, 2016/09/01 02:00:03]

FWIW, I don't understand the relevance of this comment.  I see no code that
handles a hang, nor tests the permission.  Alan, do you know why this is here?

[acleung1, 2016/09/01 17:54:42]

This comment is some what irrelevant. The issue is that before a certain version
of Android, it hangs, there might been a version check around these lines
before.

Someone might have done a code clean up and removed that version check after
Chrome no longer support those versions.

I say we remove this.

[Ilya Sherman, 2016/09/01 21:46:58]

Okay, removed -- thanks.

         Process p = Runtime.getRuntime().exec("logcat -d");
         BufferedReader reader = null;
         try {
             reader = new BufferedReader(new InputStreamReader(p.getInputStream()));
             while (exitValue == null) {
-                rawLogcat = extractLogcatFromReader(reader, LOGCAT_SIZE);
+                inMicrodump =
+                        extractLogcatChunkFromReader(reader, LOGCAT_SIZE, inMicrodump, rawLogcat);

[Ilya Sherman, 2016/09/01 02:00:03]

It feels kind of hacky to read the logcat in chunks.  Would it perhaps make more
sense to first allow the command to finish executing; and then once it's fully
executed, to read the output?  (The only reason I didn't immediately implement
it that way is that I assumed there was probably a good reason to follow the
structure that the code currently follows -- e.g. maybe not wanting to buffer
too much in memory at a time?)

[acleung1, 2016/09/01 17:54:42]

Not that I know of.

[Ilya Sherman, 2016/09/01 21:46:58]

Maria, WDYT?

                 try {
                     exitValue = p.exitValue();
                 } catch (IllegalThreadStateException itse) {
+                    // The logcat dump has not yet completed; give it some more time.
                     Thread.sleep(HALF_SECOND);
                 }
             }
@@ -267,23 +267,20 @@ public class LogcatExtractionCallable implements Callable<Boolean> {
      * Extract microdump-free logcat for more informative crash reports
      *
      * @param reader A buffered reader from which lines of initial logcat is read.
-     * @param maxLines The maximum number of lines logcat extracts from minidump.
+     * @param maxLines The maximum number of lines logcat extracts from logcat.
+     * @param inMicrodump Whether the upcoming lines in the logcat are known to be part of a
+     *         microdump.
+     * @param rawLogcat The tail of the logcat that has been read so far, as a list of strings.
      *
-     * @return Logcat up to specified length as a list of strings.
+     * @return Whether the next lines to be read from the logcat are known to be part of a
+     *         microdump.
      * @throws IOException if the buffered reader encounters an I/O error.
      */
     @VisibleForTesting
-    protected static List<String> extractLogcatFromReader(
-            BufferedReader reader, int maxLines) throws IOException {
-        return extractLogcatFromReaderInternal(reader, maxLines);
-    }
-
-    private static List<String> extractLogcatFromReaderInternal(
-            BufferedReader reader, int maxLines) throws IOException {
-        boolean inMicrodump = false;
-        List<String> rawLogcat = new LinkedList<>();
+    protected static boolean extractLogcatChunkFromReader(BufferedReader reader, int maxLines,
+            boolean inMicrodump, List<String> rawLogcat) throws IOException {
         String logLn;
-        while ((logLn = reader.readLine()) != null && rawLogcat.size() < maxLines) {
+        while ((logLn = reader.readLine()) != null) {
             if (logLn.contains(BEGIN_MICRODUMP)) {
                 // If the log contains two begin markers without an end marker
                 // in between, we ignore the second begin marker.
@@ -299,10 +296,13 @@ public class LogcatExtractionCallable implements Callable<Boolean> {
             } else {
                 if (!inMicrodump) {
                     rawLogcat.add(logLn);
+                    if (rawLogcat.size() > maxLines) {
+                        rawLogcat.remove(0);
+                    }
                 }
             }
         }
-        return rawLogcat;
+        return inMicrodump;
     }
 
     private File writeLogcat(List<String> elidedLogcat) throws IOException {