Have the unload event execute in background on cross-site navigations

content/browser/frame_host/render_frame_host_manager.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/browser/frame_host/render_frame_host_manager.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/debug/trace_event.h"
@@ skipping 54 matching lines @@
     RenderWidgetHostDelegate* render_widget_delegate,
     Delegate* delegate)
     : frame_tree_node_(frame_tree_node),
       delegate_(delegate),
       cross_navigation_pending_(false),
       render_frame_delegate_(render_frame_delegate),
       render_view_delegate_(render_view_delegate),
       render_widget_delegate_(render_widget_delegate),
       render_frame_host_(NULL),
       pending_render_frame_host_(NULL),
-      interstitial_page_(NULL) {
+      interstitial_page_(NULL),
+      weak_factory_(this) {
 }
 
 RenderFrameHostManager::~RenderFrameHostManager() {
   if (pending_render_frame_host_)
     CancelPending();
 
   // We should always have a current RenderFrameHost except in some tests.
   // TODO(creis): Now that we aren't using Shutdown, make render_frame_host_ and
   // RenderFrameHostMap use scoped_ptrs.
   RenderFrameHostImpl* render_frame_host = render_frame_host_;
@@ skipping 139 matching lines @@
   if (!cross_navigation_pending_)
     return true;
 
   // We should always have a pending RFH when there's a cross-process navigation
   // in progress.  Sanity check this for http://crbug.com/276333.
   CHECK(pending_render_frame_host_);
 
   // If the tab becomes unresponsive during {before}unload while doing a
   // cross-site navigation, proceed with the navigation.  (This assumes that
   // the pending RenderFrameHost is still responsive.)
-  if (render_frame_host_->render_view_host()->is_waiting_for_unload_ack()) {
+  if (render_frame_host_->render_view_host()->IsWaitingForUnloadACK()) {
     // The request has been started and paused while we're waiting for the
     // unload handler to finish.  We'll pretend that it did.  The pending
     // renderer will then be swapped in as part of the usual DidNavigate logic.
     // (If the unload handler later finishes, this call will be ignored because
     // the pending_nav_params_ state will already be cleaned up.)
-    current_host()->OnSwappedOut(true);
+    current_host()->SwapOut();

[nasko, 2014/01/23 23:34:58]

Calling SwapOut() will cause us to run the unload handler again. Is this
desired?
Also, why are we calling OnSwappedOut right after that with false? The parameter
is "timed out", which is consistent with the previous version.

[clamy, 2014/01/24 17:01:54]

Done.

+    current_host()->OnSwappedOut(false);
   } else if (render_frame_host_->render_view_host()->
                  is_waiting_for_beforeunload_ack()) {
     // Haven't gotten around to starting the request, because we're still
     // waiting for the beforeunload handler to finish.  We'll pretend that it
     // did finish, to let the navigation proceed.  Note that there's a danger
     // that the beforeunload handler will later finish and possibly return
     // false (meaning the navigation should not proceed), but we'll ignore it
     // in this case because it took too long.
     if (pending_render_frame_host_->render_view_host()->
             are_navigations_suspended()) {
@@ skipping 276 matching lines @@
   // ResourceDispatcherHost has told us to run the onunload handler, which
   // means it is not a download or unsafe page, and we are going to perform the
   // navigation.  Thus, we no longer need to remember that the RenderFrameHost
   // is part of a pending cross-site request.
   if (pending_render_frame_host_) {
     pending_render_frame_host_->render_view_host()->
         SetHasPendingCrossSiteRequest(false);
   }
 }
 
+void RenderFrameHostManager::ClearPendingShutdownRFHForSiteInstance(
+    int32 site_instance_id,
+    RenderFrameHostImpl* rfh) {
+  base::hash_map<int32, linked_ptr<RenderFrameHostImpl> >::iterator iter =
+      pending_delete_hosts_.find(site_instance_id);
+  if (iter != pending_delete_hosts_.end() && iter->second.get() == rfh)
+    pending_delete_hosts_.erase(site_instance_id);
+}
+
 void RenderFrameHostManager::Observe(
     int type,
     const NotificationSource& source,
     const NotificationDetails& details) {
   switch (type) {
     case NOTIFICATION_RENDERER_PROCESS_CLOSED:
     case NOTIFICATION_RENDERER_PROCESS_CLOSING:
       RendererProcessClosing(
           Source<RenderProcessHost>(source).ptr());
       break;
 
     default:
       NOTREACHED();
   }
 }
 
 bool RenderFrameHostManager::ClearSwappedOutRFHsInSiteInstance(
     int32 site_instance_id,
     FrameTreeNode* node) {
   RenderFrameHostMap::iterator iter =
       node->render_manager()->swapped_out_hosts_.find(site_instance_id);
-  if (iter != node->render_manager()->swapped_out_hosts_.end())
-    delete iter->second;
+  if (iter != node->render_manager()->swapped_out_hosts_.end()) {
+    RenderFrameHostImpl* swapped_out_rfh = iter->second;
+    // If the RVH is pending swap out, it needs to switch state to
+    // pending shutdown. Otherwise it is deleted.
+    if (swapped_out_rfh->render_view_host()->rvh_state() ==
+        RenderViewHostImpl::RVH_STATE_PENDING_SWAP_OUT) {
+      swapped_out_rfh->SetPendingShutdown(base::Bind(
+          &RenderFrameHostManager::ClearPendingShutdownRFHForSiteInstance,
+          node->render_manager()->weak_factory_.GetWeakPtr(),
+          site_instance_id,
+          swapped_out_rfh));
+      RFHPendingDeleteMap::iterator pending_delete_iter =
+          node->render_manager()->pending_delete_hosts_.find(site_instance_id);
+      if (pending_delete_iter ==
+              node->render_manager()->pending_delete_hosts_.end() ||
+          pending_delete_iter->second.get() != iter->second) {

[nasko, 2014/01/23 23:34:58]

What happens if there is already an entry for this site_instance_id and we
overwrite its entry in the map?

[clamy, 2014/01/24 17:01:54]

Since it is stored in a linked_ptr, the only ref on the linked_ptr goes away and
the RFH is detroyed. If it was the last RFH using the RVH, then Shutdown is
called on the RVH.

+        node->render_manager()->pending_delete_hosts_[site_instance_id] =
+            linked_ptr<RenderFrameHostImpl>(swapped_out_rfh);
+      }
+    } else {
+      delete swapped_out_rfh;
+    }
+    node->render_manager()->swapped_out_hosts_.erase(site_instance_id);
+  }
 
   return true;
 }
 
 bool RenderFrameHostManager::ShouldTransitionCrossSite() {
   // False in the single-process mode, as it makes RVHs to accumulate
   // in swapped_out_hosts_.
   // True if we are using process-per-site-instance (default) or
   // process-per-site (kProcessPerSite).
   return
@@ skipping 333 matching lines @@
   // If the pending navigation is to a WebUI and the RenderView is not in a
   // guest process, tell the RenderViewHost about any bindings it will need
   // enabled.
   if (pending_web_ui() && !render_view_host->GetProcess()->IsGuest()) {
     render_view_host->AllowBindings(pending_web_ui()->GetBindings());
   } else {
     // Ensure that we don't create an unprivileged RenderView in a WebUI-enabled
     // process unless it's swapped out.
     RenderViewHostImpl* rvh_impl =
         static_cast<RenderViewHostImpl*>(render_view_host);
-    if (!rvh_impl->is_swapped_out()) {
+    if (rvh_impl->rvh_state() != RenderViewHostImpl::RVH_STATE_SWAPPED_OUT) {
       CHECK(!ChildProcessSecurityPolicyImpl::GetInstance()->HasWebUIBindings(
                 render_view_host->GetProcess()->GetID()));
     }
   }
 
   return delegate_->CreateRenderViewForRenderManager(render_view_host,
                                                      opener_route_id);
 }
 
 void RenderFrameHostManager::CommitPending() {
@@ skipping 54 matching lines @@
   // to make sure the sad tab shows up, etc.
   if (!render_frame_host_->render_view_host()->GetView()) {
     delegate_->RenderProcessGoneFromRenderManager(
         render_frame_host_->render_view_host());
   } else if (!delegate_->IsHidden() && is_main_frame) {
     render_frame_host_->render_view_host()->GetView()->Show();
   }
 
   // If the old view is live and top-level, hide it now that the new one is
   // visible.
+  int32 old_site_instance_id =
+      old_render_frame_host->render_view_host()->GetSiteInstance()->GetId();
   if (old_render_frame_host->render_view_host()->GetView()) {
     if (is_main_frame) {
       old_render_frame_host->render_view_host()->GetView()->Hide();
-      old_render_frame_host->render_view_host()->WasSwappedOut();
+      old_render_frame_host->render_view_host()->WasSwappedOut(base::Bind(
+          &RenderFrameHostManager::ClearPendingShutdownRFHForSiteInstance,
+          weak_factory_.GetWeakPtr(),
+          old_site_instance_id,
+          old_render_frame_host));
     } else {
       // TODO(creis): We'll need to set this back to false if we navigate back.
       old_render_frame_host->set_swapped_out(true);
     }
   }
 
   // Make sure the size is up to date.  (Fix for bug 1079768.)
   delegate_->UpdateRenderViewSizeForRenderManager();
 
   if (will_focus_location_bar) {
@@ skipping 13 matching lines @@
         old_render_frame_host->render_view_host(),
         render_frame_host_->render_view_host());
   }
 
   // If the pending frame was on the swapped out list, we can remove it.
   swapped_out_hosts_.erase(render_frame_host_->render_view_host()->
                                GetSiteInstance()->GetId());
 
   if (old_render_frame_host->render_view_host()->IsRenderViewLive()) {
     // If the old RFH is live, we are swapping it out and should keep track of
-    // it in case we navigate back to it.
+    // it in case we navigate back to it, or it is waiting for the unload event
+    // to execute in the background.
     // TODO(creis): Swap out the subframe in --site-per-process.
     if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kSitePerProcess))
       DCHECK(old_render_frame_host->is_swapped_out() ||
-             old_render_frame_host->render_view_host()->is_swapped_out());
-
+             !RenderViewHostImpl::IsRVHStateActive(
+                  old_render_frame_host->render_view_host()->rvh_state()));
     // Temp fix for http://crbug.com/90867 until we do a better cleanup to make
     // sure we don't get different rvh instances for the same site instance
     // in the same rvhmgr.
     // TODO(creis): Clean this up.
-    int32 old_site_instance_id =
-        old_render_frame_host->render_view_host()->GetSiteInstance()->GetId();
     RenderFrameHostMap::iterator iter =
         swapped_out_hosts_.find(old_site_instance_id);
     if (iter != swapped_out_hosts_.end() &&
         iter->second != old_render_frame_host) {
       // Delete the RFH that will be replaced in the map to avoid a leak.
       delete iter->second;
     }
-    swapped_out_hosts_[old_site_instance_id] = old_render_frame_host;
+    // If the RenderViewHost backing the RenderFrameHost is pending shutdown,

[nasko, 2014/01/23 23:34:58]

nit: You are mixing terminology - pending shutdown and pending_delete_hosts. Is
it pure delete or do we need to call shutdown on those?

[clamy, 2014/01/24 17:01:54]

We call delete on the RenderFrameHost, which will eventually lead Shutdown to be
called on the RenderViewHost. Shutdwon needs to be called on the RVH, but we are
keeping RFH in the map, which are only deleted. Should it be called
pending_shutdown_hosts_?

[nasko, 2014/01/27 19:09:51]

I think it is best we keep it using "delete", since RVH will eventually
disappear and currently RFH uses delete, not shutdown.

+    // the RenderFrameHost should be put in the map of RenderFrameHost pending
+    // shutdown. Otherwise, it is stored in the map of swapped out
+    // RenderFrameHosts.
+    if (old_render_frame_host->render_view_host()->rvh_state() ==
+        RenderViewHostImpl::RVH_STATE_PENDING_SHUTDOWN) {
+      swapped_out_hosts_.erase(old_site_instance_id);
+      RFHPendingDeleteMap::iterator pending_delete_iter =
+          pending_delete_hosts_.find(old_site_instance_id);
+      if (pending_delete_iter == pending_delete_hosts_.end() ||
+          pending_delete_iter->second.get() != old_render_frame_host) {
+        pending_delete_hosts_[old_site_instance_id] =
+            linked_ptr<RenderFrameHostImpl>(old_render_frame_host);
+      }
+    } else {
+      swapped_out_hosts_[old_site_instance_id] = old_render_frame_host;
+    }
 
     // If there are no active views in this SiteInstance, it means that
     // this RFH was the last active one in the SiteInstance. Now that we
     // know that all RFHs are swapped out, we can delete all the RFHs and RVHs
     // in this SiteInstance.  We do this after ensuring the RFH is on the
     // swapped out list to simplify the deletion.
     if (!static_cast<SiteInstanceImpl*>(
             old_render_frame_host->render_view_host()->GetSiteInstance())->
                 active_view_count()) {
       ShutdownRenderFrameHostsInSiteInstance(old_site_instance_id);
@@ skipping 22 matching lines @@
       continue;
     RenderViewHostImpl* rvh =
         static_cast<RenderViewHostImpl*>(RenderViewHost::From(widget));
     if (site_instance_id == rvh->GetSiteInstance()->GetId()) {
       // This deletes all RenderFrameHosts using the |rvh|, which then causes
       // |rvh| to Shutdown.
       FrameTree* tree = rvh->GetDelegate()->GetFrameTree();
       tree->ForEach(base::Bind(
           &RenderFrameHostManager::ClearSwappedOutRFHsInSiteInstance,
           site_instance_id));
-      // rvh is now deleted.
     }
   }
 }
 
 RenderFrameHostImpl* RenderFrameHostManager::UpdateRendererStateForNavigate(
     const NavigationEntryImpl& entry) {
   // If we are currently navigating cross-process, we want to get back to normal
   // and then navigate as usual.
   if (cross_navigation_pending_) {
     if (pending_render_frame_host_)
@@ skipping 248 matching lines @@
     SiteInstance* instance) const {
   RenderFrameHostMap::const_iterator iter =
       swapped_out_hosts_.find(instance->GetId());
   if (iter != swapped_out_hosts_.end())
     return iter->second;
 
   return NULL;
 }
 
 }  // namespace content