Remove the RenderProcessHost observer and attach the WebContentsObserver earlier to the GuestView.

This fixes an issue where if the embedder WebContents is destroyed during a GuestView creation we may end up leaking the guest WebContents.

BUG=419020

Committed: https://crrev.com/52fce9455ffd688ef3752a816192f9e76555e7d1
Cr-Commit-Position: refs/heads/master@{#298934}

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

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

extensions/browser/guest_view/guest_view_base.cc

@@ -57,16 +57,15 @@ scoped_ptr<base::DictionaryValue> GuestViewBase::Event::GetArguments() {
 
 // This observer ensures that the GuestViewBase destroys itself when its
 // embedder goes away.
-class GuestViewBase::EmbedderWebContentsObserver : public WebContentsObserver {
+class GuestViewBase::EmbedderLifetimeObserver : public WebContentsObserver {
  public:
-  explicit EmbedderWebContentsObserver(GuestViewBase* guest)
-      : WebContentsObserver(guest->embedder_web_contents()),
+  EmbedderLifetimeObserver(GuestViewBase* guest,
+                           content::WebContents* embedder_web_contents)
+      : WebContentsObserver(embedder_web_contents),
         destroyed_(false),
-        guest_(guest) {
-  }
+        guest_(guest) {}
 
-  virtual ~EmbedderWebContentsObserver() {
-  }
+  virtual ~EmbedderLifetimeObserver() {}
 
   // WebContentsObserver implementation.
   virtual void WebContentsDestroyed() override {
@@ -98,7 +97,7 @@ class GuestViewBase::EmbedderWebContentsObserver : public WebContentsObserver {
     guest_->Destroy();
   }
 
-  DISALLOW_COPY_AND_ASSIGN(EmbedderWebContentsObserver);
+  DISALLOW_COPY_AND_ASSIGN(EmbedderLifetimeObserver);
 };
 
 GuestViewBase::GuestViewBase(content::BrowserContext* browser_context,
@@ -159,21 +158,29 @@ void GuestViewBase::Init(const std::string& embedder_extension_id,
                     base::Bind(&GuestViewBase::CompleteInit,
                                weak_ptr_factory_.GetWeakPtr(),
                                embedder_extension_id,
-                               embedder_process_id,
+                               embedder_web_contents,
                                callback));
 }
 
 void GuestViewBase::InitWithWebContents(
     const std::string& embedder_extension_id,
-    int embedder_render_process_id,
+    content::WebContents* embedder_web_contents,
     content::WebContents* guest_web_contents) {
   DCHECK(guest_web_contents);
+  DCHECK(embedder_web_contents);
+  int embedder_render_process_id =
+      embedder_web_contents->GetRenderProcessHost()->GetID();
   content::RenderProcessHost* embedder_render_process_host =
       content::RenderProcessHost::FromID(embedder_render_process_id);
 
   embedder_extension_id_ = embedder_extension_id;
   embedder_render_process_id_ = embedder_render_process_host->GetID();
-  embedder_render_process_host->AddObserver(this);
+
+  // At this point, we have just created the guest WebContents, we need to add
+  // an observer to the embedder WebContents. This observer will be responsible
+  // for destroying the guest WebContents if the embedder goes away.
+  embedder_web_contents_observer_.reset(
+      new EmbedderLifetimeObserver(this, embedder_web_contents));
 
   WebContentsObserver::Observe(guest_web_contents);
   guest_web_contents->SetDelegate(this);
@@ -278,23 +285,6 @@ bool GuestViewBase::IsDragAndDropEnabled() const {
   return false;
 }
 
-void GuestViewBase::RenderProcessExited(content::RenderProcessHost* host,
-                                        base::ProcessHandle handle,
-                                        base::TerminationStatus status,
-                                        int exit_code) {
-  // GuestViewBase tracks the lifetime of its embedder render process until it
-  // is attached to a particular embedder WebContents. At that point, its
-  // lifetime is restricted in scope to the lifetime of its embedder
-  // WebContents.
-  CHECK(!attached());
-  CHECK_EQ(host->GetID(), embedder_render_process_id());
-
-  // This code path may be reached if the embedder WebContents is killed for
-  // whatever reason immediately after a called to GuestViewInternal.createGuest
-  // and before attaching the new guest to a frame.
-  Destroy();
-}
-
 void GuestViewBase::DidAttach(int guest_proxy_routing_id) {
   // Give the derived class an opportunity to perform some actions.
   DidAttachToEmbedder();
@@ -324,11 +314,6 @@ void GuestViewBase::GuestSizeChanged(const gfx::Size& old_size,
 void GuestViewBase::Destroy() {
   DCHECK(web_contents());
 
-  content::RenderProcessHost* host =
-      content::RenderProcessHost::FromID(embedder_render_process_id());
-  if (host)
-    host->RemoveObserver(this);
-
   // Give the derived class an opportunity to perform some cleanup.
   WillDestroy();
 
@@ -370,13 +355,16 @@ void GuestViewBase::RegisterDestructionCallback(
 
 void GuestViewBase::WillAttach(content::WebContents* embedder_web_contents,
                                int element_instance_id) {
-  // After attachment, this GuestViewBase's lifetime is restricted to the
-  // lifetime of its embedder WebContents. Observing the RenderProcessHost
-  // of the embedder is no longer necessary.
-  embedder_web_contents->GetRenderProcessHost()->RemoveObserver(this);
   embedder_web_contents_ = embedder_web_contents;
-  embedder_web_contents_observer_.reset(
-      new EmbedderWebContentsObserver(this));
+
+  // If we are attaching to a different WebContents than the one that created
+  // the guest, we need to create a new LifetimeObserver.
+  if (embedder_web_contents !=
+      embedder_web_contents_observer_->web_contents()) {
+    embedder_web_contents_observer_.reset(
+        new EmbedderLifetimeObserver(this, embedder_web_contents));
+  }
+
   element_instance_id_ = element_instance_id;
 
   WillAttachToEmbedder();
@@ -488,7 +476,7 @@ void GuestViewBase::SendQueuedEvents() {
 }
 
 void GuestViewBase::CompleteInit(const std::string& embedder_extension_id,
-                                 int embedder_render_process_id,
+                                 content::WebContents* embedder_web_contents,
                                  const WebContentsCreatedCallback& callback,
                                  content::WebContents* guest_web_contents) {
   if (!guest_web_contents) {
@@ -498,9 +486,8 @@ void GuestViewBase::CompleteInit(const std::string& embedder_extension_id,
     callback.Run(NULL);
     return;
   }
-  InitWithWebContents(embedder_extension_id,
-                      embedder_render_process_id,
-                      guest_web_contents);
+  InitWithWebContents(
+      embedder_extension_id, embedder_web_contents, guest_web_contents);
   callback.Run(guest_web_contents);
 }
 

extensions/browser/guest_view/guest_view_base.h

@@ -27,7 +27,6 @@ namespace extensions {
 // WebContents. At that point, its lifetime is restricted in scope to the
 // lifetime of its embedder WebContents.
 class GuestViewBase : public content::BrowserPluginGuestDelegate,
-                      public content::RenderProcessHostObserver,
                       public content::WebContentsDelegate,
                       public content::WebContentsObserver {
  public:
@@ -171,10 +170,9 @@ class GuestViewBase : public content::BrowserPluginGuestDelegate,
             const base::DictionaryValue& create_params,
             const WebContentsCreatedCallback& callback);
 
-  void InitWithWebContents(
-      const std::string& embedder_extension_id,
-      int embedder_render_process_id,
-      content::WebContents* guest_web_contents);
+  void InitWithWebContents(const std::string& embedder_extension_id,
+                           content::WebContents* embedder_web_contents,
+                           content::WebContents* guest_web_contents);
 
   bool IsViewType(const char* const view_type) const {
     return !strcmp(GetViewType(), view_type);
@@ -229,12 +227,6 @@ class GuestViewBase : public content::BrowserPluginGuestDelegate,
   void SetAttachParams(const base::DictionaryValue& params);
   void SetOpener(GuestViewBase* opener);
 
-  // RenderProcessHostObserver implementation
-  virtual void RenderProcessExited(content::RenderProcessHost* host,
-                                   base::ProcessHandle handle,
-                                   base::TerminationStatus status,
-                                   int exit_code) override;
-
   // BrowserPluginGuestDelegate implementation.
   virtual void DidAttach(int guest_proxy_routing_id) override final;
   virtual void ElementSizeChanged(const gfx::Size& old_size,
@@ -257,12 +249,12 @@ class GuestViewBase : public content::BrowserPluginGuestDelegate,
   virtual ~GuestViewBase();
 
  private:
-  class EmbedderWebContentsObserver;
+  class EmbedderLifetimeObserver;
 
   void SendQueuedEvents();
 
   void CompleteInit(const std::string& embedder_extension_id,
-                    int embedder_render_process_id,
+                    content::WebContents* embedder_web_contents,
                     const WebContentsCreatedCallback& callback,
                     content::WebContents* guest_web_contents);
 
@@ -320,7 +312,7 @@ class GuestViewBase : public content::BrowserPluginGuestDelegate,
   // guests that are created from this guest.
   scoped_ptr<base::DictionaryValue> attach_params_;
 
-  scoped_ptr<EmbedderWebContentsObserver> embedder_web_contents_observer_;
+  scoped_ptr<EmbedderLifetimeObserver> embedder_web_contents_observer_;
 
   // The size of the container element.
   gfx::Size element_size_;

extensions/browser/guest_view/guest_view_manager.cc

@@ -136,7 +136,7 @@ void GuestViewManager::CreateGuest(const std::string& view_type,
 content::WebContents* GuestViewManager::CreateGuestWithWebContentsParams(
     const std::string& view_type,
     const std::string& embedder_extension_id,
-    int embedder_render_process_id,
+    content::WebContents* embedder_web_contents,
     const content::WebContents::CreateParams& create_params) {
   int guest_instance_id = GetNextInstanceID();
   GuestViewBase* guest =
@@ -147,9 +147,8 @@ content::WebContents* GuestViewManager::CreateGuestWithWebContentsParams(
   guest_create_params.guest_delegate = guest;
   content::WebContents* guest_web_contents =
       WebContents::Create(guest_create_params);
-  guest->InitWithWebContents(embedder_extension_id,
-                             embedder_render_process_id,
-                             guest_web_contents);
+  guest->InitWithWebContents(
+      embedder_extension_id, embedder_web_contents, guest_web_contents);
   return guest_web_contents;
 }
 

extensions/browser/guest_view/guest_view_manager.h

@@ -72,7 +72,7 @@ class GuestViewManager : public content::BrowserPluginGuestManager,
   content::WebContents* CreateGuestWithWebContentsParams(
       const std::string& view_type,
       const std::string& embedder_extension_id,
-      int embedder_render_process_id,
+      content::WebContents* embedder_web_contents,
       const content::WebContents::CreateParams& create_params);
 
   content::SiteInstance* GetGuestSiteInstance(

extensions/browser/guest_view/web_view/web_view_guest.cc

@@ -839,7 +839,7 @@ content::WebContents* WebViewGuest::CreateNewGuestWindow(
   return guest_manager->CreateGuestWithWebContentsParams(
       WebViewGuest::Type,
       embedder_extension_id(),
-      embedder_web_contents()->GetRenderProcessHost()->GetID(),
+      embedder_web_contents(),
       create_params);
 }