This patch improves the way that GuestViewManager tracks the destruction of GuestView embedders.

components/guest_view/browser/guest_view_manager.cc

Index: components/guest_view/browser/guest_view_manager.cc
diff --git a/components/guest_view/browser/guest_view_manager.cc b/components/guest_view/browser/guest_view_manager.cc
index 07da1adf11f022be2079599f915fbe9e41e7c06c..6d921bcb23d0e944a4e783c1c90fda9e106414ab 100644
--- a/components/guest_view/browser/guest_view_manager.cc
+++ b/components/guest_view/browser/guest_view_manager.cc
@@ -22,11 +22,42 @@
 #include "url/gurl.h"
 
 using content::BrowserContext;
+using content::RenderProcessHost;
 using content::SiteInstance;
 using content::WebContents;
 
 namespace guest_view {
 
+// This observer observes the RenderProcessHosts of GuestView embedders, and
+// notifies the GuestViewManager when they are destroyed.
+class GuestViewManager::EmbedderRenderProcessHostObserver
+    : public content::RenderProcessHostObserver {
+ public:
+  EmbedderRenderProcessHostObserver(
+      base::WeakPtr<GuestViewManager> guest_view_manager,
+      int embedder_process_id)
+      : guest_view_manager_(guest_view_manager), id_(embedder_process_id) {
+    RenderProcessHost* rph = RenderProcessHost::FromID(id_);
+    rph->AddObserver(this);
+  }
+
+  ~EmbedderRenderProcessHostObserver() override {
+    RenderProcessHost* rph = RenderProcessHost::FromID(id_);
+    if (rph)
+      rph->RemoveObserver(this);
+  }
+
+  void RenderProcessHostDestroyed(RenderProcessHost* host) override {
+    if (guest_view_manager_.get())
+      guest_view_manager_->EmbedderProcessDestroyed(id_);
+    delete this;
+  }
+
+ private:
+  base::WeakPtr<GuestViewManager> guest_view_manager_;
+  int id_;
+};
+
 // static
 GuestViewManagerFactory* GuestViewManager::factory_ = nullptr;
 
@@ -36,7 +67,8 @@ GuestViewManager::GuestViewManager(
     : current_instance_id_(0),
       last_instance_id_removed_(0),
       context_(context),
-      delegate_(delegate.Pass()) {
+      delegate_(delegate.Pass()),
+      weak_ptr_factory_(this) {
 }
 
 GuestViewManager::~GuestViewManager() {}
@@ -245,20 +277,9 @@ void GuestViewManager::RemoveGuest(int guest_instance_id) {
   }
 }
 
-void GuestViewManager::EmbedderWillBeDestroyed(int embedder_process_id) {
-  // Find and call any callbacks associated with the embedder that is being
-  // destroyed.
-  auto embedder_it = view_destruction_callback_map_.find(embedder_process_id);
-  if (embedder_it == view_destruction_callback_map_.end())
-    return;
-  CallbacksForEachViewID& callbacks_for_embedder = embedder_it->second;
-  for (auto& view_pair : callbacks_for_embedder) {
-    Callbacks& callbacks_for_view = view_pair.second;
-    for (auto& callback : callbacks_for_view) {
-      callback.Run();
-    }
-  }
-  view_destruction_callback_map_.erase(embedder_it);
+void GuestViewManager::EmbedderProcessDestroyed(int embedder_process_id) {
+  embedders_observed_.erase(embedder_process_id);
+  CallViewDestructionCallbacks(embedder_process_id);
 }
 
 void GuestViewManager::ViewCreated(int embedder_process_id,
@@ -274,18 +295,39 @@ void GuestViewManager::ViewCreated(int embedder_process_id,
   RegisterViewDestructionCallback(embedder_process_id,
                                   view_instance_id,
                                   base::Bind(view_it->second.cleanup_function,
+                                             context_,

[Fady Samuel, 2015/07/14 17:56:59]

Why does the cleanup function need the browser context?

[paulmeyer, 2015/07/14 18:04:07]

It always needed it to clean up some of the browser-side state. It used to rely
on getting it via the RenderProcessHost, but I don't think that is actually
always safe since cleanup can be triggered by the destruction of the
RenderProcessHost. So, I'm now just passing it in instead.

                                              embedder_process_id,
                                              view_instance_id));
 }
 
 void GuestViewManager::ViewGarbageCollected(int embedder_process_id,
                                             int view_instance_id) {
-  // Find and call any callbacks associated with the view that has been garbage
-  // collected.
+  CallViewDestructionCallbacks(embedder_process_id, view_instance_id);
+}
+
+void GuestViewManager::CallViewDestructionCallbacks(int embedder_process_id,
+                                                    int view_instance_id) {
+  // Find the callbacks for the embedder with ID |embedder_process_id|.
   auto embedder_it = view_destruction_callback_map_.find(embedder_process_id);
   if (embedder_it == view_destruction_callback_map_.end())
     return;
   CallbacksForEachViewID& callbacks_for_embedder = embedder_it->second;
+
+  // If |view_instance_id| is guest_view::kInstanceIDNone, then all callbacks
+  // for this embedder should be called.
+  if (view_instance_id == guest_view::kInstanceIDNone) {
+    // Call all callbacks for the embedder with ID |embedder_process_id|.
+    for (auto& view_pair : callbacks_for_embedder) {
+      Callbacks& callbacks_for_view = view_pair.second;
+      for (auto& callback : callbacks_for_view)
+        callback.Run();
+    }
+    view_destruction_callback_map_.erase(embedder_it);
+    return;
+  }
+
+  // Otherwise, call the callbacks only for the specific view with ID
+  // |view_instance_id|.
   auto view_it = callbacks_for_embedder.find(view_instance_id);
   if (view_it == callbacks_for_embedder.end())
     return;
@@ -295,6 +337,11 @@ void GuestViewManager::ViewGarbageCollected(int embedder_process_id,
   callbacks_for_embedder.erase(view_it);
 }
 
+void GuestViewManager::CallViewDestructionCallbacks(int embedder_process_id) {
+  CallViewDestructionCallbacks(embedder_process_id,
+                               guest_view::kInstanceIDNone);
+}
+
 GuestViewBase* GuestViewManager::CreateGuestInternal(
     content::WebContents* owner_web_contents,
     const std::string& view_type) {
@@ -318,6 +365,14 @@ void GuestViewManager::RegisterViewDestructionCallback(
     int embedder_process_id,
     int view_instance_id,
     const base::Closure& callback) {
+  // When an embedder is registered for the first time, create an observer to
+  // watch for its destruction.
+  if (!embedders_observed_.count(embedder_process_id)) {
+    embedders_observed_.insert(embedder_process_id);
+    new EmbedderRenderProcessHostObserver(weak_ptr_factory_.GetWeakPtr(),
+                                          embedder_process_id);
+  }
+
   view_destruction_callback_map_[embedder_process_id][view_instance_id]
       .push_back(callback);
 }