Start timeout after allowlist check is done

Previously, Timeout task never gets scheduled if OnGetDomFeatures() doesn't get called.
Move Timeout task schedule into OnWhitelistCheckDone() to make sure it always gets called.

Bug: 977719
Change-Id: I9cf5cd390f68e6f9275cf79cb8f19a9bd046001a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1736517Reviewed-by: Daniel Rubery <drubery@chromium.org>
Reviewed-by: Nathan Parker <nparker@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Commit-Queue: Xinghui Lu <xinghuilu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688221}

components/safe_browsing/password_protection/password_protection_request.cc

@@ -145,10 +145,15 @@ void PasswordProtectionRequest::OnWhitelistCheckDoneOnIO(
 
 void PasswordProtectionRequest::OnWhitelistCheckDone(bool match_whitelist) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  if (match_whitelist)
+  if (match_whitelist) {
     Finish(RequestOutcome::MATCHED_WHITELIST, nullptr);
-  else
+  } else {
+    // In case the request to Safe Browsing takes too long,
+    // we set a timer to cancel that request and return an "unspecified verdict"
+    // so that the navigation isn't blocked indefinitely.
+    StartTimeout();
     CheckCachedVerdicts();
+  }
 }
 
 void PasswordProtectionRequest::CheckCachedVerdicts() {
@@ -361,8 +366,6 @@ void PasswordProtectionRequest::SendRequest() {
     return;
   }
 
-  // In case the request take too long, we set a timer to cancel this request.
-  StartTimeout();
   net::NetworkTrafficAnnotationTag traffic_annotation =
       net::DefineNetworkTrafficAnnotation("password_protection_request", R"(
         semantics {
@@ -485,7 +488,6 @@ void PasswordProtectionRequest::Finish(
                                    response->verdict_type());
     }
   }
-
   password_protection_service_->RequestFinished(this, outcome,
                                                 std::move(response));
 }
@@ -495,8 +497,9 @@ void PasswordProtectionRequest::Cancel(bool timed_out) {
   url_loader_.reset();
   // If request is canceled because |password_protection_service_| is shutting
   // down, ignore all these deferred navigations.
-  if (!timed_out)
+  if (!timed_out) {
     throttles_.clear();
+  }
 
   Finish(timed_out ? RequestOutcome::TIMEDOUT : RequestOutcome::CANCELED,
          nullptr);

components/safe_browsing/password_protection/password_protection_request.h

@@ -129,6 +129,7 @@ class PasswordProtectionRequest
   friend struct content::BrowserThread::DeleteOnThread<
       content::BrowserThread::UI>;
   friend class base::DeleteHelper<PasswordProtectionRequest>;
+  friend class PasswordProtectionServiceTest;
   friend class ChromePasswordProtectionServiceTest;
   virtual ~PasswordProtectionRequest();
 

components/safe_browsing/password_protection/password_protection_service_unittest.cc

@@ -21,6 +21,7 @@
 #include "components/safe_browsing/features.h"
 #include "components/safe_browsing/password_protection/metrics_util.h"
 #include "components/safe_browsing/password_protection/mock_password_protection_service.h"
+#include "components/safe_browsing/password_protection/password_protection_navigation_throttle.h"
 #include "components/safe_browsing/password_protection/password_protection_request.h"
 #include "components/safe_browsing/proto/csd.pb.h"
 #include "components/safe_browsing/verdict_cache_manager.h"
@@ -190,6 +191,21 @@ class TestPasswordProtectionService : public MockPasswordProtectionService {
   DISALLOW_COPY_AND_ASSIGN(TestPasswordProtectionService);
 };
 
+class MockPasswordProtectionNavigationThrottle
+    : public PasswordProtectionNavigationThrottle {
+ public:
+  MockPasswordProtectionNavigationThrottle(
+      content::NavigationHandle* navigation_handle,
+      scoped_refptr<PasswordProtectionRequest> request,
+      bool is_warning_showing)
+      : PasswordProtectionNavigationThrottle(navigation_handle,
+                                             request,
+                                             is_warning_showing) {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockPasswordProtectionNavigationThrottle);
+};
+
 class PasswordProtectionServiceTest : public ::testing::TestWithParam<bool> {
  public:
   PasswordProtectionServiceTest() {}
@@ -333,6 +349,10 @@ class PasswordProtectionServiceTest : public ::testing::TestWithParam<bool> {
     EXPECT_EQ(should_report_content_size, request.has_content_area_width());
   }
 
+  size_t GetNumberOfNavigationThrottles() {
+    return request_ ? request_->throttles_.size() : 0u;
+  }
+
  protected:
   // |thread_bundle_| is needed here because this test involves both UI and IO
   // threads.
@@ -1263,6 +1283,28 @@ TEST_P(PasswordProtectionServiceTest, TestDomFeaturesPopulated) {
                   ->has_dom_features());
 }
 
+TEST_P(PasswordProtectionServiceTest, TestRequestCancelOnTimeout) {
+  content::WebContents* web_contents = GetWebContents();
+  InitializeAndStartPasswordOnFocusRequest(
+      true /* match whitelist */, 10000 /* timeout in ms */, web_contents);
+  auto throttle = std::make_unique<MockPasswordProtectionNavigationThrottle>(
+      nullptr, request_, false);
+  EXPECT_EQ(1U, GetNumberOfNavigationThrottles());
+  request_->Cancel(true /* timeout */);
+  EXPECT_EQ(1U, GetNumberOfNavigationThrottles());
+}
+
+TEST_P(PasswordProtectionServiceTest, TestRequestCancelNotOnTimeout) {
+  content::WebContents* web_contents = GetWebContents();
+  InitializeAndStartPasswordOnFocusRequest(
+      true /* match whitelist */, 10000 /* timeout in ms */, web_contents);
+  auto throttle = std::make_unique<MockPasswordProtectionNavigationThrottle>(
+      nullptr, request_, false);
+  EXPECT_EQ(1U, GetNumberOfNavigationThrottles());
+  request_->Cancel(false /* timeout */);
+  EXPECT_EQ(0U, GetNumberOfNavigationThrottles());
+}
+
 INSTANTIATE_TEST_SUITE_P(Regular,
                          PasswordProtectionServiceTest,
                          ::testing::Values(false));