[Crash Reporting] Implement a more direct bridge for extracting logcat output.

[Crash Reporting] Implement a more direct bridge for extracting logcat output.

When a child process crashes, Chrome attempts to attach recent logcat
output to the minidump for the crash. Previously, this was implemented
via a FileObserver on the crash reports directory: When the file observer
detected a file moved into the directory, and saw that the filename
matched the appropriate regex pattern, it would attempt to append logcat
output. This has two drawbacks:
  (1) It's spooky action at a distance, which makes the code harder to
      reason about.
  (2) There's a super subtle constraint encoded into the "moved to" check:
      When the browser process crashes, a crash dump matching the same
      file pattern appears in the observed directory, but it is *created*
      rather than *moved*. Thus, this check is intentionally excluding
      browser crashes, for which the code might otherwise have a race
      between trying to attach logcat output and tearing down the main
      Java process.

This CL replaces the FileObserver with a direct call from the C++ code
that processes the minidump into the Java code that extracts the logcat.
(Well, there's some indirection to get through the Chrome layering onion,
but the flow is still much more linear.)

BUG=719129
TEST=CrashDumpManagerTest.*
R=gsennton@chromium.org

Review-Url: https://codereview.chromium.org/2878193002
Cr-Commit-Position: refs/heads/master@{#472613}
Committed: https://chromium.googlesource.com/chromium/src/+/6a32f0233caef0ad160bb4c3adcc25bb091ab4d0

[Ilya Sherman, 2017-05-13 05:19:36]

The CQ bit was checked by isherman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-13 05:19:52]

Dry run: 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, 2017-05-13 05:24:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-13 05:24:43]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ilya Sherman, 2017-05-13 06:37:21]

The CQ bit was checked by isherman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-13 06:37:36]

Dry run: 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, 2017-05-13 07:20:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-13 07:20:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gsennton, 2017-05-15 13:00:41]

gsennton@chromium.org changed reviewers:
+ tobiasjs@chromium.org

[gsennton, 2017-05-15 13:00:42]

I like this much more than the Directory-listener (it is more clear in what
cases this solution is supposed to work).

I'm adding Toby as a FYI since he's reviewing
https://codereview.chromium.org/2833893002/ for me

lgtm % one comment

Thanks Ilya!

https://codereview.chromium.org/2878193002/diff/20001/components/crash/androi...
File
components/crash/android/java/src/org/chromium/components/crash/browser/CrashDumpManager.java
(right):

https://codereview.chromium.org/2878193002/diff/20001/components/crash/androi...
components/crash/android/java/src/org/chromium/components/crash/browser/CrashDumpManager.java:46:
public static void tryToAttachLogcat(String minidumpPath) {
This CL makes my life easier w.r.t. https://codereview.chromium.org/2833893002/
where we want to trigger WebView's minidump uploading on renderer crashes (I can
just re-use your logic here to trigger this for WebView as well :)).

Could you maybe rename this method to something a bit more generic - like
triggerMinidumpUploading, or tryToUploadMinidump, if that makes sense. For
WebView we would want a callback where we won't really care about the name of
the minidump (and we won't attach any logcat) - we'll just trigger minidump
copying + uploading for any minidumps that happen to be stored in our crash
directory (we can still pass the name of the minidump though - WebView will just
ignore it :)).

[gsennton, 2017-05-15 13:05:13]

Toby, does it make sense to use the mechanism presented in this CL for WebView
on render crashes, or would it be unwise for the cases where a renderer crash
also causes a browser/app crash?

[Ilya Sherman, 2017-05-16 07:02:41]

The CQ bit was checked by isherman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 07:03:05]

Dry run: CQ is trying da patch.

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

[Ilya Sherman, 2017-05-16 07:03:39]

Thanks, Gustav!

https://codereview.chromium.org/2878193002/diff/20001/components/crash/androi...
File
components/crash/android/java/src/org/chromium/components/crash/browser/CrashDumpManager.java
(right):

https://codereview.chromium.org/2878193002/diff/20001/components/crash/androi...
components/crash/android/java/src/org/chromium/components/crash/browser/CrashDumpManager.java:46:
public static void tryToAttachLogcat(String minidumpPath) {
On 2017/05/15 13:00:41, gsennton wrote:
> This CL makes my life easier w.r.t.
https://codereview.chromium.org/2833893002/
> where we want to trigger WebView's minidump uploading on renderer crashes (I
can
> just re-use your logic here to trigger this for WebView as well :)).
> 
> Could you maybe rename this method to something a bit more generic - like
> triggerMinidumpUploading, or tryToUploadMinidump, if that makes sense. For
> WebView we would want a callback where we won't really care about the name of
> the minidump (and we won't attach any logcat) - we'll just trigger minidump
> copying + uploading for any minidumps that happen to be stored in our crash
> directory (we can still pass the name of the minidump though - WebView will
just
> ignore it :)). 

Done.

[Ilya Sherman, 2017-05-16 07:05:32]

The CQ bit was checked by isherman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 07:05:40]

Dry run: CQ is trying da patch.

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

[Ilya Sherman, 2017-05-16 07:06:07]

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

[Ilya Sherman, 2017-05-16 07:06:08]

+Maria for OWNERS review of all the Java files.
+Mark for OWNERS review of all the //components/crash files.

[commit-bot: I haz the power, 2017-05-16 08:31:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-16 08:31:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Tobias Sargeant, 2017-05-16 14:31:12]

On 2017/05/15 13:05:13, gsennton wrote:
> Toby, does it make sense to use the mechanism presented in this CL for WebView
> on render crashes, or would it be unwise for the cases where a renderer crash
> also causes a browser/app crash?

Unfortunately it probably would be. The thread hops involved for the browser
process terminator are:

UI -> FILE -> UI (death)

And for the CrashDumpManager:

UI -> FILE

The terminator thus always runs after  the CrashDumpManager task completes, but
as it stands we would post to an async thread from that callback, which would
then be racing with the UI thread task that might eventually kill the process.
You could bounce the callback back to the UI thread, though, and that would
ensure that it only ran if the browser terminator let the application live.

Is the Chrome implementation of this callback likely to block the FILE thread
for any significant period of time?

[Mark Mentovai, 2017-05-16 14:34:31]

components/crash LGTM, mostly as a stamp to approve your new subdirectory.

[Ilya Sherman, 2017-05-16 20:26:25]

On 2017/05/16 14:31:12, Tobias Sargeant wrote:
> On 2017/05/15 13:05:13, gsennton wrote:
> > Toby, does it make sense to use the mechanism presented in this CL for
WebView
> > on render crashes, or would it be unwise for the cases where a renderer
crash
> > also causes a browser/app crash?
> 
> Unfortunately it probably would be. The thread hops involved for the browser
> process terminator are:
> 
> UI -> FILE -> UI (death)
> 
> And for the CrashDumpManager:
> 
> UI -> FILE
> 
> The terminator thus always runs after  the CrashDumpManager task completes,
but
> as it stands we would post to an async thread from that callback, which would
> then be racing with the UI thread task that might eventually kill the process.
> You could bounce the callback back to the UI thread, though, and that would
> ensure that it only ran if the browser terminator let the application live.
> 
> Is the Chrome implementation of this callback likely to block the FILE thread
> for any significant period of time?

Hmm, should we similarly hop over to the UI thread for Chrome?  How does hopping
back to the UI thread help avoid a race?  Couldn't the hops still happen in
either order, relative to one another?

I think the Chrome implementation doesn't run on the FILE thread at all... I
assume that calling a static JNI method lands on the UI thread?  Or maybe on the
thread that Chrome called the method from?  Now that you mention it, perhaps
CrashDumpManager *should* hop over to the FILE thread briefly, to check that the
minidump file exists and is a file.  Actually, do we still use a named FILE
thread for doing work?  I thought we typically just used arbitrary worker
threads -- no?

Note to self: sCallback should probably be synchronized, if this code is being
accessed from multiple threads (and I think that it probably is).

[Ilya Sherman, 2017-05-16 23:42:13]

The CQ bit was checked by isherman@chromium.org to run a CQ dry run

[Ilya Sherman, 2017-05-16 23:42:30]

On 2017/05/16 20:26:25, Ilya Sherman wrote:
> On 2017/05/16 14:31:12, Tobias Sargeant wrote:
> > On 2017/05/15 13:05:13, gsennton wrote:
> > > Toby, does it make sense to use the mechanism presented in this CL for
> WebView
> > > on render crashes, or would it be unwise for the cases where a renderer
> crash
> > > also causes a browser/app crash?
> > 
> > Unfortunately it probably would be. The thread hops involved for the browser
> > process terminator are:
> > 
> > UI -> FILE -> UI (death)
> > 
> > And for the CrashDumpManager:
> > 
> > UI -> FILE
> > 
> > The terminator thus always runs after  the CrashDumpManager task completes,
> but
> > as it stands we would post to an async thread from that callback, which
would
> > then be racing with the UI thread task that might eventually kill the
process.
> > You could bounce the callback back to the UI thread, though, and that would
> > ensure that it only ran if the browser terminator let the application live.
> > 
> > Is the Chrome implementation of this callback likely to block the FILE
thread
> > for any significant period of time?
> 
> Hmm, should we similarly hop over to the UI thread for Chrome?  How does
hopping
> back to the UI thread help avoid a race?  Couldn't the hops still happen in
> either order, relative to one another?
> 
> I think the Chrome implementation doesn't run on the FILE thread at all... I
> assume that calling a static JNI method lands on the UI thread?  Or maybe on
the
> thread that Chrome called the method from?  Now that you mention it, perhaps
> CrashDumpManager *should* hop over to the FILE thread briefly, to check that
the
> minidump file exists and is a file.  Actually, do we still use a named FILE
> thread for doing work?  I thought we typically just used arbitrary worker
> threads -- no?
> 
> Note to self: sCallback should probably be synchronized, if this code is being
> accessed from multiple threads (and I think that it probably is).

Sorry for the scattered nature of my previous reply. Answering my own questions:
  1. The C++ code in crash_dump_manager_android.cc is old-skool and running on
     the FILE thread. The JNI call happens on the same thread, so the Java call
     to CrashDumpManager.tryToUploadMinidump() runs on the same FILE thread.
  2. sCallback was incorrectly being accessed from both the UI and the FILE
     thread. I've fixed this.
  3. There doesn't seem to be any equivalent to AwBrowserTerminator within
     Chrome, so the race that Toby mentioned shouldn't affect the Chrome
     implementation.

To answer Toby's question more clearly: The Chome implementation shouldn't block
the FILE thread for very long at all. It (now) just does a null check followed
by testing for the existence of a file, then bounces the rest of the work to the
UI thread.

Finally, I'm still not convinced that adding a hop back to the UI thread, so
that the total sequence is UI -> FILE -> UI, helps with the race being discussed
in https://codereview.chromium.org/2833893002/. I think the race ends up being
resolved in the wrong order. And, TBH, I'm a bit uncomfortable with the explicit
ordering assumptions being baked into that CL, in terms of whether the
CrashDumpManager or the AwBrowserTerminator code gets to execute first. That
feels more like an implementation detail (which could change) than an API
guarantee. It would be great to at least add a very explicit test documenting
that assumption.

[commit-bot: I haz the power, 2017-05-16 23:44:05]

Dry run: CQ is trying da patch.

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

[Maria, 2017-05-17 00:39:23]

https://codereview.chromium.org/2878193002/diff/60001/components/crash/androi...
File
components/crash/android/java/src/org/chromium/components/crash/browser/CrashDumpManager.java
(right):

https://codereview.chromium.org/2878193002/diff/60001/components/crash/androi...
components/crash/android/java/src/org/chromium/components/crash/browser/CrashDumpManager.java:34:
public static void registerUploadCallback(UploadMinidumpCallback callback) {
I am concerned about thread safety in this class. Seems like sCallback is being
accessed from multiple threads?

https://codereview.chromium.org/2878193002/diff/60001/components/crash/androi...
components/crash/android/java/src/org/chromium/components/crash/browser/CrashDumpManager.java:59:
Log.e(TAG, "Minidump path should describe a file that exists! Bailing...");
Seems like a good place to print minidumpPath

[Ilya Sherman, 2017-05-17 00:59:54]

https://codereview.chromium.org/2878193002/diff/60001/components/crash/androi...
File
components/crash/android/java/src/org/chromium/components/crash/browser/CrashDumpManager.java
(right):

https://codereview.chromium.org/2878193002/diff/60001/components/crash/androi...
components/crash/android/java/src/org/chromium/components/crash/browser/CrashDumpManager.java:34:
public static void registerUploadCallback(UploadMinidumpCallback callback) {
On 2017/05/17 00:39:23, Maria wrote:
> I am concerned about thread safety in this class. Seems like sCallback is
being
> accessed from multiple threads?

Yep, already fixed in the latest patch set =)  Good catch, though!

https://codereview.chromium.org/2878193002/diff/60001/components/crash/androi...
components/crash/android/java/src/org/chromium/components/crash/browser/CrashDumpManager.java:59:
Log.e(TAG, "Minidump path should describe a file that exists! Bailing...");
On 2017/05/17 00:39:23, Maria wrote:
> Seems like a good place to print minidumpPath

Done.

[Ilya Sherman, 2017-05-17 01:01:05]

The CQ bit was checked by isherman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 01:03:32]

Dry run: CQ is trying da patch.

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

[Maria, 2017-05-17 01:06:56]

lgtm

https://codereview.chromium.org/2878193002/diff/100001/components/crash/andro...
File
components/crash/android/java/src/org/chromium/components/crash/browser/CrashDumpManager.java
(right):

https://codereview.chromium.org/2878193002/diff/100001/components/crash/andro...
components/crash/android/java/src/org/chromium/components/crash/browser/CrashDumpManager.java:20:
* An interface for providing a callback that will try to upload a minidump.
Maybe add a note here that the callback will be called on UI thread. If someone
is adding a new implementation that's good info to know (they might not look at
tryToUploadMinidump method below)

[commit-bot: I haz the power, 2017-05-17 02:50:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-17 02:50:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Tobias Sargeant, 2017-05-17 10:55:23]

On 2017/05/16 23:42:30, Ilya Sherman wrote:
> Sorry for the scattered nature of my previous reply. Answering my own
questions:
>   1. The C++ code in crash_dump_manager_android.cc is old-skool and running on
>      the FILE thread. The JNI call happens on the same thread, so the Java
call
>      to CrashDumpManager.tryToUploadMinidump() runs on the same FILE thread.

CrashDumpManager::ProcessMinidump is explicitly on the FILE thread (see the
DCHECK). When I introduced it, it was suggested that a thread pool could be
used, but changing that would make it impossible to guarantee the required
ordering constraints described above.

>   3. There doesn't seem to be any equivalent to AwBrowserTerminator within
>      Chrome, so the race that Toby mentioned shouldn't affect the Chrome
>      implementation.

Correct, it's not important in Chrome. It's only webview that has to be prepared
to terminate the browser process in response to the death of the renderer
process (to preserve single process semantics).

> To answer Toby's question more clearly: The Chome implementation shouldn't
block
> the FILE thread for very long at all. It (now) just does a null check followed
> by testing for the existence of a file, then bounces the rest of the work to
the
> UI thread.
> 
> Finally, I'm still not convinced that adding a hop back to the UI thread, so
> that the total sequence is UI -> FILE -> UI, helps with the race being
discussed
> in https://codereview.chromium.org/2833893002/. I think the race ends up being
> resolved in the wrong order. And, TBH, I'm a bit uncomfortable with the
explicit
> ordering assumptions being baked into that CL, in terms of whether the
> CrashDumpManager or the AwBrowserTerminator code gets to execute first. That
> feels more like an implementation detail (which could change) than an API
> guarantee. It would be great to at least add a very explicit test documenting
> that assumption.

You're right. I think, in retrospect, it's not possible to use the mechanism
you're introducing here to schedule webview minidump uploads, because we need to
know that the browser is not about to be terminated, which is undecided at this
point. I can think of two solutions:

1) Implement this CL as a new CrashDumpObserver::Client, added after
AwBrowserTerminator in the webview case, or after CrashDumpManager for Chrome.
2) Give up, and just have AwBrowserTerminator trigger minidump uploads in
webview. (It's a pity that we won't share the same mechanism as Chrome, but so
be it).

It would be nice to not rely on the serial nature of jobs on the FILE thread for
this, but I can't see a way of guaranteeing the ordering required while still
being able to reason about CrashDumpObserver::Client object lifetimes.

[Alexei Svitkine (slow), 2017-05-17 12:22:01]

Drive-by (re: last comment): I think you can use the worker pool and still
guarantee sequencing. Check out SequencedTaskRunner. At least in C++.

On May 17, 2017 6:55 AM, <tobiasjs@chromium.org> wrote:

> On 2017/05/16 23:42:30, Ilya Sherman wrote:
> > Sorry for the scattered nature of my previous reply. Answering my own
> questions:
> > 1. The C++ code in crash_dump_manager_android.cc is old-skool and
> running on
> > the FILE thread. The JNI call happens on the same thread, so the Java
> call
> > to CrashDumpManager.tryToUploadMinidump() runs on the same FILE thread.
>
> CrashDumpManager::ProcessMinidump is explicitly on the FILE thread (see
> the
> DCHECK). When I introduced it, it was suggested that a thread pool could be
> used, but changing that would make it impossible to guarantee the required
> ordering constraints described above.
>
> > 3. There doesn't seem to be any equivalent to AwBrowserTerminator within
> > Chrome, so the race that Toby mentioned shouldn't affect the Chrome
> > implementation.
>
> Correct, it's not important in Chrome. It's only webview that has to be
> prepared
> to terminate the browser process in response to the death of the renderer
> process (to preserve single process semantics).
>
> > To answer Toby's question more clearly: The Chome implementation
> shouldn't
> block
> > the FILE thread for very long at all. It (now) just does a null check
> followed
> > by testing for the existence of a file, then bounces the rest of the
> work to
> the
> > UI thread.
> >
> > Finally, I'm still not convinced that adding a hop back to the UI
> thread, so
> > that the total sequence is UI -> FILE -> UI, helps with the race being
> discussed
> > in https://codereview.chromium.org/2833893002/. I think the race ends
> up being
> > resolved in the wrong order. And, TBH, I'm a bit uncomfortable with the
> explicit
> > ordering assumptions being baked into that CL, in terms of whether the
> > CrashDumpManager or the AwBrowserTerminator code gets to execute first.
> That
> > feels more like an implementation detail (which could change) than an API
> > guarantee. It would be great to at least add a very explicit test
> documenting
> > that assumption.
>
> You're right. I think, in retrospect, it's not possible to use the
> mechanism
> you're introducing here to schedule webview minidump uploads, because we
> need to
> know that the browser is not about to be terminated, which is undecided at
> this
> point. I can think of two solutions:
>
> 1) Implement this CL as a new CrashDumpObserver::Client, added after
> AwBrowserTerminator in the webview case, or after CrashDumpManager for
> Chrome.
> 2) Give up, and just have AwBrowserTerminator trigger minidump uploads in
> webview. (It's a pity that we won't share the same mechanism as Chrome,
> but so
> be it).
>
> It would be nice to not rely on the serial nature of jobs on the FILE
> thread for
> this, but I can't see a way of guaranteeing the ordering required while
> still
> being able to reason about CrashDumpObserver::Client object lifetimes.
>
> https://codereview.chromium.org/2878193002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ilya Sherman, 2017-05-17 21:27:05]

https://codereview.chromium.org/2878193002/diff/100001/components/crash/andro...
File
components/crash/android/java/src/org/chromium/components/crash/browser/CrashDumpManager.java
(right):

https://codereview.chromium.org/2878193002/diff/100001/components/crash/andro...
components/crash/android/java/src/org/chromium/components/crash/browser/CrashDumpManager.java:20:
* An interface for providing a callback that will try to upload a minidump.
On 2017/05/17 01:06:55, Maria wrote:
> Maybe add a note here that the callback will be called on UI thread. If
someone
> is adding a new implementation that's good info to know (they might not look
at
> tryToUploadMinidump method below)

Done.

[Ilya Sherman, 2017-05-17 21:27:20]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2017-05-17 21:27:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
gsennton@chromium.org, mark@chromium.org, mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/2878193002/#ps120001
(title: "Document threading more, and fix check_deps")

[commit-bot: I haz the power, 2017-05-17 21:28:50]

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, 2017-05-18 00:48:31]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495056440503630,
"parent_rev": "dede6efa1ae80ebb11885c649a610548485d3979", "commit_rev":
"6a32f0233caef0ad160bb4c3adcc25bb091ab4d0"}

[commit-bot: I haz the power, 2017-05-18 00:48:47]

Description was changed from

==========
[Crash Reporting] Implement a more direct bridge for extracting logcat output.

When a child process crashes, Chrome attempts to attach recent logcat
output to the minidump for the crash. Previously, this was implemented
via a FileObserver on the crash reports directory: When the file observer
detected a file moved into the directory, and saw that the filename
matched the appropriate regex pattern, it would attempt to append logcat
output. This has two drawbacks:
  (1) It's spooky action at a distance, which makes the code harder to
      reason about.
  (2) There's a super subtle constraint encoded into the "moved to" check:
      When the browser process crashes, a crash dump matching the same
      file pattern appears in the observed directory, but it is *created*
      rather than *moved*. Thus, this check is intentionally excluding
      browser crashes, for which the code might otherwise have a race
      between trying to attach logcat output and tearing down the main
      Java process.

This CL replaces the FileObserver with a direct call from the C++ code
that processes the minidump into the Java code that extracts the logcat.
(Well, there's some indirection to get through the Chrome layering onion,
but the flow is still much more linear.)

BUG=719129
TEST=CrashDumpManagerTest.*
R=gsennton@chromium.org
==========

to

==========
[Crash Reporting] Implement a more direct bridge for extracting logcat output.

When a child process crashes, Chrome attempts to attach recent logcat
output to the minidump for the crash. Previously, this was implemented
via a FileObserver on the crash reports directory: When the file observer
detected a file moved into the directory, and saw that the filename
matched the appropriate regex pattern, it would attempt to append logcat
output. This has two drawbacks:
  (1) It's spooky action at a distance, which makes the code harder to
      reason about.
  (2) There's a super subtle constraint encoded into the "moved to" check:
      When the browser process crashes, a crash dump matching the same
      file pattern appears in the observed directory, but it is *created*
      rather than *moved*. Thus, this check is intentionally excluding
      browser crashes, for which the code might otherwise have a race
      between trying to attach logcat output and tearing down the main
      Java process.

This CL replaces the FileObserver with a direct call from the C++ code
that processes the minidump into the Java code that extracts the logcat.
(Well, there's some indirection to get through the Chrome layering onion,
but the flow is still much more linear.)

BUG=719129
TEST=CrashDumpManagerTest.*
R=gsennton@chromium.org

Review-Url: https://codereview.chromium.org/2878193002
Cr-Commit-Position: refs/heads/master@{#472613}
Committed:
https://chromium.googlesource.com/chromium/src/+/6a32f0233caef0ad160bb4c3adcc...
==========

[commit-bot: I haz the power, 2017-05-18 00:48:49]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/6a32f0233caef0ad160bb4c3adcc...