Safely cancel prerenders on threads other than the UI thread

chrome/browser/prerender/prerender_contents.cc

Index: chrome/browser/prerender/prerender_contents.cc
===================================================================
--- chrome/browser/prerender/prerender_contents.cc	(revision 85771)
+++ chrome/browser/prerender/prerender_contents.cc	(working copy)
@@ -15,6 +15,7 @@
 #include "chrome/browser/prerender/prerender_final_status.h"
 #include "chrome/browser/prerender/prerender_manager.h"
 #include "chrome/browser/prerender/prerender_render_widget_host_view.h"
+#include "chrome/browser/prerender/prerender_tracker.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/renderer_preferences_util.h"
 #include "chrome/browser/ui/login/login_prompt.h"
@@ -59,18 +60,6 @@
   GURL url_;
 };
 
-void AddChildRoutePair(ResourceDispatcherHost* resource_dispatcher_host,
-                       int child_id, int route_id) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  resource_dispatcher_host->AddPrerenderChildRoutePair(child_id, route_id);
-}
-
-void RemoveChildRoutePair(ResourceDispatcherHost* resource_dispatcher_host,
-                          int child_id, int route_id) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  resource_dispatcher_host->RemovePrerenderChildRoutePair(child_id, route_id);
-}
-
 }  // end namespace
 
 class PrerenderContentsFactoryImpl : public PrerenderContents::Factory {
@@ -95,7 +84,9 @@
       ALLOW_THIS_IN_INITIALIZER_LIST(tab_contents_observer_registrar_(this)),
       has_stopped_loading_(false),
       final_status_(FINAL_STATUS_MAX),
-      prerendering_has_started_(false) {
+      prerendering_has_started_(false),
+      child_id_(-1),
+      route_id_(-1) {
   DCHECK(prerender_manager != NULL);
 }
 
@@ -136,18 +127,14 @@
       new PrerenderRenderWidgetHostView(render_view_host_, this);
   view->Init(source_render_view_host->view());
 
-  // Register this with the ResourceDispatcherHost as a prerender
-  // RenderViewHost. This must be done before the Navigate message to catch all
-  // resource requests, but as it is on the same thread as the Navigate message
-  // (IO) there is no race condition.
-  int process_id = render_view_host_->process()->id();
-  int view_id = render_view_host_->routing_id();
+  child_id_ = render_view_host_->process()->id();
+  route_id_ = render_view_host_->routing_id();
 
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      NewRunnableFunction(&AddChildRoutePair,
-                          g_browser_process->resource_dispatcher_host(),
-                          process_id, view_id));
+  // Register this with the PrerenderTracker as a prerendering RenderViewHost.
+  // This must be done before the Navigate message to catch all resource
+  // requests.
+  PrerenderTracker::GetInstance()->OnPrerenderingStarted(child_id_, route_id_,
+                                                         prerender_manager_);
 
   // Close ourselves when the application is shutting down.
   notification_registrar_.Add(this, NotificationType::APP_TERMINATING,
@@ -238,20 +225,15 @@
   render_view_host_observer_.reset(
       new PrerenderRenderViewHostObserver(this, render_view_host_mutable()));
 
-  int process_id;
-  int view_id;
-  CHECK(GetChildId(&process_id));
-  CHECK(GetRouteId(&view_id));
+  child_id_ = render_view_host()->process()->id();
+  route_id_ = render_view_host()->routing_id();
 
   // Register this with the ResourceDispatcherHost as a prerender
   // RenderViewHost. This must be done before the Navigate message to catch all
   // resource requests, but as it is on the same thread as the Navigate message
   // (IO) there is no race condition.
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      NewRunnableFunction(&AddChildRoutePair,
-                          g_browser_process->resource_dispatcher_host(),
-                          process_id, view_id));
+  PrerenderTracker::GetInstance()->OnPrerenderingStarted(child_id_, route_id_,
+                                                         prerender_manager_);
 
   // Close ourselves when the application is shutting down.
   notification_registrar_.Add(this, NotificationType::APP_TERMINATING,
@@ -299,22 +281,14 @@
 
 bool PrerenderContents::GetChildId(int* child_id) const {
   CHECK(child_id);
-  const RenderViewHost* prerender_render_view_host = render_view_host();
-  if (prerender_render_view_host) {
-    *child_id = prerender_render_view_host->process()->id();
-    return true;
-  }
-  return false;
+  *child_id = child_id_;
+  return child_id_ >= 0;

[dominich, 2011/05/19 15:49:33]

This should be a != -1 as you never expect a value < -1. You could DCHECK that
too.

[mmenke, 2011/05/19 16:40:42]

Done (for all the checks in this file, also updated the tracker's corresponding
DCHECKs)

 }
 
 bool PrerenderContents::GetRouteId(int* route_id) const {
   CHECK(route_id);
-  const RenderViewHost* prerender_render_view_host = render_view_host();
-  if (prerender_render_view_host) {
-    *route_id = prerender_render_view_host->routing_id();
-    return true;
-  }
-  return false;
+  *route_id = route_id_;
+  return route_id_ >= 0;

[dominich, 2011/05/19 15:49:33]

See above comment re != -1.

 }
 
 void PrerenderContents::set_final_status(FinalStatus final_status) {
@@ -343,21 +317,15 @@
     RecordFinalStatus(final_status_);
 
   if (render_view_host_ || prerender_contents_.get()) {
-    RenderViewHost* prerender_render_view_host = render_view_host_mutable();
-
-    int process_id = prerender_render_view_host->process()->id();
-    int view_id = prerender_render_view_host->routing_id();
-
-    BrowserThread::PostTask(
-        BrowserThread::IO, FROM_HERE,
-        NewRunnableFunction(&RemoveChildRoutePair,
-                            g_browser_process->resource_dispatcher_host(),
-                            process_id, view_id));
-
     // Only delete the RenderViewHost if we own it.
     if (render_view_host_)
       render_view_host_->Shutdown();
   }
+
+  if (child_id_ >= 0 && route_id_ >= 0) {
+    PrerenderTracker::GetInstance()->OnPrerenderingFinished(
+        child_id_, route_id_);
+  }
 }
 
 RenderViewHostDelegate::View* PrerenderContents::GetViewDelegate() {
@@ -681,6 +649,24 @@
   if (prerender_manager_->IsPendingDelete(this))
     return;
 
+  if (child_id_ >= 0 && route_id_ >= 0) {
+    // Cancel the prerender in the PrerenderTracker.  This is needed
+    // because destroy may be called directly from the UI thread without calling
+    // TryCancel().  This is difficult to completely avoid, since prerendering
+    // can be cancelled before a RenderView is created.
+    bool is_cancelled =
+        PrerenderTracker::GetInstance()->TryCancel(child_id_, route_id_,
+                                                   final_status);
+    DCHECK(is_cancelled);
+
+    // A different final status may have been set already from another thread.
+    // If so, use it instead.
+    if (!PrerenderTracker::GetInstance()->GetFinalStatus(child_id_, route_id_,
+                                                         &final_status)) {
+      NOTREACHED();

[dominich, 2011/05/19 15:49:33]

Can this not be CHECK(PrerenderTracker::GetInstance()->GetFinalStatus(...)) ?

[mmenke, 2011/05/19 16:40:42]

A CHECK results in a crash in release builds, which I prefer to avoid unless
we're about to crash anyways.

[dominich, 2011/05/19 16:55:47]

I thought NOTREACHED was a CHECK rather than a DCHECK. My bad.

+    }
+  }
+
   prerender_manager_->MoveEntryToPendingDelete(this);
   set_final_status(final_status);
   // We may destroy the PrerenderContents before we have initialized the