predictors: Add browsertests that check fetching through javascript.

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 "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/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/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 {
 
 static const char kImagePath[] = "/predictors/image.png";
+static const char kImagePath2[] = "/predictors/image2.png";
 static const char kStylePath[] = "/predictors/style.css";
+static const char kStylePath2[] = "/predictors/style2.css";

[pasko, 2016/11/30 13:52:48]

it would be nice (i.e. more readable with less surprises) to rename these to be
like: "/handled-by-test/style2.css" and explain in the comment that they return
empty responses with controllable response headers.

Then kHtmlSubresourcesPath (plus html_{script,iframe} would be defined
separately, as it is loaded from a file by the test server.

[alexilin, 2016/11/30 18:43:50]

Done.

 static const char kScriptPath[] = "/predictors/script.js";
+static const char kScriptPath2[] = "/predictors/script2.js";
 static const char kFontPath[] = "/predictors/font.ttf";
 static const char kHtmlSubresourcesPath[] =
     "/predictors/html_subresources.html";
 static const char kRedirectPath[] = "/predictors/redirect.html";
 static const char kRedirectPath2[] = "/predictors/redirect2.html";
 static const char kRedirectPath3[] = "/predictors/redirect3.html";
+static const char kHtmlScriptPath[] = "/predictors/html_script.html";
+static const char kHtmlIframePath[] = "/predictors/html_iframe.html";
 
 struct ResourceSummary {
-  ResourceSummary() : is_no_store(false), version(0) {}
+  ResourceSummary()
+      : is_no_store(false), version(0), should_be_recorded(true) {}
 
   ResourcePrefetchPredictor::URLRequestSummary request;
   std::string content;
   bool is_no_store;
   size_t version;
+  bool should_be_recorded;
+
+  ResourceSummary* WithContent(const std::string& i_content) {

[pasko, 2016/11/30 13:52:47]

we do not make prefixed names in chromium codebase, so it should be just
"content"

[alexilin, 2016/11/30 18:43:50]

Not quite true:
https://cs.chromium.org/chromium/src/chrome/browser/predictors/resource_prefe...
This is an attempt to avoid the collision between parameter name and field name.
Anyway, I deleted these setters at all.

+    content = i_content;
+    return this;
+  }
+
+  ResourceSummary* NotRecorded() {
+    should_be_recorded = false;
+    return this;

[pasko, 2016/11/30 13:52:48]

while I appreciate the added elegance, returning |this| like builders commonly
do in Java, I did not observe this as a common pattern in chromium code, and
also was advised against it on codereviews (cannot remember the arguments they
used - perhaps consistency?).

Also using |this| approaches the boundary of trivial and suggests using a class
instead of a struct.

Can we "just" have one method to register a list of not-recorded
URLs/paths/files in the fixture? Call it once per test. This would save lines of
code as well.

[alexilin, 2016/11/30 18:43:50]

Done.

+  }
 };
 
 struct RedirectEdge {
   // This response code should be returned by previous url in the chain.
   net::HttpStatusCode code;
   GURL url;
 };
 
 class InitializationObserver : public TestObserver {
  public:
@@ skipping 92 matching lines @@
     predictor_ =
         ResourcePrefetchPredictorFactory::GetForProfile(browser()->profile());
     ASSERT_TRUE(predictor_);
     EnsurePredictorInitialized();
   }
 
   void NavigateToURLAndCheckSubresources(const GURL& main_frame_url) {
     GURL endpoint_url = GetRedirectEndpoint(main_frame_url);
     std::vector<URLRequestSummary> url_request_summaries;
     for (const auto& kv : resources_) {
-      if (kv.second.is_no_store)
+      if (kv.second.is_no_store || !kv.second.should_be_recorded)

[pasko, 2016/11/30 13:52:47]

what does "should_be_recorded" mean? Can we EXPECT that after all requests are
being made this resource is not in the database? If that would make a reliable
test, let's do it, more easy checks are better.

Also please explain what this field is responsible for in the struct definition.

[alexilin, 2016/11/30 18:43:50]

It means that this resource should or should not be recorded in
ResourcePrefetchPredictor database. But we still want to declare this resource
in test to not return 404 response code. There is no need of explicitly checking
the absence of these resources in database because we already have the check in
ResourcePrefetchPredictorTestObserver.
There is possibly confusion because I have a single list of resources for
serving in request handler and for checking expectations in TestObserver.

         continue;
       url_request_summaries.push_back(
           GetURLRequestSummaryForResource(endpoint_url, kv.second));
     }
     ResourcePrefetchPredictorTestObserver observer(
         predictor_, UpdateAndGetVisitCount(main_frame_url),
         CreatePageRequestSummary(endpoint_url.spec(), main_frame_url.spec(),
                                  url_request_summaries));
     ui_test_utils::NavigateToURL(browser(), main_frame_url);
     observer.Wait();
@@ skipping 199 matching lines @@
               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));
 }
 
+IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest,
+                       LearningJavascriptOriginated) {
+  AddResource(GetURL(kScriptPath), content::RESOURCE_TYPE_SCRIPT, net::MEDIUM)
+      ->WithContent(
+          "document.write(\'<img src=\"image.png\">\');"

[pasko, 2016/11/30 13:52:48]

Code in one language containing code in another language containing code in yet
another language, mixed with escapes, is unnecessarily hard to read.

There is even a name for it: https://puroh.it/turducken-code/

[alexilin, 2016/11/30 18:43:50]

Acknowledged.

+          "document.write(\'<link rel=\"stylesheet\" href=\"style.css\">\');"

[pasko, 2016/11/30 13:52:48]

there are other ways for JS to request a resource: innerHtml, appendChild.

Can we try them as well to be extra confident that we are not missing these
cases?

[alexilin, 2016/11/30 18:43:50]

I'm not sure that it's secure to perform DOM manipulations before onload event.
Is it guaranteed that all elements before script are ready before script
execution?

We can see interesting things about tests with appendChild and innerHTML.
Script requested by appendChild has lower priority and adding script tag by
innerHTML doesn't cause fetching of script at all!

+          "document.write(\'<script src=\"script2.js\"></script>\');");

[pasko, 2016/11/30 13:52:48]

creating a file for this would be preferable. Is there any flexibility required
here that the test server loading from a file does not provide? If it is for
setting priorities from the test, I would argue that it is a mis-feature of an
integration test, covering the code setting the priorities in Blink would be
good here (unless it is flaky - which we could workaround in the test with more
complicated expectations).

[alexilin, 2016/11/30 18:43:50]

Done.

+  AddResource(GetURL(kImagePath), content::RESOURCE_TYPE_IMAGE, net::LOWEST);
+  AddResource(GetURL(kStylePath), content::RESOURCE_TYPE_STYLESHEET,
+              net::HIGHEST);
+  AddResource(GetURL(kScriptPath2), content::RESOURCE_TYPE_SCRIPT, net::MEDIUM);
+  NavigateToURLAndCheckSubresources(GetURL(kHtmlScriptPath));
+}
+
+// Javascript fetch requests are ignored by the ResourcePrefetchPredictor
+// because they have unsupported resource type content::RESOURCE_TYPE_XHR.

[pasko, 2016/11/30 13:52:47]

what's our status on fixing it? is there a bug? If there is, please reference
the bug in the comment.

[alexilin, 2016/11/30 18:43:50]

Well, I just discovered it by writing this test so it's new.
I don't know how often xhr requests are used to load scripts and images. I
suspect that it is rather used for dynamically load some json or xml data than
something from {css,script,image,font}.

Some data from HTTP archive. I filtered all requests with "X-Requested-With:
XMLHttpRequest" header and grouped by "type" column. It's a relatively small
percent of all requests (40M).
type    count
total   232939
script	113159	 
html	97751	 
text	7521	 
xml	6133	 
other	4485	 
image	3431	 
css	251	 
audio	151	 
font	35	 
video	22	 
I'm not sure how accurate this "type" column and what percent of xhr requests
actually set this header. Also notice that "script" type isn't javascript only.
It encloses files with various types (php, html etc.)

+IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest,
+                       LearningIgnoringJavascriptFetches) {
+  AddResource(GetURL(kScriptPath), content::RESOURCE_TYPE_SCRIPT, net::MEDIUM)
+      ->WithContent(
+          "fetch(\"image.png\"); fetch(\"style.css\"); fetch(\"script2.js\")");
+  AddResource(GetURL(kImagePath), content::RESOURCE_TYPE_IMAGE, net::LOWEST)
+      ->NotRecorded();
+  AddResource(GetURL(kStylePath), content::RESOURCE_TYPE_STYLESHEET,
+              net::HIGHEST)
+      ->NotRecorded();
+  AddResource(GetURL(kScriptPath2), content::RESOURCE_TYPE_SCRIPT, net::MEDIUM)
+      ->NotRecorded();
+  NavigateToURLAndCheckSubresources(GetURL(kHtmlScriptPath));
+}
+
+// ResourcePrefetchPredictor ignores all resources requested from subframes.
+IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest,
+                       LearningWithIframe) {
+  // 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.
+  AddResource(GetURL(kImagePath), content::RESOURCE_TYPE_IMAGE, net::LOWEST)
+      ->NotRecorded();
+  AddResource(GetURL(kStylePath), content::RESOURCE_TYPE_STYLESHEET,
+              net::HIGHEST)
+      ->NotRecorded();
+  AddResource(GetURL(kScriptPath), content::RESOURCE_TYPE_SCRIPT, net::MEDIUM)
+      ->NotRecorded();
+  AddResource(GetURL(kFontPath), content::RESOURCE_TYPE_FONT_RESOURCE,
+              net::HIGHEST)
+      ->NotRecorded();
+  NavigateToURLAndCheckSubresources(GetURL(kHtmlIframePath));
+}
+
 }  // namespace predictors