predictors: Make speculative_prefetch_predictor work with PlzNavigate

chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc

Index: chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
diff --git a/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc b/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
index 70c3131eee72b3618091e2b5e499350e753622f6..a1a4e8736ab9b732cd99ea77d3d51ab401ec5966 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
+++ b/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
@@ -2,6 +2,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <stddef.h>
+
+#include <set>
+
 #include "base/command_line.h"
 #include "chrome/browser/predictors/resource_prefetch_predictor.h"
 #include "chrome/browser/predictors/resource_prefetch_predictor_factory.h"
@@ -13,8 +17,7 @@
 #include "chrome/common/chrome_switches.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
-#include "content/public/browser/render_frame_host.h"
-#include "content/public/browser/render_process_host.h"
+#include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/http_request.h"
 #include "net/test/embedded_test_server/http_response.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -95,6 +98,12 @@ class InitializationObserver : public TestObserver {
 using PageRequestSummary = ResourcePrefetchPredictor::PageRequestSummary;
 using URLRequestSummary = ResourcePrefetchPredictor::URLRequestSummary;
 
+void ReplaceHost(GURL* gurl, const std::string& host) {

[Benoit L, 2016/12/21 12:38:26]

nit: This is used in one place only, please inline it.

[alexilin, 2016/12/21 13:08:12]

Actually, it was my suggestion.
This is copypaste of EmbeddedTestServer::GetURL(host, path) function and I think
it would be clearer to use our function to replace host. (See comment below).
But this is actually nit.

Btw, shouldn't we extract self-contained pieces of code even it is used only in
one place?

[ahemery, 2016/12/21 15:42:40]

Un-inlined. Also as discussed with Alex, we still need this to avoid replacing
the port on top of the host for HTTPS tests

+  GURL::Replacements replace_host;
+  replace_host.SetHostStr(host);
+  *gurl = gurl->ReplaceComponents(replace_host);
+}
+
 void RemoveDuplicateSubresources(std::vector<URLRequestSummary>* subresources) {
   std::stable_sort(subresources->begin(), subresources->end(),
                    [](const URLRequestSummary& x, const URLRequestSummary& y) {
@@ -111,8 +120,7 @@ void RemoveDuplicateSubresources(std::vector<URLRequestSummary>* subresources) {
 // Fill a NavigationID with "empty" data that does not trigger
 // the is_valid DCHECK(). Allows comparing.
 void SetValidNavigationID(NavigationID* navigation_id) {
-  navigation_id->render_process_id = 0;
-  navigation_id->render_frame_id = 0;
+  navigation_id->session_id = 0;
   navigation_id->main_frame_url = GURL("http://127.0.0.1");
 }
 
@@ -171,6 +179,8 @@ class LearningObserver : public TestObserver {
     EXPECT_EQ(url_visit_count, url_visit_count_);
     EXPECT_EQ(summary.main_frame_url, summary_.main_frame_url);
     EXPECT_EQ(summary.initial_url, summary_.initial_url);
+    for (const auto& resource : summary.subresource_requests)
+      current_navigation_ids_.insert(resource.navigation_id);
     CompareSubresources(summary.subresource_requests,
                         summary_.subresource_requests, match_navigation_id_);
     run_loop_.Quit();
@@ -178,11 +188,16 @@ class LearningObserver : public TestObserver {
 
   void Wait() { run_loop_.Run(); }
 
+  std::set<NavigationID>& current_navigation_ids() {
+    return current_navigation_ids_;
+  }
+
  private:
   base::RunLoop run_loop_;
   size_t url_visit_count_;
   PageRequestSummary summary_;
   bool match_navigation_id_;
+  std::set<NavigationID> current_navigation_ids_;
 
   DISALLOW_COPY_AND_ASSIGN(LearningObserver);
 };
@@ -226,6 +241,9 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
   }
 
   void SetUpOnMainThread() override {
+    // Resolving all hosts to local allows us to have
+    // cross domains navigations (matching url_visit_count_, etc).
+    host_resolver()->AddRule("*", "127.0.0.1");
     embedded_test_server()->RegisterRequestHandler(
         base::Bind(&ResourcePrefetchPredictorBrowserTest::HandleRedirectRequest,
                    base::Unretained(this)));
@@ -263,11 +281,16 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
       url_request_summaries.push_back(
           GetURLRequestSummaryForResource(endpoint_url, kv.second));
     }
+
+    bool match_navigation_id = true;
+    if (disposition != WindowOpenDisposition::CURRENT_TAB)
+      match_navigation_id = false;
+
     LearningObserver observer(
         predictor_, UpdateAndGetVisitCount(main_frame_url),
         CreatePageRequestSummary(endpoint_url.spec(), main_frame_url.spec(),
                                  url_request_summaries),
-        true);  // Matching navigation id by default
+        match_navigation_id);
     ui_test_utils::NavigateToURLWithDisposition(
         browser(), main_frame_url, disposition,
         ui_test_utils::BROWSER_TEST_NONE);
@@ -276,6 +299,8 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
       if (!kv.second.is_no_store && kv.second.should_be_recorded)
         kv.second.request.was_cached = true;
     }
+    for (const auto& nav : observer.current_navigation_ids())
+      navigation_id_history_.insert(nav);
   }
 
   void PrefetchURL(const GURL& main_frame_url) {
@@ -331,6 +356,8 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
     }
   }
 
+  void ClearResources() { resources_.clear(); }
+
   void ClearCache() {
     chrome::ClearCache(browser());
     for (auto& kv : resources_)
@@ -338,10 +365,16 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
   }
 
   // Shortcut for convenience.
+  // Using two functions instead of default parameter to preserve
+  // original function parameters order.
   GURL GetURL(const std::string& path) const {
     return embedded_test_server()->GetURL(path);
   }
 
+  GURL GetURL(const std::string& path, const std::string& host) const {

[alexilin, 2016/12/21 13:08:12]

nit: Would it be better to use ReplaceHost for this purpose?
We could do something like that:

AddResource(ReplaceHost(GetURL(kImagePath), "bar.com"),
content::RESOURCE_TYPE_IMAGE, net::LOWEST);

But I'm also ok with the current implementation. The only thing that bothers me
is that EmbeddedTestServer::GetURL(host, path) and ReplaceHost do the same thing
(no matter if ReplaceHost is inlined or not)

[ahemery, 2016/12/21 15:42:40]

Removed the one with more parameters as it was not really necessary and
confusing. Calling embedded_test_server() directly when dealing with custom
hosts.

+    return embedded_test_server()->GetURL(host, path);
+  }
+
   void EnableHttpsServer() {
     ASSERT_FALSE(https_server_);
     https_server_ = base::MakeUnique<net::EmbeddedTestServer>(
@@ -365,6 +398,8 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
 
   net::EmbeddedTestServer* https_server() { return https_server_.get(); }
 
+  size_t navigation_ids_history_size() { return navigation_id_history_.size(); }

[Benoit L, 2016/12/21 12:38:25]

nit: make the function const.

[ahemery, 2016/12/21 15:42:40]

Done.

+
  private:
   // ResourcePrefetchPredictor needs to be initialized before the navigation
   // happens otherwise this navigation will be ignored by predictor.
@@ -388,10 +423,8 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
     URLRequestSummary summary(resource_summary.request);
     content::WebContents* web_contents =
         browser()->tab_strip_model()->GetActiveWebContents();
-    int process_id = web_contents->GetRenderProcessHost()->GetID();
-    int frame_id = web_contents->GetMainFrame()->GetRoutingID();
     summary.navigation_id =
-        CreateNavigationID(process_id, frame_id, main_frame_url.spec());
+        NavigationID(web_contents, main_frame_url, base::TimeTicks::Now());
     return summary;
   }
 
@@ -408,7 +441,16 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
 
   std::unique_ptr<net::test_server::HttpResponse> HandleResourceRequest(
       const net::test_server::HttpRequest& request) const {
-    auto resource_it = resources_.find(request.GetURL());
+    GURL resource_url = request.GetURL();
+    // Retrieve the host that was used in the request.
+    // The resource_url contains a resolved host (e.g. 127.0.0.1).
+    std::string host = request.headers.at("Host");

[Benoit L, 2016/12/21 12:38:25]

GURL has has_port() and port().
Also, why using .at() here?

[alexilin, 2016/12/21 13:08:12]

Consider to use HostPortPair
https://cs.chromium.org/chromium/src/net/base/host_port_pair.h?dr=C&q=HostPor...

[ahemery, 2016/12/21 15:42:40]

Removed the manual work and used the structure suggested by Alex as it seems to
be the one with minimal overhead.
We are using .at() because the underlying map is const, de facto disabling
operator[]. Let me know if error handling here is acceptable.

+    size_t port_pos = host.find(':');
+    if (port_pos != std::string::npos)
+      host = host.substr(0, port_pos);
+    ReplaceHost(&resource_url, host);
+
+    auto resource_it = resources_.find(resource_url);
     if (resource_it == resources_.end())
       return nullptr;
 
@@ -459,6 +501,20 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
   std::map<GURL, ResourceSummary> resources_;
   std::map<GURL, RedirectEdge> redirects_;
   std::map<GURL, size_t> visit_count_;
+  std::set<NavigationID> navigation_id_history_;
+};
+
+// This browser test override is specifically used to have ONLY
+// the learning part of the prefetcher without the actual prefetching

[alexilin, 2016/12/21 13:08:12]

period here as well because this is separate sentence

[ahemery, 2016/12/21 15:42:40]

Done.

+// Used to avoid having caching issues for matcher.
+class ResourcePrefetchPredictorLearningBrowserTest
+    : public ResourcePrefetchPredictorBrowserTest {
+ protected:
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    command_line->AppendSwitchASCII(
+        switches::kSpeculativeResourcePrefetching,
+        switches::kSpeculativeResourcePrefetchingLearning);
+  }
 };
 
 IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest, Simple) {
@@ -500,7 +556,7 @@ IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest, RedirectChain) {
 }
 
 IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest,
-                       LearningAfterHttpToHttpsRedirect) {
+                       HttpToHttpsRedirect) {
   EnableHttpsServer();
   AddRedirectChain(GetURL(kRedirectPath),
                    {{net::HTTP_MOVED_PERMANENTLY,
@@ -513,9 +569,7 @@ IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest,
               content::RESOURCE_TYPE_SCRIPT, net::MEDIUM);
   AddResource(https_server()->GetURL(kFontPath),
               content::RESOURCE_TYPE_FONT_RESOURCE, net::HIGHEST);
-  NavigateToURLAndCheckSubresources(GetURL(kRedirectPath));
-  // TODO(alexilin): Test learning and prefetching once crbug.com/650246 is
-  // fixed.
+  TestLearningAndPrefetching(GetURL(kRedirectPath));
 }
 
 IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest,
@@ -591,4 +645,69 @@ IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest,
   TestLearningAndPrefetching(GetURL(kHtmlIframePath));
 }
 
+IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest,
+                       CrossSiteLearning) {
+  AddResource(GetURL(kImagePath, "foo.com"), content::RESOURCE_TYPE_IMAGE,
+              net::LOWEST);
+  AddResource(GetURL(kStylePath, "foo.com"), content::RESOURCE_TYPE_STYLESHEET,
+              net::HIGHEST);
+  AddResource(GetURL(kScriptPath, "foo.com"), content::RESOURCE_TYPE_SCRIPT,
+              net::MEDIUM);
+  AddResource(GetURL(kFontPath, "foo.com"),
+              content::RESOURCE_TYPE_FONT_RESOURCE, net::HIGHEST);
+  TestLearningAndPrefetching(GetURL(kHtmlSubresourcesPath, "foo.com"));
+  ClearResources();
+
+  AddResource(GetURL(kImagePath, "bar.com"), content::RESOURCE_TYPE_IMAGE,
+              net::LOWEST);
+  AddResource(GetURL(kStylePath, "bar.com"), content::RESOURCE_TYPE_STYLESHEET,
+              net::HIGHEST);
+  AddResource(GetURL(kScriptPath, "bar.com"), content::RESOURCE_TYPE_SCRIPT,
+              net::MEDIUM);
+  AddResource(GetURL(kFontPath, "bar.com"),
+              content::RESOURCE_TYPE_FONT_RESOURCE, net::HIGHEST);
+  TestLearningAndPrefetching(GetURL(kHtmlSubresourcesPath, "bar.com"));
+}
+
+IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorLearningBrowserTest,
+                       SessionIdBehavingAsExpected) {
+  AddResource(GetURL(kImagePath), content::RESOURCE_TYPE_IMAGE, net::LOWEST);
+  AddResource(GetURL(kStylePath), content::RESOURCE_TYPE_STYLESHEET,
+              net::HIGHEST);
+  AddResource(GetURL(kScriptPath), content::RESOURCE_TYPE_SCRIPT, net::MEDIUM);
+  AddResource(GetURL(kFontPath), content::RESOURCE_TYPE_FONT_RESOURCE,
+              net::HIGHEST);
+  NavigateToURLAndCheckSubresources(GetURL(kHtmlSubresourcesPath));
+  EXPECT_EQ(navigation_ids_history_size(), 1U);
+  ClearCache();
+  NavigateToURLAndCheckSubresources(GetURL(kHtmlSubresourcesPath));
+  // Checking that repeated navigation to a tab reuses the same SessionID.
+  EXPECT_EQ(navigation_ids_history_size(), 1U);
+  ClearCache();
+
+  // Checking that navigation to a new tab yields the same result as
+  // navigating to the same tab resource wise.
+  NavigateToURLAndCheckSubresources(GetURL(kHtmlSubresourcesPath),
+                                    WindowOpenDisposition::NEW_BACKGROUND_TAB);
+  // Checking that navigation to a new tab uses a new SessionID.
+  EXPECT_EQ(navigation_ids_history_size(), 2U);
+  ClearCache();
+
+  // Checking that navigation to a new window yields the same result as

[alexilin, 2016/12/21 13:08:12]

There is no need to repeat the same things several times, I think. Feel free to
just move this description on a top of the function. (Before
IN_PROC_BROWSER_TEST_F)

[ahemery, 2016/12/21 15:42:40]

Condensed on top of the function

+  // navigating to the same tab resource wise.
+  NavigateToURLAndCheckSubresources(GetURL(kHtmlSubresourcesPath),
+                                    WindowOpenDisposition::NEW_WINDOW);
+  // Checking that navigation to a new window uses a new SessionID.
+  EXPECT_EQ(navigation_ids_history_size(), 3U);
+  ClearCache();
+
+  // Checking that navigation to a popup yields the same result as
+  // navigating to the same tab resource wise.
+  NavigateToURLAndCheckSubresources(GetURL(kHtmlSubresourcesPath),
+                                    WindowOpenDisposition::NEW_POPUP);
+  // Checking that navigation to a new tab uses a new SessionID that is
+  // also different from the new window one (e.g. not always reusing 1).
+  EXPECT_EQ(navigation_ids_history_size(), 4U);
+}
+
 }  // namespace predictors