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 <memory>
+#include <vector>
+
+#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
+// purpose of reacting to child process crashes.
+// The CrashDumpObserver instance exists on the browser main thread.
+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> {

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

Hm, it would be nice not to make the refcountedness part of the interface (see
also my comment in the .cc file).

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

Done, as we discussed. Described above.

+   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 void Create();
+  static CrashDumpObserver* GetInstance();
+
+  CrashDumpObserver();

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

Can you make constructor and destructor private? And then add comments about the
lifetime and ownership of this class? :)

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

Done.

+  ~CrashDumpObserver() override;
+
+  void RegisterClient(scoped_refptr<Client> client);

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

not sure if there's a new rule, but at least according to old rules, these
should be const&

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

If the method takes a reference, the rule is to pass a scoped_refptr. It can be
std::move()d to avoid ref churn.

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

Acknowledged.

+  void UnregisterClient(scoped_refptr<Client> client);
+
+  void BrowserChildProcessStarted(int child_process_id,

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

this one seems odd because embedder is supposed to actually call this at the
right time.

also at this point, seems like uneeded complexity, each embedder can just call
its platform-specific thing, be that CrashDumpManager or AwBrowserTerminator,
without going through Observer here

are you envisioning webview have both Terminator and CrashManager at some point?

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

Yes, I am. So while you're correct that it's a bit odd, it makes informing
multiple clients easier.

+                                  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::vector<scoped_refptr<Client>> registered_clients_;

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

there is only ever one client

is there ever going to be more than one?

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

Yes. WebView will eventually have both CrashDumpManager and AwBrowserTerminator,
and potentially a third Client that processes the renderer crash handling API.

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