predictors: ResourcePrefetchPredictorBrowserTest improvements.

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 "base/memory/ptr_util.h"
@@ skipping 47 matching lines @@
 const char kHtmlAppendChildPath[] = "/predictors/append_child.html";
 const char kScriptAppendChildPath[] = "/predictors/append_child.js";
 const char kHtmlInnerHtmlPath[] = "/predictors/inner_html.html";
 const char kScriptInnerHtmlPath[] = "/predictors/inner_html.js";
 const char kHtmlXHRPath[] = "/predictors/xhr.html";
 const char kScriptXHRPath[] = "/predictors/xhr.js";
 const char kHtmlIframePath[] = "/predictors/html_iframe.html";
 
 struct ResourceSummary {
   ResourceSummary()
-      : is_no_store(false),
-        version(0),
+      : version(0),
+        is_no_store(false),
         is_external(false),
-        should_be_recorded(true) {}
+        is_observable(true),
+        is_prohibited(false) {}
 
   ResourcePrefetchPredictor::URLRequestSummary request;
   std::string content;
+  // Allows to update HTTP ETag.
+  size_t version;
+  // True iff "Cache-control: no-store" header is present.
   bool is_no_store;
-  size_t version;
+  // True iff a request for this resource must be ignored by the custom handler.
   bool is_external;
-  bool should_be_recorded;
+  // True iff the LearningObserver must observe this resource.
+  bool is_observable;
+  // A request with |is_prohibited| set to true makes the test that originates
+  // the request fail.
+  bool is_prohibited;
 };
 
 struct RedirectEdge {
   // This response code should be returned by previous url in the chain.
   net::HttpStatusCode code;
   GURL url;
 };
 
 // Helper class to track and allow waiting for ResourcePrefetchPredictor
 // initialization. WARNING: OnPredictorInitialized event will not be fired if
@@ skipping 79 matching lines @@
 
   for (auto& subresource : actual_subresources)
     ModifySubresourceForComparison(&subresource, match_navigation_id);
   for (auto& subresource : expected_subresources)
     ModifySubresourceForComparison(&subresource, match_navigation_id);
 
   EXPECT_THAT(actual_subresources,
               testing::UnorderedElementsAreArray(expected_subresources));
 }
 
+std::string CreateVersionnedETag(size_t version, const std::string& path) {
+  return base::StringPrintf("'%zu%s'", version, path.c_str());
+}
+
 }  // namespace
 
 // Helper class to track and allow waiting for a single OnNavigationLearned
 // event. The information provided by this event is also used to verify that
 // ResourcePrefetchPredictor works as expected.
 class LearningObserver : public TestObserver {
  public:
   using PageRequestSummary = ResourcePrefetchPredictor::PageRequestSummary;
 
   LearningObserver(ResourcePrefetchPredictor* predictor,
@@ skipping 75 matching lines @@
   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)));
+    embedded_test_server()->RegisterRequestMonitor(base::Bind(
+        &ResourcePrefetchPredictorBrowserTest::MonitorResourceRequest,
+        base::Unretained(this)));
     ASSERT_TRUE(embedded_test_server()->Start());
     predictor_ =
         ResourcePrefetchPredictorFactory::GetForProfile(browser()->profile());
     ASSERT_TRUE(predictor_);
     EnsurePredictorInitialized();
   }
 
   void TestLearningAndPrefetching(const GURL& main_frame_url) {
     // Navigate to |main_frame_url| and check all the expectations.
     NavigateToURLAndCheckSubresources(main_frame_url);
     ClearCache();
     // It is needed to have at least two resource hits to trigger prefetch.
     NavigateToURLAndCheckSubresources(main_frame_url);
     ClearCache();
     // Prefetch all needed resources and change expectations so that all
     // cacheable resources should be served from cache next navigation.
     PrefetchURL(main_frame_url);
+    // To be sure that the browser send no requests to the server after
+    // prefetching.
+    for (auto& kv : resources_) {
+      if (kv.second.is_observable)
+        kv.second.is_prohibited = true;
+    }
     NavigateToURLAndCheckSubresources(main_frame_url);
+    for (auto& kv : resources_) {
+      if (kv.second.is_observable)
+        kv.second.is_prohibited = false;
+    }
   }
 
   void NavigateToURLAndCheckSubresources(
       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));
+      if (kv.second.is_observable) {
+        url_request_summaries.push_back(
+            GetURLRequestSummaryForResource(endpoint_url, kv.second));
+      }
     }
 
     bool match_navigation_id =
         disposition == WindowOpenDisposition::CURRENT_TAB;
 
     LearningObserver observer(
         predictor_, UpdateAndGetVisitCount(main_frame_url),
         CreatePageRequestSummary(endpoint_url.spec(), main_frame_url.spec(),
                                  url_request_summaries),
         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)
+      if (kv.second.is_observable)
         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)
+      if (kv.second.is_observable)
         kv.second.request.was_cached = true;
     }
   }
 
   ResourceSummary* AddResource(const GURL& resource_url,
                                content::ResourceType resource_type,
                                net::RequestPriority priority) {
     auto pair_and_whether_inserted =
         resources_.insert(std::make_pair(resource_url, ResourceSummary()));
     EXPECT_TRUE(pair_and_whether_inserted.second) << resource_url
                                                   << " was inserted twice";
     ResourceSummary* resource = &pair_and_whether_inserted.first->second;
     resource->request.resource_url = resource_url;
     resource->request.resource_type = resource_type;
     resource->request.priority = priority;
     resource->request.has_validators = true;
     return resource;
   }
 
   ResourceSummary* AddExternalResource(const GURL& resource_url,
                                        content::ResourceType resource_type,
                                        net::RequestPriority priority) {
     auto resource = AddResource(resource_url, resource_type, priority);
     resource->is_external = true;
     return resource;
   }
 
-  void AddUnrecordedResources(const std::vector<GURL>& resource_urls) {
+  void AddUnobservableResources(const std::vector<GURL>& resource_urls) {
     for (const GURL& resource_url : resource_urls) {
       auto resource =
           AddResource(resource_url, content::RESOURCE_TYPE_SUB_RESOURCE,
                       net::DEFAULT_PRIORITY);
-      resource->should_be_recorded = false;
+      resource->is_observable = false;
     }
   }
 
   void AddRedirectChain(const GURL& initial_url,
                         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.";
@@ skipping 26 matching lines @@
     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)));
+    https_server()->RegisterRequestMonitor(base::Bind(
+        &ResourcePrefetchPredictorBrowserTest::MonitorResourceRequest,
+        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(); }
@@ skipping 26 matching lines @@
     content::WebContents* web_contents =
         browser()->tab_strip_model()->GetActiveWebContents();
     summary.navigation_id =
         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);
+      RedirectMap::const_iterator it = redirects_.find(current);
       if (it == redirects_.end())
         break;
       current = it->second.url;
     }
     return current;
   }
 
+  void MonitorResourceRequest(
+      const net::test_server::HttpRequest& request) const {
+    ResourceMap::const_iterator resource_it = resources_.find(request.GetURL());
+    if (resource_it == resources_.end())
+      return;
+
+    const ResourceSummary& summary = resource_it->second;
+    EXPECT_FALSE(summary.is_prohibited) << request.GetURL() << "\n"
+                                        << request.all_headers;
+  }
+
+  // The custom handler for resource requests from the browser to an
+  // EmbeddedTestServer. Runs on the EmbeddedTestServer IO thread.
+  // Finds the data to serve requests in |resources_| map keyed by a request
+  // URL.
+  // Uses also the following headers from the |request|:
+  //   - "Host" to retrieve the host that actually was issued by the browser.
+  //   - "If-None-Match" to determine if the resource is still valid. If the
+  //   ETag values match, the handler responds with a HTTP 304 status.

[pasko, 2016/12/26 17:56:49]

nit: please add two spaces before "ETag"

[alexilin, 2016/12/26 18:27:08]

Done.

   std::unique_ptr<net::test_server::HttpResponse> HandleResourceRequest(
       const net::test_server::HttpRequest& request) const {
     GURL resource_url = request.GetURL();
     // Retrieve the host that was used in the request because
     // resource_url contains a resolved host (e.g. 127.0.0.1).
     if (request.headers.find("Host") != request.headers.end()) {
       auto host_port_pair =
           net::HostPortPair::FromString(request.headers.at("Host"));
       GURL::Replacements replace_host;
       replace_host.SetHostStr(host_port_pair.host());
       resource_url = resource_url.ReplaceComponents(replace_host);
     } else {
       ADD_FAILURE() << "Host header was not found in a HttpRequest to url: "
                     << resource_url.spec();
     }
 
-    auto resource_it = resources_.find(resource_url);
+    ResourceMap::const_iterator 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);
+
+    if (request.headers.find("If-None-Match") != request.headers.end() &&
+        request.headers.at("If-None-Match") ==
+            CreateVersionnedETag(summary.version, request.relative_url)) {
+      http_response->set_code(net::HTTP_NOT_MODIFIED);
+    } else {
+      http_response->set_code(net::HTTP_OK);
+    }
+
     if (!summary.request.mime_type.empty())
       http_response->set_content_type(summary.request.mime_type);
     if (!summary.content.empty())
       http_response->set_content(summary.content);
     if (summary.is_no_store)
       http_response->AddCustomHeader("Cache-Control", "no-store");
     if (summary.request.has_validators) {
       http_response->AddCustomHeader(
-          "ETag", base::StringPrintf("'%zu%s'", summary.version,
-                                     request.relative_url.c_str()));
+          "ETag", CreateVersionnedETag(summary.version, request.relative_url));
     }
     if (summary.request.always_revalidate)
       http_response->AddCustomHeader("Cache-Control", "no-cache");
     else
       http_response->AddCustomHeader("Cache-Control", "max-age=2147483648");
     return std::move(http_response);
   }
 
+  // The custom handler for redirect requests from the browser to an
+  // EmbeddedTestServer. Running on the EmbeddedTestServer IO thread.
+  // Finds the data to serve requests in |redirects_| map keyed by a request
+  // URL.
   std::unique_ptr<net::test_server::HttpResponse> HandleRedirectRequest(
       const net::test_server::HttpRequest& request) const {
-    auto redirect_it = redirects_.find(request.GetURL());
+    RedirectMap::const_iterator redirect_it = redirects_.find(request.GetURL());
     if (redirect_it == redirects_.end())
       return nullptr;
 
     auto http_response =
         base::MakeUnique<net::test_server::BasicHttpResponse>();
     http_response->set_code(redirect_it->second.code);
     http_response->AddCustomHeader("Location", redirect_it->second.url.spec());
     return std::move(http_response);
   }
 
   size_t UpdateAndGetVisitCount(const GURL& main_frame_url) {
     return ++visit_count_[main_frame_url];
   }
 
+  typedef std::map<GURL, ResourceSummary> ResourceMap;

[pasko, 2016/12/26 17:56:49]

Use type aliases instead of typedef:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/8dOAMzgR4ao

[alexilin, 2016/12/26 18:27:08]

Thanks for pointing me at this thread. We have a mix of both styles in
/predictors directory and I didn't know whether there was a preference.
I also have found the same recommendation on https://chromium-cpp.appspot.com/
so we definitely should follow it.

[pasko, 2016/12/26 18:50:56]

That's basically where I found this thread :)

+  typedef std::map<GURL, RedirectEdge> RedirectMap;
+
   ResourcePrefetchPredictor* predictor_;
   std::unique_ptr<net::EmbeddedTestServer> https_server_;
-  std::map<GURL, ResourceSummary> resources_;
-  std::map<GURL, RedirectEdge> redirects_;
+  ResourceMap resources_;
+  RedirectMap redirects_;
   std::map<GURL, size_t> visit_count_;
   std::set<NavigationID> navigation_id_history_;
 };
 
 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);
@@ skipping 76 matching lines @@
 IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest,
                        JavascriptInnerHtml) {
   auto externalScript = AddExternalResource(
       GetURL(kScriptInnerHtmlPath), 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,
               net::HIGHEST);
   // https://www.w3.org/TR/2014/REC-html5-20141028/scripting-1.html#the-script-element
   // Script elements don't execute when inserted using innerHTML attribute.
-  AddUnrecordedResources({GetURL(kScriptPath)});
+  AddUnobservableResources({GetURL(kScriptPath)});
   TestLearningAndPrefetching(GetURL(kHtmlInnerHtmlPath));
 }
 
 // Requests originated by XMLHttpRequest have content::RESOURCE_TYPE_XHR.
 // Actual resource type is inferred from the mime-type.
 IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest, JavascriptXHR) {
   auto externalScript = AddExternalResource(
       GetURL(kScriptXHRPath), content::RESOURCE_TYPE_SCRIPT, net::MEDIUM);
   externalScript->request.mime_type = kJavascriptMime;
   auto image = AddResource(GetURL(kImagePath), content::RESOURCE_TYPE_IMAGE,
                            net::HIGHEST);
   image->request.mime_type = kImageMime;
   auto style = AddResource(GetURL(kStylePath),
                            content::RESOURCE_TYPE_STYLESHEET, net::HIGHEST);
   style->request.mime_type = kStyleMime;
   auto script = AddResource(GetURL(kScriptPath), content::RESOURCE_TYPE_SCRIPT,
                             net::HIGHEST);
   script->request.mime_type = kJavascriptMime;
   TestLearningAndPrefetching(GetURL(kHtmlXHRPath));
 }
 
 // ResourcePrefetchPredictor ignores all resources requested from subframes.
 IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest,
                        IframeShouldBeIgnored) {
   // Included from html_iframe.html.
   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)});
+  // Included from <iframe src="html_subresources.html"> and shouldn't be
+  // observed.
+  AddUnobservableResources({GetURL(kImagePath), GetURL(kStylePath),
+                            GetURL(kScriptPath), GetURL(kFontPath)});
   TestLearningAndPrefetching(GetURL(kHtmlIframePath));
 }
 
 IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest,
                        CrossSiteNavigation) {
   AddResource(embedded_test_server()->GetURL(kFooHost, kImagePath),
               content::RESOURCE_TYPE_IMAGE, net::LOWEST);
   AddResource(embedded_test_server()->GetURL(kFooHost, kStylePath),
               content::RESOURCE_TYPE_STYLESHEET, net::HIGHEST);
   AddResource(embedded_test_server()->GetURL(kFooHost, kScriptPath),
@@ skipping 42 matching lines @@
   ClearCache();
   NavigateToURLAndCheckSubresources(GetURL(kHtmlSubresourcesPath),
                                     WindowOpenDisposition::NEW_WINDOW);
   EXPECT_EQ(navigation_ids_history_size(), 3U);
   ClearCache();
   NavigateToURLAndCheckSubresources(GetURL(kHtmlSubresourcesPath),
                                     WindowOpenDisposition::NEW_POPUP);
   EXPECT_EQ(navigation_ids_history_size(), 4U);
 }
 
+IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest, AlwaysRevalidate) {
+  std::vector<ResourceSummary*> resources = {
+      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),
+  };
+  for (auto& resource : resources)
+    resource->request.always_revalidate = true;
+  TestLearningAndPrefetching(GetURL(kHtmlSubresourcesPath));
+}
+
 }  // namespace predictors