Refactor CrashDump*Manager to use a shared CrashDumpObserver.

android_webview/browser/aw_browser_terminator.cc

Index: android_webview/browser/aw_browser_terminator.cc
diff --git a/android_webview/browser/aw_browser_terminator.cc b/android_webview/browser/aw_browser_terminator.cc
new file mode 100644
index 0000000000000000000000000000000000000000..0943e58fb2acd7a413c7d33530eeba51084c5946
--- /dev/null
+++ b/android_webview/browser/aw_browser_terminator.cc
@@ -0,0 +1,88 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "android_webview/browser/aw_browser_terminator.h"
+
+#include <unistd.h>
+
+#include "android_webview/common/aw_descriptors.h"
+#include "base/bind.h"
+#include "base/logging.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/child_process_data.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/browser/notification_types.h"
+#include "content/public/browser/render_process_host.h"
+
+namespace android_webview {
+
+AwBrowserTerminator::AwBrowserTerminator() {}
+
+AwBrowserTerminator::~AwBrowserTerminator() {
+  child_process_id_to_pipe_.clear();

[Bernhard Bauer, 2016/08/04 16:01:32]

Is this actually necessary if you're in the destructor anyway?

[Tobias Sargeant, 2016/08/05 14:59:21]

Agree, it's not necessary.

+}
+
+void AwBrowserTerminator::OnChildStart(int child_process_id,
+                                       content::FileDescriptorInfo* mappings) {
+  base::AutoLock auto_lock(child_process_id_to_pipe_lock_);
+  DCHECK(!ContainsKey(child_process_id_to_pipe_, child_process_id));

[Robert Sesek, 2016/08/04 16:59:01]

Should this be just a DCHECK? This code will continue in release mode...

[Robert Sesek, 2016/08/04 16:59:01]

IWYU: base/stl_util.h

[Tobias Sargeant, 2016/08/05 14:59:21]

The danger of overwriting one pid->pipe mapping (and thus maybe not terminating
because we expect data on the wrong pipe) is probably not sufficiently high to
justify terminating the process immediately. And then if the two choices are
between overwriting the mapping and not doing so, I think there are cases where
either would be right. I think the DCHECK is sufficient, but I am on the fence
whether to overwrite or return immediately in release mode.

[Tobias Sargeant, 2016/08/05 14:59:21]

Done.

+
+  std::unique_ptr<base::SyncSocket> local_pipe(new base::SyncSocket());
+  std::unique_ptr<base::SyncSocket> child_pipe(new base::SyncSocket());
+  if (base::SyncSocket::CreatePair(local_pipe.get(), child_pipe.get())) {
+    LOG(WARNING) << "tobiasjs: mapping: " << child_process_id << " to "

[Bernhard Bauer, 2016/08/04 16:01:32]

Nit: You might not want to immortalize yourself in here that much :)

[Tobias Sargeant, 2016/08/05 14:59:21]

Definitely not!

+                 << local_pipe->handle();
+    child_process_id_to_pipe_[child_process_id] = std::move(local_pipe);
+    mappings->Transfer(kAndroidWebViewCrashSignalDescriptor,
+                       base::ScopedFD(dup(child_pipe->handle())));
+  }
+}
+
+void AwBrowserTerminator::OnChildExit(
+    int child_process_id,
+    base::ProcessHandle pid,
+    content::ProcessType process_type,
+    base::TerminationStatus termination_status,
+    base::android::ApplicationState app_state) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::FILE);

[Bernhard Bauer, 2016/08/04 16:01:32]

Is there a particular reason you need the FILE thread for this as opposed to,
say, the blocking pool?

+  LOG(WARNING) << "tobiasjs: " << __PRETTY_FUNCTION__ << " child_process_id "
+               << child_process_id << " pid " << pid << " process_type "
+               << process_type << " termination_status " << termination_status
+               << " app_state " << app_state;
+  std::unique_ptr<base::SyncSocket> pipe;
+
+  {
+    base::AutoLock auto_lock(child_process_id_to_pipe_lock_);
+    const auto& iter = child_process_id_to_pipe_.find(child_process_id);
+    if (iter == child_process_id_to_pipe_.end()) {
+      // We might get a NOTIFICATION_RENDERER_PROCESS_TERMINATED and a
+      // NOTIFICATION_RENDERER_PROCESS_CLOSED.
+      return;
+    }
+    pipe = std::move(iter->second);
+    child_process_id_to_pipe_.erase(iter);
+  }
+  LOG(WARNING) << "tobiasjs: found pipe " << pipe->handle();
+  DCHECK(pipe->handle() != base::SyncSocket::kInvalidHandle);
+  LOG(WARNING) << "tobiasjs: pipe is valid";
+
+  if (termination_status == base::TERMINATION_STATUS_NORMAL_TERMINATION)
+    return;
+  LOG(WARNING) << "tobiasjs: termination status is not normal";
+  if (pipe->Peek() >= sizeof(int)) {
+    int exit_code;
+    pipe->Receive(&exit_code, sizeof(exit_code));
+    LOG(WARNING) << "tobiasjs: read exit code " << exit_code << " and dying";
+    LOG(FATAL) << "Renderer process crash detected (code " << exit_code
+               << "). Terminating browser.";
+  } else {
+    // The child process hasn't written anything into the pipe. This implies
+    // that it was terminated via SIGKILL by the low memory killer, and thus we
+    // need to perform a clean exit.
+    LOG(WARNING) << "tobiasjs: clean exit";
+    exit(0);
+  }
+}
+
+}  // namespace breakpad