Fix RenderFrameCreated and RenderFrameDeleted WebContentsObserver methods

content/browser/web_contents/web_contents_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/web_contents/web_contents_impl.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/lazy_instance.h"
@@ skipping 390 matching lines @@
       NOTIFICATION_WEB_CONTENTS_DESTROYED,
       Source<WebContents>(this),
       NotificationService::NoDetails());
 
   // Destroy all frame tree nodes except for the root; this notifies observers.
   frame_tree_.ResetForMainFrameSwap();
   GetRenderManager()->ResetProxyHosts();
 
   // 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,
                       observers_,
                       RenderViewDeleted(root->pending_render_view_host()));
   }
 
   FOR_EACH_OBSERVER(WebContentsObserver,
                     observers_,
                     RenderViewDeleted(root->current_host()));
@@ skipping 69 matching lines @@
                                         const IPC::Message& message) {
   DCHECK(render_view_host || render_frame_host);
   if (GetWebUI() &&
       static_cast<WebUIImpl*>(GetWebUI())->OnMessageReceived(message)) {
     return true;
   }
 
   ObserverListBase<WebContentsObserver>::Iterator it(observers_);
   WebContentsObserver* observer;
   if (render_frame_host) {
-    while ((observer = it.GetNext()) != NULL)
-      if (observer->OnMessageReceived(message, render_frame_host))
-        return true;
+    if (!static_cast<RenderFrameHostImpl*>(render_frame_host)
+             ->is_swapped_out()) {

[Charlie Reis, 2015/02/12 00:29:31]

I'm worried about regressions from this change.  There's a bunch of IPCs in
CanHandleWhileSwappedOut that might get broken due to this.

What made this necessary?  I'm wondering if we can get by without it until we
kill swappedout://.

[nasko, 2015/02/12 17:52:26]

Hmm, don't quite recall, but after all the followup changes, it isn't needed
anymore.

+      while ((observer = it.GetNext()) != NULL)
+        if (observer->OnMessageReceived(message, render_frame_host))
+          return true;
+    }
   } else {
     while ((observer = it.GetNext()) != NULL)
       if (observer->OnMessageReceived(message))
         return true;
   }
 
   // Message handlers should be aware of which
   // RenderViewHost/RenderFrameHost sent the message, which is temporarily
   // stored in render_(view|frame)_message_source_.
   if (render_frame_host)
@@ skipping 738 matching lines @@
   date_time_chooser_.reset(new DateTimeChooserAndroid());
 #endif
 
   // BrowserPluginGuest::Init needs to be called after this WebContents has
   // a RenderWidgetHostViewGuest. That is, |view_->CreateView| above.
   if (browser_plugin_guest_)
     browser_plugin_guest_->Init();
 
   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.

[Charlie Reis, 2015/02/12 00:29:31]

Wow.  There are so many ways to create RenderFrames!!

[nasko, 2015/02/12 17:52:26]

Acknowledged.

+  // Ensure observers are notified about this.
+  if (params.renderer_created) {

[Charlie Reis, 2015/02/12 00:29:31]

Maybe params.renderer_initiated_creation?

[nasko, 2015/02/12 17:52:26]

Done.

+    RenderViewCreated(GetRenderViewHost());
+    GetRenderManager()->current_frame_host()->SetRenderFrameCreated(true);
+  }
 }
 
 void WebContentsImpl::OnWebContentsDestroyed(WebContentsImpl* web_contents) {
   RemoveDestructionObserver(web_contents);
 
   // Clear the opener if it has been closed.
   if (web_contents == opener_) {
     opener_ = NULL;
     return;
   }
@@ skipping 298 matching lines @@
 
   // Create the new web contents. This will automatically create the new
   // WebContentsView. In the future, we may want to create the view separately.
   CreateParams create_params(GetBrowserContext(), site_instance.get());
   create_params.routing_id = route_id;
   create_params.main_frame_routing_id = main_frame_route_id;
   create_params.opener = this;
   create_params.opener_suppressed = params.opener_suppressed;
   if (params.disposition == NEW_BACKGROUND_TAB)
     create_params.initially_hidden = true;
+  create_params.renderer_created = true;
 
   WebContentsImpl* new_contents = NULL;
   if (!is_guest) {
     create_params.context = view_->GetNativeView();
     create_params.initial_size = GetContainerBounds().size();
     new_contents = static_cast<WebContentsImpl*>(
         WebContents::Create(create_params));
   }  else {
     new_contents = GetBrowserPluginGuest()->CreateNewGuestWindow(create_params);
   }
   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).
   if (!params.opener_suppressed) {
     if (!is_guest) {
       WebContentsView* new_view = new_contents->view_.get();
 
       // TODO(brettw): It seems bogus that we have to call this function on the
       // newly created object and give it one of its own member variables.
       new_view->CreateViewForWidget(new_contents->GetRenderViewHost(), false);
@@ skipping 2031 matching lines @@
     // Put the renderer in view source mode.
     render_view_host->Send(
         new ViewMsg_EnableViewSourceMode(render_view_host->GetRoutingID()));
   }
 
   view_->RenderViewCreated(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);

[Charlie Reis, 2015/02/12 00:29:31]

Where does SetAccessibilityModeOnFrame get called now?

[nasko, 2015/02/12 17:52:26]

WebContentsImpl::RenderFrameCreated has pretty much this exact code - notify
observers and set accessibility.

-
   DevToolsManager::GetInstance()->RenderViewCreated(this, render_view_host);
 }
 
 void WebContentsImpl::RenderViewReady(RenderViewHost* rvh) {
   if (rvh != GetRenderViewHost()) {
     // Don't notify the world, since this came from a renderer in the
     // background.
     return;
   }
 
@@ skipping 13 matching lines @@
       (!delegate_ || delegate_->ShouldFocusPageAfterCrash())) {
     view_->Focus();
   }
 
   FOR_EACH_OBSERVER(WebContentsObserver, observers_, RenderViewReady());
 }
 
 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());

[Charlie Reis, 2015/02/12 00:29:31]

Just to confirm, we now get RenderFrameDeleted in addition to
RenderViewTerminated on a crash?

(It'd be nice to have a test that shows that.  Maybe we can log
RenderViewTerminated in FrameTreeTest.ObserverWalksTreeAfterCrash?)

[nasko, 2015/02/12 17:52:26]

Correct.
 
This is already tested in ObserverWalksTreeAfterCrash, it is just
RenderProcessGone, not RenderViewTerminated, as no such method exists in the
observer interface.

-
   if (rvh != GetRenderViewHost()) {
     // The pending page's RenderViewHost is gone.
     return;
   }
 
   // Ensure fullscreen mode is exited in the |delegate_| since a crashed
   // renderer may not have made a clean exit.
   if (IsFullscreenForCurrentTab())
     ExitFullscreenMode();
 
@@ skipping 802 matching lines @@
   node->render_manager()->ResumeResponseDeferredAtStart();
 }
 
 void WebContentsImpl::SetForceDisableOverscrollContent(bool force_disable) {
   force_disable_overscroll_content_ = force_disable;
   if (view_)
     view_->SetOverscrollControllerEnabled(CanOverscrollContent());
 }
 
 }  // namespace content