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);
+}
+
 // 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_(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_) {
       same_site_preconnected_ = std::max(same_site_preconnected_, count);
-    } else {
+    } else if (strict_) {
       ADD_FAILURE() << "Preconnected " << original_url
                     << " when should only be preconnecting the source host: "
                     << source_host_
                     << " or the cross site host: " << cross_site_host_;
     }
   }
 
   void OnLearnFromNavigation(const GURL& referring_url,
                              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_ &&
+               !(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_;
   }
 
+  // Optionally allows the object to observe preconnects / learning from other
+  // hosts.
+  void SetStrict(bool strict) {
+    base::AutoLock lock(lock_);
+    strict_ = 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. It will also allow
+  // preconnects to other hosts.
+  bool strict_;
+
   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();
   }
 
-  // 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)));
   }
 
   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() {
     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) {
+  GURL redirector_url = GURL("http://redirector.com");
+
+  NavigateToCrossSiteHtmlUrl(1 /* num_cors */, "" /* file_suffix */);
+  EXPECT_EQ(1, observer()->CrossSiteLearned());
+  EXPECT_EQ(0, observer()->CrossSitePreconnected());
+
+  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 with the redirector url as base url should redirect to the
+  // embedded_test_server_.
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::Bind(
+          &PredictorBrowserTest::StartInterceptingHostWithCreateJobCallback,
+          redirector_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()->SetStrict(false);
+
+  GURL redirecting_url = cross_site_test_server()->GetURL(base::StringPrintf(

[mmenke, 2016/05/25 18:16:04]

The URL doesn't redirect, it requests a subresource that does.  Maybe
redirect_requesting_url?  url_with_cors_subresources?

[Charlie Harrison, 2016/05/25 18:45:15]

Done.

+      "/predictor/"
+      "predictor_cross_site.html?subresourceHost=%s&numCORSResources=1",
+      redirector_url.spec().c_str()));
+  ui_test_utils::NavigateToURL(browser(), redirecting_url);
+  bool result = false;
+
+  int navigation_preconnects = observer()->CrossSitePreconnected();

[mmenke, 2016/05/25 18:16:04]

Should we call WaitForAcceptedConnectionsOnUI?  Looks like a couple other tests
aren't doing that before checking preconnects.

[Charlie Harrison, 2016/05/25 18:45:15]

I don't think we should here. We stopped blocking requests to navigate to the
cross site url, so it's not clear how many sockets will be accepted at this
point. In testing, it shows that 3 sockets are accepted after the navigation
finishes.

The other tests which don't wait have similar issues (e.g. don't block the cross
site test server).

+  EXPECT_EQ(2, navigation_preconnects);
+
+  EXPECT_TRUE(content::ExecuteScriptAndExtractBool(
+      browser()->tab_strip_model()->GetActiveWebContents(),
+      "startFetchesAndWaitForReply()", &result));
+  EXPECT_TRUE(result);
+
+  // The number of preconnects should not increase. Note that the predictor
+  // would preconnect 4 sockets if it were doing so based on learning.
+  EXPECT_EQ(navigation_preconnects, observer()->CrossSitePreconnected());
+}
+
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, PredictBasedOnSubframeRedirect) {
+  // A test server is needed here because data url navigations with redirect
+  // interceptors don't interact well with the ResourceTiming API.
+  // TODO(csharrison): Possibly this is a bug in either net or Blink, and it
+  // might be worthwhile to investigate.
+  std::unique_ptr<net::EmbeddedTestServer> redirector =
+      base::WrapUnique(new net::EmbeddedTestServer());
+  ASSERT_TRUE(redirector->Start());
+
+  NavigateToCrossSiteHtmlUrl(1 /* num_cors */, "" /* file_suffix */);
+  EXPECT_EQ(1, observer()->CrossSiteLearned());
+  EXPECT_EQ(0u, cross_site_connection_listener_->GetAcceptedSocketCount());
+
+  redirector->RegisterRequestHandler(
+      base::Bind(&RedirectForPathHandler, "/",
+                 embedded_test_server()->GetURL("/title1.html")));
+
+  // Note that the observer will see preconnects to the redirector, and the
+  // predictor will learn redirector->embedded_test_server.
+  observer()->SetStrict(false);
+
+  NavigateToDataURLWithContent(base::StringPrintf(
+      "<iframe src='%s'></iframe>", redirector->base_url().spec().c_str()));
+

[mmenke, 2016/05/25 18:16:04]

WaitForAcceptedConnectionsOnUI?

[Charlie Harrison, 2016/05/25 18:45:15]

We do the check after checking observer()->CrossSitePreconnected(). This is
because the observer will receive a preconnect notification before the load
event, so it will be guaranteed to happen at this point. Also, it will give
better error messages on failure.

+  EXPECT_EQ(4, observer()->CrossSitePreconnected());
+  cross_site_connection_listener_->WaitForAcceptedConnectionsOnUI(4u);
+}
+
 // 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