Fix RenderFrameCreated and RenderFrameDeleted WebContentsObserver met…

…hods

The RenderFrameCreated and RenderFrameDeleted observer methods aren't
matching the lifetime of the renderer-side RenderFrame object. This CL
fixes the methods to be called at the right times to match.

BUG=450799, 425397

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

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

chrome/browser/background/background_contents.cc

@@ -41,6 +41,7 @@ BackgroundContents::BackgroundContents(
   WebContents::CreateParams create_params(profile_, site_instance);
   create_params.routing_id = routing_id;
   create_params.main_frame_routing_id = main_frame_routing_id;
+  create_params.renderer_initiated_creation = true;
   if (session_storage_namespace) {
     content::SessionStorageNamespaceMap session_storage_namespace_map;
     session_storage_namespace_map.insert(

chrome/browser/geolocation/geolocation_permission_context_unittest.cc

@@ -220,8 +220,7 @@ void GeolocationPermissionContextTests::CheckPermissionMessageSentInternal(
 }
 
 void GeolocationPermissionContextTests::AddNewTab(const GURL& url) {
-  content::WebContents* new_tab = content::WebContents::Create(
-      content::WebContents::CreateParams(profile()));
+  content::WebContents* new_tab = CreateTestWebContents();
   new_tab->GetController().LoadURL(
       url, content::Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
   content::RenderFrameHostTester::For(new_tab->GetMainFrame())

chrome/browser/ui/autofill/autofill_popup_controller_interactive_uitest.cc

@@ -13,6 +13,7 @@
 #include "components/autofill/content/browser/content_autofill_driver_factory.h"
 #include "components/autofill/core/browser/autofill_manager.h"
 #include "components/autofill/core/browser/test_autofill_external_delegate.h"
+#include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/test/test_utils.h"
@@ -42,8 +43,6 @@ class TestAutofillExternalDelegate : public AutofillExternalDelegate {
 
     if (message_loop_runner_.get())
       message_loop_runner_->Quit();
-
-    AutofillExternalDelegate::OnPopupHidden();
   }
 
   void WaitForPopupHidden() {
@@ -90,7 +89,10 @@ class AutofillPopupControllerBrowserTest
 
   // Normally the WebContents will automatically delete the delegate, but here
   // the delegate is owned by this test, so we have to manually destroy.
-  void WebContentsDestroyed() override { autofill_external_delegate_.reset(); }
+  void RenderFrameDeleted(content::RenderFrameHost* rfh) override {
+    if (!rfh->GetParent())
+      autofill_external_delegate_.reset();
+  }
 
  protected:
   scoped_ptr<TestAutofillExternalDelegate> autofill_external_delegate_;

content/browser/frame_host/frame_tree_unittest.cc

@@ -69,9 +69,13 @@ class TreeWalkingWebContentsLogger : public WebContentsObserver {
 
   void RenderFrameHostChanged(RenderFrameHost* old_host,
                               RenderFrameHost* new_host) override {
+    // TODO(nasko): Re-enable this logging once RenderFrameHostChanged observer
+    // methods are fixed. See https://crbug.com/450799.
+    /*
     if (old_host)
       LogWhatHappened("RenderFrameChanged(old)", old_host);
     LogWhatHappened("RenderFrameChanged(new)", new_host);
+    */
   }
 
   void RenderFrameDeleted(RenderFrameHost* render_frame_host) override {
@@ -229,11 +233,13 @@ TEST_F(FrameTreeTest, ObserverWalksTreeAfterCrash) {
   EXPECT_EQ("RenderFrameCreated(23) -> 1: [22: [], 23: []]", activity.GetLog());
 
   // Crash the renderer
-  main_rfh()->OnMessageReceived(FrameHostMsg_RenderProcessGone(
-      0, base::TERMINATION_STATUS_PROCESS_CRASHED, -1));
+  main_test_rfh()->OnMessageReceived(FrameHostMsg_RenderProcessGone(
+      main_test_rfh()->GetRoutingID(), base::TERMINATION_STATUS_PROCESS_CRASHED,
+      -1));
   EXPECT_EQ(
       "RenderFrameDeleted(22) -> 1: []\n"
       "RenderFrameDeleted(23) -> 1: []\n"
+      "RenderFrameDeleted(1) -> 1: []\n"
       "RenderProcessGone -> 1: []",
       activity.GetLog());
 }

content/browser/frame_host/render_frame_host_impl.cc

@@ -184,7 +184,7 @@ RenderFrameHostImpl::~RenderFrameHostImpl() {
   g_routing_id_frame_map.Get().erase(
       RenderFrameHostID(GetProcess()->GetID(), routing_id_));
 
-  if (delegate_)
+  if (delegate_ && render_frame_created_)
     delegate_->RenderFrameDeleted(this);
 
   FrameAccessibility::GetInstance()->OnRenderFrameHostDestroyed(this);
@@ -619,6 +619,15 @@ bool RenderFrameHostImpl::IsRenderFrameLive() {
 }
 
 void RenderFrameHostImpl::SetRenderFrameCreated(bool created) {
+  // If the current status is different than the new status, the delegate
+  // needs to be notified.
+  if (delegate_ && (created != render_frame_created_)) {
+    if (created)
+      delegate_->RenderFrameCreated(this);
+    else
+      delegate_->RenderFrameDeleted(this);
+  }
+
   render_frame_created_ = created;
   if (created && render_widget_host_)
     render_widget_host_->InitForFrame();
@@ -669,14 +678,11 @@ void RenderFrameHostImpl::OnCreateChildFrame(int new_routing_id,
   if (!new_frame)
     return;
 
+  new_frame->frame_tree_node()->set_sandbox_flags(sandbox_flags);
+
   // We know that the RenderFrame has been created in this case, immediately
   // after the CreateChildFrame IPC was sent.
   new_frame->SetRenderFrameCreated(true);
-
-  new_frame->frame_tree_node()->set_sandbox_flags(sandbox_flags);
-
-  if (delegate_)
-    delegate_->RenderFrameCreated(new_frame);
 }
 
 void RenderFrameHostImpl::OnDetach() {
@@ -1049,9 +1055,6 @@ void RenderFrameHostImpl::OnRenderProcessGone(int status, int exit_code) {
         static_cast<base::TerminationStatus>(status);
   }
 
-  SetRenderFrameCreated(false);
-  InvalidateMojoConnection();
-
   // Reset frame tree state associated with this process.  This must happen
   // before RenderViewTerminated because observers expect the subframes of any
   // affected frames to be cleared first.
@@ -1061,6 +1064,11 @@ void RenderFrameHostImpl::OnRenderProcessGone(int status, int exit_code) {
   if (!is_swapped_out())
     frame_tree_node_->ResetForNewProcess();
 
+  // Reset state for the current RenderFrameHost once the FrameTreeNode has been
+  // reset.
+  SetRenderFrameCreated(false);
+  InvalidateMojoConnection();
+
   if (frame_tree_node_->IsMainFrame()) {
     // RenderViewHost/RenderWidgetHost needs to reset some stuff.
     render_view_host_->RendererExited(

content/browser/frame_host/render_frame_host_manager.cc

@@ -87,6 +87,10 @@ RenderFrameHostManager::~RenderFrameHostManager() {
   // the current RenderFrameHost and uses it during its destructor.
   STLDeleteValues(&proxy_hosts_);
 
+  // Release the WebUI prior to resetting the current RenderFrameHost, as the
+  // WebUI accesses the RenderFrameHost during cleanup.
+  web_ui_.reset();
+
   // We should always have a current RenderFrameHost except in some tests.
   SetRenderFrameHost(scoped_ptr<RenderFrameHostImpl>());
 }
@@ -1411,13 +1415,6 @@ scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrame(
     if (success) {
       if (view_routing_id_ptr)
         *view_routing_id_ptr = render_view_host->GetRoutingID();
-
-      // A brand new RenderFrame was created by one of the Init calls above.
-      // Announce it to observers.
-      if (swapped_out)
-        render_frame_delegate_->RenderFrameCreated(proxy->render_frame_host());
-      else
-        render_frame_delegate_->RenderFrameCreated(new_render_frame_host.get());
     }
   }
 

content/browser/manifest/manifest_manager_host.cc

@@ -4,6 +4,7 @@
 
 #include "content/browser/manifest/manifest_manager_host.h"
 
+#include "base/stl_util.h"
 #include "content/common/manifest_manager_messages.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
@@ -30,6 +31,7 @@ ManifestManagerHost::ManifestManagerHost(WebContents* web_contents)
 }
 
 ManifestManagerHost::~ManifestManagerHost() {
+  STLDeleteValues(&pending_callbacks_);
 }
 
 ManifestManagerHost::CallbackMap* ManifestManagerHost::GetCallbackMapForFrame(

content/browser/renderer_host/render_view_host_impl.cc

@@ -317,6 +317,11 @@ bool RenderViewHostImpl::CreateRenderView(
   // Let our delegate know that we created a RenderView.
   delegate_->RenderViewCreated(this);
 
+  // Since this method creates the main RenderFrame in the renderer process,
+  // set the proper state on its corresponding RenderFrameHost.
+  RenderFrameHostImpl::FromID(GetProcess()->GetID(), main_frame_routing_id_)
+      ->SetRenderFrameCreated(true);
+
   return true;
 }
 

content/browser/web_contents/web_contents_impl.cc

@@ -406,14 +406,10 @@ WebContentsImpl::~WebContentsImpl() {
 
   // Manually call the observer methods for the root frame tree node.
   RenderFrameHostManager* root = GetRenderManager();
-  if (root->pending_frame_host()) {
-    FOR_EACH_OBSERVER(WebContentsObserver,
-                      observers_,
-                      RenderFrameDeleted(root->pending_frame_host()));
-  }
-  FOR_EACH_OBSERVER(WebContentsObserver,
-                    observers_,
-                    RenderFrameDeleted(root->current_frame_host()));
+
+  if (root->pending_frame_host())
+    root->pending_frame_host()->SetRenderFrameCreated(false);
+  root->current_frame_host()->SetRenderFrameCreated(false);
 
   if (root->pending_render_view_host()) {
     FOR_EACH_OBSERVER(WebContentsObserver,
@@ -1265,6 +1261,14 @@ void WebContentsImpl::Init(const WebContents::CreateParams& params) {
 
   for (size_t i = 0; i < g_created_callbacks.Get().size(); i++)
     g_created_callbacks.Get().at(i).Run(this);
+
+  // If the WebContents creation was renderer-initiated, it means that the
+  // corresponding RenderView and main RenderFrame have already been created.
+  // Ensure observers are notified about this.
+  if (params.renderer_initiated_creation) {
+    RenderViewCreated(GetRenderViewHost());
+    GetRenderManager()->current_frame_host()->SetRenderFrameCreated(true);
+  }
 }
 
 void WebContentsImpl::OnWebContentsDestroyed(WebContentsImpl* web_contents) {
@@ -1594,6 +1598,7 @@ void WebContentsImpl::CreateNewWindow(
   create_params.opener_suppressed = params.opener_suppressed;
   if (params.disposition == NEW_BACKGROUND_TAB)
     create_params.initially_hidden = true;
+  create_params.renderer_initiated_creation = true;
 
   WebContentsImpl* new_contents = NULL;
   if (!is_guest) {
@@ -1607,7 +1612,6 @@ void WebContentsImpl::CreateNewWindow(
   new_contents->GetController().SetSessionStorageNamespace(
       partition_id,
       session_storage_namespace);
-  new_contents->RenderViewCreated(new_contents->GetRenderViewHost());
 
   // Save the window for later if we're not suppressing the opener (since it
   // will be shown immediately).
@@ -3663,14 +3667,6 @@ void WebContentsImpl::RenderViewCreated(RenderViewHost* render_view_host) {
   FOR_EACH_OBSERVER(
       WebContentsObserver, observers_, RenderViewCreated(render_view_host));
 
-  // We tell the observers now instead of when the main RenderFrameHostImpl is
-  // constructed because otherwise it would be too early (i.e. IPCs sent to the
-  // frame would be dropped because it's not created yet).
-  RenderFrameHost* main_frame = render_view_host->GetMainFrame();
-  FOR_EACH_OBSERVER(
-      WebContentsObserver, observers_, RenderFrameCreated(main_frame));
-  SetAccessibilityModeOnFrame(accessibility_mode_, main_frame);
-
   DevToolsManager::GetInstance()->RenderViewCreated(this, render_view_host);
 }
 
@@ -3704,10 +3700,6 @@ void WebContentsImpl::RenderViewReady(RenderViewHost* rvh) {
 void WebContentsImpl::RenderViewTerminated(RenderViewHost* rvh,
                                            base::TerminationStatus status,
                                            int error_code) {
-  // TODO(nasko): This isn't ideal; the termination process should be handled by
-  // RenderFrameDeleted().  See http://crbug.com/455943.
-  ClearPowerSaveBlockers(rvh->GetMainFrame());
-
   if (rvh != GetRenderViewHost()) {
     // The pending page's RenderViewHost is gone.
     return;

content/browser/web_contents/web_contents_impl_unittest.cc

@@ -520,6 +520,7 @@ TEST_F(WebContentsImplTest, CrossSiteBoundaries) {
   }
 
   // DidNavigate from the pending page
+  pending_rfh->PrepareForCommit(url2);
   contents()->TestDidNavigate(
       pending_rfh, 1, url2, ui::PAGE_TRANSITION_TYPED);
   SiteInstance* instance2 = contents()->GetSiteInstance();

content/public/browser/web_contents.cc

@@ -17,7 +17,8 @@ WebContents::CreateParams::CreateParams(BrowserContext* context)
       main_frame_routing_id(MSG_ROUTING_NONE),
       initially_hidden(false),
       guest_delegate(nullptr),
-      context(nullptr) {}
+      context(nullptr),
+      renderer_initiated_creation(false) {}
 
 WebContents::CreateParams::CreateParams(
     BrowserContext* context, SiteInstance* site)
@@ -29,7 +30,8 @@ WebContents::CreateParams::CreateParams(
       main_frame_routing_id(MSG_ROUTING_NONE),
       initially_hidden(false),
       guest_delegate(nullptr),
-      context(nullptr) {}
+      context(nullptr),
+      renderer_initiated_creation(false) {}
 
 WebContents::CreateParams::~CreateParams() {
 }

content/public/browser/web_contents.h

@@ -122,6 +122,12 @@ class WebContents : public PageNavigator,
     // Used to specify the location context which display the new view should
     // belong. This can be nullptr if not needed.
     gfx::NativeView context;
+
+    // Used to specify that the new WebContents creation is driven by the
+    // renderer process. In this case, the renderer-side objects, such as
+    // RenderFrame, have already been created on the renderer side, and
+    // WebContents construction should take this into account.
+    bool renderer_initiated_creation;
   };
 
   // Creates a new WebContents.