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

content/browser/frame_host/render_frame_host_manager.cc

Index: content/browser/frame_host/render_frame_host_manager.cc
diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc
index 885a6b628e80e3ebe16f5444f275d6666581c0d2..b738a3eb6295a8b09d87312ff9d881780731bc30 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -72,7 +72,8 @@ RenderFrameHostManager::RenderFrameHostManager(
       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() {
@@ -232,13 +233,14 @@ bool RenderFrameHostManager::ShouldCloseTabOnUnresponsiveRenderer() {
   // 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
@@ -535,6 +537,15 @@ void RenderFrameHostManager::SwapOutOldPage() {
   }
 }
 
+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,
@@ -556,8 +567,30 @@ bool RenderFrameHostManager::ClearSwappedOutRFHsInSiteInstance(
     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;
 }
@@ -911,7 +944,7 @@ bool RenderFrameHostManager::InitRenderView(RenderViewHost* render_view_host,
     // 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()));
     }
@@ -986,10 +1019,16 @@ void RenderFrameHostManager::CommitPending() {
 
   // 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);
@@ -1023,18 +1062,17 @@ void RenderFrameHostManager::CommitPending() {
 
   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() &&
@@ -1042,7 +1080,23 @@ void RenderFrameHostManager::CommitPending() {
       // 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
@@ -1085,7 +1139,6 @@ void RenderFrameHostManager::ShutdownRenderFrameHostsInSiteInstance(
       tree->ForEach(base::Bind(
           &RenderFrameHostManager::ClearSwappedOutRFHsInSiteInstance,
           site_instance_id));
-      // rvh is now deleted.
     }
   }
 }