Refactor prerender pending swap logic.

chrome/browser/prerender/prerender_manager.cc

Index: chrome/browser/prerender/prerender_manager.cc
diff --git a/chrome/browser/prerender/prerender_manager.cc b/chrome/browser/prerender/prerender_manager.cc
index ec44cfdcdbde4bdd4fcf07fa5494da34c814e4e1..205a80b2fb906b58ea558a92a9f8daf33050d9e1 100644
--- a/chrome/browser/prerender/prerender_manager.cc
+++ b/chrome/browser/prerender/prerender_manager.cc
@@ -434,102 +434,6 @@ void PrerenderManager::CancelAllPrerenders() {
   }
 }
 
-void PrerenderManager::ProcessMergeResult(
-    PrerenderData* prerender_data,
-    bool timed_out,
-    content::SessionStorageNamespace::MergeResult result) {
-  PendingSwap* pending_swap = prerender_data->pending_swap();
-  DCHECK(pending_swap);
-  // No pending_swap should never happen. If it does anyways (in a retail
-  // build), log this and bail.
-  if (!pending_swap) {
-    RecordEvent(prerender_data->contents(),
-                PRERENDER_EVENT_MERGE_RESULT_NO_PENDING_SWAPIN);
-    return;
-  }
-  if (timed_out) {
-    RecordEvent(prerender_data->contents(),
-                PRERENDER_EVENT_MERGE_RESULT_TIMEOUT_CB);
-  } else {
-    RecordEvent(prerender_data->contents(),
-                PRERENDER_EVENT_MERGE_RESULT_RESULT_CB);
-    UMA_HISTOGRAM_TIMES("Prerender.SessionStorageNamespaceMergeTime",
-                        pending_swap->GetElapsedTime());
-  }
-
-  // Any return here must call ClearPendingSwap on |prerender_data| before
-  // returning, with one exception: when the prerender was ultimately swapped
-  // in. In that case, SwapInternal will take care of deleting
-  // |prerender_data| and sending the appropriate notifications to the tracker.
-  if (timed_out) {
-    RecordEvent(prerender_data->contents(),
-                PRERENDER_EVENT_MERGE_RESULT_TIMED_OUT);
-    prerender_data->ClearPendingSwap();
-    return;
-  }
-
-  RecordEvent(prerender_data->contents(),
-              PRERENDER_EVENT_MERGE_RESULT_MERGE_DONE);
-
-  // Log the exact merge result in a histogram.
-  switch (result) {
-    case content::SessionStorageNamespace::MERGE_RESULT_NAMESPACE_NOT_FOUND:
-      RecordEvent(prerender_data->contents(),
-                  PRERENDER_EVENT_MERGE_RESULT_RESULT_NAMESPACE_NOT_FOUND);
-      break;
-    case content::SessionStorageNamespace::MERGE_RESULT_NAMESPACE_NOT_ALIAS:
-      RecordEvent(prerender_data->contents(),
-                  PRERENDER_EVENT_MERGE_RESULT_RESULT_NAMESPACE_NOT_ALIAS);
-      break;
-    case content::SessionStorageNamespace::MERGE_RESULT_NOT_LOGGING:
-      RecordEvent(prerender_data->contents(),
-                  PRERENDER_EVENT_MERGE_RESULT_RESULT_NOT_LOGGING);
-      break;
-    case content::SessionStorageNamespace::MERGE_RESULT_NO_TRANSACTIONS:
-      RecordEvent(prerender_data->contents(),
-                  PRERENDER_EVENT_MERGE_RESULT_RESULT_NO_TRANSACTIONS);
-      break;
-    case content::SessionStorageNamespace::MERGE_RESULT_TOO_MANY_TRANSACTIONS:
-      RecordEvent(prerender_data->contents(),
-                  PRERENDER_EVENT_MERGE_RESULT_RESULT_TOO_MANY_TRANSACTIONS);
-      break;
-    case content::SessionStorageNamespace::MERGE_RESULT_NOT_MERGEABLE:
-      RecordEvent(prerender_data->contents(),
-                  PRERENDER_EVENT_MERGE_RESULT_RESULT_NOT_MERGEABLE);
-      break;
-    case content::SessionStorageNamespace::MERGE_RESULT_MERGEABLE:
-      RecordEvent(prerender_data->contents(),
-                  PRERENDER_EVENT_MERGE_RESULT_RESULT_MERGEABLE);
-      break;
-    default:
-      NOTREACHED();
-  }
-
-  if (result != content::SessionStorageNamespace::MERGE_RESULT_MERGEABLE &&
-      result !=
-      content::SessionStorageNamespace::MERGE_RESULT_NO_TRANSACTIONS) {
-    RecordEvent(prerender_data->contents(),
-                PRERENDER_EVENT_MERGE_RESULT_MERGE_FAILED);
-    prerender_data->ClearPendingSwap();
-    return;
-  }
-
-  RecordEvent(prerender_data->contents(),
-              PRERENDER_EVENT_MERGE_RESULT_SWAPPING_IN);
-  // Notice that SwapInInternal, on success, will delete |prerender_data|
-  // and |pending_swap|. Therefore, we have to pass a new GURL object rather
-  // than a reference to the one in |pending_swap|.
-  content::WebContents* new_web_contents =
-      SwapInternal(GURL(pending_swap->url()),
-                   pending_swap->target_contents(),
-                   prerender_data);
-  if (!new_web_contents) {
-    RecordEvent(prerender_data->contents(),
-                PRERENDER_EVENT_MERGE_RESULT_SWAPIN_FAILED);
-    prerender_data->ClearPendingSwap();
-  }
-}
-
 bool PrerenderManager::MaybeUsePrerenderedPage(const GURL& url,
                                                chrome::NavigateParams* params) {
   DCHECK(CalledOnValidThread());
@@ -541,35 +445,6 @@ bool PrerenderManager::MaybeUsePrerenderedPage(const GURL& url,
   if (params->uses_post || !params->extra_headers.empty())
     return false;
 
-  content::WebContents* new_web_contents = SwapInternal(url, web_contents,
-                                                          NULL);
-  if (!new_web_contents)
-    return false;
-
-  // Record the new target_contents for the callers.
-  params->target_contents = new_web_contents;
-  return true;
-}
-
-content::WebContents* PrerenderManager::SwapInternal(
-    const GURL& url,
-    content::WebContents* web_contents,
-    PrerenderData* swap_candidate) {
-  DCHECK(CalledOnValidThread());
-  DCHECK(!IsWebContentsPrerendering(web_contents, NULL));
-
-  DeleteOldEntries();
-  to_delete_prerenders_.clear();
-  // TODO(ajwong): This doesn't handle isolated apps correctly.
-
-  // Only if this WebContents is used in a tabstrip may be swap.
-  // We check this by examining whether its CoreTabHelper has a delegate.
-  CoreTabHelper* core_tab_helper = CoreTabHelper::FromWebContents(web_contents);
-  if (!core_tab_helper || !core_tab_helper->delegate()) {
-    RecordEvent(NULL, PRERENDER_EVENT_SWAPIN_NO_DELEGATE);
-    return NULL;
-  }
-
   // First, try to find prerender data with the correct session storage
   // namespace.
   PrerenderData* prerender_data = FindPrerenderData(
@@ -587,16 +462,15 @@ content::WebContents* PrerenderManager::SwapInternal(
   }
 
   if (!prerender_data)
-    return NULL;
+    return false;
   RecordEvent(prerender_data->contents(), PRERENDER_EVENT_SWAPIN_CANDIDATE);
   DCHECK(prerender_data->contents());
 
   // If there is currently a merge pending for this prerender data,
   // or this webcontents, do not swap in, but give the merge a chance to
   // finish and swap into the intended target webcontents.

[mmenke, 2013/12/09 17:06:36]

"Or this webcontents"...Where do we check if the web contents has a pending
swap?

[davidben, 2013/12/09 20:00:32]

Hrmf. I think that got lost in the original CL's rewrites and the comment never
got updated. I think there may be a handful of races here, including this one,
with browser-initiated navigations. All the cancel hooks come from the renderer,
which is asynchronous with the browser initiating a navigation. Those already
exist before this cleanup CL, so I'll deal with them in a follow-up I guess.

-  if (prerender_data != swap_candidate && prerender_data->pending_swap()) {

[mmenke, 2013/12/09 17:06:36]

I'm confused....  In calls from MaybeUsePrerenderedPage, swap_candidate used to
be NULL, so wouldn't this always fail here before in that case?

[mmenke, 2013/12/09 17:13:34]

Oh, this is an "and", rather than an "or"....that makes more sense.

-    return NULL;
-  }
+  if (prerender_data->pending_swap())
+    return false;
 
   RecordEvent(prerender_data->contents(),
               PRERENDER_EVENT_SWAPIN_NO_MERGE_PENDING);
@@ -611,46 +485,43 @@ content::WebContents* PrerenderManager::SwapInternal(
     if (!ShouldMergeSessionStorageNamespaces()) {
       RecordEvent(prerender_data->contents(),
                   PRERENDER_EVENT_SWAPIN_MERGING_DISABLED);
-      return NULL;
+      return false;
     }
     RecordEvent(prerender_data->contents(),
                 PRERENDER_EVENT_SWAPIN_ISSUING_MERGE);
-    // There should never be a |pending_swap| if we get to here:
-    // Per check above, |pending_swap| may only be != NULL when
-    // processing a successful merge, as indicated by |swap_candidate|
-    // != NULL. But in that case, there should be no need for yet another merge.
-    DCHECK(!prerender_data->pending_swap());
-    if (prerender_data->pending_swap()) {
-      // In retail builds, log this error and bail.
-      RecordEvent(prerender_data->contents(),
-                  PRERENDER_EVENT_MERGE_FOR_SWAPIN_CANDIDATE);
-      return NULL;
-    }
-    PendingSwap* pending_swap = new PendingSwap(
-        prerender_tracker_,
-        web_contents,
-        prerender_data,
-        url,
-        base::Bind(&PrerenderManager::ProcessMergeResult,
-                   AsWeakPtr(),
-                   prerender_data,
-                   true,
-                   SessionStorageNamespace::MERGE_RESULT_NOT_MERGEABLE),
-        base::Bind(&PrerenderManager::ProcessMergeResult,
-                   AsWeakPtr(),
-                   prerender_data,
-                   false));
-    prerender_data->set_pending_swap(pending_swap);
-    prerender_namespace->Merge(
-        true,
-        prerender_data->contents()->child_id(),
-        target_namespace,
-        pending_swap->GetMergeResultCallback());
-    base::MessageLoop::current()->PostDelayedTask(
-        FROM_HERE,
-        pending_swap->GetTimeoutCallback(),
-        base::TimeDelta::FromMilliseconds(
-            kSessionStorageNamespaceMergeTimeoutMs));
+    prerender_data->set_pending_swap(new PendingSwap(
+        this, web_contents, prerender_data, url));
+    prerender_data->pending_swap()->BeginSwap();
+    return false;
+  }
+
+  // No need to merge; swap synchronously.
+  WebContents* new_web_contents = SwapInternal(url, web_contents,
+                                               prerender_data);
+  if (!new_web_contents)
+    return false;
+
+  // Record the new target_contents for the callers.
+  params->target_contents = new_web_contents;
+  return true;
+}
+
+WebContents* PrerenderManager::SwapInternal(
+    const GURL& url,
+    WebContents* web_contents,
+    PrerenderData* prerender_data) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(!IsWebContentsPrerendering(web_contents, NULL));
+
+  DeleteOldEntries();

[mmenke, 2013/12/09 17:06:36]

Couldn't this delete prerender_data?

[davidben, 2013/12/09 20:00:32]

Hrm. So it can. I guess that's what the ClearPendingSwap call and second
FindPrerenderData lookup in MoveEntryToPendingDelete prevented. This is a little
annoyingly precarious... I've moved the cleanup code back to the top of
MaybeUsePrerenderedPage which is probably where it should be anyway. Though it
does still mean that SwapInternal may be called on a PrerenderData in
to_delete_prerenders_.

Also moved the TODO(ajwong) to the line it was presumably attached to. Got
attached to the wrong block somewhere.

+  to_delete_prerenders_.clear();
+  // TODO(ajwong): This doesn't handle isolated apps correctly.
+
+  // Only if this WebContents is used in a tabstrip may be swap.
+  // We check this by examining whether its CoreTabHelper has a delegate.
+  CoreTabHelper* core_tab_helper = CoreTabHelper::FromWebContents(web_contents);
+  if (!core_tab_helper || !core_tab_helper->delegate()) {
+    RecordEvent(prerender_data->contents(), PRERENDER_EVENT_SWAPIN_NO_DELEGATE);
     return NULL;
   }
 
@@ -851,7 +722,6 @@ void PrerenderManager::MoveEntryToPendingDelete(PrerenderContents* entry,
     (*it)->MakeIntoMatchCompleteReplacement();
   } else {
     to_delete_prerenders_.push_back(*it);
-    (*it)->ClearPendingSwap();

[davidben, 2013/12/06 19:36:34]

Removing this line avoids nuisances where SwapInternal may or may not delete the
pending swap on failure if it calls Destroy. Instead, leave the pending swap
there; it will be cleaned up in the cleanup task. If the merge happens to
complete in the meantime, SwapInternal will refuse to swap anyway because the
final status is set.

     active_prerenders_.weak_erase(it);
   }
 
@@ -1282,45 +1152,44 @@ PrerenderContents* PrerenderManager::PrerenderData::ReleaseContents() {
 }
 
 PrerenderManager::PendingSwap::PendingSwap(
-    PrerenderTracker* prerender_tracker,
+    PrerenderManager* manager,
     content::WebContents* target_contents,
     PrerenderData* prerender_data,
-    const GURL& url,
-    const base::Closure& timeout_cb,
-    const SessionStorageNamespace::MergeResultCallback& merge_result_cb)
+    const GURL& url)
     : content::WebContentsObserver(target_contents),
-      prerender_tracker_(prerender_tracker),
+      manager_(manager),
       target_contents_(target_contents),
       prerender_data_(prerender_data),
       url_(url),
-      timeout_cb_(timeout_cb),
-      merge_result_cb_(merge_result_cb),
-      start_time_(base::TimeTicks::Now()) {
+      start_time_(base::TimeTicks::Now()),
+      weak_factory_(this) {
   RenderViewCreated(target_contents->GetRenderViewHost());
 }
 
 PrerenderManager::PendingSwap::~PendingSwap() {
-    timeout_cb_.Cancel();
-    merge_result_cb_.Cancel();
-    for (size_t i = 0; i < rvh_ids_.size(); i++) {
-      prerender_tracker_->RemovePrerenderPendingSwap(rvh_ids_[i], false);
-    }
-}
-
-const base::Closure& PrerenderManager::PendingSwap::GetTimeoutCallback() {
-  return timeout_cb_.callback();
-}
-
-void PrerenderManager::PendingSwap::SwapSuccessful() {
   for (size_t i = 0; i < rvh_ids_.size(); i++) {
-    prerender_tracker_->RemovePrerenderPendingSwap(rvh_ids_[i], true);
+    manager_->prerender_tracker()->RemovePrerenderPendingSwap(
+        rvh_ids_[i], false);
   }
-  rvh_ids_.clear();
 }
 
-const SessionStorageNamespace::MergeResultCallback&
-PrerenderManager::PendingSwap::GetMergeResultCallback() {
-  return merge_result_cb_.callback();
+void PrerenderManager::PendingSwap::BeginSwap() {
+  SessionStorageNamespace* target_namespace =
+      target_contents_->GetController().GetDefaultSessionStorageNamespace();
+  SessionStorageNamespace* prerender_namespace =
+      prerender_data_->contents()->GetSessionStorageNamespace();
+
+  prerender_namespace->Merge(
+      true, prerender_data_->contents()->child_id(),
+      target_namespace,
+      base::Bind(&PrerenderManager::PendingSwap::OnMergeCompleted,
+                 weak_factory_.GetWeakPtr()));
+
+  merge_timeout_.Start(
+      FROM_HERE,
+      base::TimeDelta::FromMilliseconds(
+          kSessionStorageNamespaceMergeTimeoutMs),
+      this, &PrerenderManager::PendingSwap::OnMergeTimeout);
 }
 
 void PrerenderManager::PendingSwap::ProvisionalChangeToMainFrameUrl(
@@ -1343,17 +1212,19 @@ void PrerenderManager::PendingSwap::DidCommitProvisionalLoadForFrame(
         content::RenderViewHost* render_view_host){
   if (!is_main_frame)
     return;
-  if (validated_url != url_)

[davidben, 2013/12/06 19:36:34]

This check is unnecessary; the throttled load can't commit.

[mmenke, 2013/12/09 17:06:36]

Random comment not for this CL:  Wonder if we should cancel earlier, like if a
new load starts, rather than waiting until commit.

[davidben, 2013/12/09 20:00:32]

We do. I believe the main interesting cancel hook here is
DidFailProvisionalLoad. The others mostly act as sanity checks I think?

-    prerender_data_->ClearPendingSwap();
+  prerender_data_->ClearPendingSwap();
 }
 
 void PrerenderManager::PendingSwap::RenderViewCreated(
     content::RenderViewHost* render_view_host) {
+  // Record the RVH id in the tracker to install throttles on MAIN_FRAME
+  // requests from that route.
   int child_id = render_view_host->GetProcess()->GetID();
   int route_id = render_view_host->GetRoutingID();
   PrerenderTracker::ChildRouteIdPair child_route_id_pair(child_id, route_id);
   rvh_ids_.push_back(child_route_id_pair);
-  prerender_tracker_->AddPrerenderPendingSwap(child_route_id_pair, url_);
+  manager_->prerender_tracker()->AddPrerenderPendingSwap(
+      child_route_id_pair, url_);
 }
 
 void PrerenderManager::PendingSwap::DidFailProvisionalLoad(
@@ -1364,6 +1235,8 @@ void PrerenderManager::PendingSwap::DidFailProvisionalLoad(
         int error_code,
         const string16& error_description,
         content::RenderViewHost* render_view_host) {
+  if (!is_main_frame)
+    return;
   prerender_data_->ClearPendingSwap();
 }
 
@@ -1372,8 +1245,72 @@ void PrerenderManager::PendingSwap::WebContentsDestroyed(
   prerender_data_->ClearPendingSwap();
 }
 
-base::TimeDelta PrerenderManager::PendingSwap::GetElapsedTime() {
-  return base::TimeTicks::Now() - start_time_;
+void PrerenderManager::PendingSwap::SwapSuccessful() {
+  for (size_t i = 0; i < rvh_ids_.size(); i++) {
+    manager_->prerender_tracker()->RemovePrerenderPendingSwap(
+        rvh_ids_[i], true);
+  }
+  rvh_ids_.clear();
+}
+
+void PrerenderManager::PendingSwap::RecordEvent(PrerenderEvent event) const {
+  manager_->RecordEvent(prerender_data_->contents(), event);
+}
+
+void PrerenderManager::PendingSwap::OnMergeCompleted(
+    SessionStorageNamespace::MergeResult result) {
+  UMA_HISTOGRAM_TIMES("Prerender.SessionStorageNamespaceMergeTime",
+                      base::TimeTicks::Now() - start_time_);
+  RecordEvent(PRERENDER_EVENT_MERGE_RESULT_MERGE_DONE);
+
+  // Log the exact merge result in a histogram.
+  switch (result) {
+    case SessionStorageNamespace::MERGE_RESULT_NAMESPACE_NOT_FOUND:
+      RecordEvent(PRERENDER_EVENT_MERGE_RESULT_RESULT_NAMESPACE_NOT_FOUND);
+      break;
+    case SessionStorageNamespace::MERGE_RESULT_NAMESPACE_NOT_ALIAS:
+      RecordEvent(PRERENDER_EVENT_MERGE_RESULT_RESULT_NAMESPACE_NOT_ALIAS);
+      break;
+    case SessionStorageNamespace::MERGE_RESULT_NOT_LOGGING:
+      RecordEvent(PRERENDER_EVENT_MERGE_RESULT_RESULT_NOT_LOGGING);
+      break;
+    case SessionStorageNamespace::MERGE_RESULT_NO_TRANSACTIONS:
+      RecordEvent(PRERENDER_EVENT_MERGE_RESULT_RESULT_NO_TRANSACTIONS);
+      break;
+    case SessionStorageNamespace::MERGE_RESULT_TOO_MANY_TRANSACTIONS:
+      RecordEvent(PRERENDER_EVENT_MERGE_RESULT_RESULT_TOO_MANY_TRANSACTIONS);
+      break;
+    case SessionStorageNamespace::MERGE_RESULT_NOT_MERGEABLE:
+      RecordEvent(PRERENDER_EVENT_MERGE_RESULT_RESULT_NOT_MERGEABLE);
+      break;
+    case SessionStorageNamespace::MERGE_RESULT_MERGEABLE:
+      RecordEvent(PRERENDER_EVENT_MERGE_RESULT_RESULT_MERGEABLE);
+      break;
+    default:
+      NOTREACHED();
+  }
+
+  if (result != SessionStorageNamespace::MERGE_RESULT_MERGEABLE &&
+      result != SessionStorageNamespace::MERGE_RESULT_NO_TRANSACTIONS) {
+    RecordEvent(PRERENDER_EVENT_MERGE_RESULT_MERGE_FAILED);
+    prerender_data_->ClearPendingSwap();
+    return;
+  }
+
+  RecordEvent(PRERENDER_EVENT_MERGE_RESULT_SWAPPING_IN);
+  // Note that SwapInternal, on success, will delete |prerender_data_| and
+  // |this|. Pass in a new GURL object rather than a reference to |url_|.
+  WebContents* new_web_contents =
+      manager_->SwapInternal(GURL(url_), target_contents_, prerender_data_);
+  if (!new_web_contents) {
+    RecordEvent(PRERENDER_EVENT_MERGE_RESULT_SWAPIN_FAILED);
+    prerender_data_->ClearPendingSwap();
+  }
+}
+
+void PrerenderManager::PendingSwap::OnMergeTimeout() {
+  RecordEvent(PRERENDER_EVENT_MERGE_RESULT_TIMED_OUT);
+  prerender_data_->ClearPendingSwap();
 }
 
 void PrerenderManager::SetPrerenderContentsFactory(