Don't log PasswordProtectionRequest metrics on shutdown

When the PasswordProtectionService is destroyed, we cancel all pending
PasswordProtectionRequests from the destructor. This leads to pure
virtual calls, since the derived ChromePasswordProtectionService has
been destroyed. So don't log metrics in this situation, since we can't
do so accurately.

CANCELED outcomes are currently 0.0002% of outcomes, so I don't
think the loss of histogram data is important here.

Bug: 957019
Change-Id: Icbadaefc4802c177932375c7a5e638358e834ac5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1585985Reviewed-by: Varun Khaneja <vakh@chromium.org>
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Cr-Commit-Position: refs/heads/master@{#654607}

components/safe_browsing/password_protection/password_protection_request.cc

@@ -16,6 +16,7 @@
 #include "components/password_manager/core/browser/password_reuse_detector.h"
 #include "components/safe_browsing/common/safe_browsing.mojom.h"
 #include "components/safe_browsing/db/whitelist_checker_client.h"
+#include "components/safe_browsing/password_protection/metrics_util.h"
 #include "components/safe_browsing/password_protection/password_protection_navigation_throttle.h"
 #include "components/safe_browsing/password_protection/visual_utils.h"
 #include "components/safe_browsing/proto/csd.pb.h"
@@ -440,24 +441,29 @@ void PasswordProtectionRequest::Finish(
     std::unique_ptr<LoginReputationClientResponse> response) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   tracker_.TryCancelAll();
-  if (trigger_type_ == LoginReputationClientRequest::UNFAMILIAR_LOGIN_PAGE) {
-    LogPasswordOnFocusRequestOutcome(outcome);
-  } else {
-    LogPasswordEntryRequestOutcome(
-        outcome, reused_password_type_,
-        password_protection_service_->GetSyncAccountType());
-    if (reused_password_type_ ==
-        LoginReputationClientRequest::PasswordReuseEvent::SIGN_IN_PASSWORD) {
-      password_protection_service_->MaybeLogPasswordReuseLookupEvent(
-          web_contents_, outcome, response.get());
+
+  // If the request is canceled, the PasswordProtectionService is already
+  // partially destroyed, and we won't be able to log accurate metrics.
+  if (outcome != RequestOutcome::CANCELED) {
+    if (trigger_type_ == LoginReputationClientRequest::UNFAMILIAR_LOGIN_PAGE) {
+      LogPasswordOnFocusRequestOutcome(outcome);
+    } else {
+      LogPasswordEntryRequestOutcome(
+          outcome, reused_password_type_,
+          password_protection_service_->GetSyncAccountType());
+      if (reused_password_type_ ==
+          LoginReputationClientRequest::PasswordReuseEvent::SIGN_IN_PASSWORD) {
+        password_protection_service_->MaybeLogPasswordReuseLookupEvent(
+            web_contents_, outcome, response.get());
+      }
     }
-  }
 
-  if (outcome == RequestOutcome::SUCCEEDED && response) {
-    LogPasswordProtectionVerdict(
-        trigger_type_, reused_password_type_,
-        password_protection_service_->GetSyncAccountType(),
-        response->verdict_type());
+    if (outcome == RequestOutcome::SUCCEEDED && response) {
+      LogPasswordProtectionVerdict(
+          trigger_type_, reused_password_type_,
+          password_protection_service_->GetSyncAccountType(),
+          response->verdict_type());
+    }
   }
 
   password_protection_service_->RequestFinished(

components/safe_browsing/password_protection/password_protection_service_unittest.cc

@@ -36,6 +36,7 @@
 using testing::_;
 using testing::AnyNumber;
 using testing::ElementsAre;
+using testing::IsEmpty;
 using testing::Return;
 
 namespace {
@@ -985,9 +986,11 @@ TEST_P(PasswordProtectionServiceTest, TestTearDownWithPendingRequests) {
   password_protection_service_.reset();
   base::RunLoop().RunUntilIdle();
 
+  // We should not log on TearDown, since that can dispatch calls to pure
+  // virtual methods.
   EXPECT_THAT(
       histograms_.GetAllSamples(kPasswordOnFocusRequestOutcomeHistogram),
-      ElementsAre(base::Bucket(2 /* CANCELED */, 1)));
+      IsEmpty());
 }
 
 TEST_P(PasswordProtectionServiceTest, TestCleanUpExpiredVerdict) {