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 b06e4ef037cbcde697c896c7965a5d2c9b4b28e6..5d1afafc80af3a08ef880de85db617f83a819546 100644
--- a/chrome/browser/net/predictor_browsertest.cc
+++ b/chrome/browser/net/predictor_browsertest.cc
@@ -16,11 +16,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,6 +32,11 @@
 #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/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"
 
 using content::BrowserThread;
@@ -71,8 +78,12 @@ class ConnectionListener
 
   // 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 {
     base::AutoLock lock(lock_);

[mmenke, 2016/04/15 15:44:03]

This can be below the rv <= 0 check

[Charlie Harrison, 2016/04/20 12:36:42]

Done.

+    // 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;
     uint16_t socket = GetPort(connection);
     EXPECT_FALSE(sockets_.find(socket) == sockets_.end());
 
@@ -88,7 +99,6 @@ class ConnectionListener
 
   // Returns the number of sockets that were read from by the server.
   size_t GetReadSocketCount() const {
-    base::AutoLock lock(lock_);
     size_t read_sockets = 0;

[mmenke, 2016/04/15 15:44:02]

Why did you remove the lock?  If this coincides with a AcceptedSocket on the
server thread, where we add a socket to sockets_, we're in trouble.  Running at
the same time as ReadFromSocket would result in a race, and threadsanitizer may
whine about it, even if it is a benign race.

[Charlie Harrison, 2016/04/20 12:36:43]

This was a careless mistake probably from rearranging code. My apologies. Fixed.

     for (const auto& socket : sockets_) {
       if (socket.second == SOCKET_READ_FROM)
@@ -101,6 +111,11 @@ class ConnectionListener
 
   void WaitUntilFirstConnectionRead() { read_loop_.Run(); }
 
+  void ClearSockets() {
+    base::AutoLock lock(lock_);
+    sockets_.clear();
+  }
+
  private:
   static uint16_t GetPort(const net::StreamSocket& connection) {
     // Get the remote port of the peer, since the local port will always be the
@@ -202,20 +217,58 @@ class HostResolutionRequestRecorder : public net::HostResolverProc {
   DISALLOW_COPY_AND_ASSIGN(HostResolutionRequestRecorder);
 };
 
+// This class intercepts URLRequests and responds with 404s.
+class NotFoundRequestInterceptor : public net::URLRequestInterceptor {

[mmenke, 2016/04/15 15:44:03]

Give this a public constructor and destructor, and a private
DISALLOW_COPY_AND_ASSIGN.

[Charlie Harrison, 2016/04/20 12:36:43]

Done.

+  net::URLRequestJob* MaybeInterceptRequest(
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate) const override {
+    const char kHeaders[] =
+        "HTTP/1.1 404 Not Found\n"
+        "\n";
+    std::string headers(kHeaders, arraysize(kHeaders));
+
+    return new net::URLRequestTestJob(request, network_delegate, headers, "",
+                                      true);

[mmenke, 2016/04/15 15:44:02]

URLRequestTestJobs are weird.  Can we just use URLRequestFailedJob?

[Charlie Harrison, 2016/04/20 12:36:43]

Sure. I'm not sure what the best error is, but ABORTED holds a certain favor in
my heart.

+  }
+};
+
 }  // namespace
 
 namespace chrome_browser_net {
 
-class PredictorBrowserTest : public InProcessBrowserTest {
+class PredictorBrowserTest : public InProcessBrowserTest,
+                             public PredictorObserver {
  public:
   PredictorBrowserTest()
       : startup_url_("http://host1:1"),
         referring_url_("http://host2:1"),
         target_url_("http://host3:1"),
-        host_resolution_request_recorder_(new HostResolutionRequestRecorder) {
-  }
+        source_target_learned_(0),
+        preconnected_target_(0),
+        preconnected_source_(0),
+        host_resolution_request_recorder_(new HostResolutionRequestRecorder),
+        target_test_server_(new net::EmbeddedTestServer()) {}
 
  protected:
+  // PredictorObserver:
+  void OnPreconnectUrl(const GURL& original_url,

[mmenke, 2016/04/15 15:44:02]

I don't think these should be part of the test fixture - they seem very out of
place in the tests of the renderer-initiated preconnects.  Think it's best just
making a separate class implementing PredictorObserver.

[Charlie Harrison, 2016/04/20 12:36:42]

Fair enough, done.

+                       const GURL& first_party_for_cookies,
+                       UrlInfo::ResolutionMotivation motivation,
+                       int count) override {

[mmenke, 2016/04/15 15:44:02]

These are called on the IO thread, but you're accessing preconnected_target_ and
preconnected_source_ on the UI thread.  Should either add locks around them or
make these PostTask over to the UI thread.  Could make them atomic, but I think
that's overkill, except in production code.  Atomics are in base::subtle for a
reason.

[Charlie Harrison, 2016/04/20 12:36:42]

I added locks.

+    if (original_url == target_test_server()->base_url())
+      preconnected_target_ = std::max(preconnected_target_, count);
+    else if (original_url == embedded_test_server()->base_url())
+      preconnected_source_ = std::max(preconnected_source_, count);

[mmenke, 2016/04/15 15:44:02]

nit:  Use braces in if/else (Style varies in Chrome, but generally in the
network stack, we use them for all if-elses).

[mmenke, 2016/04/15 15:44:03]

If we're preconnecting some other URL, should we fail?

[mmenke, 2016/04/15 15:44:03]

If it's neither of those URLs, should we fail?

[Charlie Harrison, 2016/04/20 12:36:42]

Done.

[Charlie Harrison, 2016/04/20 12:36:42]

Yeah. Done.

[Charlie Harrison, 2016/04/20 12:36:43]

Yupp, done.

+  }
+
+  void OnLearnFromNavigation(const GURL& referring_url,
+                             const GURL& target_url) override {
+    if (referring_url == embedded_test_server()->base_url() &&
+        target_url == target_test_server()->base_url()) {
+      source_target_learned_++;
+    }

[mmenke, 2016/04/15 15:44:02]

What if they aren't equal?  Should we do something then?

[Charlie Harrison, 2016/04/20 12:36:42]

I've added failure conditions.

+  }
+
   void SetUpInProcessBrowserTestFixture() override {
     scoped_host_resolver_proc_.reset(new net::ScopedDefaultHostResolverProc(
         host_resolution_request_recorder_.get()));
@@ -229,10 +282,41 @@ class PredictorBrowserTest : public InProcessBrowserTest {
         switches::kEnableBlinkFeatures, kBlinkPreconnectFeature);
   }
 
+  std::unique_ptr<net::test_server::HttpResponse> HandleRequest(
+      const net::test_server::HttpRequest& request) {
+    if (request.GetURL().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", target_test_server()->GetURL("/title1.html").spec());
+    return std::move(response);
+  }
+
   void SetUpOnMainThread() override {
+    embedded_test_server()->RegisterRequestHandler(base::Bind(
+        &PredictorBrowserTest::HandleRequest, base::Unretained(this)));
+    predictor()->SetObserver(this);

[mmenke, 2016/04/15 15:44:03]

Calling this on the UI thread after the IOThread has already been started seems
like a bad idea - we should post a task to do it over to the IO thread.

[Charlie Harrison, 2016/04/20 12:36:42]

Done. The tests now call "InstallPredictorObserver" which posts to the IO
thread.

+    predictor()->set_preconnect_enabled(true);
+    target_test_server_->ServeFilesFromSourceDirectory("chrome/test/data/");
+
     connection_listener_.reset(new ConnectionListener());
+    target_connection_listener_.reset(new ConnectionListener());
     embedded_test_server()->SetConnectionListener(connection_listener_.get());
+    target_test_server_->SetConnectionListener(
+        target_connection_listener_.get());
     ASSERT_TRUE(embedded_test_server()->Start());
+    ASSERT_TRUE(target_test_server_->Start());
+
+    // 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 we won't connect any
+    // sockets to the test server.

[mmenke, 2016/04/15 15:44:03]

nit avoid "we" in comments.

[Charlie Harrison, 2016/04/20 12:36:43]

Done. Removed all "we"s in comments in this file.

+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&PredictorBrowserTest::AddUrlInterceptor,
+                   target_test_server()->GetURL("/favicon.ico")));

[mmenke, 2016/04/13 17:50:23]

Do we still learn from these requests?  Just wondering if there's still a
possible race here.

[Charlie Harrison, 2016/04/13 18:16:00]

I think we do still learn from these. They are a hint for self-preconnection,
which I realize now I don't have tests for. There might be a race in that case
:/. There may be some way to detect the favicon loading in Javascript.

   }
 
   void TearDownOnMainThread() override {
@@ -254,29 +338,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,17 +376,308 @@ class PredictorBrowserTest : public InProcessBrowserTest {
     return host_resolution_request_recorder_->RequestedHostnameCount();
   }
 
+  net::EmbeddedTestServer* target_test_server() {
+    return target_test_server_.get();
+  }
+
+  Predictor* predictor() { return browser()->profile()->GetNetworkPredictor(); }
+
+  // Navigate to an html file and request async fetches. Wait until the fetches
+  // complete before continuing.
+  void NavigateToCrossSiteHTMLURL(int num_cors,
+                                  int num_non_cors,

[mmenke, 2016/04/15 15:44:02]

Think we should rename cors/non_cors to match the test server names (currently
target_test_server/embedded_test_server).  See comment on second test server
name further down.

[mmenke, 2016/04/15 20:27:17]

Oh...The fact that non_cors does not mean same site is confusing, at least to
people as unfamiliar with the fetch spec as I am.

[Charlie Harrison, 2016/04/20 12:36:43]

Ok, I will update with a comment.

[Charlie Harrison, 2016/04/20 12:36:43]

So actually the non cors perform a fetch() to the target test server with mode:
'no-cors', which performs the fetch with an "opaque" response. So both end up
going to the target test server.

I think this might be a little unnecessary as a core part of the test harness.

+                                  const char* file_suffix) {
+    const GURL& base_url = target_test_server()->base_url();
+    std::string url = base::StringPrintf(

[mmenke, 2016/04/15 15:44:03]

Not a url - this is a path.  As-is, the GetURL(url) line is particularly
confusing.

[Charlie Harrison, 2016/04/20 12:36:42]

Done.

+        "/predictor/"
+        "predictor_cross_site%s.html?subresourceHost=%s&"
+        "numCORSResources=%d&numNonCORSResources=%d",
+        file_suffix, base_url.spec().c_str(), num_cors, num_non_cors);
+    ui_test_utils::NavigateToURL(browser(),
+                                 embedded_test_server()->GetURL(url));
+    bool result = false;
+    EXPECT_TRUE(content::ExecuteScriptAndExtractBool(
+        browser()->tab_strip_model()->GetActiveWebContents(), "sendFetches()",
+        &result));
+    EXPECT_TRUE(result);
+  }
+
+  // Simulate the target server restarting. Delete all sockets and reset members
+  // tracking preconnects.
+  void ResetTarget() {
+    preconnected_target_ = 0;
+    target_test_server()->FlushAllSocketsAndConnectionsOnUIThread();
+    target_connection_listener_->ClearSockets();
+    EXPECT_EQ(0u, target_connection_listener_->GetAcceptedSocketCount());
+  }
+
+  static void AddUrlInterceptor(const GURL url) {
+    net::URLRequestFilter::GetInstance()->AddUrlInterceptor(
+        url, make_scoped_ptr(new NotFoundRequestInterceptor()));
+  }
+
   const GURL startup_url_;
   const GURL referring_url_;
   const GURL target_url_;
-  scoped_ptr<ConnectionListener> connection_listener_;
+  std::unique_ptr<ConnectionListener> connection_listener_;
+  std::unique_ptr<ConnectionListener> target_connection_listener_;
+
+  int source_target_learned_;
+  int preconnected_target_;
+  int preconnected_source_;
 
  private:
   scoped_refptr<HostResolutionRequestRecorder>
       host_resolution_request_recorder_;
-  scoped_ptr<net::ScopedDefaultHostResolverProc> scoped_host_resolver_proc_;
+  std::unique_ptr<net::ScopedDefaultHostResolverProc>
+      scoped_host_resolver_proc_;
+  std::unique_ptr<net::EmbeddedTestServer> target_test_server_;

[mmenke, 2016/04/15 15:44:02]

Think this needs a comment.  I suggest calling it something more general - like
maybe secondary_test_server or cross_site_test_server.  If it were only
redirects, "target" would make sense, but it's not.

[Charlie Harrison, 2016/04/20 12:36:42]

Done. Renamed to "cross_site_test_server".

 };
 
+// Test the html test harness used to initiate cross site fetches. These
+// initiate cross site subresource requests (CORS and non-CORS) to the target
+// test server. Inspect the predictor's internal state to make sure that they
+// are properly logged.
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, CrossSiteNonCORSUseOneSocket) {
+  NavigateToCrossSiteHTMLURL(0, 1, "");

[mmenke, 2016/04/15 15:44:03]

Suggest you should add comments with the variable names for the first two
arguments on all these tests, to make it clearer what's going on.

[Charlie Harrison, 2016/04/20 12:36:42]

Done.

+  EXPECT_EQ(1u, target_connection_listener_->GetAcceptedSocketCount());
+  EXPECT_EQ(1, source_target_learned_);
+  EXPECT_EQ(2, preconnected_source_);
+}
+
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, CrossSiteNonCORSUseThreeSockets) {
+  NavigateToCrossSiteHTMLURL(0, 3, "");
+  EXPECT_EQ(3u, target_connection_listener_->GetAcceptedSocketCount());
+  EXPECT_EQ(3, source_target_learned_);
+  EXPECT_EQ(2, preconnected_source_);
+}
+
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest,
+                       CrossSiteCORSAndNonCORSUseOneSocket) {
+  NavigateToCrossSiteHTMLURL(1, 1, "");

[mmenke, 2016/04/15 20:27:17]

Why do we care so much about the cors vs. non-cors cases?

[Charlie Harrison, 2016/04/20 12:36:42]

It was originally because these separate into privacy-mode pools and non privacy
mode pools. My tests didn't really use that fact so I think I'm going to remove
the distinction.

We can add specialized tests later to test the privacy mode functionality (like
the existing tests for link rel).

+  EXPECT_EQ(2u, target_connection_listener_->GetAcceptedSocketCount());
+  EXPECT_EQ(2, source_target_learned_);
+  EXPECT_EQ(2, preconnected_source_);
+}
+
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, CrossSiteCORSUseTwoSockets) {
+  NavigateToCrossSiteHTMLURL(2, 0, "");
+  EXPECT_EQ(2u, target_connection_listener_->GetAcceptedSocketCount());
+  EXPECT_EQ(2, source_target_learned_);
+  EXPECT_EQ(2, preconnected_source_);
+}
+
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, CrossSiteNoReferrerNoLearning) {
+  NavigateToCrossSiteHTMLURL(1, 0, "_no_referrer");
+  EXPECT_EQ(1u, target_connection_listener_->GetAcceptedSocketCount());
+  EXPECT_EQ(0, source_target_learned_);
+  EXPECT_EQ(2, preconnected_source_);
+}
+
+// The following tests confirm that we accurately predict preconnects after
+// learning from navigations. Note that every "learned" connection adds ~.33 to
+// the expected connection number, which starts at 2. Every preconnect we
+// perform multiplies the expected connections by .66. One additional complexity
+// is that if we've learned to preconnect to host B on a navigation from host A,
+// we add a preconnect if the host part of A and B match (ignoring port). This
+// logic could probably be removed.

[mmenke, 2016/04/15 20:27:16]

Oh wow...that's funky.  Could we have a test where the host names don't match? 
Two ways to do it:  localhost vs 127.0.0.1, or add mock host names - there's a
mock host resolver somewhere in the BrowserTest fixture you can add rules to,
though I think by default, it may map everything to 127.0.0.1 anyways.

[Charlie Harrison, 2016/04/20 12:36:43]

Added a test using localhost.

+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest,
+                       CrossSiteSimplePredictionAfterOneNavigation) {
+  NavigateToCrossSiteHTMLURL(2, 0, "");
+  EXPECT_EQ(2u, target_connection_listener_->GetAcceptedSocketCount());
+  EXPECT_EQ(2, source_target_learned_);
+  EXPECT_EQ(2, preconnected_source_);
+
+  ResetTarget();
+
+  // Navigate again and confirm we preconnect. Note that because the two
+  // embedded test servers have the same host_piece, we preconnect an additional
+  // time. This gives us ceil(2.66) + 1 = 4 preconnects.
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/title1.html"));
+  EXPECT_EQ(4u, target_connection_listener_->GetAcceptedSocketCount());
+  EXPECT_EQ(4, preconnected_target_);
+}
+
+// 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')",
+          target_test_server()->GetURL("/title1.html").spec().c_str()),
+      &result));
+  EXPECT_TRUE(result);
+  EXPECT_EQ(2u, target_connection_listener_->GetAcceptedSocketCount());
+  EXPECT_EQ(1, source_target_learned_);
+  EXPECT_EQ(2, preconnected_source_);
+
+  ResetTarget();
+
+  // Navigate again and confirm we preconnect. Note that because the two
+  // embedded test servers have the same host_piece, we preconnect an additional
+  // time. This gives us ceil(2 + .33) + 1 = 4 preconnects.
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/title1.html"));
+  EXPECT_EQ(4u, target_connection_listener_->GetAcceptedSocketCount());
+  EXPECT_EQ(4, preconnected_target_);

[mmenke, 2016/04/15 20:27:17]

How about also having a test where you just re-navigate the subframe?

[Charlie Harrison, 2016/04/20 12:36:42]

Hm, so this test classifies the subframe as "subresource". It seems like you
want a test that tests that subframe -> subresource prediction works. I've added
a simple test that does this.

+}
+
+// This test navigates to an html file with a tag:
+// <meta name="referrer" content="never">

[mmenke, 2016/04/15 20:27:17]

Is this really desired behavior, or just testing an implementation fluke, to be
updated once that fluke is fixed?

[Charlie Harrison, 2016/04/20 12:36:42]

Well, the referrer tag is pretty architecturally important for the predictor, so
it makes sense to test its limitations. Ideally we will lose this restriction.

+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest,
+                       CrossSiteNoReferrerNoPredictionAfterOneNavigation) {
+  NavigateToCrossSiteHTMLURL(2, 0, "_no_referrer");
+  EXPECT_EQ(2u, target_connection_listener_->GetAcceptedSocketCount());
+  EXPECT_EQ(0, source_target_learned_);
+  EXPECT_EQ(2, preconnected_source_);
+
+  ResetTarget();
+
+  // Navigate again and confirm that no preconnects occurred.
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/title1.html"));
+  EXPECT_EQ(0u, target_connection_listener_->GetAcceptedSocketCount());
+  EXPECT_EQ(0, preconnected_target_);
+}
+
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest,
+                       CrossSiteCORSSimplePredictionAfterTwoNavigations) {
+  NavigateToCrossSiteHTMLURL(0, 1, "");
+  NavigateToCrossSiteHTMLURL(0, 1, "");
+
+  ResetTarget();
+
+  // Navigate again and confirm we preconnect. Note that because the two
+  // embedded test servers have the same host_piece, we preconnect an additional
+  // time. This gives us (2 + .33)*.66 + .33 + 1 ~= 2.87.

[mmenke, 2016/04/15 20:27:17]

nit:  --we (x3).  Well, one's an "us".

[Charlie Harrison, 2016/04/20 12:36:42]

Done.

+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/title1.html"));
+  EXPECT_EQ(3u, target_connection_listener_->GetAcceptedSocketCount());
+  EXPECT_EQ(3, preconnected_target_);
+}
+
+IN_PROC_BROWSER_TEST_F(
+    PredictorBrowserTest,
+    CrossSiteCORSAndNonCORSSimplePredictionAfterTwoNavigations) {
+  NavigateToCrossSiteHTMLURL(1, 1, "");
+  NavigateToCrossSiteHTMLURL(1, 1, "");
+
+  ResetTarget();
+
+  // Navigate again and confirm we preconnect. Note that because the two
+  // embedded test servers have the same host_piece, we preconnect an additional
+  // time. This gives us (((2 + .66)*.66) + .66) + 1 ~= 3.4
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/title1.html"));
+  EXPECT_EQ(4u, target_connection_listener_->GetAcceptedSocketCount());
+  EXPECT_EQ(4, preconnected_target_);
+}
+
+// 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) {
+  NavigateToCrossSiteHTMLURL(1, 0, "");
+
+  ResetTarget();
+
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/title1.html"));
+  // ceil(2 + .33) + 1 = 4.
+  EXPECT_EQ(4u, target_connection_listener_->GetAcceptedSocketCount());
+  EXPECT_EQ(4, preconnected_target_);
+
+  ResetTarget();
+
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/title1.html"));
+
+  // ceil((2 + .33) * .66) + 1 = 3.
+  EXPECT_EQ(3u, target_connection_listener_->GetAcceptedSocketCount());
+  EXPECT_EQ(3, preconnected_target_);
+
+  ResetTarget();
+
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/title1.html"));
+  // ceil((2 + .33) * .66 * .66) + 1 = 3.
+  EXPECT_EQ(3u, target_connection_listener_->GetAcceptedSocketCount());
+  EXPECT_EQ(3, preconnected_target_);
+
+  ResetTarget();
+
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/title1.html"));
+  // Finally, (2 + .33) * .66^3 ~= .67. Not enough for a preconnect.
+  EXPECT_EQ(0u, target_connection_listener_->GetAcceptedSocketCount());
+  EXPECT_EQ(0, preconnected_target_);
+}
+
+// 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) {
+  ui_test_utils::NavigateToURL(
+      browser(),
+      embedded_test_server()->GetURL(base::StringPrintf(
+          "/server-redirect?%s",
+          target_test_server()->GetURL("/title1.html").spec().c_str())));
+  EXPECT_EQ(2u, target_connection_listener_->GetAcceptedSocketCount());
+  EXPECT_EQ(0, source_target_learned_);
+  EXPECT_EQ(2, preconnected_source_);
+
+  ResetTarget();
+
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/title1.html"));
+  EXPECT_EQ(0u, target_connection_listener_->GetAcceptedSocketCount());
+  EXPECT_EQ(0, preconnected_target_);
+}
+
+// The predictor does follow redirects if the original url had an empty path
+// (a path that was more than just "/"). Here we use the registered "/" path to
+// redirect to the target test server.
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest,
+                       CrossSiteRedirectPredictionWithNoPath) {
+  ui_test_utils::NavigateToURL(browser(), embedded_test_server()->GetURL("/"));
+  EXPECT_EQ(2u, target_connection_listener_->GetAcceptedSocketCount());
+  EXPECT_EQ(1, source_target_learned_);
+  EXPECT_EQ(2, preconnected_source_);
+
+  ResetTarget();
+
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/title1.html"));
+  // We preconnect 4 sockets because ceil(2 + .33) + 1 = 4.
+  EXPECT_EQ(4u, target_connection_listener_->GetAcceptedSocketCount());
+  EXPECT_EQ(4, preconnected_target_);
+}
+
+// Perform the "/" redirect twice and make sure the predictor updates twice.
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest,
+                       CrossSiteTwoRedirectsPredictionWithNoPath) {
+  ui_test_utils::NavigateToURL(browser(), embedded_test_server()->GetURL("/"));
+  EXPECT_EQ(1, source_target_learned_);
+  EXPECT_EQ(2, preconnected_source_);
+
+  ui_test_utils::NavigateToURL(browser(), embedded_test_server()->GetURL("/"));
+  EXPECT_EQ(2, source_target_learned_);
+  EXPECT_EQ(2, preconnected_source_);
+
+  ResetTarget();
+
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/title1.html"));
+  // 3 preconnects expected because (2 + .33) * .66 + .33 + 1 ~= 2.87.
+  EXPECT_EQ(3u, target_connection_listener_->GetAcceptedSocketCount());
+  EXPECT_EQ(3, preconnected_target_);
+}
+
 IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, PRE_ShutdownStartupCycle) {
   // Prepare state that will be serialized on this shut-down and read on next
   // start-up.
@@ -333,6 +701,7 @@ IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, ShutdownStartupCycle) {
 
   // But also make sure this data has been first loaded into the Predictor, by
   // inspecting that the Predictor starts making the expected hostname requests.
+  predictor()->set_preconnect_enabled(false);
   PrepareFrameSubresources(referring_url_);
   WaitUntilHostHasBeenRequested(startup_url_.host());
   WaitUntilHostHasBeenRequested(target_url_.host());