Remove PrerenderTracker::TryCancel* methods and associated ones, since destruction of prerender onl…

chrome/browser/prerender/prerender_manager.cc

Index: chrome/browser/prerender/prerender_manager.cc
===================================================================
--- chrome/browser/prerender/prerender_manager.cc	(revision 243953)
+++ chrome/browser/prerender/prerender_manager.cc	(working copy)
@@ -413,15 +413,6 @@
                       session_storage_namespace);
 }
 
-void PrerenderManager::DestroyPrerenderForRenderView(
-    int process_id, int view_id, FinalStatus final_status) {
-  DCHECK(CalledOnValidThread());
-  if (PrerenderData* prerender_data =
-          FindPrerenderDataForChildAndRoute(process_id, view_id)) {
-    prerender_data->contents()->Destroy(final_status);
-  }
-}
-
 void PrerenderManager::CancelAllPrerenders() {
   DCHECK(CalledOnValidThread());
   while (!active_prerenders_.empty()) {
@@ -600,12 +591,6 @@
   CHECK(prerender_data->contents()->GetChildId(&child_id));
   CHECK(prerender_data->contents()->GetRouteId(&route_id));
 
-  // Try to set the prerendered page as used, so any subsequent attempts to
-  // cancel on other threads will fail.  If this fails because the prerender
-  // was already cancelled, possibly on another thread, fail.
-  if (!prerender_tracker_->TryUse(child_id, route_id))
-    return NULL;
-
   // At this point, we've determined that we will use the prerender.
   if (prerender_data->pending_swap())
     prerender_data->pending_swap()->set_swap_successful(true);
@@ -1281,14 +1266,21 @@
   }
 
   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_|.
-  //
-  // TODO(davidben): See about deleting PrerenderData asynchronously so this
-  // behavior is more reasonable.
-  WebContents* new_web_contents =
-      manager_->SwapInternal(GURL(url_), target_contents_, prerender_data_,
-                             should_replace_current_entry_);
+
+  WebContents* new_web_contents = NULL;
+  // Ensure that the prerendering hasn't been destroyed in the meantime.
+  if (prerender_data_->contents()->final_status() == FINAL_STATUS_MAX) {

[tburkard, 2014/01/10 06:14:03]

Why is this check necessary?

I assume because you deleted TryUse above?

But since everything has been reworked now to only cancel on the UI thread, and
since Destroy() on a webcontents removes the item from active_prerenders_, the
prerender should always be unused if we get here.

[jam, 2014/01/10 07:45:00]

Yep
My reading of the code, and please correct me if I'm wrong, is that
OnMergeCompleted is called asynchronously. So I was trying to guard for the
prerender being cancelled while the merge is happening. Is this not possible?
The PrerenderContents is still in active_prerenders_ map while this async
operation is happening, so it could be destroyed?

[tburkard, 2014/01/10 09:57:12]

So, when PrerenderData is destroyed, PendingSwaped along with it is destroyed,
too, cancelling the async callback from the merge.

When a PrerenderContents is Destroy'ed, it is removed from active_entries_, and
when I originally wrote this code, being removed from active_entries_ would
imply cancelling the callbacks.

However, I just noticed
https://codereview.chromium.org/98373010/diff/120001/chrome/browser/prerender...
which removed line 845, ClearPendingSwap.

So I guess it could happen now that the callback still happens, and the item is
no longer in active_prerenders_, and has a final status, so I think the check
you added is fine now.

However, I think the way it was before, ensuring that if something is no longer
available (i.e. part of active_prerenders_), no more callbacks will be called,
was cleaner.

Your choice, you can either leave it the way it is, or remove it and add back
the stuff David removed.

Thanks.

[tburkard, 2014/01/10 10:06:14]

sorry, in the comment above, "line 845" should be "line 854"

[jam, 2014/01/10 15:56:08]

I agree that the way you describe is easier to read/follow. I was a little
surprised that the callback runs even if the prerender is destroyed. Done.

[davidben, 2014/01/10 16:03:09]

I think I had to remove it there because SwapInternal will call Destroy in some
cases, which meant that |this| may or may not have been deleted by the time
SwapInternal returns. (This logic is screwy... having |this| deleted is already
awkward, but when it only happens some of the time...) Maybe if SwapInternal
took care of the ClearPendingSwap() call in all cases? Slight complication in
that, in the success case, the PendingSwap can't be deleted until it knows the
swap succeeded, so the destructor can cancel the throttles instead of resume
them.

[jam, 2014/01/10 16:24:09]

hmm, since I'm not familiar with this code, I will leave as it was before my
change. undid the change I did per Timo's comment.

+    // Note that SwapInternal, on success, will delete |prerender_data_| and
+    // |this|. Pass in a new GURL object rather than a reference to |url_|.
+    //
+    // TODO(davidben): See about deleting PrerenderData asynchronously so this
+    // behavior is more reasonable.
+
+    new_web_contents =  manager_->SwapInternal(
+        GURL(url_), target_contents_, prerender_data_,
+        should_replace_current_entry_);
+  }
+
   if (!new_web_contents) {
     RecordEvent(PRERENDER_EVENT_MERGE_RESULT_SWAPIN_FAILED);
     prerender_data_->ClearPendingSwap();