Refactor CrashDump*Manager to use a shared CrashDumpObserver singleton.

components/crash/content/browser/crash_dump_observer_android.cc

Index: components/crash/content/browser/crash_dump_observer_android.cc
diff --git a/components/crash/content/browser/crash_dump_observer_android.cc b/components/crash/content/browser/crash_dump_observer_android.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d1a4f0d47f614dd27535cfd1576a73c7f3d41f48
--- /dev/null
+++ b/components/crash/content/browser/crash_dump_observer_android.cc
@@ -0,0 +1,152 @@
+// Copyright 2015 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 "components/crash/content/browser/crash_dump_observer_android.h"
+
+#include <unistd.h>
+
+#include "base/bind.h"
+#include "base/lazy_instance.h"
+#include "base/logging.h"
+#include "base/stl_util.h"
+#include "base/threading/sequenced_worker_pool.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"
+
+using content::BrowserThread;
+
+namespace breakpad {
+
+CrashDumpObserver::Client::Client() {}
+
+CrashDumpObserver::Client::~Client() {}
+
+namespace {
+base::LazyInstance<CrashDumpObserver> g_instance = LAZY_INSTANCE_INITIALIZER;
+}
+
+// static
+void CrashDumpObserver::Create() {
+  GetInstance();
+}
+
+// static
+CrashDumpObserver* CrashDumpObserver::GetInstance() {
+  return g_instance.Pointer();

[boliu, 2016/10/13 21:07:19]

If you have an explicit Create, you probably want to ensure it's called first
before GetInstance is called?

[Bernhard Bauer, 2016/10/14 10:41:01]

And vice-versa, would it make sense to verify that no instance exists when
Create() is called?

[Tobias Sargeant, 2016/12/08 16:41:41]

Done.

+}
+
+CrashDumpObserver::CrashDumpObserver() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  notification_registrar_.Add(this,
+                              content::NOTIFICATION_RENDERER_PROCESS_TERMINATED,
+                              content::NotificationService::AllSources());
+  notification_registrar_.Add(this,
+                              content::NOTIFICATION_RENDERER_PROCESS_CLOSED,
+                              content::NotificationService::AllSources());
+  BrowserChildProcessObserver::Add(this);
+}
+
+CrashDumpObserver::~CrashDumpObserver() {
+  BrowserChildProcessObserver::Remove(this);
+}
+
+void CrashDumpObserver::OnChildExit(int child_process_id,
+                                    base::ProcessHandle pid,
+                                    content::ProcessType process_type,
+                                    base::TerminationStatus termination_status,
+                                    base::android::ApplicationState app_state) {
+  base::AutoLock auto_lock(registered_clients_lock_);
+  base::SequencedWorkerPool* pool = BrowserThread::GetBlockingPool();

[boliu, 2016/10/13 21:07:19]

old implementation uses FILE thread?

probably should avoid behavior changes for a refactor CL. you can always change
this later?

[Tobias Sargeant, 2016/12/08 16:41:41]

Done.

+  base::SequencedWorkerPool::SequenceToken token = pool->GetSequenceToken();
+
+  for (auto& client : registered_clients_) {
+    pool->PostSequencedWorkerTask(
+        token, FROM_HERE,
+        base::Bind(&Client::OnChildExit, client, child_process_id, pid,

[Bernhard Bauer, 2016/10/14 10:41:01]

Actually, why do you need to post this to a background thread? You could just
make it the responsibility of the client to handle the callback in the
background.

[Tobias Sargeant, 2016/12/08 16:41:41]

Per the discussion offline, the post has been removed from here.

+                   process_type, termination_status, app_state));
+  }
+}
+
+void CrashDumpObserver::RegisterClient(scoped_refptr<Client> client) {
+  base::AutoLock auto_lock(registered_clients_lock_);
+  if (!ContainsValue(registered_clients_, client))
+    registered_clients_.push_back(std::move(client));
+}
+
+void CrashDumpObserver::UnregisterClient(scoped_refptr<Client> client) {
+  base::AutoLock auto_lock(registered_clients_lock_);
+  registered_clients_.erase(std::remove(registered_clients_.begin(),
+                                        registered_clients_.end(), client),
+                            registered_clients_.end());
+}
+
+void CrashDumpObserver::BrowserChildProcessStarted(
+    int child_process_id,
+    content::FileDescriptorInfo* mappings) {
+  base::AutoLock auto_lock(registered_clients_lock_);
+  for (auto& client : registered_clients_) {
+    client->OnChildStart(child_process_id, mappings);

[boliu, 2016/10/13 21:07:19]

if a client decides to remove itself inside OnChildStart, that would deadlock..

[Tobias Sargeant, 2016/12/08 16:41:41]

Clients can't be removed now, but registering new clients could cause the same
problem, and that's fixed.

+  }
+}
+
+void CrashDumpObserver::BrowserChildProcessHostDisconnected(
+    const content::ChildProcessData& data) {

[Bernhard Bauer, 2016/10/14 10:41:01]

Can you add some DCHECKs for the threads the various methods are called on?

[Tobias Sargeant, 2016/12/08 16:41:41]

Done.

+  OnChildExit(data.id, data.handle,
+              static_cast<content::ProcessType>(data.process_type),
+              base::TERMINATION_STATUS_MAX_ENUM,
+              base::android::APPLICATION_STATE_UNKNOWN);
+}
+
+void CrashDumpObserver::BrowserChildProcessCrashed(
+    const content::ChildProcessData& data,
+    int exit_code) {
+  OnChildExit(data.id, data.handle,
+              static_cast<content::ProcessType>(data.process_type),
+              base::TERMINATION_STATUS_ABNORMAL_TERMINATION,
+              base::android::APPLICATION_STATE_UNKNOWN);
+}
+
+void CrashDumpObserver::Observe(int type,
+                                const content::NotificationSource& source,
+                                const content::NotificationDetails& details) {
+  content::RenderProcessHost* rph =
+      content::Source<content::RenderProcessHost>(source).ptr();
+  base::TerminationStatus term_status = base::TERMINATION_STATUS_MAX_ENUM;
+  base::android::ApplicationState app_state =
+      base::android::APPLICATION_STATE_UNKNOWN;
+  switch (type) {
+    case content::NOTIFICATION_RENDERER_PROCESS_TERMINATED: {
+      // NOTIFICATION_RENDERER_PROCESS_TERMINATED is sent when the renderer
+      // process is cleanly shutdown. However, we still need to close the
+      // minidump_fd we kept open.
+      term_status = base::TERMINATION_STATUS_NORMAL_TERMINATION;
+      break;
+    }
+    case content::NOTIFICATION_RENDERER_PROCESS_CLOSED: {
+      // We do not care about android fast shutdowns as it is a known case where
+      // the renderer is intentionally killed when we are done with it.
+      if (rph->FastShutdownStarted()) {
+        break;
+      }
+      content::RenderProcessHost::RendererClosedDetails* process_details =
+          content::Details<content::RenderProcessHost::RendererClosedDetails>(
+              details)
+              .ptr();
+      term_status = process_details->status;
+      app_state = base::android::ApplicationStatusListener::GetState();
+      break;
+    }
+    default:
+      NOTREACHED();
+      return;
+  }
+
+  OnChildExit(rph->GetID(), rph->GetHandle(), content::PROCESS_TYPE_RENDERER,
+              term_status, app_state);
+}
+
+}  // namespace breakpad