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

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

More precisely, reads the last N lines preceding the microdump, if any,
since any lines that follow the microdump are most likely not relevant to
the crash.

BUG=641655
R=gayane@chromium.org

Committed: https://crrev.com/4a1d16c19d99d81f644c90365751fe9408963f36
Cr-Commit-Position: refs/heads/master@{#421117}

[Ilya Sherman, 2016-08-27 06:41:02]

isherman@chromium.org changed reviewers:
+ yfriedman@chromium.org

[Ilya Sherman, 2016-08-27 06:41:03]

Gayane and Yaron, as I mentioned in more detail on the bug, I'm not sure whether
this is really the best approach.  Do either of you have suggestions?

[gayane -on leave until 09-2017, 2016-08-29 19:09:18]

Thanks Ilya for considering this problem.

https://codereview.chromium.org/2282173003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java
(right):

https://codereview.chromium.org/2282173003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java:255:

If I understand correctly with this logic the rawLogcat will contain the first
LOGCAT_SIZE lines of output from the time we start reading, but not necessarily
the last lines of output before the process will finish. I am not sure whether
this is desirable outcome or not.

[Ilya Sherman, 2016-08-29 19:19:48]

https://codereview.chromium.org/2282173003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java
(right):

https://codereview.chromium.org/2282173003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java:255:

On 2016/08/29 19:09:18, gayane wrote:
> If I understand correctly with this logic the rawLogcat will contain the first
> LOGCAT_SIZE lines of output from the time we start reading, but not
necessarily
> the last lines of output before the process will finish. I am not sure whether
> this is desirable outcome or not.

Hmm, good point!  I guess probably the last LOGCAT_SIZE lines are more useful. 
Do you know how the logcat data is ultimately consumed?  I think both of the
people who wrote most of the crash reporting code for Android are no longer on
the Chrome team, though I could probably still pick their brains if there's no
current owners.  (Are you the current owner?  I'm actually not sure how much
you've worked with this code and its consumers.)

[gayane -on leave until 09-2017, 2016-08-29 19:24:08]

On 2016/08/29 19:19:48, Ilya Sherman wrote:
>
https://codereview.chromium.org/2282173003/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java
> (right):
> 
>
https://codereview.chromium.org/2282173003/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java:255:
> 
> On 2016/08/29 19:09:18, gayane wrote:
> > If I understand correctly with this logic the rawLogcat will contain the
first
> > LOGCAT_SIZE lines of output from the time we start reading, but not
> necessarily
> > the last lines of output before the process will finish. I am not sure
whether
> > this is desirable outcome or not.
> 
> Hmm, good point!  I guess probably the last LOGCAT_SIZE lines are more useful.

> Do you know how the logcat data is ultimately consumed?  I think both of the
> people who wrote most of the crash reporting code for Android are no longer on
> the Chrome team, though I could probably still pick their brains if there's no
> current owners.  (Are you the current owner?  I'm actually not sure how much
> you've worked with this code and its consumers.)

I am not the owner and actually not that knowledgeable about the logcat info :/

[acleung1, 2016-08-31 18:06:07]

acleung@chromium.org changed reviewers:
+ acleung@chromium.org

[acleung1, 2016-08-31 18:06:07]

https://codereview.chromium.org/2282173003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java
(right):

https://codereview.chromium.org/2282173003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java:255:

On 2016/08/29 19:19:47, Ilya Sherman wrote:
> On 2016/08/29 19:09:18, gayane wrote:
> > If I understand correctly with this logic the rawLogcat will contain the
first
> > LOGCAT_SIZE lines of output from the time we start reading, but not
> necessarily
> > the last lines of output before the process will finish. I am not sure
whether
> > this is desirable outcome or not.
> 
> Hmm, good point!  I guess probably the last LOGCAT_SIZE lines are more useful.

> Do you know how the logcat data is ultimately consumed?  I think both of the
> people who wrote most of the crash reporting code for Android are no longer on
> the Chrome team, though I could probably still pick their brains if there's no
> current owners.  (Are you the current owner?  I'm actually not sure how much
> you've worked with this code and its consumers.)

Yes, gayane's point is important. As discussed on chat, we'd like to lines
closer to the crash event.

Otherwise, the doc I pointed to discussed the workflow of logcat extraction.
Please let me know if you need any clarifications.

[Ilya Sherman, 2016-09-01 01:58:48]

Description was changed from

==========
[Android] More robustly read the logcat file associated with a crash.

BUG=641655
R=gayane@chromium.org
==========

to

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

BUG=641655
R=gayane@chromium.org
==========

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

Okay, PTAL.  Please note that reading the last N lines actually introduces a
behavioral change, as covered by the tests -- specifically, please take a look
at testLogcatWithoutBeginOrEnd_largeLogcat.  Does that change look correct? 
(I'm surprised that there's explicit test coverage for behavior that seems
wrong...)

https://codereview.chromium.org/2282173003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java
(right):

https://codereview.chromium.org/2282173003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java:238:
// hang.
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?

https://codereview.chromium.org/2282173003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java:245:
extractLogcatChunkFromReader(reader, LOGCAT_SIZE, inMicrodump, rawLogcat);
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]

Thanks, Ilya.

I agree with the unit test changes as well as the overall CL but I'd be more
comfortable if someone else from the active Chrome team LGTM it.

https://codereview.chromium.org/2282173003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java
(right):

https://codereview.chromium.org/2282173003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java:238:
// hang.
On 2016/09/01 02:00:03, Ilya Sherman wrote:
> 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?

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.

https://codereview.chromium.org/2282173003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java:245:
extractLogcatChunkFromReader(reader, LOGCAT_SIZE, inMicrodump, rawLogcat);
On 2016/09/01 02:00:03, Ilya Sherman wrote:
> 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?)

Not that I know of.

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

isherman@chromium.org changed reviewers:
+ mariakhomenko@chromium.org

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

Gayane, please take another look =)

Maria, could you PTAL for OWNERS approval, and also let me know your thoughts on
whether it makes sense to read the input stream in chunks?  (Please see the
discussion inline on Patch Set 2.)

https://codereview.chromium.org/2282173003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java
(right):

https://codereview.chromium.org/2282173003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java:238:
// hang.
On 2016/09/01 17:54:42, acleung1 wrote:
> On 2016/09/01 02:00:03, Ilya Sherman wrote:
> > 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?
> 
> 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.

Okay, removed -- thanks.

https://codereview.chromium.org/2282173003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java:245:
extractLogcatChunkFromReader(reader, LOGCAT_SIZE, inMicrodump, rawLogcat);
On 2016/09/01 17:54:42, acleung1 wrote:
> On 2016/09/01 02:00:03, Ilya Sherman wrote:
> > 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?)
> 
> Not that I know of.

Maria, WDYT?

[gayane -on leave until 09-2017, 2016-09-01 22:30:12]

lgtm

[Maria, 2016-09-02 18:04:09]

On 2016/09/01 21:46:58, Ilya Sherman wrote:
> Gayane, please take another look =)
> 
> Maria, could you PTAL for OWNERS approval, and also let me know your thoughts
on
> whether it makes sense to read the input stream in chunks?  (Please see the
> discussion inline on Patch Set 2.)
> 
>
https://codereview.chromium.org/2282173003/diff/20001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java
> (right):
> 
>
https://codereview.chromium.org/2282173003/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java:238:
> // hang.
> On 2016/09/01 17:54:42, acleung1 wrote:
> > On 2016/09/01 02:00:03, Ilya Sherman wrote:
> > > 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?
> > 
> > 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.
> 
> Okay, removed -- thanks.
> 
>
https://codereview.chromium.org/2282173003/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java:245:
> extractLogcatChunkFromReader(reader, LOGCAT_SIZE, inMicrodump, rawLogcat);
> On 2016/09/01 17:54:42, acleung1 wrote:
> > On 2016/09/01 02:00:03, Ilya Sherman wrote:
> > > 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?)
> > 
> > Not that I know of.
> 
> Maria, WDYT?

A couple of thoughts:
a) I do think logcat can be a pretty large output to read into memory. So I
would actually prefer chunked reading in the current code. Alternatively, we
could exec something like "logcat -d | tail -n MAX_LINES + MINIDUMP_OVERHEAD"
and then read approximately the right number of lines all at once. The idea
being that instead of getting all of logcat, we'd just get the tail end of it
that we are interested in.
b) I think neither the original behaviour nor the new behaviour is exactly what
I would want as a user. Generally we use the logcat output for debugging crashes
inside the crash/ interface. Every time I've tried to use it, I found that it
shows the wrong part of the logcat stream. What I would really like are the
MAX_LINES before the crash stack + some indication of where the minidump was
(but not the minidump itself). If we keep the early lines, then we might have
data from too early on before the crash and with the late lines we can get data
after the crash occurred, which is also not too useful. So I'd really want to
focus on getting lines before the minidump, if possible.

[Ilya Sherman, 2016-09-03 00:53:42]

Thanks, that's very helpful feedback!

On 2016/09/02 18:04:09, Maria wrote:
> A couple of thoughts:
> a) I do think logcat can be a pretty large output to read into memory. So I
> would actually prefer chunked reading in the current code. Alternatively, we
> could exec something like "logcat -d | tail -n MAX_LINES + MINIDUMP_OVERHEAD"
> and then read approximately the right number of lines all at once. The idea
> being that instead of getting all of logcat, we'd just get the tail end of it
> that we are interested in.

Okay, that makes sense.  I kind of like the idea of piping to the "tail"
command, but I'm wondering whether it's possible we'd still lose the data we're
interested in by doing so, because a bunch of unrelated stuff could be dumped
into the log in the meantime?

> b) I think neither the original behaviour nor the new behaviour is exactly
what
> I would want as a user. Generally we use the logcat output for debugging
crashes
> inside the crash/ interface. Every time I've tried to use it, I found that it
> shows the wrong part of the logcat stream. What I would really like are the
> MAX_LINES before the crash stack + some indication of where the minidump was
> (but not the minidump itself). If we keep the early lines, then we might have
> data from too early on before the crash and with the late lines we can get
data
> after the crash occurred, which is also not too useful. So I'd really want to
> focus on getting lines before the minidump, if possible.

That makes a lot of sense.  Do you know whether we can rely on a microdump
always being written to the logcat?  Or, if it's absent, would just grabbing the
last N lines make sense?  (By the way, I'm assuming you meant "microdump"
everywhere where you wrote "minidump" above -- is that correct?)

[Maria, 2016-09-03 03:03:10]

On 2016/09/03 00:53:42, Ilya Sherman wrote:
> Thanks, that's very helpful feedback!
> 
> On 2016/09/02 18:04:09, Maria wrote:
> > A couple of thoughts:
> > a) I do think logcat can be a pretty large output to read into memory. So I
> > would actually prefer chunked reading in the current code. Alternatively, we
> > could exec something like "logcat -d | tail -n MAX_LINES +
MINIDUMP_OVERHEAD"
> > and then read approximately the right number of lines all at once. The idea
> > being that instead of getting all of logcat, we'd just get the tail end of
it
> > that we are interested in.
> 
> Okay, that makes sense.  I kind of like the idea of piping to the "tail"
> command, but I'm wondering whether it's possible we'd still lose the data
we're
> interested in by doing so, because a bunch of unrelated stuff could be dumped
> into the log in the meantime?

Yes, that's definitely a possible issue. I do think we could conservatively keep
2x MAX_LINES and still be okay memory-wise. I double there would be that much
extra stuff in logcat. But there are definitely edge cases possible here.


> 
> > b) I think neither the original behaviour nor the new behaviour is exactly
> what
> > I would want as a user. Generally we use the logcat output for debugging
> crashes
> > inside the crash/ interface. Every time I've tried to use it, I found that
it
> > shows the wrong part of the logcat stream. What I would really like are the
> > MAX_LINES before the crash stack + some indication of where the minidump was
> > (but not the minidump itself). If we keep the early lines, then we might
have
> > data from too early on before the crash and with the late lines we can get
> data
> > after the crash occurred, which is also not too useful. So I'd really want
to
> > focus on getting lines before the minidump, if possible.
> 
> That makes a lot of sense.  Do you know whether we can rely on a microdump
> always being written to the logcat?  Or, if it's absent, would just grabbing
the
> last N lines make sense?  (By the way, I'm assuming you meant "microdump"
> everywhere where you wrote "minidump" above -- is that correct?)

Yes, I did mean microdump :)

I believe a microdump is always written to logcat (I imagine Alan can verify if
what I am saying is correct). But from the tests I assume that Alan saw cases
where there wasn't one to be found. In which case last N lines make sense to me.
I think it'd be nice to keep BEGIN_MICRODUMP line in the log, so that it's clear
whether the log is next to microdump or we didn't find one.

I wonder if there could also be multiple micordumps written, which would
complicate the extraction.

[Maria, 2016-09-06 18:12:39]

https://codereview.chromium.org/2282173003/diff/40001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/crash/LogcatExtractionCallableTest.java
(right):

https://codereview.chromium.org/2282173003/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/crash/LogcatExtractionCallableTest.java:137:
assertEquals(false, inMicrodump);
nit: use assertFalse instead

[Ilya Sherman, 2016-09-10 00:22:28]

Thanks for all of the feedback, Maria!  I'm about to head out on vacation for a
week and a half, but I'll try to pick up this CL again not too long after I get
back.  Or, if anyone wants to steal it from me in the meantime, that's okay too!
:)

[Ilya Sherman, 2016-09-24 02:01:00]

Description was changed from

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

BUG=641655
R=gayane@chromium.org
==========

to

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

More precisely, reads the last N lines preceding the microdump, if any,
since any lines that follow the microdump are most likely not relevant to
the crash.

BUG=641655
R=gayane@chromium.org
==========

[Ilya Sherman, 2016-09-24 02:01:35]

Hokay, reworked this to read the last lines preceding the last seen microdump
(if any).  Maria, PTAL =)

https://codereview.chromium.org/2282173003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java
(right):

https://codereview.chromium.org/2282173003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java:281:
// Trim off the last microdump, and anything after it.
Is it worth also trimming out any other microdumps that might be within the tail
of the logcat?  I'm guessing it's a rare event in practice, but it might be
interesting to see that two crashes occurred in rapid sequence.  Is there
something else in the logcat that would indicate the previous crash, even
without the microdump?

[Maria, 2016-09-26 17:28:29]

Thanks for making the changes.

https://codereview.chromium.org/2282173003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java
(right):

https://codereview.chromium.org/2282173003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java:236:
Process p = Runtime.getRuntime().exec("logcat -d | tail -n " + LOGCAT_SIZE * 4);
java style nit: parenthesis around LOGCAT_SIZE * 4

https://codereview.chromium.org/2282173003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java:281:
// Trim off the last microdump, and anything after it.
On 2016/09/24 02:01:35, Ilya Sherman wrote:
> Is it worth also trimming out any other microdumps that might be within the
tail
> of the logcat?  I'm guessing it's a rare event in practice, but it might be
> interesting to see that two crashes occurred in rapid sequence.  Is there
> something else in the logcat that would indicate the previous crash, even
> without the microdump?

Hmm, not sure there is. I think it's okay to just leave the previous lines
as-is. After the change lands maybe we could just go and spot check the actual
crashes for this issue and if it's common to see a microdump then we can always
add that.

https://codereview.chromium.org/2282173003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java:284:
rawLogcat = rawLogcat.subList(0, i);
Could we add a placeholder line in here -- it helps hugely to know that the logs
are actually preceeding the crash or whether this extraction just didn't work
out for some reason.

E.g. let's insert a line "EDITED OUT MICRODUMP FOR THIS CRASH" or something
along those lines.

[Ilya Sherman, 2016-09-27 03:08:28]

Thanks, Maria.

https://codereview.chromium.org/2282173003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java
(right):

https://codereview.chromium.org/2282173003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java:236:
Process p = Runtime.getRuntime().exec("logcat -d | tail -n " + LOGCAT_SIZE * 4);
On 2016/09/26 17:28:28, Maria wrote:
> java style nit: parenthesis around LOGCAT_SIZE * 4

Done.

https://codereview.chromium.org/2282173003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java:281:
// Trim off the last microdump, and anything after it.
On 2016/09/26 17:28:28, Maria wrote:
> On 2016/09/24 02:01:35, Ilya Sherman wrote:
> > Is it worth also trimming out any other microdumps that might be within the
> tail
> > of the logcat?  I'm guessing it's a rare event in practice, but it might be
> > interesting to see that two crashes occurred in rapid sequence.  Is there
> > something else in the logcat that would indicate the previous crash, even
> > without the microdump?
> 
> Hmm, not sure there is. I think it's okay to just leave the previous lines
> as-is. After the change lands maybe we could just go and spot check the actual
> crashes for this issue and if it's common to see a microdump then we can
always
> add that.

Okay.  If I'm understanding you correctly, you're saying we can try the simple
approach of leaving any potential additional microdumps alone, correct?  Or did
you mean to restore the previous implementation, which stripped out *all*
microdumps?

https://codereview.chromium.org/2282173003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java:284:
rawLogcat = rawLogcat.subList(0, i);
On 2016/09/26 17:28:28, Maria wrote:
> Could we add a placeholder line in here -- it helps hugely to know that the
logs
> are actually preceeding the crash or whether this extraction just didn't work
> out for some reason.
> 
> E.g. let's insert a line "EDITED OUT MICRODUMP FOR THIS CRASH" or something
> along those lines.

Done.

[Maria, 2016-09-27 03:48:33]

lgtm

https://codereview.chromium.org/2282173003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java
(right):

https://codereview.chromium.org/2282173003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionCallable.java:281:
// Trim off the last microdump, and anything after it.
On 2016/09/27 03:08:28, Ilya Sherman wrote:
> On 2016/09/26 17:28:28, Maria wrote:
> > On 2016/09/24 02:01:35, Ilya Sherman wrote:
> > > Is it worth also trimming out any other microdumps that might be within
the
> > tail
> > > of the logcat?  I'm guessing it's a rare event in practice, but it might
be
> > > interesting to see that two crashes occurred in rapid sequence.  Is there
> > > something else in the logcat that would indicate the previous crash, even
> > > without the microdump?
> > 
> > Hmm, not sure there is. I think it's okay to just leave the previous lines
> > as-is. After the change lands maybe we could just go and spot check the
actual
> > crashes for this issue and if it's common to see a microdump then we can
> always
> > add that.
> 
> Okay.  If I'm understanding you correctly, you're saying we can try the simple
> approach of leaving any potential additional microdumps alone, correct?  Or
did
> you mean to restore the previous implementation, which stripped out *all*
> microdumps?

Yes, let's try the simple approach first.

[Ilya Sherman, 2016-09-27 04:47:51]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2016-09-27 04:47:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
acleung@chromium.org, gayane@chromium.org
Link to the patchset: https://codereview.chromium.org/2282173003/#ps80001
(title: "Leave a record of snipped microdumps")

[commit-bot: I haz the power, 2016-09-27 04:48:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 05:46:57]

Description was changed from

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

More precisely, reads the last N lines preceding the microdump, if any,
since any lines that follow the microdump are most likely not relevant to
the crash.

BUG=641655
R=gayane@chromium.org
==========

to

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

More precisely, reads the last N lines preceding the microdump, if any,
since any lines that follow the microdump are most likely not relevant to
the crash.

BUG=641655
R=gayane@chromium.org
==========

[commit-bot: I haz the power, 2016-09-27 05:46:58]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-27 05:48:54]

Description was changed from

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

More precisely, reads the last N lines preceding the microdump, if any,
since any lines that follow the microdump are most likely not relevant to
the crash.

BUG=641655
R=gayane@chromium.org
==========

to

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

More precisely, reads the last N lines preceding the microdump, if any,
since any lines that follow the microdump are most likely not relevant to
the crash.

BUG=641655
R=gayane@chromium.org

Committed: https://crrev.com/4a1d16c19d99d81f644c90365751fe9408963f36
Cr-Commit-Position: refs/heads/master@{#421117}
==========

[commit-bot: I haz the power, 2016-09-27 05:48:55]

Patchset 5 (id:??) landed as
https://crrev.com/4a1d16c19d99d81f644c90365751fe9408963f36
Cr-Commit-Position: refs/heads/master@{#421117}