This patch improves the way that GuestViewManager tracks the destruction of GuestView embedders. A RenderProcessHostObserver is now used instead of a WebContentsObserver.

There were two main problems with the existing method that are now fixed:
1) The WebContentsObserver would only notify if an embedder crashed, and did nothingwhen it closed normally. (this led to the WebView in the chrome sign-in page not getting cleaned up properly when navigating away from that page)
2) The WebContentsObserver was created in GuestViewBase, so it would not even exist if a GuestView's embedder crashed before its GuestViewBase instance was created.

BUG=355360

Review URL: https://codereview.chromium.org/1232603002

Cr-Commit-Position: refs/heads/master@{#339658}

11 files changed, 136 insertions, 48 deletions

diff --git a/components/guest_view/browser/guest_view_base.cc b/components/guest_view/browser/guest_view_base.cc
index 599f8c0..ef65c87 100644
--- a/components/guest_view/browser/guest_view_base.cc
+++ b/components/guest_view/browser/guest_view_base.cc
@@ -76,11 +76,6 @@ class GuestViewBase::OwnerContentsObserver : public WebContentsObserver {
   void RenderProcessGone(base::TerminationStatus status) override {
     if (destroyed_)
       return;
-
-    GuestViewManager::FromBrowserContext(web_contents()->GetBrowserContext())
-        ->EmbedderWillBeDestroyed(
-            web_contents()->GetRenderProcessHost()->GetID());
-
     // If the embedder process is destroyed, then destroy the guest.
     Destroy();
   }
@@ -322,7 +317,9 @@ void GuestViewBase::SetSize(const SetSizeParams& params) {
 }
 
 // static
-void GuestViewBase::CleanUp(int embedder_process_id, int view_instance_id) {
+void GuestViewBase::CleanUp(content::BrowserContext* browser_context,
+                            int embedder_process_id,
+                            int view_instance_id) {
   // TODO(paulmeyer): Add in any general GuestView cleanup work here.
 }
 
diff --git a/components/guest_view/browser/guest_view_base.h b/components/guest_view/browser/guest_view_base.h
index 6a52980..2e7862d 100644
--- a/components/guest_view/browser/guest_view_base.h
+++ b/components/guest_view/browser/guest_view_base.h
@@ -65,7 +65,9 @@ class GuestViewBase : public content::BrowserPluginGuestDelegate,
   // potentially be created and destroyed in JavaScript before getting a
   // GuestViewBase instance. This method can be hidden by a CleanUp() method in
   // a derived class, in which case the derived method should call this one.
-  static void CleanUp(int embedder_process_id, int view_instance_id);
+  static void CleanUp(content::BrowserContext* browser_context,
+                      int embedder_process_id,
+                      int view_instance_id);
 
   static GuestViewBase* FromWebContents(
       const content::WebContents* web_contents);
diff --git a/components/guest_view/browser/guest_view_manager.cc b/components/guest_view/browser/guest_view_manager.cc
index 07da1ad..fa40a76 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_,
                                              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);
 }
diff --git a/components/guest_view/browser/guest_view_manager.h b/components/guest_view/browser/guest_view_manager.h
index 5364ede..87b1831 100644
--- a/components/guest_view/browser/guest_view_manager.h
+++ b/components/guest_view/browser/guest_view_manager.h
@@ -6,12 +6,14 @@
 #define COMPONENTS_GUEST_VIEW_BROWSER_GUEST_VIEW_MANAGER_H_
 
 #include <map>
+#include <set>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/gtest_prod_util.h"
 #include "base/lazy_instance.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "content/public/browser/browser_plugin_guest_manager.h"
 #include "content/public/browser/site_instance.h"
 #include "content/public/browser/web_contents.h"
@@ -132,6 +134,8 @@ class GuestViewManager : public content::BrowserPluginGuestManager,
   friend class GuestViewEvent;
   friend class GuestViewMessageFilter;
 
+  class EmbedderRenderProcessHostObserver;
+
   // These methods are virtual so that they can be overriden in tests.
 
   virtual void AddGuest(int guest_instance_id,
@@ -139,8 +143,8 @@ class GuestViewManager : public content::BrowserPluginGuestManager,
   virtual void RemoveGuest(int guest_instance_id);
 
   // This method is called when the embedder process with ID
-  // |embedder_process_id| is about to be destroyed.
-  virtual void EmbedderWillBeDestroyed(int embedder_process_id);
+  // |embedder_process_id| has been destroyed.
+  virtual void EmbedderProcessDestroyed(int embedder_process_id);
 
   // Called when a GuestView has been created in JavaScript.
   virtual void ViewCreated(int embedder_process_id,
@@ -151,6 +155,15 @@ class GuestViewManager : public content::BrowserPluginGuestManager,
   virtual void ViewGarbageCollected(int embedder_process_id,
                                     int view_instance_id);
 
+  // Calls all destruction callbacks registered for the GuestView identified by
+  // |embedder_process_id| and |view_instance_id|.
+  void CallViewDestructionCallbacks(int embedder_process_id,
+                                    int view_instance_id);
+
+  // Calls all destruction callbacks registered for GuestViews in the embedder
+  // with ID |embedder_process_id|.
+  void CallViewDestructionCallbacks(int embedder_process_id);
+
   // Creates a guest of the provided |view_type|.
   GuestViewBase* CreateGuestInternal(content::WebContents* owner_web_contents,
                                      const std::string& view_type);
@@ -215,7 +228,8 @@ class GuestViewManager : public content::BrowserPluginGuestManager,
 
   using GuestViewCreateFunction =
       base::Callback<GuestViewBase*(content::WebContents*)>;
-  using GuestViewCleanUpFunction = base::Callback<void(int, int)>;
+  using GuestViewCleanUpFunction =
+      base::Callback<void(content::BrowserContext*, int, int)>;
   struct GuestViewData {
     GuestViewData(const GuestViewCreateFunction& create_function,
                   const GuestViewCleanUpFunction& cleanup_function);
@@ -241,6 +255,9 @@ class GuestViewManager : public content::BrowserPluginGuestManager,
 
   scoped_ptr<GuestViewManagerDelegate> delegate_;
 
+  // This tracks which GuestView embedders are currently being observed.
+  std::set<int> embedders_observed_;
+
   // |view_destruction_callback_map_| maps from embedder process ID to view ID
   // to a vector of callback functions to be called when that view is destroyed.
   using Callbacks = std::vector<base::Closure> ;
@@ -248,6 +265,10 @@ class GuestViewManager : public content::BrowserPluginGuestManager,
   using CallbacksForEachEmbedderID = std::map<int, CallbacksForEachViewID> ;
   CallbacksForEachEmbedderID view_destruction_callback_map_;
 
+  // This is used to ensure that an EmbedderRenderProcessHostObserver will not
+  // call into this GuestViewManager after it has been destroyed.
+  base::WeakPtrFactory<GuestViewManager> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(GuestViewManager);
 };
 
diff --git a/components/guest_view/browser/test_guest_view_manager.cc b/components/guest_view/browser/test_guest_view_manager.cc
index c23399a..210e74f 100644
--- a/components/guest_view/browser/test_guest_view_manager.cc
+++ b/components/guest_view/browser/test_guest_view_manager.cc
@@ -113,9 +113,9 @@ void TestGuestViewManager::RemoveGuest(int guest_instance_id) {
   GuestViewManager::RemoveGuest(guest_instance_id);
 }
 
-void TestGuestViewManager::EmbedderWillBeDestroyed(int embedder_process_id) {
+void TestGuestViewManager::EmbedderProcessDestroyed(int embedder_process_id) {
   ++num_embedder_processes_destroyed_;
-  GuestViewManager::EmbedderWillBeDestroyed(embedder_process_id);
+  GuestViewManager::EmbedderProcessDestroyed(embedder_process_id);
 }
 
 void TestGuestViewManager::ViewGarbageCollected(int embedder_process_id,
diff --git a/components/guest_view/browser/test_guest_view_manager.h b/components/guest_view/browser/test_guest_view_manager.h
index 5367737..4f6d201 100644
--- a/components/guest_view/browser/test_guest_view_manager.h
+++ b/components/guest_view/browser/test_guest_view_manager.h
@@ -80,7 +80,7 @@ class TestGuestViewManager : public GuestViewManager {
   void AddGuest(int guest_instance_id,
                 content::WebContents* guest_web_contents) override;
   void RemoveGuest(int guest_instance_id) override;
-  void EmbedderWillBeDestroyed(int embedder_process_id) override;
+  void EmbedderProcessDestroyed(int embedder_process_id) override;
   void ViewGarbageCollected(int embedder_process_id,
                             int view_instance_id) override;
 
diff --git a/extensions/browser/api/declarative/rules_registry_service.cc b/extensions/browser/api/declarative/rules_registry_service.cc
index 939b01e..bca6fd5 100644
--- a/extensions/browser/api/declarative/rules_registry_service.cc
+++ b/extensions/browser/api/declarative/rules_registry_service.cc
@@ -137,6 +137,13 @@ RulesRegistryService* RulesRegistryService::Get(
   return BrowserContextKeyedAPIFactory<RulesRegistryService>::Get(context);
 }
 
+// static
+RulesRegistryService* RulesRegistryService::GetIfExists(
+    content::BrowserContext* context) {
+  return BrowserContextKeyedAPIFactory<RulesRegistryService>::GetIfExists(
+      context);
+}
+
 void RulesRegistryService::RegisterRulesRegistry(
     scoped_refptr<RulesRegistry> rule_registry) {
   const std::string event_name(rule_registry->event_name());
diff --git a/extensions/browser/api/declarative/rules_registry_service.h b/extensions/browser/api/declarative/rules_registry_service.h
index f03adba..8f16cb2 100644
--- a/extensions/browser/api/declarative/rules_registry_service.h
+++ b/extensions/browser/api/declarative/rules_registry_service.h
@@ -61,9 +61,15 @@ class RulesRegistryService : public BrowserContextKeyedAPI,
   static BrowserContextKeyedAPIFactory<RulesRegistryService>*
       GetFactoryInstance();
 
-  // Convenience method to get the RulesRegistryService for a context.
+  // Convenience method to get the RulesRegistryService for a context. If a
+  // RulesRegistryService does not already exist for |context|, one will be
+  // created and returned.
   static RulesRegistryService* Get(content::BrowserContext* context);
 
+  // The same as Get(), except that if a RulesRegistryService does not already
+  // exist for |context|, nullptr is returned.
+  static RulesRegistryService* GetIfExists(content::BrowserContext* context);
+
   int GetNextRulesRegistryID();
 
   // Registers the default RulesRegistries used in Chromium.
diff --git a/extensions/browser/browser_context_keyed_api_factory.h b/extensions/browser/browser_context_keyed_api_factory.h
index fc0add2..ad87e22 100644
--- a/extensions/browser/browser_context_keyed_api_factory.h
+++ b/extensions/browser/browser_context_keyed_api_factory.h
@@ -76,6 +76,11 @@ class BrowserContextKeyedAPIFactory : public BrowserContextKeyedServiceFactory {
         T::GetFactoryInstance()->GetServiceForBrowserContext(context, true));
   }
 
+  static T* GetIfExists(content::BrowserContext* context) {
+    return static_cast<T*>(
+        T::GetFactoryInstance()->GetServiceForBrowserContext(context, false));
+  }
+
   // Declare dependencies on other factories.
   // By default, ExtensionSystemFactory is the only dependency; however,
   // specializations can override this. Declare your specialization in
diff --git a/extensions/browser/guest_view/web_view/web_view_guest.cc b/extensions/browser/guest_view/web_view/web_view_guest.cc
index 63d2275..4ab87e2 100644
--- a/extensions/browser/guest_view/web_view/web_view_guest.cc
+++ b/extensions/browser/guest_view/web_view/web_view_guest.cc
@@ -198,20 +198,11 @@ static base::LazyInstance<WebViewKeyToIDMap> web_view_key_to_id_map =
 }  // namespace
 
 // static
-void WebViewGuest::CleanUp(int embedder_process_id, int view_instance_id) {
-  GuestViewBase::CleanUp(embedder_process_id, view_instance_id);
-
-  auto rph = content::RenderProcessHost::FromID(embedder_process_id);
-  // TODO(paulmeyer): It should be impossible for rph to be nullptr here, but
-  // this check is needed here for now as there seems to be occasional crashes
-  // because of this (http//crbug.com/499438). This should be removed once the
-  // cause is discovered and fixed.
-  DCHECK(rph != nullptr)
-      << "Cannot find RenderProcessHost for embedder process ID# "
-      << embedder_process_id;
-  if (rph == nullptr)
-    return;
-  auto browser_context = rph->GetBrowserContext();
+void WebViewGuest::CleanUp(content::BrowserContext* browser_context,
+                           int embedder_process_id,
+                           int view_instance_id) {
+  GuestViewBase::CleanUp(browser_context, embedder_process_id,
+                         view_instance_id);
 
   // Clean up rules registries for the WebView.
   WebViewKey key(embedder_process_id, view_instance_id);
@@ -219,8 +210,10 @@ void WebViewGuest::CleanUp(int embedder_process_id, int view_instance_id) {
   if (it != web_view_key_to_id_map.Get().end()) {
     auto rules_registry_id = it->second;
     web_view_key_to_id_map.Get().erase(it);
-    RulesRegistryService::Get(browser_context)
-        ->RemoveRulesRegistriesByID(rules_registry_id);
+    RulesRegistryService* rrs =
+        RulesRegistryService::GetIfExists(browser_context);
+    if (rrs)
+      rrs->RemoveRulesRegistriesByID(rules_registry_id);
   }
 
   // Clean up web request event listeners for the WebView.
diff --git a/extensions/browser/guest_view/web_view/web_view_guest.h b/extensions/browser/guest_view/web_view/web_view_guest.h
index 3014f92..d19ddf8 100644
--- a/extensions/browser/guest_view/web_view/web_view_guest.h
+++ b/extensions/browser/guest_view/web_view/web_view_guest.h
@@ -42,7 +42,9 @@ class WebViewGuest : public guest_view::GuestView<WebViewGuest>,
  public:
   // Clean up state when this GuestView is being destroyed. See
   // GuestViewBase::CleanUp().
-  static void CleanUp(int embedder_process_id, int view_instance_id);
+  static void CleanUp(content::BrowserContext* browser_context,
+                      int embedder_process_id,
+                      int view_instance_id);
 
   static GuestViewBase* Create(content::WebContents* owner_web_contents);