Only commit cookie changes in prerenders after a prerender is shown

chrome/browser/chrome_content_browser_client.cc

Index: chrome/browser/chrome_content_browser_client.cc
===================================================================
--- chrome/browser/chrome_content_browser_client.cc	(revision 265252)
+++ chrome/browser/chrome_content_browser_client.cc	(working copy)
@@ -635,7 +635,8 @@
 
 namespace chrome {
 
-ChromeContentBrowserClient::ChromeContentBrowserClient() {
+ChromeContentBrowserClient::ChromeContentBrowserClient()
+    : prerender_tracker_(NULL) {
 #if defined(ENABLE_PLUGINS)
   for (size_t i = 0; i < arraysize(kPredefinedAllowedFileHandleOrigins); ++i)
     allowed_file_handle_origins_.insert(kPredefinedAllowedFileHandleOrigins[i]);
@@ -1207,6 +1208,20 @@
       privilege_required;
 }
 
+bool ChromeContentBrowserClient::MayReuseHost(
+    content::RenderProcessHost* process_host) {
+  // If there is currently a prerender in progress for the host provided,
+  // it may not be shared. We require prerenders to be by themselves in a
+  // separate process, so that we can monitor their resource usage, and so that
+  // we can track the cookies that they change.
+  if (prerender_tracker_ &&
+      prerender_tracker_->IsProcessPrerendering(process_host)) {

[Charlie Reis, 2014/05/07 17:21:32]

Can we add a DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)) in this
function?

[tburkard, 2014/05/07 18:34:27]

Done. It is actually on UI, not on IO.

+    return false;
+  }
+
+  return true;
+}
+
 // This function is trying to limit the amount of processes used by extensions
 // with background pages. It uses a globally set percentage of processes to
 // run such extensions and if the limit is exceeded, it returns true, to
@@ -1711,6 +1726,9 @@
   CookieSettings* cookie_settings = io_data->GetCookieSettings();
   bool allow = cookie_settings->IsSettingCookieAllowed(url, first_party);
 
+  if (prerender_tracker_)
+    prerender_tracker_->OnCookieChangedForURL(render_process_id, url);
+
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(&TabSpecificContentSettings::CookieChanged, render_process_id,
@@ -2196,6 +2214,8 @@
 }
 
 void ChromeContentBrowserClient::ResourceDispatcherHostCreated() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  prerender_tracker_ = g_browser_process->prerender_tracker();
   return g_browser_process->ResourceDispatcherHostCreated();

[davidben, 2014/05/07 19:57:00]

I think we're fine with this threading behavior, though it's a little screwy.
This hook is called in RDH's ctor. The RDH is created on the UI thread, calls
this hook, and then posts to inform the IO thread.

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo...


If there were a race here, we'd have quite a lot of other problems.
BrowserProcessImpl::ResourceDispatcherHostCreated initializes other things for
the IO thread:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro...

The RDH's delegate gets set in there. That delegate also gets passed the
PrerenderTracker which does the same thing as here. (Initialize on UI, use on
IO.)

(Oh, for more fun, BrowserProcessImpl::prerender_tracker() isn't actually
thread-safe despite looking like an accessor.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro...
)

[tburkard, 2014/05/08 10:59:54]

Done.

 }
 
@@ -2699,6 +2719,16 @@
 #endif
 }
 
+net::CookieStore*
+ChromeContentBrowserClient::OverrideCookieStoreForRenderProcess(
+    int render_process_id) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  if (!prerender_tracker_)
+    return NULL;
+  return prerender_tracker_->
+      GetPrerenderCookieStoreForRenderProcess(render_process_id);
+}
+
 #if defined(ENABLE_WEBRTC)
 void ChromeContentBrowserClient::MaybeCopyDisableWebRtcEncryptionSwitch(
     CommandLine* to_command_line,