Refactor net predictor to use ResourceThrottles

chrome/browser/net/predictor_browsertest.cc

Index: chrome/browser/net/predictor_browsertest.cc
diff --git a/chrome/browser/net/predictor_browsertest.cc b/chrome/browser/net/predictor_browsertest.cc
index be8568ae32814a97956936b9c6ac16cc3cb4b7b7..0b1e8de37f7476a85e3bc99afb7664c9ed17af3f 100644
--- a/chrome/browser/net/predictor_browsertest.cc
+++ b/chrome/browser/net/predictor_browsertest.cc
@@ -67,6 +67,20 @@ net::URLRequestJob* CreateEmptyBodyRequestJob(net::URLRequest* request,
                                     true);
 }
 
+net::URLRequestJob* CreateRedirectRequestJob(std::string location,
+                                             net::URLRequest* request,
+                                             net::NetworkDelegate* delegate) {
+  char kPlainTextHeaders[] =
+      "HTTP/1.1 302 \n"
+      "Location: %s\n"
+      "Access-Control-Allow-Origin: *\n"
+      "\n";
+  return new net::URLRequestTestJob(
+      request, delegate,
+      base::StringPrintf(kPlainTextHeaders, location.c_str()), "", true);
+}
+
+

[mmenke, 2016/05/24 20:27:34]

Here's another extra line break for you to remove. Have to get them early, or
they'll grow to encompass the entire chrome code base!

[Charlie Harrison, 2016/05/24 21:43:22]

Done.

 // Override the test server to redirect requests matching some path. This is
 // used because the predictor only learns simple redirects with a path of "/"
 std::unique_ptr<net::test_server::HttpResponse> RedirectForPathHandler(
@@ -362,7 +376,8 @@ class CrossSitePredictorObserver
         cross_site_host_(cross_site_host),
         cross_site_learned_(0),
         cross_site_preconnected_(0),
-        same_site_preconnected_(0) {}
+        same_site_preconnected_(0),
+        strict_learning_checking_(true) {}
 
   void OnPreconnectUrl(
       const GURL& original_url,
@@ -393,7 +408,8 @@ class CrossSitePredictorObserver
       cross_site_learned_++;
     } else if (referring_url == source_host_ && target_url == source_host_) {
       // Same site learned. Branch retained for clarity.
-    } else if (!(referring_url == cross_site_host_ &&
+    } else if (strict_learning_checking_ &&
+               !(referring_url == cross_site_host_ &&
                  target_url == cross_site_host_)) {
       ADD_FAILURE() << "Learned " << referring_url << " => " << target_url
                     << " when should only be learning the source host: "
@@ -424,6 +440,11 @@ class CrossSitePredictorObserver
     return same_site_preconnected_;
   }
 
+  void SetStrictLearningChecking(bool strict) {
+    base::AutoLock lock(lock_);
+    strict_learning_checking_ = strict;
+  }
+
  private:
   const GURL source_host_;
   const GURL cross_site_host_;
@@ -436,6 +457,10 @@ class CrossSitePredictorObserver
   int cross_site_preconnected_;
   int same_site_preconnected_;
 
+  // This member can be set to optionally allow url learning other than from
+  // source => source, source => target, or target => target.
+  bool strict_learning_checking_;
+
   DISALLOW_COPY_AND_ASSIGN(CrossSitePredictorObserver);
 };
 
@@ -491,31 +516,36 @@ class PredictorBrowserTest : public InProcessBrowserTest {
     StartInterceptingCrossSiteOnUI();
   }
 
-  // Intercepts all requests to the specified host and returns a response with
-  // an empty body. Needed to prevent requests from actually going to the test
-  // server, to avoid any races related to socket accounting. Note, the
-  // interceptor also looks at the port, to differentiate between the
-  // two test servers.
-  static void StartInterceptingHost(const GURL& url) {
+  static void StartInterceptingHostWithCreateJobCallback(
+      const GURL& url,
+      const MatchingPortRequestInterceptor::CreateJobCallback& callback) {
     DCHECK_CURRENTLY_ON(BrowserThread::IO);
     net::URLRequestFilter::GetInstance()->AddHostnameInterceptor(
         url.scheme(), url.host(),
         base::WrapUnique(new MatchingPortRequestInterceptor(
-            url.EffectiveIntPort(), base::Bind(&CreateEmptyBodyRequestJob))));
+            url.EffectiveIntPort(), callback)));
   }
 
+

[Charlie Harrison, 2016/05/23 21:13:32]

Will remove this line.

[Charlie Harrison, 2016/05/24 21:43:22]

Done.

   static void StopInterceptingHost(const GURL& url) {
     DCHECK_CURRENTLY_ON(BrowserThread::IO);
     net::URLRequestFilter::GetInstance()->RemoveHostnameHandler(url.scheme(),
                                                                 url.host());
   }
 
+  // Intercepts all requests to the specified host and returns a response with
+  // an empty body. Needed to prevent requests from actually going to the test
+  // server, to avoid any races related to socket accounting. Note, the
+  // interceptor also looks at the port, to differentiate between the
+  // two test servers.
   void StartInterceptingCrossSiteOnUI() {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
     content::BrowserThread::PostTask(
         content::BrowserThread::IO, FROM_HERE,
-        base::Bind(&PredictorBrowserTest::StartInterceptingHost,
-                   cross_site_test_server()->base_url()));
+        base::Bind(
+            &PredictorBrowserTest::StartInterceptingHostWithCreateJobCallback,
+            cross_site_test_server()->base_url(),
+            base::Bind(&CreateEmptyBodyRequestJob)));
   }
 
   void StopInterceptingCrossSiteOnUI() {
@@ -777,6 +807,56 @@ IN_PROC_BROWSER_TEST_F(
   EXPECT_EQ(4, observer()->CrossSitePreconnected());
 }
 
+// 1. Navigate to A.com learning B.com
+// 2. Navigate to B.com with subresource from C.com redirecting to A.com.
+// 3. Assert that the redirect does not cause us to preconnect to B.com (via
+// A.com).
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, DontPredictBasedOnSubresources) {
+  std::unique_ptr<net::EmbeddedTestServer> redirector =
+      base::WrapUnique(new net::EmbeddedTestServer());
+  redirector->ServeFilesFromSourceDirectory("chrome/test/data/");

[mmenke, 2016/05/24 20:27:34]

I don't think we actually need a test server here, do we?  We can just use the
URL http://redirector/predictor/empty.js", right?

[Charlie Harrison, 2016/05/24 21:43:22]

Right. Done.

+  ASSERT_TRUE(redirector->Start());
+
+  NavigateToCrossSiteHtmlUrl(1 /* num_cors */, "" /* file_suffix */);
+  EXPECT_EQ(1, observer()->CrossSiteLearned());
+
+  EXPECT_EQ(0u, cross_site_connection_listener_->GetAcceptedSocketCount());
+
+  // Stop intercepting so that the test can actually navigate to the cross site
+  // server.
+  StopInterceptingCrossSiteOnUI();
+
+  // All requests to the redirector server should redirect to the

[mmenke, 2016/05/24 20:27:34]

-"the"?  There are multiple embedded test servers here.

[Charlie Harrison, 2016/05/24 21:43:22]

Done.

+  // embedded_test_server_.
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::Bind(
+          &PredictorBrowserTest::StartInterceptingHostWithCreateJobCallback,
+          redirector->base_url(),
+          base::Bind(
+              &CreateRedirectRequestJob,
+              embedded_test_server()->GetURL("/predictor/empty.js").spec())));
+
+  // Reduce the strictness, because the below logic causes the predictor to
+  // learn cross_site_test_server_ => redirector, as well as
+  // cross_site_test_server_ => embedded_test_server_ (via referrer header).
+  observer()->SetStrictLearningChecking(false);
+
+  GURL redirecting_url = cross_site_test_server()->GetURL(base::StringPrintf(
+      "/predictor/"
+      "predictor_cross_site.html?subresourceHost=%s&numCORSResources=1",
+      redirector->base_url().spec().c_str()));
+  ui_test_utils::NavigateToURL(browser(), redirecting_url);
+  bool result = false;
+  EXPECT_TRUE(content::ExecuteScriptAndExtractBool(
+      browser()->tab_strip_model()->GetActiveWebContents(),
+      "startFetchesAndWaitForReply()", &result));
+  EXPECT_TRUE(result);
+
+  // The number of preconnects should not increase.
+  EXPECT_EQ(1, observer()->CrossSitePreconnected());

[mmenke, 2016/05/24 20:27:34]

Hrm...Should we make a subframe test that matches this?  It's kind of nice to be
able to look at the above test and say, "Aha, we aren't doing bonus
preconnections on subresource redirects, but we are on subframe redirects".  We
do have subframe tests, and redirect tests already, though, I suppose...

[Charlie Harrison, 2016/05/24 21:43:22]

Done. I also fixed this test up a little.

+}
+
 // Expect that the predictor correctly predicts subframe navigations.
 IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, SubframeCrossSitePrediction) {
   ui_test_utils::NavigateToURL(