media: Add BrowserCdmManagerProcessWatcher.

content/browser/media/cdm/browser_cdm_manager.cc

Index: content/browser/media/cdm/browser_cdm_manager.cc
diff --git a/content/browser/media/cdm/browser_cdm_manager.cc b/content/browser/media/cdm/browser_cdm_manager.cc
index 3c4438ac200d47286018dfffa0339445a085003a..6d1da97d0f4b91aa90d64dfc13793f698d162caa 100644
--- a/content/browser/media/cdm/browser_cdm_manager.cc
+++ b/content/browser/media/cdm/browser_cdm_manager.cc
@@ -17,7 +17,7 @@
 #include "content/public/browser/permission_type.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
-#include "content/public/browser/render_view_host.h"
+#include "content/public/browser/render_process_host_observer.h"
 #include "content/public/browser/web_contents.h"
 #include "media/base/browser_cdm.h"
 #include "media/base/browser_cdm_factory.h"
@@ -105,20 +105,55 @@ void CdmPromiseInternal<std::string>::resolve(const std::string& session_id) {
 typedef CdmPromiseInternal<> SimplePromise;
 typedef CdmPromiseInternal<std::string> NewSessionPromise;
 
-}  // namespace
-
 // Render process ID to BrowserCdmManager map.
 typedef std::map<int, BrowserCdmManager*> BrowserCdmManagerMap;
 base::LazyInstance<BrowserCdmManagerMap>::Leaky g_browser_cdm_manager_map =
     LAZY_INSTANCE_INITIALIZER;
 
-BrowserCdmManager* BrowserCdmManager::FromProcess(int render_process_id) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+// Keeps the BrowserCdmManager alive, and in the global map, for as long as the
+// RenderProcessHost is connected to the child process. This class is a
+// self-owned observer.
+class BrowserCdmManagerProcessWatcher : public RenderProcessHostObserver {
+ public:
+  BrowserCdmManagerProcessWatcher(
+      int render_process_id,
+      const scoped_refptr<BrowserCdmManager>& manager)
+      : browser_cdm_manager_(manager) {
+    RenderProcessHost::FromID(render_process_id)->AddObserver(this);
+    CHECK(g_browser_cdm_manager_map.Get()
+              .insert(std::make_pair(render_process_id, manager.get()))
+              .second);

[ddorwin, 2015/07/09 17:52:22]

Is this checking that manager.get() is not NULL? Is there a reason not to do
that independent of the insert?

[xhwang, 2015/07/09 19:04:40]

This is to check that the insert succeeded, meaning that there wasn't an
existing entry in the map with |render_process_id|.

+  }
 
-  if (!g_browser_cdm_manager_map.Get().count(render_process_id))
-    return NULL;
+  // RenderProcessHostObserver:
+  void RenderProcessExited(RenderProcessHost* host,
+                           base::TerminationStatus /* status */,
+                           int /* exit_code */) override {
+    Destroy(host);

[ddorwin, 2015/07/09 17:52:22]

nit: This looks like it destroys |host|.

It's a bit strange that we need a parameter to destroy |this|. Should we have
saved it at line 122 then DCHECKed it here and had no parameter for Destroy()?

Alternatively, s/Destroy/RemoveHostOberverAndDestroy/ or something like that.

[xhwang, 2015/07/09 19:04:39]

Hmm, all HostObservers know which Host they are observing. But still we have
|host| in RenderProcessExited() and RenderProcessHostDestroyed(). I think this
is root of this strangeness.
Done.

[ncarter (slow), 2015/07/09 19:16:22]

It's structured this way to allow a RPHObserver instance to be an observer of
multiple RPH's at the same time.

+  }
+
+  void RenderProcessHostDestroyed(RenderProcessHost* host) override {
+    Destroy(host);
+  }
 
-  return g_browser_cdm_manager_map.Get()[render_process_id];
+ private:
+  void Destroy(RenderProcessHost* host) {
+    CHECK(g_browser_cdm_manager_map.Get().erase(host->GetID()));
+    host->RemoveObserver(this);
+    delete this;
+  }
+
+  const scoped_refptr<BrowserCdmManager> browser_cdm_manager_;
+};
+
+}  // namespace
+
+// static
+BrowserCdmManager* BrowserCdmManager::FromProcess(int render_process_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  auto& map = g_browser_cdm_manager_map.Get();
+  auto iterator = map.find(render_process_id);
+  return (iterator == map.end()) ? nullptr : iterator->second;
 }
 
 BrowserCdmManager::BrowserCdmManager(
@@ -131,23 +166,18 @@ BrowserCdmManager::BrowserCdmManager(
   DVLOG(1) << __FUNCTION__ << ": " << render_process_id_;
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 

[ddorwin, 2015/07/09 17:52:22]

Can/should we check that there is no entry for the render_process_id? Can there
be an existing entry? If so, you might comment that. I don't think so, though,
because that would break FromProcess().

[xhwang, 2015/07/09 19:04:39]

This is indirectly checked on line 123.
No. Basically you should never have two BrowserCdmManagerProcessWatcher alive
for the same process_id.
Ack.

+  new BrowserCdmManagerProcessWatcher(render_process_id, this);
+
   if (!task_runner_.get()) {
     task_runner_ =
         BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI);
   }
-
-  DCHECK(!g_browser_cdm_manager_map.Get().count(render_process_id_))
-      << render_process_id_;
-  g_browser_cdm_manager_map.Get()[render_process_id] = this;
 }
 
 BrowserCdmManager::~BrowserCdmManager() {
   DVLOG(1) << __FUNCTION__ << ": " << render_process_id_;
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  DCHECK(g_browser_cdm_manager_map.Get().count(render_process_id_));
-  DCHECK_EQ(this, g_browser_cdm_manager_map.Get()[render_process_id_]);
-
-  g_browser_cdm_manager_map.Get().erase(render_process_id_);
+  DCHECK(g_browser_cdm_manager_map.Get().count(render_process_id_) == 0);
 }
 
 // Makes sure BrowserCdmManager is always deleted on the Browser UI thread.