Delete scoped_refptr of PasswordProtectionRequest

The PasswordProtectionService and
PasswordProtectionNavigationThrottle both
own a scoped_refptr to PasswordProtectionRequest.
When both the Service and Throttle class are
created, two scoped_refptrs are created for
the same PasswordProtectionRequest. When
the request is finished, the Service class
erases the scoped_reptrs, but the Throttle
class still contains a reference to the
scoped_refptr, which causes a page hang.
Thus, the Throttle class also needs to
release the scoped_refptr when navigation
is resumed or canceled.

A follow up is to refactor the PasswordProtectionRequest
and not have it RefCounted.

Bug: 1110427
Change-Id: Idf66b0451036099d0a417a42b6a37f5a5b10cf13
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2406599
Commit-Queue: Bettina Dea <bdea@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Reviewed-by: Daniel Rubery <drubery@chromium.org>
Cr-Commit-Position: refs/heads/master@{#811911}

components/safe_browsing/content/password_protection/BUILD.gn

@@ -71,7 +71,10 @@ source_set("password_protection_metrics_util") {
 source_set("password_protection_unittest") {
   testonly = true
   if (safe_browsing_mode > 0) {
-    sources = [ "password_protection_service_unittest.cc" ]
+    sources = [
+      "password_protection_navigation_throttle_unittest.cc",
+      "password_protection_service_unittest.cc",
+    ]
     if (safe_browsing_mode == 1) {
       sources += [ "visual_utils_unittest.cc" ]
     }

components/safe_browsing/content/password_protection/password_protection_navigation_throttle.cc

@@ -29,6 +29,10 @@ PasswordProtectionNavigationThrottle::~PasswordProtectionNavigationThrottle() {
 
 content::NavigationThrottle::ThrottleCheckResult
 PasswordProtectionNavigationThrottle::WillStartRequest() {
+  // If a modal warning is being shown right now, we don't
+  // want to continue navigation. Otherwise, we assume that
+  // the PasswordProtectionRequest is still waiting for a
+  // verdict and so we defer the navigation.
   if (is_warning_showing_)
     return content::NavigationThrottle::CANCEL;
   return content::NavigationThrottle::DEFER;
@@ -36,9 +40,15 @@ PasswordProtectionNavigationThrottle::WillStartRequest() {
 
 content::NavigationThrottle::ThrottleCheckResult
 PasswordProtectionNavigationThrottle::WillRedirectRequest() {
+  // If a modal warning is being shown right now, we don't
+  // want to redirect navigation. Otherwise, if the
+  // PasswordProtectionRequest still exists, we assume that the
+  // request is still waiting for a verdict and so we defer the
+  // navigation, otherwise we proceed navigation.
   if (is_warning_showing_)
     return content::NavigationThrottle::CANCEL;
-  return content::NavigationThrottle::DEFER;
+  return request_ ? content::NavigationThrottle::DEFER
+                  : content::NavigationThrottle::PROCEED;
 }
 
 const char* PasswordProtectionNavigationThrottle::GetNameForLogging() {
@@ -47,11 +57,20 @@ const char* PasswordProtectionNavigationThrottle::GetNameForLogging() {
 
 void PasswordProtectionNavigationThrottle::ResumeNavigation() {
   Resume();
+  // When navigation is resumed, we do not need to keep track of the
+  // PasswordProtectionRequest because this method is only called
+  // after the request received a verdict and has finished.
+  request_.reset();
 }
 
 void PasswordProtectionNavigationThrottle::CancelNavigation(
     content::NavigationThrottle::ThrottleCheckResult result) {
+  // When navigation is resumed, we do not need to keep track of the
+  // PasswordProtectionRequest because this method is only called
+  // after the request received a verdict, showing a modal warning and has
+  // finished.
   CancelDeferredNavigation(result);
+  request_.reset();
 }
 
 }  // namespace safe_browsing

components/safe_browsing/content/password_protection/password_protection_navigation_throttle.h

@@ -39,7 +39,11 @@ class PasswordProtectionNavigationThrottle
       override;
   const char* GetNameForLogging() override;
 
+  // Called to resume a deferred navigation once the PasswordProtectionRequest
+  // has received a verdict and there is no modal warning shown.
   void ResumeNavigation();
+  // Called when the PasswordProtectionRequest has received a verdict and there
+  // is a modal warning shown.
   void CancelNavigation(
       content::NavigationThrottle::ThrottleCheckResult result);
 

components/safe_browsing/content/password_protection/password_protection_navigation_throttle_unittest.cc

+// Copyright 2020 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 "components/safe_browsing/content/password_protection/password_protection_navigation_throttle.h"
+
+#include <memory>
+
+#include "base/test/bind_test_util.h"
+#include "components/safe_browsing/content/password_protection/mock_password_protection_service.h"
+#include "components/safe_browsing/content/password_protection/password_protection_request.h"
+#include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/navigation_throttle.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "content/public/test/navigation_simulator.h"
+#include "content/public/test/test_renderer_host.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+namespace safe_browsing {
+
+class PasswordProtectionNavigationThrottleTest
+    : public content::RenderViewHostTestHarness,
+      public content::WebContentsObserver {
+ public:
+  PasswordProtectionNavigationThrottleTest() = default;
+  ~PasswordProtectionNavigationThrottleTest() override = default;
+
+  // content::RenderViewHostTestHarness:
+  void SetUp() override {
+    content::RenderViewHostTestHarness::SetUp();
+
+    // Observe the WebContents to add the throttle.
+    Observe(RenderViewHostTestHarness::web_contents());
+  }
+
+  void TearDown() override {
+    content::RenderViewHostTestHarness::TearDown();
+    throttle_.release();
+  }
+
+  void SetIsWarningShown(bool is_warning_shown) {
+    is_warning_shown_ = is_warning_shown;
+  }
+
+  // content::WebContentsObserver:
+  void DidStartNavigation(
+      content::NavigationHandle* navigation_handle) override {
+    std::vector<password_manager::MatchingReusedCredential> credentials = {
+        {"http://example.test"}, {"http://2.example.com"}};
+    std::unique_ptr<safe_browsing::MockPasswordProtectionService>
+        password_protection_service =
+            std::make_unique<safe_browsing::MockPasswordProtectionService>();
+
+    scoped_refptr<PasswordProtectionRequest> request =
+        new PasswordProtectionRequest(
+            RenderViewHostTestHarness::web_contents(), GURL(), GURL(), GURL(),
+            "username", PasswordType::PASSWORD_TYPE_UNKNOWN, credentials,
+            LoginReputationClientRequest::PASSWORD_REUSE_EVENT,
+            /* password_field_exists*/ true, password_protection_service.get(),
+            /*request_timeout_in_ms=*/0);
+    std::unique_ptr<PasswordProtectionNavigationThrottle> throttle =
+        std::make_unique<PasswordProtectionNavigationThrottle>(
+            navigation_handle, request, is_warning_shown_);
+    throttle_ = std::move(throttle);
+  }
+
+  std::unique_ptr<PasswordProtectionNavigationThrottle> throttle() {
+    return std::move(throttle_);
+  }
+
+ protected:
+  std::unique_ptr<content::NavigationSimulator> StartNavigation(
+      const GURL& first_url) {
+    auto navigation_simulator =
+        content::NavigationSimulator::CreateRendererInitiated(first_url,
+                                                              main_rfh());
+    navigation_simulator->SetAutoAdvance(false);
+    navigation_simulator->Start();
+    return navigation_simulator;
+  }
+
+  bool is_warning_shown_ = false;
+  std::unique_ptr<PasswordProtectionNavigationThrottle> throttle_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(PasswordProtectionNavigationThrottleTest);
+};
+
+TEST_F(PasswordProtectionNavigationThrottleTest, DeferOnNavigation) {
+  auto navigation_simulator = StartNavigation(GURL("http://www.example.com/"));
+  EXPECT_EQ(content::NavigationThrottle::DEFER, throttle()->WillStartRequest());
+}
+
+TEST_F(PasswordProtectionNavigationThrottleTest,
+       CancelNavigationOnWarningShown) {
+  SetIsWarningShown(true);
+
+  auto navigation_simulator = StartNavigation(GURL("http://www.example.com/"));
+  EXPECT_EQ(content::NavigationThrottle::CANCEL,
+            throttle()->WillStartRequest());
+}
+
+TEST_F(PasswordProtectionNavigationThrottleTest, WillDeferRedirectRequest) {
+  auto navigation_simulator = StartNavigation(GURL("http://www.example.com/"));
+
+  std::unique_ptr<PasswordProtectionNavigationThrottle> throttle_copy =
+      std::move(throttle_);
+  throttle_copy->set_resume_callback_for_testing(
+      base::BindLambdaForTesting([&]() {}));
+  EXPECT_EQ(content::NavigationThrottle::DEFER,
+            throttle_copy->WillRedirectRequest());
+
+  // Release request.
+  throttle_copy->ResumeNavigation();
+  EXPECT_EQ(content::NavigationThrottle::PROCEED,
+            throttle_copy->WillRedirectRequest());
+}
+
+TEST_F(PasswordProtectionNavigationThrottleTest, WillCancelRedirectRequest) {
+  auto navigation_simulator = StartNavigation(GURL("http://www.example.com/"));
+
+  // Release request.
+  std::unique_ptr<PasswordProtectionNavigationThrottle> throttle_copy =
+      std::move(throttle_);
+  throttle_copy->set_cancel_deferred_navigation_callback_for_testing(
+      base::BindRepeating(
+          [](content::NavigationThrottle::ThrottleCheckResult result) {}));
+  throttle_copy->CancelNavigation(content::NavigationThrottle::DEFER);
+
+  EXPECT_EQ(content::NavigationThrottle::PROCEED,
+            throttle_copy->WillRedirectRequest());
+}
+
+TEST_F(PasswordProtectionNavigationThrottleTest,
+       WillCancelRedirectRequestOnWarningShown) {
+  SetIsWarningShown(true);
+
+  auto navigation_simulator = StartNavigation(GURL("http://www.example.com/"));
+  EXPECT_EQ(content::NavigationThrottle::CANCEL,
+            throttle()->WillRedirectRequest());
+}
+
+}  // namespace safe_browsing
\ No newline at end of file