Remove swapped-out usage in --site-per-process.

content/renderer/render_frame_impl.cc

 // Copyright 2013 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/renderer/render_frame_impl.h"
 
 #include <map>
 #include <string>
 
 #include "base/auto_reset.h"
@@ skipping 519 matching lines @@
 
 // static
 void RenderFrameImpl::CreateFrame(
     int routing_id,
     int parent_routing_id,
     int previous_sibling_routing_id,
     int proxy_routing_id,
     const FrameReplicationState& replicated_state,
     CompositorDependencies* compositor_deps,
     const FrameMsg_NewFrame_WidgetParams& widget_params) {
-  // TODO(nasko): For now, this message is only sent for subframes, as the
-  // top level frame is created when the RenderView is created through the
-  // ViewMsg_New IPC.
-  CHECK_NE(MSG_ROUTING_NONE, parent_routing_id);
-
   blink::WebLocalFrame* web_frame;
   RenderFrameImpl* render_frame;
   if (proxy_routing_id == MSG_ROUTING_NONE) {
     RenderFrameProxy* parent_proxy =
         RenderFrameProxy::FromRoutingID(parent_routing_id);
     // If the browser is sending a valid parent routing id, it should already
     // be created and registered.
     CHECK(parent_proxy);
     blink::WebRemoteFrame* parent_web_frame = parent_proxy->web_frame();
 
@@ skipping 16 matching lines @@
     CHECK(proxy);
     render_frame = RenderFrameImpl::Create(proxy->render_view(), routing_id);
     web_frame =
         blink::WebLocalFrame::create(replicated_state.scope, render_frame);
     render_frame->proxy_routing_id_ = proxy_routing_id;
     web_frame->initializeToReplaceRemoteFrame(
         proxy->web_frame(), WebString::fromUTF8(replicated_state.name),
         replicated_state.sandbox_flags);
   }
   render_frame->SetWebFrame(web_frame);
+  CHECK_IMPLIES(parent_routing_id == MSG_ROUTING_NONE, !web_frame->parent());
 
   if (widget_params.routing_id != MSG_ROUTING_NONE) {
     CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
         switches::kSitePerProcess));
     render_frame->render_widget_ = RenderWidget::CreateForFrame(
         widget_params.routing_id, widget_params.surface_id,
         widget_params.hidden, render_frame->render_view_->screen_info(),
         compositor_deps, web_frame);
     // TODO(kenrb): Observing shouldn't be necessary when we sort out
     // WasShown and WasHidden, separating page-level visibility from
@@ skipping 98 matching lines @@
   if (!is_subframe_) {
     // RenderFrameProxy is "owned" by RenderFrameImpl in the case it is
     // the main frame. Ensure it is deleted along with this object.
     if (render_frame_proxy_) {
       // The following method calls back into this object and clears
       // |render_frame_proxy_|.
       render_frame_proxy_->frameDetached();
     }
 
     // Ensure the RenderView doesn't point to this object, once it is destroyed.
-    CHECK_EQ(render_view_->main_render_frame_, this);
+    // TODO(nasko): Restore this check once we don't leak this object on
+    // frame swap.
+    //CHECK_EQ(render_view_->main_render_frame_, this);

[Charlie Reis, 2015/06/04 00:02:11]

What's the plan here?  Will Lucas's CL fix it?

[nasko, 2015/06/04 14:57:13]

Enable the check once Lucas's CLs land. Yes, that should fix it.

     render_view_->main_render_frame_ = nullptr;
   }
 
   render_view_->UnregisterRenderFrame(this);
   g_routing_id_frame_map.Get().erase(routing_id_);
   RenderThread::Get()->RemoveRoute(routing_id_);
 }
 
 void RenderFrameImpl::SetWebFrame(blink::WebLocalFrame* web_frame) {
   DCHECK(!frame_);
@@ skipping 431 matching lines @@
     // willCheckAndDispatchMessageEvent() that needs the proxy.
     if (proxy)
       set_render_frame_proxy(proxy);
 
     // Now that we're swapped out and filtering IPC messages, stop loading to
     // ensure that no other in-progress navigation continues.  We do this here
     // to avoid sending a DidStopLoading message to the browser process.
     // TODO(creis): Should we be stopping all frames here and using
     // StopAltErrorPageFetcher with RenderView::OnStop, or just stopping this
     // frame?
-    OnStop();
+    if (!is_site_per_process)

[Charlie Reis, 2015/06/04 00:02:11]

Are we planning to remove this OnStop entirely?  (I think I remember that coming
up with you and Daniel.)

[nasko, 2015/06/04 14:57:13]

I don't know for certain, but Daniel felt that it is a hack and should not be
done in OOPIF, so it probably should go away eventually.

+      OnStop();
 
     // Transfer settings such as initial drawing parameters to the remote frame,
     // if one is created, that will replace this frame.
     if (!is_main_frame && proxy)
       proxy->web_frame()->initializeFromFrame(frame_);
 
     // Replace the page with a blank dummy URL. The unload handler will not be
     // run a second time, thanks to a check in FrameLoader::stopLoading.
     // TODO(creis): Need to add a better way to do this that avoids running the
     // beforeunload handler. For now, we just run it a second time silently.
-    if (!is_site_per_process || is_main_frame)
+    if (!is_site_per_process)
       NavigateToSwappedOutURL();
 
     // Let WebKit know that this view is hidden so it can drop resources and
     // stop compositing.
     // TODO(creis): Support this for subframes as well.
     if (is_main_frame) {
       render_view_->webview()->setVisibilityState(
           blink::WebPageVisibilityStateHidden, false);
     }
   }
 
   // It is now safe to show modal dialogs again.
   // TODO(creis): Deal with modal dialogs from subframes.
   if (is_main_frame)
     render_view_->suppress_dialogs_until_swap_out_ = false;
 
   Send(new FrameHostMsg_SwapOut_ACK(routing_id_));
 
+  RenderViewImpl* render_view = render_view_.get();
+
   // Now that all of the cleanup is complete and the browser side is notified,
   // start using the RenderFrameProxy, if one is created.
   if (proxy) {
-    if (!is_main_frame) {
+    if (is_site_per_process || !is_main_frame) {
       frame_->swap(proxy->web_frame());
 
       if (is_loading)
         proxy->OnDidStartLoading();
-
-      if (is_site_per_process) {
-        // TODO(nasko): delete the frame here, since we've replaced it with a
-        // proxy.
-      }
     }
   }
 
   // In --site-per-process, initialize the WebRemoteFrame with the replication
   // state passed by the process that is now rendering the frame.
   // TODO(alexmos): We cannot yet do this for swapped-out main frames, because
   // in that case we leave the LocalFrame as the main frame visible to Blink
   // and don't call swap() above. Because swap() is what creates a RemoteFrame
   // in proxy->web_frame(), the RemoteFrame will not exist for main frames.
   // When we do an unconditional swap for all frames, we can remove
   // !is_main_frame below.
-  if (is_site_per_process && proxy && !is_main_frame)
+  if (is_site_per_process && proxy)
     proxy->SetReplicatedState(replicated_frame_state);
 
   // Safe to exit if no one else is using the process.
-  if (is_main_frame)
-    render_view_->WasSwappedOut();
+  // TODO(nasko): Remove the dependency on RenderViewImpl here and ref count
+  // the process based on the lifetime of this RenderFrameImpl object.
+  if (is_main_frame) {
+    render_view->WasSwappedOut();
+
+    // TODO(nasko): Currently, this RenderFrame is leaked due to issues in

[Charlie Reis, 2015/06/04 00:02:11]

Hmm.  Will Lucas's CL fix this for us?

[nasko, 2015/06/04 14:57:13]

Indeed it is supposed to. This is why I cache the RenderViewImpl pointer in a
stack variable, so we don't touch |this| after the swap is complete.
This whole block is something that will be handled (and already is there) in the
destructor, but since it currently doesn't run, I need to duplicate it here to
ensure we do it.

[Charlie Reis, 2015/06/04 22:27:31]

Ok.  Please add a reference to Lucas's bug number, so that we know when it can
be resolved.

[nasko, 2015/06/04 23:38:36]

Done.

+    // Blink, therefore the destructor won't be invoked to destroy this
+    // object. In the meantime, set the main frame of the RenderView to null
+    // here.
+    if (is_site_per_process) {
+      CHECK_EQ(render_view_->main_render_frame_, this);
+      render_view->main_render_frame_ = nullptr;
+    }
+  }
 }
 
 void RenderFrameImpl::OnContextMenuClosed(
     const CustomContextMenuContext& custom_context) {
   if (custom_context.request_id) {
     // External request, should be in our map.
     ContextMenuClient* client =
         pending_context_menus_.Lookup(custom_context.request_id);
     if (client) {
       client->OnMenuClosed(custom_context.request_id);
@@ skipping 1349 matching lines @@
       static_cast<NavigationStateImpl*>(document_state->navigation_state());
 
   if (proxy_routing_id_ != MSG_ROUTING_NONE) {
     RenderFrameProxy* proxy =
         RenderFrameProxy::FromRoutingID(proxy_routing_id_);
     CHECK(proxy);
     proxy->web_frame()->swap(frame_);
     proxy_routing_id_ = MSG_ROUTING_NONE;
 
     // If this is the main frame going from a remote frame to a local frame,
-    // it needs to set RenderViewImpl's pointer for the main frame to itself.
+    // it needs to set RenderViewImpl's pointer for the main frame to itself
+    // and ensure RenderWidget is no longer in swapped out mode.
     if (!is_subframe_) {
       CHECK(!render_view_->main_render_frame_);
       render_view_->main_render_frame_ = this;
+      if (render_view_->is_swapped_out())
+        render_view_->SetSwappedOut(false);
     }
   }
 
   // When we perform a new navigation, we need to update the last committed
   // session history entry with state for the page we are leaving. Do this
   // before updating the HistoryController state.
   render_view_->UpdateSessionHistory(frame);
 
   render_view_->history_controller()->UpdateForCommit(
       this, item, commit_type, navigation_state->WasWithinSamePage());
@@ skipping 1748 matching lines @@
   }
 
   Send(new FrameHostMsg_OpenURL(routing_id_, params));
 }
 
 void RenderFrameImpl::NavigateInternal(
     const CommonNavigationParams& common_params,
     const StartNavigationParams& start_params,
     const RequestNavigationParams& request_params,
     scoped_ptr<StreamOverrideParameters> stream_params) {
+  if (proxy_routing_id_ != MSG_ROUTING_NONE) {

[nasko, 2015/06/04 14:57:13]

This code is sufficiently similar to the code in didCommitProvisionalLoad. Do
you think it is worthwhile pulling into a helper function? Or is leaving it as
is ok?

[Charlie Reis, 2015/06/04 22:27:31]

Ah, I didn't notice-- thanks for pointing it out.

Why are we doing it in both places?  Didn't we intentionally do the swap at
commit time?

(And yes, we should abstract it out if we end up needing it in both, though that
seems confusing to me at the moment.)

[nasko, 2015/06/04 23:38:36]

Indeed this isn't needed. For some reason I've added it in one of the iterations
of my work and didn't remove it later. It does expose the problem that we can't
navigate back, since HistoryController::GoToEntry assumes non-null main frame.
The one test that fails due to this is RenderFrameHostManagerTest.DisownOpener,
so I'll disable it on the bot.

+    RenderFrameProxy* proxy =
+        RenderFrameProxy::FromRoutingID(proxy_routing_id_);
+    CHECK(proxy);
+    proxy->web_frame()->swap(frame_);
+    proxy_routing_id_ = MSG_ROUTING_NONE;
+
+    // If this is the main frame going from a remote frame to a local frame,
+    // it needs to set RenderViewImpl's pointer for the main frame to itself.
+    if (!is_subframe_) {
+      CHECK(!render_view_->main_render_frame_);
+      render_view_->main_render_frame_ = this;
+    }
+  }
+
   bool browser_side_navigation =
       base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableBrowserSideNavigation);
   bool is_reload = IsReload(common_params.navigation_type);
   bool is_history_navigation = request_params.page_state.IsValid();
   WebURLRequest::CachePolicy cache_policy =
       WebURLRequest::UseProtocolCachePolicy;
   if (!RenderFrameImpl::PrepareRenderViewForNavigation(
           common_params.url, is_history_navigation, request_params, &is_reload,
           &cache_policy)) {
@@ skipping 525 matching lines @@
 #elif defined(ENABLE_BROWSER_CDMS)
         cdm_manager_,
 #endif
         this);
   }
 
   return cdm_factory_;
 }
 
 }  // namespace content