Refactor CrashDump*Manager to use a shared CrashDumpObserver singleton.

components/crash/content/browser/crash_dump_observer_android.h

+// 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.
+
+#ifndef COMPONENTS_CRASH_CONTENT_BROWSER_CRASH_DUMP_OBSERVER_ANDROID_H_
+#define COMPONENTS_CRASH_CONTENT_BROWSER_CRASH_DUMP_OBSERVER_ANDROID_H_
+
+#include <list>
+#include <memory>
+
+#include "base/android/application_status_listener.h"
+#include "base/memory/ref_counted.h"
+#include "base/process/kill.h"
+#include "base/process/process.h"
+#include "content/public/browser/browser_child_process_observer.h"
+#include "content/public/browser/file_descriptor_info.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
+#include "content/public/common/process_type.h"
+
+namespace breakpad {
+
+// This class centralises the observation of child processes for the

[Lei Zhang, 2016/10/05 18:00:00]

Can you say something about what thread this class lives on?

[Tobias Sargeant, 2016/10/06 17:42:22]

The observer lives on the browser main thread. (I'll add a comment for that).

+// purpose of reacting to child process crashes.
+class CrashDumpObserver : public content::BrowserChildProcessObserver,
+                          public content::NotificationObserver {
+ public:
+  // CrashDumpObserver client interface.
+  // Note: do not access the CrashDumpObserver singleton to add or
+  // remove clients from within client callbacks.
+  class Client : public base::RefCountedThreadSafe<Client> {
+   public:
+    // Called on the launcher thread.
+    virtual void OnChildStart(int child_process_id,
+                              content::FileDescriptorInfo* mappings) = 0;
+    // Called on the blocking pool.
+    virtual void OnChildExit(int child_process_id,
+                             base::ProcessHandle pid,
+                             content::ProcessType process_type,
+                             base::TerminationStatus termination_status,
+                             base::android::ApplicationState app_state) = 0;
+
+   protected:
+    friend class base::RefCountedThreadSafe<Client>;
+    Client();
+    virtual ~Client();
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(Client);
+  };
+
+  static CrashDumpObserver* GetInstance();
+
+  CrashDumpObserver();
+  ~CrashDumpObserver() override;
+
+  void RegisterClient(scoped_refptr<Client> client);
+  void UnregisterClient(scoped_refptr<Client> client);
+
+  void BrowserChildProcessStarted(int child_process_id,
+                                  content::FileDescriptorInfo* mappings);
+
+ private:
+  // content::BrowserChildProcessObserver implementation:
+  void BrowserChildProcessHostDisconnected(
+      const content::ChildProcessData& data) override;
+  void BrowserChildProcessCrashed(const content::ChildProcessData& data,
+                                  int exit_code) override;
+
+  // NotificationObserver implementation:
+  void Observe(int type,
+               const content::NotificationSource& source,
+               const content::NotificationDetails& details) override;
+
+  // Called on child process exit (including crash).
+  void OnChildExit(int child_process_id,
+                   base::ProcessHandle pid,
+                   content::ProcessType process_type,
+                   base::TerminationStatus termination_status,
+                   base::android::ApplicationState app_state);
+
+  content::NotificationRegistrar notification_registrar_;
+
+  base::Lock registered_clients_lock_;
+  std::list<scoped_refptr<Client>> registered_clients_;

[Lei Zhang, 2016/10/05 18:00:00]

As is, there's no advantage of having this as a list over a vector.

[Tobias Sargeant, 2016/10/13 12:52:14]

Have changed it.

+
+  static CrashDumpObserver* instance_;

[Lei Zhang, 2016/10/05 18:00:00]

Can this just be declared in crash_dump_observer_android.cc instead? Can it be a
LazyInstance?

[Tobias Sargeant, 2016/10/06 17:42:22]

It could be done this way. Would it be preferable to do that, and and have the
lifetime of the observer not be directly owned by BrowserMainParts instances? I
guess that would mean that the individual clients then would be, and they would
be explicitly unregistered.

[Tobias Sargeant, 2016/10/13 12:52:14]

PS4 does things this way.

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

LazyInstance seems overkill and wrong, because this class needs to be
constructed on the UI thread.

+
+  DISALLOW_COPY_AND_ASSIGN(CrashDumpObserver);
+};
+
+}  // namespace breakpad
+
+#endif  // COMPONENTS_CRASH_CONTENT_BROWSER_CRASH_DUMP_OBSERVER_ANDROID_H_