Wait until all SafeBrowsing checks complete before calling NotifyResult.

This CL is the first step in having NotifyResult select the check with the
highest priority.  In order to do that (and not store intermediate data), all of
the checks must have finished prior to NotifyResult being called.

This *may* regress performance, and we should track the potential regression in
SubresourceFilter.PageLoad.SafeBrowsingDelay.

Bug: 823414
Change-Id: I1339e3795e9178dd7dd789ac98fcd21a2b76d726
Reviewed-on: https://chromium-review.googlesource.com/984672
Commit-Queue: Eric Robinson <ericrobinson@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Reviewed-by: Asanka Herath <asanka@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558178}

chrome/browser/subresource_filter/subresource_filter_intercepting_browsertest.cc

@@ -8,6 +8,7 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "base/timer/elapsed_timer.h"
 #include "chrome/browser/safe_browsing/test_safe_browsing_database_helper.h"
 #include "chrome/browser/subresource_filter/subresource_filter_browser_test_harness.h"
 #include "chrome/test/base/ui_test_utils.h"
@@ -125,4 +126,58 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterInterceptingBrowserTest,
   EXPECT_EQ(warn_console_observer.message(), kActivationWarningConsoleMessage);
 }
 
+// Verify that the navigation waits on all safebrowsing results to be retrieved,
+// and doesn't just return after the final (used) result.  Also verify that the
+// results reported are correct when messages arrive out-of-order.
+IN_PROC_BROWSER_TEST_F(SubresourceFilterInterceptingBrowserTest,
+                       SafeBrowsingNotificationsWaitOnAllRedirects) {
+  // TODO(ericrobinson): If servers are slow for this test, the test will pass
+  //   by default (the delay will be high due to server time rather than due
+  //   to waiting on safebrowsing results).  While this won't cause flakiness,
+  //   it's not ideal.  Look into using a ControllableHttpResponse for each
+  //   request, and completing the first after we know the second got to
+  //   the activation throttle and check that it didn't call NotifyResults.
+  ASSERT_NO_FATAL_FAILURE(
+      SetRulesetToDisallowURLsWithPathSuffix("included_script.js"));
+  const std::string initial_host("a.com");
+  const std::string redirected_host("b.com");
+  base::TimeDelta delay = base::TimeDelta::FromSeconds(2);
+
+  GURL redirect_url(embedded_test_server()->GetURL(
+      redirected_host, "/subresource_filter/frame_with_included_script.html"));
+  GURL url(embedded_test_server()->GetURL(
+      initial_host, "/server-redirect?" + redirect_url.spec()));
+
+  // Mark the prefixes as bad so that safe browsing will request full hashes
+  // from the v4 server.
+  database_helper()->LocallyMarkPrefixAsBad(
+      url, safe_browsing::GetUrlSubresourceFilterId());
+  database_helper()->LocallyMarkPrefixAsBad(
+      redirect_url, safe_browsing::GetUrlSubresourceFilterId());
+
+  // Map URLs to policies, enforce on the initial, and warn on the redirect.
+  std::map<GURL, safe_browsing::ThreatMatch> response_map{
+      {url, GetBetterAdsMatch(url, "enforce")},
+      {redirect_url, GetBetterAdsMatch(redirect_url, "warn")}};
+  // Delay the initial response , so it arrives after the final.
+  std::map<GURL, base::TimeDelta> delay_map{{url, delay}};
+  safe_browsing::StartRedirectingV4RequestsForTesting(
+      response_map, safe_browsing_test_server(), delay_map);
+  safe_browsing_test_server()->StartAcceptingConnections();
+
+  // The navigation should wait for all safebrowsing results, and not just
+  // return the last result, which is computed quickly.
+  content::ConsoleObserverDelegate warn_console_observer(
+      web_contents(), kActivationWarningConsoleMessage);
+  web_contents()->SetDelegate(&warn_console_observer);
+  base::ElapsedTimer timer;
+  ui_test_utils::NavigateToURL(browser(), url);
+  EXPECT_GE(timer.Elapsed(), delay);
+
+  // Make sure the action is the last one in the redirect chain, a warning.
+  warn_console_observer.Wait();
+  EXPECT_TRUE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));
+  EXPECT_EQ(warn_console_observer.message(), kActivationWarningConsoleMessage);
+}
+
 }  // namespace subresource_filter

components/safe_browsing/db/v4_embedded_test_server_util.cc

@@ -59,6 +59,7 @@ std::vector<HashPrefix> GetPrefixesForRequest(const GURL& url) {
 // predetermined responses.
 std::unique_ptr<net::test_server::HttpResponse> HandleFullHashRequest(
     const std::map<GURL, ThreatMatch>& response_map,
+    const std::map<GURL, base::TimeDelta>& delay_map,
     const net::test_server::HttpRequest& request) {
   if (!(net::test_server::ShouldHandle(request, "/v4/fullHashes:find")))
     return nullptr;
@@ -72,12 +73,17 @@ std::unique_ptr<net::test_server::HttpResponse> HandleFullHashRequest(
   // that match the prefix.
   std::vector<HashPrefix> request_prefixes =
       GetPrefixesForRequest(request.GetURL());
+  const base::TimeDelta* delay = nullptr;
   for (const HashPrefix& prefix : request_prefixes) {
     for (const auto& response : response_map) {
       FullHash full_hash = GetFullHash(response.first);
       if (V4ProtocolManagerUtil::FullHashMatchesHashPrefix(full_hash, prefix)) {
         ThreatMatch* match = find_full_hashes_response.add_matches();
         *match = response.second;
+        auto it = delay_map.find(response.first);
+        if (it != delay_map.end()) {
+          delay = &(it->second);
+        }
       }
     }
   }
@@ -85,7 +91,9 @@ std::unique_ptr<net::test_server::HttpResponse> HandleFullHashRequest(
   std::string serialized_response;
   find_full_hashes_response.SerializeToString(&serialized_response);
 
-  auto http_response = std::make_unique<net::test_server::BasicHttpResponse>();
+  auto http_response =
+      (delay ? std::make_unique<net::test_server::DelayedHttpResponse>(*delay)
+             : std::make_unique<net::test_server::BasicHttpResponse>());
   http_response->set_content(serialized_response);
   return http_response;
 }
@@ -94,13 +102,14 @@ std::unique_ptr<net::test_server::HttpResponse> HandleFullHashRequest(
 
 void StartRedirectingV4RequestsForTesting(
     const std::map<GURL, ThreatMatch>& response_map,
-    net::test_server::EmbeddedTestServer* embedded_test_server) {
+    net::test_server::EmbeddedTestServer* embedded_test_server,
+    const std::map<GURL, base::TimeDelta>& delay_map) {
   // Static so accessing the underlying buffer won't cause use-after-free.
   static std::string url_prefix;
   url_prefix = embedded_test_server->GetURL("/v4").spec();
   SetSbV4UrlPrefixForTesting(url_prefix.c_str());
   embedded_test_server->RegisterRequestHandler(
-      base::Bind(&HandleFullHashRequest, response_map));
+      base::BindRepeating(&HandleFullHashRequest, response_map, delay_map));
 }
 
 }  // namespace safe_browsing

components/safe_browsing/db/v4_embedded_test_server_util.h

@@ -7,6 +7,7 @@
 
 #include <map>
 
+#include "base/time/time.h"
 #include "components/safe_browsing/db/safebrowsing.pb.h"
 #include "url/gurl.h"
 
@@ -18,12 +19,15 @@ class EmbeddedTestServer;
 
 namespace safe_browsing {
 
-// This method does two things:
+// This method does three things:
 // 1. Rewrites the global V4 server URL prefix to point to the test server.
 // 2. Registers the FullHash request handler with the server.
+// 3. (Optionally) associates some delay with the resulting http response.
 void StartRedirectingV4RequestsForTesting(
     const std::map<GURL, ThreatMatch>& response_map,
-    net::test_server::EmbeddedTestServer* embedded_test_server);
+    net::test_server::EmbeddedTestServer* embedded_test_server,
+    const std::map<GURL, base::TimeDelta>& delay_map =
+        std::map<GURL, base::TimeDelta>());
 
 }  // namespace safe_browsing
 

components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc

@@ -115,7 +115,7 @@ content::NavigationThrottle::ThrottleCheckResult
 SubresourceFilterSafeBrowsingActivationThrottle::WillProcessResponse() {
   DCHECK(!database_client_ || !check_results_.empty());
   // No need to defer the navigation if the check already happened.
-  if (!database_client_ || check_results_.back().finished) {
+  if (!database_client_ || HasFinishedAllSafeBrowsingChecks()) {
     NotifyResult();
     return PROCEED;
   }
@@ -143,7 +143,7 @@ void SubresourceFilterSafeBrowsingActivationThrottle::OnCheckUrlResultOnUI(
 
   UMA_HISTOGRAM_TIMES("SubresourceFilter.SafeBrowsing.TotalCheckTime",
                       base::TimeTicks::Now() - check_start_times_[request_id]);
-  if (deferring_ && request_id == check_results_.size() - 1) {
+  if (deferring_ && HasFinishedAllSafeBrowsingChecks()) {
     NotifyResult();
 
     deferring_ = false;
@@ -239,6 +239,16 @@ void SubresourceFilterSafeBrowsingActivationThrottle::NotifyResult() {
       no_redirect_speculation_delay);
 }
 
+bool SubresourceFilterSafeBrowsingActivationThrottle::
+    HasFinishedAllSafeBrowsingChecks() {
+  for (const auto& check_result : check_results_) {
+    if (!check_result.finished) {
+      return false;
+    }
+  }
+  return true;
+}
+
 ActivationDecision
 SubresourceFilterSafeBrowsingActivationThrottle::ComputeActivation(
     ActivationList matched_list,

components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h

@@ -63,6 +63,8 @@ class SubresourceFilterSafeBrowsingActivationThrottle
  private:
   void CheckCurrentUrl();
   void NotifyResult();
+  // Determines whether all of the results in check_results_ are finished.
+  bool HasFinishedAllSafeBrowsingChecks();
 
   ActivationDecision ComputeActivation(ActivationList matched_list,
                                        Configuration* configuration);

net/test/embedded_test_server/default_handlers.cc

@@ -565,24 +565,6 @@ std::unique_ptr<HttpResponse> HandleDefaultResponse(
   return std::move(http_response);
 }
 
-// Delays |delay| seconds before sending a response to the client.
-class DelayedHttpResponse : public BasicHttpResponse {
- public:
-  explicit DelayedHttpResponse(double delay) : delay_(delay) {}
-
-  void SendResponse(const SendBytesCallback& send,
-                    const SendCompleteCallback& done) override {
-    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-        FROM_HERE, base::BindOnce(send, ToResponseString(), done),
-        base::TimeDelta::FromSecondsD(delay_));
-  }
-
- private:
-  const double delay_;
-
-  DISALLOW_COPY_AND_ASSIGN(DelayedHttpResponse);
-};
-
 // /slow?N
 // Returns a response to the server delayed by N seconds.
 std::unique_ptr<HttpResponse> HandleSlowServer(const HttpRequest& request) {
@@ -593,7 +575,7 @@ std::unique_ptr<HttpResponse> HandleSlowServer(const HttpRequest& request) {
     delay = std::atof(request_url.query().c_str());
 
   std::unique_ptr<BasicHttpResponse> http_response(
-      new DelayedHttpResponse(delay));
+      new DelayedHttpResponse(base::TimeDelta::FromSecondsD(delay)));
   http_response->set_content_type("text/plain");
   http_response->set_content(base::StringPrintf("waited %.1f seconds", delay));
   return std::move(http_response);

net/test/embedded_test_server/http_response.cc

@@ -4,10 +4,12 @@
 
 #include "net/test/embedded_test_server/http_response.h"
 
+#include "base/bind.h"
 #include "base/format_macros.h"
 #include "base/logging.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "net/http/http_status_code.h"
 
 namespace net {
@@ -85,6 +87,17 @@ void BasicHttpResponse::SendResponse(const SendBytesCallback& send,
   send.Run(ToResponseString(), done);
 }
 
+DelayedHttpResponse::DelayedHttpResponse(const base::TimeDelta delay)
+    : delay_(delay) {}
+
+DelayedHttpResponse::~DelayedHttpResponse() = default;
+
+void DelayedHttpResponse::SendResponse(const SendBytesCallback& send,
+                                       const SendCompleteCallback& done) {
+  base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE, base::BindOnce(send, ToResponseString(), done), delay_);
+}
+
 void HungResponse::SendResponse(const SendBytesCallback& send,
                                 const SendCompleteCallback& done) {}
 

net/test/embedded_test_server/http_response.h

@@ -11,6 +11,7 @@
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/strings/string_split.h"
+#include "base/time/time.h"
 #include "net/http/http_status_code.h"
 
 namespace net {
@@ -79,6 +80,22 @@ class BasicHttpResponse : public HttpResponse {
   DISALLOW_COPY_AND_ASSIGN(BasicHttpResponse);
 };
 
+class DelayedHttpResponse : public BasicHttpResponse {
+ public:
+  DelayedHttpResponse(const base::TimeDelta delay);
+  ~DelayedHttpResponse() override;
+
+  // Issues a delayed send to the to the task runner.
+  void SendResponse(const SendBytesCallback& send,
+                    const SendCompleteCallback& done) override;
+
+ private:
+  // The delay time for the response.
+  const base::TimeDelta delay_;
+
+  DISALLOW_COPY_AND_ASSIGN(DelayedHttpResponse);
+};
+
 class RawHttpResponse : public HttpResponse {
  public:
   RawHttpResponse(const std::string& headers, const std::string& contents);