Refactor net predictor to use ResourceThrottles

chrome/browser/net/predictor_browsertest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
 #include <memory>
 
@@ skipping 49 matching lines @@
                                               net::NetworkDelegate* delegate) {
   const char kPlainTextHeaders[] =
       "HTTP/1.1 200 OK\n"
       "Content-Type: text/plain\n"
       "Access-Control-Allow-Origin: *\n"
       "\n";
   return new net::URLRequestTestJob(request, delegate, kPlainTextHeaders, "",
                                     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(
     const std::string& path,
     const GURL& redirect_url,
     const net::test_server::HttpRequest& request) {
   if (request.GetURL().path() != path)
     return nullptr;
   std::unique_ptr<net::test_server::BasicHttpResponse> response(
       new net::test_server::BasicHttpResponse);
@@ skipping 275 matching lines @@
 // two.
 class CrossSitePredictorObserver
     : public chrome_browser_net::PredictorObserver {
  public:
   CrossSitePredictorObserver(const GURL& source_host,
                              const GURL& cross_site_host)
       : source_host_(source_host),
         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,
       const GURL& first_party_for_cookies,
       chrome_browser_net::UrlInfo::ResolutionMotivation motivation,
       int count) override {
     base::AutoLock lock(lock_);
     if (original_url == cross_site_host_) {
       cross_site_preconnected_ = std::max(cross_site_preconnected_, count);
     } else if (original_url == source_host_) {
@@ skipping 10 matching lines @@
                              const GURL& target_url) override {
     base::AutoLock lock(lock_);
     // There are three possibilities:
     //     source => target
     //     source => source
     //     target => target
     if (referring_url == source_host_ && target_url == cross_site_host_) {
       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: "
                     << source_host_
                     << " or the cross site host: " << cross_site_host_;
     }
   }
 
   void ResetCounts() {
     base::AutoLock lock(lock_);
@@ skipping 10 matching lines @@
   int CrossSitePreconnected() {
     base::AutoLock lock(lock_);
     return cross_site_preconnected_;
   }
 
   int SameSitePreconnected() {
     base::AutoLock lock(lock_);
     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_;
 
   // Protects all following members. They are read and updated from different
   // threads.
   base::Lock lock_;
 
   int cross_site_learned_;
   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);
 };
 
 }  // namespace
 
 namespace chrome_browser_net {
 
 class PredictorBrowserTest : public InProcessBrowserTest {
  public:
   PredictorBrowserTest()
@@ skipping 35 matching lines @@
     embedded_test_server()->RegisterRequestHandler(
         base::Bind(&RedirectForPathHandler, "/",
                    cross_site_test_server()->GetURL("/title1.html")));
 
     predictor()->SetPreconnectEnabledForTest(true);
     InstallPredictorObserver(embedded_test_server()->base_url(),
                              cross_site_test_server()->base_url());
     StartInterceptingCrossSiteOnUI();
   }
 
+  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(), 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.
-  static void StartInterceptingHost(const GURL& url) {
-    DCHECK_CURRENTLY_ON(BrowserThread::IO);
-    net::URLRequestFilter::GetInstance()->AddHostnameInterceptor(
-        url.scheme(), url.host(),
-        base::WrapUnique(new MatchingPortRequestInterceptor(
-            url.EffectiveIntPort(), base::Bind(&CreateEmptyBodyRequestJob))));
-  }
-
-  static void StopInterceptingHost(const GURL& url) {
-    DCHECK_CURRENTLY_ON(BrowserThread::IO);
-    net::URLRequestFilter::GetInstance()->RemoveHostnameHandler(url.scheme(),
-                                                                url.host());
-  }
-
   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() {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
     content::BrowserThread::PostTask(
         content::BrowserThread::IO, FROM_HERE,
         base::Bind(&PredictorBrowserTest::StopInterceptingHost,
                    cross_site_test_server()->base_url()));
   }
 
@@ skipping 241 matching lines @@
   // embedded test servers have the same host_piece, an extra preconnect is
   // issued. This results in ceil(2.33) + 1 = 4 preconnects.
   ui_test_utils::NavigateToURL(browser(),
                                embedded_test_server()->GetURL("/title1.html"));
   // Just check that predictor has initiated preconnects to the cross site test
   // server. It's tricky to reset the connections to the test server, and
   // sockets can be reused.
   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(
       browser(), embedded_test_server()->GetURL(
                      "/predictor/predictor_cross_site_subframe_nav.html"));
   bool result = false;
   EXPECT_TRUE(content::ExecuteScriptAndExtractBool(
       browser()->tab_strip_model()->GetActiveWebContents(),
       base::StringPrintf(
           "navigateSubframe('%s')",
@@ skipping 459 matching lines @@
   // Second navigation to content with an img.
   std::string img_content =
       "<img src=\"" + preconnect_url.spec() + "test.gif\">";
   NavigateToDataURLWithContent(img_content);
   connection_listener_->WaitUntilFirstConnectionRead();
   EXPECT_EQ(2u, connection_listener_->GetAcceptedSocketCount());
   EXPECT_EQ(1u, connection_listener_->GetReadSocketCount());
 }
 
 }  // namespace chrome_browser_net