Add a browsertest suite for net predictor

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 0b1884856d51aaa37ca2f5757b48499e2ff05839..e69e630f67ee145fbfc42947129fcfd67d55e8c7 100644
--- a/chrome/browser/net/predictor_browsertest.cc
+++ b/chrome/browser/net/predictor_browsertest.cc
@@ -9,6 +9,7 @@
 #include "base/command_line.h"
 #include "base/json/json_string_value_serializer.h"
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "base/synchronization/lock.h"
 #include "base/thread_task_runner_handle.h"
 #include "build/build_config.h"
@@ -16,11 +17,13 @@
 #include "chrome/browser/net/predictor.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/prefs/pref_service.h"
 #include "content/public/common/content_switches.h"
+#include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_utils.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/ip_endpoint.h"
@@ -30,13 +33,38 @@
 #include "net/socket/stream_socket.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "net/test/embedded_test_server/embedded_test_server_connection_listener.h"
+#include "net/test/embedded_test_server/http_request.h"
+#include "net/test/embedded_test_server/http_response.h"
+#include "net/test/url_request/url_request_failed_job.h"
+#include "net/url_request/url_request_filter.h"
+#include "net/url_request/url_request_interceptor.h"
+#include "net/url_request/url_request_test_job.h"
 #include "testing/gmock/include/gmock/gmock.h"
+#include "url/gurl.h"
 
 using content::BrowserThread;
 using testing::HasSubstr;
 
 namespace {
 
+net::URLRequestJob* EmptyRequestJob(net::URLRequest* request,

[mmenke, 2016/05/06 19:54:01]

CreateEmptyBodyRequestJob?  The method itself isn't the job, and
ERR_EMPTY_RESPONSE means no headers, either.

[Charlie Harrison, 2016/05/09 19:47:05]

Done.

+                                    net::NetworkDelegate* delegate) {
+  const char kPlainTextHeaders[] =
+      "HTTP/1.1 200 OK\n"
+      "Content-Type: text/plain\n"
+      "Access-Control-Allow-Origin: *\n"
+      "Expires: Thu, 1 Jan 2100 20:00:00 GMT\n"

[mmenke, 2016/05/06 19:54:01]

I don't think we need the expires header?

[Charlie Harrison, 2016/05/09 19:47:04]

Done.

+      "\n";
+  return new net::URLRequestTestJob(
+      request, delegate,
+      std::string(kPlainTextHeaders, arraysize(kPlainTextHeaders)), "", true);

[mmenke, 2016/05/06 19:54:00]

std::string(kPlainTextHeaders, arraysize(kPlainTextHeaders)) can be replaced
with "kPlainTextHeaders", since the string doesn't have any nulls in it, and it
null terminated.

[Charlie Harrison, 2016/05/09 19:47:04]

Done.

+}
+
+net::URLRequestJob* NotFoundRequestJob(net::URLRequest* request,

[mmenke, 2016/05/06 19:54:01]

CreateFailingRequestJob?  (It's not a 404 response, or an ERR_NOT_FOUND
response, as this name impies)

[Charlie Harrison, 2016/05/09 19:47:03]

Done.

+                                       net::NetworkDelegate* delegate) {
+  return new net::URLRequestFailedJob(request, delegate, net::ERR_ABORTED);

[mmenke, 2016/05/06 19:54:01]

ERR_ABORTED is a magical error code, that net shouldn't really be returning
(Though it does, in some obscure cases).  Can we just use ERR_FAILED?

If this has to be ERR_ABORTED due to predictor logic, that should be found.

[Charlie Harrison, 2016/05/09 19:47:04]

Done.

+}
+
 const char kBlinkPreconnectFeature[] = "LinkPreconnect";
 const char kChromiumHostname[] = "chromium.org";
 const char kInvalidLongHostname[] = "illegally-long-hostname-over-255-"
@@ -54,7 +82,10 @@ const char kInvalidLongHostname[] = "illegally-long-hostname-over-255-"
 class ConnectionListener
     : public net::test_server::EmbeddedTestServerConnectionListener {
  public:
-  ConnectionListener() : task_runner_(base::ThreadTaskRunnerHandle::Get()) {}
+  ConnectionListener()
+      : accept_n_(0),
+        accept_n_loop_(nullptr),
+        task_runner_(base::ThreadTaskRunnerHandle::Get()) {}
 
   ~ConnectionListener() override {}
 
@@ -67,11 +98,16 @@ class ConnectionListener
 
     sockets_[socket] = SOCKET_ACCEPTED;
     task_runner_->PostTask(FROM_HERE, accept_loop_.QuitClosure());
+    CheckAccepted();
   }
 
   // Get called from the EmbeddedTestServer thread to be notified that
   // a connection was read from.
-  void ReadFromSocket(const net::StreamSocket& connection) override {
+  void ReadFromSocket(const net::StreamSocket& connection, int rv) override {
+    // Don't log a read if no data was transferred. This case often happens if
+    // the sockets of the test server are being flushed and disconnected.
+    if (rv <= 0)
+      return;
     base::AutoLock lock(lock_);
     uint16_t socket = GetPort(connection);
     EXPECT_FALSE(sockets_.find(socket) == sockets_.end());
@@ -98,9 +134,39 @@ class ConnectionListener
   }
 
   void WaitUntilFirstConnectionAccepted() { accept_loop_.Run(); }
-
   void WaitUntilFirstConnectionRead() { read_loop_.Run(); }
 
+  // The UI thread will wait for exactly |n| items in soeckts_.

[mmenke, 2016/05/06 19:54:01]

nit:  |sockets_|

[Charlie Harrison, 2016/05/09 19:47:04]

Done.

+  void WaitForAcceptedConnectionsOnUI(int n) {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+    base::RunLoop run_loop;
+    {
+      base::AutoLock lock(lock_);
+      accept_n_ = n;
+      accept_n_loop_ = &run_loop;
+      BrowserThread::PostTask(
+          BrowserThread::IO, FROM_HERE,
+          base::Bind(&ConnectionListener::CheckAcceptedLocked,
+                     base::Unretained(this)));
+    }
+    run_loop.Run();
+  }
+
+  void CheckAcceptedLocked() {
+    base::AutoLock lock(lock_);
+    CheckAccepted();
+  }
+
+  void CheckAccepted() {
+    if (accept_n_loop_ && accept_n_) {
+      if (accept_n_ == sockets_.size()) {
+        task_runner_->PostTask(FROM_HERE, accept_n_loop_->QuitClosure());
+        accept_n_loop_ = nullptr;
+        accept_n_ = 0;
+      }
+    }
+  }
+
  private:
   static uint16_t GetPort(const net::StreamSocket& connection) {
     // Get the remote port of the peer, since the local port will always be the
@@ -123,6 +189,12 @@ class ConnectionListener
 
   base::RunLoop accept_loop_;
   base::RunLoop read_loop_;
+
+  // If accept_n_ > 0, then the object is waiting for |accept_n_| sockets to be

[mmenke, 2016/05/06 19:54:01]

nit:  |accept_n_|

[Charlie Harrison, 2016/05/09 19:47:04]

Done.

+  // accepted before quiting the loop.
+  size_t accept_n_;
+  base::RunLoop* accept_n_loop_;
+
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
 
   mutable base::Lock lock_;
@@ -202,6 +274,114 @@ class HostResolutionRequestRecorder : public net::HostResolverProc {
   DISALLOW_COPY_AND_ASSIGN(HostResolutionRequestRecorder);
 };
 
+// This class intercepts URLRequests and responds with empty 200s. Note that the
+// port of the URL must match the port given in the constructor.

[mmenke, 2016/05/06 19:54:01]

Update comment.

[Charlie Harrison, 2016/05/09 19:47:04]

Done.

+class MatchingPortRequestInterceptor : public net::URLRequestInterceptor {
+ public:
+  MatchingPortRequestInterceptor(
+      int port,
+      base::Callback<net::URLRequestJob*(net::URLRequest*,
+                                         net::NetworkDelegate*)> job_callback)
+      : callback_(job_callback), port_(port) {}

[mmenke, 2016/05/06 19:54:01]

suggest a clearer name for both the argument and the member variable.  maybe
create_job_callback_(create_job_callback)

[Charlie Harrison, 2016/05/09 19:47:05]

Done.

+  ~MatchingPortRequestInterceptor() override {}
+
+ private:
+  net::URLRequestJob* MaybeInterceptRequest(
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate) const override {
+    if (request->url().EffectiveIntPort() != port_)
+      return nullptr;
+    return callback_.Run(request, network_delegate);
+  }

[mmenke, 2016/05/06 19:54:01]

nit:  Blank ine between methods and member variables.

[Charlie Harrison, 2016/05/09 19:47:04]

Done.

+  base::Callback<net::URLRequestJob*(net::URLRequest*, net::NetworkDelegate*)>
+      callback_;
+  int port_;

[mmenke, 2016/05/06 19:54:00]

nit: both these can be const

[Charlie Harrison, 2016/05/09 19:47:05]

Done.

+  DISALLOW_COPY_AND_ASSIGN(MatchingPortRequestInterceptor);

[mmenke, 2016/05/06 19:54:01]

nit:  Blank line before DISALLOW_COPY_AND_ASSIGN

[Charlie Harrison, 2016/05/09 19:47:04]

Done.

+};
+
+class CrossSitePredictorObserver

[mmenke, 2016/05/06 19:54:01]

Add description

[Charlie Harrison, 2016/05/09 19:47:04]

Done.

+    : 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) {}
+
+  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 {
+      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_ &&
+                 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_);
+    cross_site_learned_ = 0;
+    cross_site_preconnected_ = 0;
+    same_site_preconnected_ = 0;
+  }
+
+  int cross_site_learned() const {

[mmenke, 2016/05/06 19:54:00]

These should use the BigSlowFunctionNamingScheme because of the lock

[Charlie Harrison, 2016/05/09 19:47:04]

Done.

+    base::AutoLock lock(lock_);
+    return cross_site_learned_;
+  }
+
+  int cross_site_preconnected() const {
+    base::AutoLock lock(lock_);
+    return cross_site_preconnected_;
+  }
+
+  int same_site_preconnected() const {
+    base::AutoLock lock(lock_);
+    return same_site_preconnected_;
+  }
+
+ private:
+  const GURL source_host_;
+  const GURL cross_site_host_;
+
+  int cross_site_learned_;
+
+  int cross_site_preconnected_;
+  int same_site_preconnected_;
+  mutable base::Lock lock_;

[mmenke, 2016/05/06 19:54:00]

suggest not making this mutable, and getting rid of the consts - think the
mutable costs us more than the consts get us, in terms of cleanliness.

[Charlie Harrison, 2016/05/09 19:47:04]

Done.

+  DISALLOW_COPY_AND_ASSIGN(CrossSitePredictorObserver);

[mmenke, 2016/05/06 19:54:01]

nit:  Blank line before DISALLOW_COPY_AND_ASSIGN

[Charlie Harrison, 2016/05/09 19:47:04]

Done.

+};
+
 }  // namespace
 
 namespace chrome_browser_net {
@@ -212,7 +392,30 @@ class PredictorBrowserTest : public InProcessBrowserTest {
       : startup_url_("http://host1:1"),
         referring_url_("http://host2:1"),
         target_url_("http://host3:1"),
-        host_resolution_request_recorder_(new HostResolutionRequestRecorder) {
+        host_resolution_request_recorder_(new HostResolutionRequestRecorder),
+        cross_site_test_server_(new net::EmbeddedTestServer()) {}
+  void StartInterceptingCrossSite() {

[mmenke, 2016/05/06 19:54:00]

Should have thread-safety DCHECKs on all of these
(DCHECK_CURRENTLY_ON(BrowserThread::IO))

[Charlie Harrison, 2016/05/09 19:47:04]

Done. Put it on the most relevant methods.

+    net::URLRequestFilter::GetInstance()->AddHostnameInterceptor(
+        "http", cross_site_test_server()->GetURL("/").host(),
+        base::WrapUnique(new MatchingPortRequestInterceptor(
+            cross_site_test_server()->GetURL("/").EffectiveIntPort(),

[mmenke, 2016/05/06 19:54:01]

The test server's GetURL method isn't guaranteed to be threadsafe.  Suggest just
making this method static, and taking the URL as an argument.

[Charlie Harrison, 2016/05/09 19:47:05]

Done.

+            base::Bind(&EmptyRequestJob))));
+  }
+  void StopInterceptingCrossSite() {

[mmenke, 2016/05/06 19:54:00]

Suggest you make this static, per above comment.

[Charlie Harrison, 2016/05/09 19:47:04]

Done.

+    net::URLRequestFilter::GetInstance()->RemoveHostnameHandler(
+        "http", cross_site_test_server()->GetURL("/").host());
+  }
+  void StartInterceptingCrossSiteOnUI() {
+    content::BrowserThread::PostTask(
+        content::BrowserThread::IO, FROM_HERE,
+        base::Bind(&PredictorBrowserTest::StartInterceptingCrossSite,
+                   base::Unretained(this)));
+  }
+  void StopInterceptingCrossSiteOnUI() {
+    content::BrowserThread::PostTask(
+        content::BrowserThread::IO, FROM_HERE,
+        base::Bind(&PredictorBrowserTest::StopInterceptingCrossSite,
+                   base::Unretained(this)));
   }

[mmenke, 2016/05/06 19:54:01]

Suggest putting these 4 methods below SetUpOnIOThread - that way, all the set up
during test fixture creation stuff is together, at the top.

[mmenke, 2016/05/06 19:54:01]

nit:  Suggest a blank line between methods.

[Charlie Harrison, 2016/05/09 19:47:03]

Done.

[Charlie Harrison, 2016/05/09 19:47:04]

Done.

 
  protected:
@@ -229,10 +432,48 @@ class PredictorBrowserTest : public InProcessBrowserTest {
         switches::kEnableBlinkFeatures, kBlinkPreconnectFeature);
   }
 
+  // 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(

[mmenke, 2016/05/06 19:54:02]

This should be static.  Actually, could even just be moved out of the test
fixture.

[Charlie Harrison, 2016/05/09 19:47:04]

Done.

+      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);
+    response->set_code(net::HTTP_MOVED_PERMANENTLY);
+    response->AddCustomHeader("Location", redirect_url.spec());
+    return std::move(response);

[mmenke, 2016/05/06 19:54:01]

Is the std::move needed here?  May be because it's a
unique_ptr<net::test_server::BasicHttpResponse> and you're returning it as a
unique_ptr<net::test_server::HttpResponse>, but I'm not sure.

[Charlie Harrison, 2016/05/09 19:47:03]

Yeah it's needed.

+  }
+
   void SetUpOnMainThread() override {
+    cross_site_test_server_->ServeFilesFromSourceDirectory("chrome/test/data/");
+
     connection_listener_.reset(new ConnectionListener());
+    cross_site_connection_listener_.reset(new ConnectionListener());
     embedded_test_server()->SetConnectionListener(connection_listener_.get());
+    cross_site_test_server_->SetConnectionListener(
+        cross_site_connection_listener_.get());
     ASSERT_TRUE(embedded_test_server()->Start());
+    ASSERT_TRUE(cross_site_test_server_->Start());
+
+    embedded_test_server()->RegisterRequestHandler(base::Bind(
+        &PredictorBrowserTest::RedirectForPathHandler, base::Unretained(this),
+        "/", cross_site_test_server()->GetURL("/title1.html")));
+
+    predictor()->set_preconnect_enabled_for_test(true);
+    BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+                            base::Bind(&PredictorBrowserTest::SetUpOnIOThread,
+                                       base::Unretained(this)));
+  }
+
+  void SetUpOnIOThread() {
+    // Ignore all favicon requests. These are tricky to deal with because they
+    // are scheduled on the UI thread and are not accounted for in the load
+    // event. The interceptor will respond with 404s, and won't connect any
+    // sockets to the test server.
+    AddUrlInterceptor(cross_site_test_server()->GetURL("/favicon.ico"));
   }
 
   void TearDownOnMainThread() override {
@@ -254,29 +495,22 @@ class PredictorBrowserTest : public InProcessBrowserTest {
   }
 
   void LearnAboutInitialNavigation(const GURL& url) {
-    Predictor* predictor = browser()->profile()->GetNetworkPredictor();
-    BrowserThread::PostTask(BrowserThread::IO,
-                            FROM_HERE,
+    BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
                             base::Bind(&Predictor::LearnAboutInitialNavigation,
-                                       base::Unretained(predictor),
-                                       url));
+                                       base::Unretained(predictor()), url));
     content::RunAllPendingInMessageLoop(BrowserThread::IO);
   }
 
   void LearnFromNavigation(const GURL& referring_url, const GURL& target_url) {
-    Predictor* predictor = browser()->profile()->GetNetworkPredictor();
-    BrowserThread::PostTask(BrowserThread::IO,
-                            FROM_HERE,
-                            base::Bind(&Predictor::LearnFromNavigation,
-                                       base::Unretained(predictor),
-                                       referring_url,
-                                       target_url));
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&Predictor::LearnFromNavigation,
+                   base::Unretained(predictor()), referring_url, target_url));
     content::RunAllPendingInMessageLoop(BrowserThread::IO);
   }
 
   void PrepareFrameSubresources(const GURL& url) {
-    Predictor* predictor = browser()->profile()->GetNetworkPredictor();
-    predictor->PredictFrameSubresources(url, GURL());
+    predictor()->PredictFrameSubresources(url, GURL());
   }
 
   void GetListFromPrefsAsString(const char* list_path,
@@ -299,26 +533,467 @@ class PredictorBrowserTest : public InProcessBrowserTest {
     return host_resolution_request_recorder_->RequestedHostnameCount();
   }
 
+  net::EmbeddedTestServer* cross_site_test_server() {
+    return cross_site_test_server_.get();
+  }
+
+  Predictor* predictor() { return browser()->profile()->GetNetworkPredictor(); }
+
+  void InstallPredictorObserver() {
+    observer_.reset(
+        new CrossSitePredictorObserver(embedded_test_server()->base_url(),
+                                       cross_site_test_server()->base_url()));
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&PredictorBrowserTest::InstallPredictorObserverOnIOThread,
+                   base::Unretained(this), base::Unretained(predictor())));
+  }
+
+  void InstallPredictorObserverOnIOThread(
+      chrome_browser_net::Predictor* predictor) {
+    predictor->SetObserver(observer_.get());
+  }
+
+  CrossSitePredictorObserver* observer() { return observer_.get(); }
+
+  // Navigate to an html file on embedded_test_server and tell it to request
+  // |num_cors| resources from the cross_site_test_server. It then waits for
+  // those requests to complete. Note that "cors" here means using cors-mode in
+  // correspondence with the fetch spec.
+  void NavigateToCrossSiteHTMLURL(int num_cors, const char* file_suffix) {

[mmenke, 2016/05/06 19:54:00]

nit:  HtmlUrl (A lot of class and methods capitalize both words, but camel case
is now the preferred style).

[Charlie Harrison, 2016/05/09 19:47:05]

Done.

+    const GURL& base_url = cross_site_test_server()->base_url();
+    std::string path = base::StringPrintf(
+        "/predictor/"
+        "predictor_cross_site%s.html?subresourceHost=%s&"
+        "numCORSResources=%d",
+        file_suffix, base_url.spec().c_str(), num_cors);
+    ui_test_utils::NavigateToURL(browser(),
+                                 embedded_test_server()->GetURL(path));
+    bool result = false;
+    EXPECT_TRUE(content::ExecuteScriptAndExtractBool(
+        browser()->tab_strip_model()->GetActiveWebContents(),
+        "startFetchesAndWaitForReply()", &result));
+    EXPECT_TRUE(result);
+  }
+
+  static void AddUrlInterceptor(const GURL& url) {

[mmenke, 2016/05/06 19:54:00]

This method name doesn't make the behavior clear.  AddFailRequestInterceptor?

[mmenke, 2016/05/06 19:54:01]

I guess this is needed because failed requests aren't learned by the predictor? 
Where's that logic?  ChromeNetworkDelegate::OnBeforeURLRequest calls
WitnessURLRequest, which happens before requests make it to the interceptors.

[Charlie Harrison, 2016/05/09 19:47:04]

Actually, I think this could use the regular interceptor. This is an artifact of
previous patches. favicon is self-learned, so we don't interfere with cross site
learning. We only need to avoid connecting a socket.

+    net::URLRequestFilter::GetInstance()->AddUrlInterceptor(
+        url, base::WrapUnique(new MatchingPortRequestInterceptor(
+                 url.EffectiveIntPort(), base::Bind(&NotFoundRequestJob))));
+  }
+
   const GURL startup_url_;
   const GURL referring_url_;
   const GURL target_url_;
   std::unique_ptr<ConnectionListener> connection_listener_;
+  std::unique_ptr<ConnectionListener> cross_site_connection_listener_;
 
  private:
   scoped_refptr<HostResolutionRequestRecorder>
       host_resolution_request_recorder_;
   std::unique_ptr<net::ScopedDefaultHostResolverProc>
       scoped_host_resolver_proc_;
+  std::unique_ptr<net::EmbeddedTestServer> cross_site_test_server_;
+  std::unique_ptr<CrossSitePredictorObserver> observer_;
 };
 
-IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, PRE_ShutdownStartupCycle) {
+// Test the html test harness used to initiate cross site fetches. These
+// initiate cross site subresource requests to the cross site test server.
+// Inspect the predictor's internal state to make sure that they are properly
+// logged.
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, CrossSiteUseOneSocket) {
+  InstallPredictorObserver();
+  NavigateToCrossSiteHTMLURL(1 /* num_cors */, "" /* file_suffix */);
+  EXPECT_EQ(1u, cross_site_connection_listener_->GetAcceptedSocketCount());
+  EXPECT_EQ(1, observer()->cross_site_learned());
+  EXPECT_EQ(2, observer()->same_site_preconnected());
+}
+
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, CrossSiteUseThreeSockets) {
+  InstallPredictorObserver();
+  NavigateToCrossSiteHTMLURL(3 /* num_cors */, "" /* file_suffix */);
+  EXPECT_EQ(3u, cross_site_connection_listener_->GetAcceptedSocketCount());
+  EXPECT_EQ(3, observer()->cross_site_learned());
+  EXPECT_EQ(2, observer()->same_site_preconnected());
+}
+
+// The following tests confirm that Chrome accurately predicts preconnects after
+// learning from navigations. Note that every "learned" connection adds ~.33 to
+// the expected connection number, which starts at 2. Every preconnect Chrome
+// performs multiplies the expected connections by .66.
+//
+// In order to simplify things, many of the following tests intercept requests
+// and disable preconnects to the cross site test server. This allows the test
+// server to maintain a "clean slate" with no connected sockets, so that when
+// the tests need to check that predictor initiated preconnects occur, there are
+// no races and the connections are deterministic.

[mmenke, 2016/05/06 19:54:01]

Could we just wait until we see the expected number of sockets from a "clean
slate" state, and then flush them?  Turning the predictor off and then on just
seems a bit unusual from the usual case, from the predictor's standpoint. 
Coincidentally, it also gives us coverage of the number of preconnects we make
in the no-knowledge state.

+//
+// One additional complexity is that if a preconnect from A to B is learned, an
+// extra preconnect will be issued if the host part of A and B match (ignoring
+// port). TODO(csharrison): This logic could probably be removed from the
+// predictor.
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest,
+                       CrossSiteSimplePredictionAfterOneNavigation) {
+  predictor()->set_preconnect_enabled_for_test(false);
+  StartInterceptingCrossSiteOnUI();
+
+  InstallPredictorObserver();
+  NavigateToCrossSiteHTMLURL(2 /* num_cors */, "" /* file_suffix */);
+  EXPECT_EQ(2, observer()->cross_site_learned());
+
+  StopInterceptingCrossSiteOnUI();
+  predictor()->set_preconnect_enabled_for_test(true);
+  EXPECT_EQ(0u, cross_site_connection_listener_->GetAcceptedSocketCount());
+
+  // Navigate again and confirm a preconnect. Note that because the two
+  // embedded test servers have the same host_piece, an extra preconnect is
+  // issued. This results in ceil(2.66) + 1 = 4 preconnects.
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/title1.html"));
+  EXPECT_EQ(4, observer()->cross_site_preconnected());
+  cross_site_connection_listener_->WaitForAcceptedConnectionsOnUI(4u);
+}
+
+// This test does not intercept initial requests / preconnects to the cross site
+// test server.
+IN_PROC_BROWSER_TEST_F(
+    PredictorBrowserTest,
+    CrossSiteSimplePredictionAfterOneNavigationNoInterceptor) {
+  InstallPredictorObserver();
+  NavigateToCrossSiteHTMLURL(1 /* num_cors */, "" /* file_suffix */);
+  EXPECT_EQ(1, observer()->cross_site_learned());
+  cross_site_connection_listener_->WaitForAcceptedConnectionsOnUI(1u);
+
+  // Navigate again and confirm a preconnect. Note that because the two
+  // 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()->cross_site_preconnected());
+}
+
+// Expect that the predictor correctly predicts subframe navigations.
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, SubframeCrossSitePrediction) {
+  InstallPredictorObserver();
+  predictor()->set_preconnect_enabled_for_test(false);
+  StartInterceptingCrossSiteOnUI();
+
+  InstallPredictorObserver();
+  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')",
+          cross_site_test_server()->GetURL("/title1.html").spec().c_str()),
+      &result));
+  EXPECT_TRUE(result);
+  EXPECT_EQ(1, observer()->cross_site_learned());
+
+  StopInterceptingCrossSiteOnUI();
+  predictor()->set_preconnect_enabled_for_test(true);
+  EXPECT_EQ(0u, cross_site_connection_listener_->GetAcceptedSocketCount());
+
+  // Navigate again and confirm a preconnect. Note that because the two
+  // 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"));
+  EXPECT_EQ(4, observer()->cross_site_preconnected());
+  cross_site_connection_listener_->WaitForAcceptedConnectionsOnUI(4u);
+}
+
+// Expect that the predictor correctly preconnects an already learned resource
+// if the host shows up in a subframe.
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, SubframeInitiatesPreconnects) {
+  InstallPredictorObserver();
+  predictor()->set_preconnect_enabled_for_test(false);
+  StartInterceptingCrossSiteOnUI();
+
+  // Navigate to the normal cross site URL to learn the relationship.
+  NavigateToCrossSiteHTMLURL(1 /* num_cors */, "" /* file_suffix */);
+  EXPECT_EQ(1, observer()->cross_site_learned());
+
+  predictor()->set_preconnect_enabled_for_test(true);
+  StopInterceptingCrossSiteOnUI();
+  EXPECT_EQ(0u, cross_site_connection_listener_->GetAcceptedSocketCount());
+
+  // Navigate again and confirm a preconnect. Note that because the two
+  // embedded test servers have the same host_piece, an extra preconnect is
+  // issued. This results in ceil(2 + .33) + 1 = 4 preconnects.
+  NavigateToDataURLWithContent(
+      "<iframe src=\"" + embedded_test_server()->GetURL("/title1.html").spec() +
+      "\"></iframe>");
+  EXPECT_EQ(4, observer()->cross_site_preconnected());
+  cross_site_connection_listener_->WaitForAcceptedConnectionsOnUI(4u);
+}
+
+// Expect that the predictor correctly learns the subresources a subframe needs
+// to preconnect to.
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, SubframeLearning) {
+  InstallPredictorObserver();
+  predictor()->set_preconnect_enabled_for_test(false);
+  StartInterceptingCrossSiteOnUI();
+
+  std::string path = base::StringPrintf(
+      "/predictor/"
+      "predictor_cross_site.html?subresourceHost=%s&"
+      "numCORSResources=1&sendImmediately=1",
+      cross_site_test_server()->base_url().spec().c_str());
+  NavigateToDataURLWithContent(
+      base::StringPrintf("<iframe src=\"%s\"></iframe>",
+                         embedded_test_server()->GetURL(path).spec().c_str()));
+  EXPECT_EQ(1, observer()->cross_site_learned());
+
+  StopInterceptingCrossSiteOnUI();
+  predictor()->set_preconnect_enabled_for_test(true);
+  EXPECT_EQ(0u, cross_site_connection_listener_->GetAcceptedSocketCount());
+
+  // Navigate again and confirm a preconnect. Note that because the two
+  // embedded test servers have the same host_piece, an extra preconnect is
+  // issued. This results in ceil(2 + .33) + 1 = 4 preconnects.
+  NavigateToDataURLWithContent(
+      "<iframe src=\"" + embedded_test_server()->GetURL("/title1.html").spec() +
+      "\"></iframe>");
+  cross_site_connection_listener_->WaitForAcceptedConnectionsOnUI(4u);
+  EXPECT_EQ(4u, cross_site_connection_listener_->GetAcceptedSocketCount());
+  EXPECT_EQ(4, observer()->cross_site_preconnected());
+}
+
+// This tests the implementation details of the predictor. The current predictor
+// only learns via the referrer header.
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, CrossSiteNoReferrerNoLearning) {
+  InstallPredictorObserver();
+  NavigateToCrossSiteHTMLURL(1 /* num_cors */,
+                             "_no_referrer" /* file_suffix */);
+  cross_site_connection_listener_->WaitUntilFirstConnectionAccepted();
+  EXPECT_EQ(1u, cross_site_connection_listener_->GetAcceptedSocketCount());
+  EXPECT_EQ(0, observer()->cross_site_learned());
+  EXPECT_EQ(2, observer()->same_site_preconnected());
+}
+
+// This test navigates to an html file with a tag:
+// <meta name="referrer" content="never">
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest,
+                       CrossSiteNoReferrerNoPredictionAfterOneNavigation) {
+  InstallPredictorObserver();
+  predictor()->set_preconnect_enabled_for_test(false);
+  StartInterceptingCrossSiteOnUI();
+
+  NavigateToCrossSiteHTMLURL(2 /* num_cors */,
+                             "_no_referrer" /* file_suffix */);
+  EXPECT_EQ(0, observer()->cross_site_learned());
+
+  StopInterceptingCrossSiteOnUI();
+  predictor()->set_preconnect_enabled_for_test(false);
+  EXPECT_EQ(0u, cross_site_connection_listener_->GetAcceptedSocketCount());
+
+  // Navigate again and confirm that no preconnects occurred.
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/title1.html"));
+  EXPECT_EQ(0, observer()->cross_site_preconnected());
+  EXPECT_EQ(0u, cross_site_connection_listener_->GetAcceptedSocketCount());
+}
+
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest,
+                       CrossSiteSimplePredictionAfterTwoNavigations) {
+  InstallPredictorObserver();
+  predictor()->set_preconnect_enabled_for_test(false);
+  StartInterceptingCrossSiteOnUI();
+
+  NavigateToCrossSiteHTMLURL(1 /* num_cors */, "" /* file_suffix */);
+  NavigateToCrossSiteHTMLURL(1 /* num_cors */, "" /* file_suffix */);
+
+  StopInterceptingCrossSiteOnUI();
+  predictor()->set_preconnect_enabled_for_test(true);
+  EXPECT_EQ(0u, cross_site_connection_listener_->GetAcceptedSocketCount());
+
+  // Navigate again and confirm a preconnect. Note that because the two
+  // embedded test servers have the same host_piece, an extra preconnect is
+  // issued. This results in ceil((2 + .33) + .33) + 1 ~= 4 preconnects.
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/title1.html"));
+  EXPECT_EQ(4, observer()->cross_site_preconnected());
+  cross_site_connection_listener_->WaitForAcceptedConnectionsOnUI(4u);
+}
+
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest,
+                       CrossSiteSimplePredictionAfterTwoNavigations2) {
+  InstallPredictorObserver();
+  predictor()->set_preconnect_enabled_for_test(false);
+  StartInterceptingCrossSiteOnUI();
+
+  NavigateToCrossSiteHTMLURL(2 /* num_cors */, "" /* file_suffix */);
+  NavigateToCrossSiteHTMLURL(2 /* num_cors */, "" /* file_suffix */);
+
+  StopInterceptingCrossSiteOnUI();
+  predictor()->set_preconnect_enabled_for_test(true);
+  EXPECT_EQ(0u, cross_site_connection_listener_->GetAcceptedSocketCount());
+
+  // ((2 + .66) + .66) + 1 ~= 4.2.
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/title1.html"));
+  EXPECT_EQ(5, observer()->cross_site_preconnected());
+  cross_site_connection_listener_->WaitForAcceptedConnectionsOnUI(5u);
+}
+
+// The first navigation uses a subresource. Subsequent navigations don't use
+// that subresource. This tests how the predictor forgets about these bad
+// navigations.
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, ForgetBadPrediction) {
+  InstallPredictorObserver();
+
+  NavigateToCrossSiteHTMLURL(1 /* num_cors */, "" /* file_suffix */);
+  EXPECT_EQ(1, observer()->cross_site_learned());
+
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/title1.html"));
+  // (2 + .33) + 1 = 3.33.
+  EXPECT_EQ(4, observer()->cross_site_preconnected());
+  observer()->ResetCounts();
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/title1.html"));
+  // ceil((2 + .33) * .66) + 1 = 3.
+  EXPECT_EQ(3, observer()->cross_site_preconnected());
+  observer()->ResetCounts();
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/title1.html"));
+  // ceil((2 + .33) * .66 * .66) + 1 = 3.
+  EXPECT_EQ(3, observer()->cross_site_preconnected());
+  observer()->ResetCounts();
+
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/title1.html"));
+  // Finally, (2 + .33) * .66^3 ~= .67. Not enough for a preconnect.
+  EXPECT_EQ(0, observer()->cross_site_preconnected());
+}
+
+// The predictor does not follow redirects if the original url had a non-empty
+// path (a path that was more than just "/").
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest,
+                       CrossSiteRedirectNoPredictionWithPath) {
+  InstallPredictorObserver();
+  predictor()->set_preconnect_enabled_for_test(false);
+  StartInterceptingCrossSiteOnUI();
+
+  ui_test_utils::NavigateToURL(
+      browser(),
+      embedded_test_server()->GetURL(base::StringPrintf(
+          "/server-redirect?%s",
+          cross_site_test_server()->GetURL("/title1.html").spec().c_str())));
+  EXPECT_EQ(0, observer()->cross_site_learned());
+
+  StopInterceptingCrossSiteOnUI();
+  predictor()->set_preconnect_enabled_for_test(true);
+  EXPECT_EQ(0u, cross_site_connection_listener_->GetAcceptedSocketCount());
+
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/title1.html"));
+  EXPECT_EQ(0u, cross_site_connection_listener_->GetAcceptedSocketCount());
+  EXPECT_EQ(0, observer()->cross_site_preconnected());
+}
+
+// The predictor does follow redirects if the original url had an empty path
+// (a path that was more than just "/"). Use the registered "/" path to redirect
+// to the target test server.
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest,
+                       CrossSiteRedirectPredictionWithNoPath) {
+  InstallPredictorObserver();
+  predictor()->set_preconnect_enabled_for_test(false);
+  StartInterceptingCrossSiteOnUI();
+
+  ui_test_utils::NavigateToURL(browser(), embedded_test_server()->GetURL("/"));
+  EXPECT_EQ(1, observer()->cross_site_learned());
+
+  StopInterceptingCrossSiteOnUI();
+  predictor()->set_preconnect_enabled_for_test(true);
+  EXPECT_EQ(0u, cross_site_connection_listener_->GetAcceptedSocketCount());
+
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/title1.html"));
+  // Preconnect 4 sockets because ceil(2 + .33) + 1 = 4.
+  EXPECT_EQ(4, observer()->cross_site_preconnected());
+  cross_site_connection_listener_->WaitForAcceptedConnectionsOnUI(4u);
+}
+
+// This test uses "localhost" instead of "127.0.0.1" to avoid extra preconnects
+// to hosts with the same host piece (ignoring port). Note that the preconnect
+// observer is not used here due to its strict checks on hostname.
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest,
+                       CrossSiteRedirectPredictionWithNoPathDifferentHostName) {
+  predictor()->set_preconnect_enabled_for_test(false);
+  StartInterceptingCrossSiteOnUI();
+
+  GURL localhost_source = GURL(base::StringPrintf(
+      "http://localhost:%s/predictor/"
+      "predictor_cross_site.html?subresourceHost=%s&numCORSResources=1",
+      embedded_test_server()->base_url().port().c_str(),
+      cross_site_test_server()->base_url().spec().c_str()));
+  ui_test_utils::NavigateToURL(browser(), localhost_source);
+  bool result = false;
+  EXPECT_TRUE(content::ExecuteScriptAndExtractBool(
+      browser()->tab_strip_model()->GetActiveWebContents(),
+      "startFetchesAndWaitForReply()", &result));
+  EXPECT_TRUE(result);
+
+  StopInterceptingCrossSiteOnUI();
+  predictor()->set_preconnect_enabled_for_test(true);
+  EXPECT_EQ(0u, cross_site_connection_listener_->GetAcceptedSocketCount());
+
+  ui_test_utils::NavigateToURL(
+      browser(), GURL(base::StringPrintf(
+                     "http://localhost:%s/title1.html",
+                     embedded_test_server()->base_url().port().c_str())));
+  // Preconnect 3 sockets because ceil(2 + .33) = 3. Note that this time there
+  // is no additional connection due to same host.
+  cross_site_connection_listener_->WaitForAcceptedConnectionsOnUI(3u);
+}
+
+// Perform the "/" redirect twice and make sure the predictor updates twice.
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest,
+                       CrossSiteTwoRedirectsPredictionWithNoPath) {
+  InstallPredictorObserver();
+  predictor()->set_preconnect_enabled_for_test(false);
+  StartInterceptingCrossSiteOnUI();
+
+  ui_test_utils::NavigateToURL(browser(), embedded_test_server()->GetURL("/"));
+  EXPECT_EQ(1, observer()->cross_site_learned());
+
+  ui_test_utils::NavigateToURL(browser(), embedded_test_server()->GetURL("/"));
+  EXPECT_EQ(2, observer()->cross_site_learned());
+
+  StopInterceptingCrossSiteOnUI();
+  predictor()->set_preconnect_enabled_for_test(true);
+  EXPECT_EQ(0u, cross_site_connection_listener_->GetAcceptedSocketCount());
+
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/title1.html"));
+  // 4 preconnects expected because (2 + .33) + .33 + 1 = 3.66.
+  EXPECT_EQ(4, observer()->cross_site_preconnected());
+  cross_site_connection_listener_->WaitForAcceptedConnectionsOnUI(4u);
+}
+
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest,
+                       PRE_ShutdownStartupCyclePreresolve) {
   // Prepare state that will be serialized on this shut-down and read on next
-  // start-up.
+  // start-up. Ensure preresolution over preconnection.
   LearnAboutInitialNavigation(startup_url_);
+  // The target url will have a expected connection count of 2 after this call.
   LearnFromNavigation(referring_url_, target_url_);
+
+  // In order to reduce the expected connection count < .8, issue predictions 3
+  // times. 2 * .66^3 ~= .58.
+  PrepareFrameSubresources(referring_url_);
+  PrepareFrameSubresources(referring_url_);
+  PrepareFrameSubresources(referring_url_);
 }
 
-IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, ShutdownStartupCycle) {
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, ShutdownStartupCyclePreresolve) {
   // Make sure that the Preferences file is actually wiped of all DNS prefetch
   // related data after start-up.
   std::string cleared_startup_list;
@@ -462,4 +1137,28 @@ IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, PreconnectCORSAndFetchNonCORS) {
   EXPECT_EQ(1u, connection_listener_->GetReadSocketCount());
 }
 
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, SimplePreconnectOne) {

[mmenke, 2016/05/06 19:54:00]

Suggest putting these first (Think that putting the more basic tests before the
ones that depend on it can help with picking the best test to start debugging
when you run into a regression).

[Charlie Harrison, 2016/05/09 19:47:04]

Done.

+  predictor()->PreconnectUrl(
+      embedded_test_server()->base_url(), GURL(),
+      UrlInfo::ResolutionMotivation::EARLY_LOAD_MOTIVATED,
+      false /* allow credentials */, 1);
+  connection_listener_->WaitForAcceptedConnectionsOnUI(1u);
+}
+
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, SimplePreconnectTwo) {
+  predictor()->PreconnectUrl(
+      embedded_test_server()->base_url(), GURL(),
+      UrlInfo::ResolutionMotivation::EARLY_LOAD_MOTIVATED,
+      false /* allow credentials */, 2);
+  connection_listener_->WaitForAcceptedConnectionsOnUI(2u);
+}
+
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, SimplePreconnectFour) {
+  predictor()->PreconnectUrl(
+      embedded_test_server()->base_url(), GURL(),
+      UrlInfo::ResolutionMotivation::EARLY_LOAD_MOTIVATED,
+      false /* allow credentials */, 4);
+  connection_listener_->WaitForAcceptedConnectionsOnUI(4u);
+}
+
 }  // namespace chrome_browser_net