predictors: Make speculative_prefetch_predictor work with PlzNavigate

chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc

 // Copyright 2016 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 <set>
+
 #include "base/command_line.h"
 #include "chrome/browser/predictors/resource_prefetch_predictor.h"
 #include "chrome/browser/predictors/resource_prefetch_predictor_factory.h"
 #include "chrome/browser/predictors/resource_prefetch_predictor_test_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #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"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace predictors {
 
 namespace {
 
 const char kImageMime[] = "image/png";
@@ skipping 60 matching lines @@
 
  private:
   base::RunLoop run_loop_;
 
   DISALLOW_COPY_AND_ASSIGN(InitializationObserver);
 };
 
 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) {
                      return x.resource_url < y.resource_url;
                    });
   subresources->erase(
       std::unique(subresources->begin(), subresources->end(),
                   [](const URLRequestSummary& x, const URLRequestSummary& y) {
                     return x.resource_url == y.resource_url;
                   }),
       subresources->end());
 }
 
 // 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");
 }
 
 void ModifySubresourceForComparison(URLRequestSummary* subresource,
                                     bool match_navigation_id) {
   if (!match_navigation_id)
     SetValidNavigationID(&subresource->navigation_id);
   if (subresource->resource_type == content::RESOURCE_TYPE_IMAGE &&
       subresource->priority == net::LOWEST) {
     // Fuzzy comparison for images because an image priority can be
@@ skipping 38 matching lines @@
         url_visit_count_(expected_url_visit_count),
         summary_(expected_summary),
         match_navigation_id_(match_navigation_id) {}
 
   // TestObserver:
   void OnNavigationLearned(size_t url_visit_count,
                            const PageRequestSummary& summary) override {
     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();
   }
 
   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);
 };
 
 // Helper class to track and allow waiting for a single OnPrefetchingFinished
 // event. No learning events should be fired while this observer is active.
 class PrefetchingObserver : public TestObserver {
  public:
   PrefetchingObserver(ResourcePrefetchPredictor* predictor,
                       const GURL& expected_main_frame_url)
@@ skipping 23 matching lines @@
  protected:
   using URLRequestSummary = ResourcePrefetchPredictor::URLRequestSummary;
 
   void SetUpCommandLine(base::CommandLine* command_line) override {
     command_line->AppendSwitchASCII(
         switches::kSpeculativeResourcePrefetching,
         switches::kSpeculativeResourcePrefetchingEnabled);
   }
 
   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)));
     embedded_test_server()->RegisterRequestHandler(
         base::Bind(&ResourcePrefetchPredictorBrowserTest::HandleResourceRequest,
                    base::Unretained(this)));
     ASSERT_TRUE(embedded_test_server()->Start());
     predictor_ =
         ResourcePrefetchPredictorFactory::GetForProfile(browser()->profile());
     ASSERT_TRUE(predictor_);
@@ skipping 17 matching lines @@
       const GURL& main_frame_url,
       WindowOpenDisposition disposition = WindowOpenDisposition::CURRENT_TAB) {
     GURL endpoint_url = GetRedirectEndpoint(main_frame_url);
     std::vector<URLRequestSummary> url_request_summaries;
     for (const auto& kv : resources_) {
       if (kv.second.is_no_store || !kv.second.should_be_recorded)
         continue;
       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);
     observer.Wait();
     for (auto& kv : resources_) {
       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) {
     PrefetchingObserver observer(predictor_, main_frame_url);
     predictor_->StartPrefetching(main_frame_url, PrefetchOrigin::EXTERNAL);
     observer.Wait();
     for (auto& kv : resources_) {
       if (!kv.second.is_no_store && kv.second.should_be_recorded)
         kv.second.request.was_cached = true;
     }
@@ skipping 35 matching lines @@
                         const std::vector<RedirectEdge>& redirect_chain) {
     ASSERT_FALSE(redirect_chain.empty());
     GURL current = initial_url;
     for (const auto& edge : redirect_chain) {
       auto result = redirects_.insert(std::make_pair(current, edge));
       EXPECT_TRUE(result.second) << current << " already has a redirect.";
       current = edge.url;
     }
   }
 
+  void ClearResources() { resources_.clear(); }
+
   void ClearCache() {
     chrome::ClearCache(browser());
     for (auto& kv : resources_)
       kv.second.request.was_cached = false;
   }
 
   // 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>(
         net::EmbeddedTestServer::TYPE_HTTPS);
     https_server()->AddDefaultHandlers(
         base::FilePath(FILE_PATH_LITERAL("chrome/test/data")));
     https_server()->RegisterRequestHandler(
         base::Bind(&ResourcePrefetchPredictorBrowserTest::HandleRedirectRequest,
                    base::Unretained(this)));
     https_server()->RegisterRequestHandler(
         base::Bind(&ResourcePrefetchPredictorBrowserTest::HandleResourceRequest,
                    base::Unretained(this)));
     ASSERT_TRUE(https_server()->Start());
   }
 
   // Returns the embedded test server working over HTTPS. Must be enabled by
   // calling EnableHttpsServer() before use.
   const net::EmbeddedTestServer* https_server() const {
     return https_server_.get();
   }
 
   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.
   void EnsurePredictorInitialized() {
     if (predictor_->initialization_state_ ==
         ResourcePrefetchPredictor::INITIALIZED) {
       return;
     }
 
     InitializationObserver observer(predictor_);
     if (predictor_->initialization_state_ ==
         ResourcePrefetchPredictor::NOT_INITIALIZED) {
       predictor_->StartInitialization();
     }
     observer.Wait();
   }
 
   URLRequestSummary GetURLRequestSummaryForResource(
       const GURL& main_frame_url,
       const ResourceSummary& resource_summary) const {
     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;
   }
 
   GURL GetRedirectEndpoint(const GURL& initial_url) const {
     GURL current = initial_url;
     while (true) {
       auto it = redirects_.find(current);
       if (it == redirects_.end())
         break;
       current = it->second.url;
     }
     return current;
   }
 
   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;
 
     const ResourceSummary& summary = resource_it->second;
     if (summary.is_external)
       return nullptr;
 
     auto http_response =
         base::MakeUnique<net::test_server::BasicHttpResponse>();
     http_response->set_code(net::HTTP_OK);
@@ skipping 30 matching lines @@
 
   size_t UpdateAndGetVisitCount(const GURL& main_frame_url) {
     return ++visit_count_[main_frame_url];
   }
 
   ResourcePrefetchPredictor* predictor_;
   std::unique_ptr<net::EmbeddedTestServer> https_server_;
   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) {
   // These resources have default priorities that correspond to
   // blink::typeToPriority function.
   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,
@@ skipping 21 matching lines @@
   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);
   TestLearningAndPrefetching(GetURL(kRedirectPath));
 }
 
 IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest,
-                       LearningAfterHttpToHttpsRedirect) {
+                       HttpToHttpsRedirect) {
   EnableHttpsServer();
   AddRedirectChain(GetURL(kRedirectPath),
                    {{net::HTTP_MOVED_PERMANENTLY,
                      https_server()->GetURL(kHtmlSubresourcesPath)}});
   AddResource(https_server()->GetURL(kImagePath), content::RESOURCE_TYPE_IMAGE,
               net::LOWEST);
   AddResource(https_server()->GetURL(kStylePath),
               content::RESOURCE_TYPE_STYLESHEET, net::HIGHEST);
   AddResource(https_server()->GetURL(kScriptPath),
               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,
                        JavascriptDocumentWrite) {
   auto externalScript =
       AddExternalResource(GetURL(kScriptDocumentWritePath),
                           content::RESOURCE_TYPE_SCRIPT, net::MEDIUM);
   externalScript->request.mime_type = kJavascriptMime;
   AddResource(GetURL(kImagePath), content::RESOURCE_TYPE_IMAGE, net::LOWEST);
   AddResource(GetURL(kStylePath), content::RESOURCE_TYPE_STYLESHEET,
@@ skipping 55 matching lines @@
   AddResource(GetURL(kImagePath2), content::RESOURCE_TYPE_IMAGE, net::LOWEST);
   AddResource(GetURL(kStylePath2), content::RESOURCE_TYPE_STYLESHEET,
               net::HIGHEST);
   AddResource(GetURL(kScriptPath2), content::RESOURCE_TYPE_SCRIPT, net::MEDIUM);
   // Included from <iframe src="html_subresources.html"> and not recored.
   AddUnrecordedResources({GetURL(kImagePath), GetURL(kStylePath),
                           GetURL(kScriptPath), GetURL(kFontPath)});
   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