Ensure all safety tip heuristics always run.

This CL ensures that all safety tip heuristics are always run, in lieu
of the previous short-circuiting behavior, in preparation for recording
metrics. This CL also refactors the reputation check results out into a
wrapper struct, to cut down on the number of variables that need to be
passed down the call chain. Finally, this CL fixes a few miscellaneous
typos.

Bug: 1014598
Change-Id: Ibeda24f817f46d8d36f23fab60f30615821e544f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1881126
Commit-Queue: Aidan Beggs <beggs@google.com>
Reviewed-by: Joe DeBlasio <jdeblasio@chromium.org>
Cr-Commit-Position: refs/heads/master@{#709953}

chrome/browser/reputation/local_heuristics.cc

@@ -99,25 +99,25 @@ bool ShouldTriggerSafetyTipFromKeywordInURL(
       url, net::registry_controlled_domains::EXCLUDE_UNKNOWN_REGISTRIES,
       net::registry_controlled_domains::EXCLUDE_PRIVATE_REGISTRIES);
 
-  // Getting a registry length of 0 means that our URL has an uknown registry.
+  // Getting a registry length of 0 means that our URL has an unknown registry.
   if (registry_length == 0) {
     return false;
   }
 
-  // "eTLD + 1 - 1": "www.google.com" -> "google.com" -> "google"
+  // "eTLD + 1 - 1": "www.google.com" -> "google.com" -> "google".
   std::string eTLD_plusminus =
       eTLD_plus_one.substr(0, eTLD_plus_one.size() - registry_length - 1);
 
-  // We should never end up with a "." in our eTLD + 1 - 1
+  // We should never end up with a "." in our eTLD + 1 - 1.
   DCHECK_EQ(eTLD_plusminus.find("."), std::string::npos);
   // Any problems that would result in an empty eTLD + 1 - 1 should have been
-  // caught bia the |eTLD_plus_one| check.
+  // caught via the |eTLD_plus_one| check.
 
   const std::vector<std::string> eTLD_plusminus_parts = base::SplitString(
       eTLD_plusminus, "-", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
 
   // We only care about finding a keyword here if there's more than one part to
-  // the tokenized eTLD+1-1.
+  // the tokenized eTLD + 1 - 1.
   if (eTLD_plusminus_parts.size() <= 1) {
     return false;
   }

chrome/browser/reputation/reputation_service.cc

@@ -21,6 +21,7 @@
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "components/keyed_service/content/browser_context_keyed_service_factory.h"
 #include "components/safe_browsing/db/v4_protocol_manager_util.h"
+#include "components/security_state/core/security_state.h"
 #include "components/url_formatter/spoof_checks/top_domains/top500_domains.h"
 #include "url/url_constants.h"
 
@@ -51,7 +52,7 @@ class ReputationServiceFactory : public BrowserContextKeyedServiceFactory {
             "ReputationServiceFactory",
             BrowserContextDependencyManager::GetInstance()) {}
 
-  ~ReputationServiceFactory() override {}
+  ~ReputationServiceFactory() override = default;
 
   // BrowserContextKeyedServiceFactory:
   KeyedService* BuildServiceInstanceFor(
@@ -205,13 +206,19 @@ void ReputationService::GetReputationStatusWithEngagedSites(
     const std::vector<DomainInfo>& engaged_sites) {
   const DomainInfo navigated_domain = GetDomainInfo(url);
 
+  ReputationCheckResult result;
+
+  // We evaluate every heuristic for metrics, but only display the first result
+  // for the UI. We use |done_checking_reputation_status| to track when we've
+  // settled on the safety tip to show in the UI, so as to not overwrite this
+  // decision with other heuristics that may trigger later.
+  bool done_checking_reputation_status = false;
+
   // 0. Server-side warning suppression.
   // If the URL is on the allowlist list, do nothing else. This is only used to
   // mitigate false positives, so no further processing should be done.
   if (ShouldSuppressWarning(url)) {
-    std::move(callback).Run(security_state::SafetyTipStatus::kNone,
-                            IsIgnored(url), url, GURL());
-    return;
+    done_checking_reputation_status = true;
   }
 
   // 1. Engagement check
@@ -225,47 +232,55 @@ void ReputationService::GetReputationStatusWithEngagedSites(
                              engaged_domain.domain_and_registry);
                    });
   if (already_engaged != engaged_sites.end()) {
-    std::move(callback).Run(security_state::SafetyTipStatus::kNone,
-                            IsIgnored(url), url, GURL());
-    return;
+    done_checking_reputation_status = true;
   }
 
   // 2. Server-side blocklist check.
   security_state::SafetyTipStatus status = GetSafetyTipUrlBlockType(url);
   if (status != security_state::SafetyTipStatus::kNone) {
-    std::move(callback).Run(status, IsIgnored(url), url, GURL());
-    return;
+    if (!done_checking_reputation_status) {
+      result.safety_tip_status = status;
+    }
+
+    result.triggered_heuristics.blocklist_heuristic_triggered = true;
+    done_checking_reputation_status = true;
   }
 
   // 3. Protect against bad false positives by allowing top domains.
   // Empty domain_and_registry happens on private domains.
   if (navigated_domain.domain_and_registry.empty() ||
       IsTopDomain(navigated_domain)) {
-    std::move(callback).Run(security_state::SafetyTipStatus::kNone,
-                            IsIgnored(url), url, GURL());
-    return;
+    done_checking_reputation_status = true;
   }
 
   // 4. Lookalike heuristics.
   GURL safe_url;
-  if (ShouldTriggerSafetyTipFromLookalike(url, navigated_domain, engaged_sites,
+  if (already_engaged == engaged_sites.end() &&
+      ShouldTriggerSafetyTipFromLookalike(url, navigated_domain, engaged_sites,
                                           &safe_url)) {
-    std::move(callback).Run(security_state::SafetyTipStatus::kLookalike,
-                            IsIgnored(url), url, safe_url);
-    return;
+    if (!done_checking_reputation_status) {
+      result.suggested_url = safe_url;
+      result.safety_tip_status = security_state::SafetyTipStatus::kLookalike;
+    }
+
+    result.triggered_heuristics.lookalike_heuristic_triggered = true;
+    done_checking_reputation_status = true;
   }
 
   // 5. Keyword heuristics.
   if (ShouldTriggerSafetyTipFromKeywordInURL(
           url, navigated_domain, top500_domains::kTop500Keywords, 500)) {
-    std::move(callback).Run(security_state::SafetyTipStatus::kBadKeyword,
-                            IsIgnored(url), url, GURL());
-    return;
+    if (!done_checking_reputation_status) {
+      result.safety_tip_status = security_state::SafetyTipStatus::kBadKeyword;
+    }
+
+    result.triggered_heuristics.keywords_heuristic_triggered = true;
+    done_checking_reputation_status = true;
   }
 
-  // TODO(crbug/984725): 6. Additional client-side heuristics.
-  std::move(callback).Run(security_state::SafetyTipStatus::kNone,
-                          IsIgnored(url), url, GURL());
+  result.user_previously_ignored = IsIgnored(url);
+  result.url = url;
+  std::move(callback).Run(result);
 }
 
 security_state::SafetyTipStatus GetSafetyTipUrlBlockType(const GURL& url) {

chrome/browser/reputation/reputation_service.h

@@ -19,14 +19,33 @@
 class Profile;
 struct DomainInfo;
 
-// Callback type used for retrieving reputation status. |ignored| indicates
-// whether the user has dismissed the warning and thus should not be warned
-// again. |url| is the URL applicable for this result,
+// All potential heuristics that can trigger. Used temporarily to keep track of
+// which heuristics trigger during a reputation check, and later used to decide
+// which metrics get recorded.
+struct TriggeredHeuristics {
+  bool blocklist_heuristic_triggered = false;
+  bool lookalike_heuristic_triggered = false;
+  bool keywords_heuristic_triggered = false;
+};
+
+// Wrapper used to store the results of a reputation check. Specifically, this
+// is passed to the callback given to GetReputationStatus.
+// |user_previously_ignored| indicates whether the use has dismissed the warning
+// previously, and thus should not be warned again. |url| is the URL applicable
+// for this result.
+struct ReputationCheckResult {
+  security_state::SafetyTipStatus safety_tip_status =
+      security_state::SafetyTipStatus::kNone;
+  bool user_previously_ignored = false;
+  GURL url;
+  GURL suggested_url;
+  TriggeredHeuristics triggered_heuristics;
+};
+
+// Callback type used for retrieving reputation status. The results of the
+// reputation check are given in |result|.
 using ReputationCheckCallback =
-    base::OnceCallback<void(security_state::SafetyTipStatus,
-                            bool ignored,
-                            const GURL& url,
-                            const GURL& suggested_url)>;
+    base::OnceCallback<void(ReputationCheckResult result)>;
 
 // Provides reputation information on URLs for Safety Tips.
 class ReputationService : public KeyedService {

chrome/browser/reputation/reputation_web_contents_observer.cc

@@ -11,6 +11,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "build/build_config.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/reputation/reputation_service.h"
 #include "components/security_state/core/features.h"
 #include "content/public/browser/navigation_entry.h"
 
@@ -135,22 +136,20 @@ void ReputationWebContentsObserver::MaybeShowSafetyTip() {
 }
 
 void ReputationWebContentsObserver::HandleReputationCheckResult(
-    security_state::SafetyTipStatus safety_tip_status,
-    bool user_ignored,
-    const GURL& url,
-    const GURL& suggested_url) {
+    ReputationCheckResult result) {
   UMA_HISTOGRAM_ENUMERATION("Security.SafetyTips.SafetyTipShown",
-                            safety_tip_status);
+                            result.safety_tip_status);
 
-  if (safety_tip_status == security_state::SafetyTipStatus::kNone ||
-      safety_tip_status == security_state::SafetyTipStatus::kBadKeyword) {
+  if (result.safety_tip_status == security_state::SafetyTipStatus::kNone ||
+      result.safety_tip_status ==
+          security_state::SafetyTipStatus::kBadKeyword) {
     MaybeCallReputationCheckCallback();
     return;
   }
 
-  if (user_ignored) {
+  if (result.user_previously_ignored) {
     UMA_HISTOGRAM_ENUMERATION("Security.SafetyTips.SafetyTipIgnoredPageLoad",
-                              safety_tip_status);
+                              result.safety_tip_status);
     MaybeCallReputationCheckCallback();
     return;
   }
@@ -158,7 +157,8 @@ void ReputationWebContentsObserver::HandleReputationCheckResult(
   // Set this field independent of whether the feature to show the UI is
   // enabled/disabled. Metrics code uses this field and we want to record
   // metrics regardless of the feature being enabled/disabled.
-  last_navigation_safety_tip_info_ = {safety_tip_status, suggested_url};
+  last_navigation_safety_tip_info_ = {result.safety_tip_status,
+                                      result.suggested_url};
 
   // A navigation entry should always exist because reputation checks are only
   // triggered when a committed navigation finishes.
@@ -175,8 +175,10 @@ void ReputationWebContentsObserver::HandleReputationCheckResult(
     return;
   }
   ShowSafetyTipDialog(
-      web_contents(), safety_tip_status, url, suggested_url,
-      base::BindOnce(OnSafetyTipClosed, safety_tip_status, base::Time::Now()));
+      web_contents(), result.safety_tip_status, result.url,
+      result.suggested_url,
+      base::BindOnce(OnSafetyTipClosed, result.safety_tip_status,
+                     base::Time::Now()));
 }
 
 void ReputationWebContentsObserver::MaybeCallReputationCheckCallback() {

chrome/browser/reputation/reputation_web_contents_observer.h

@@ -5,8 +5,6 @@
 #ifndef CHROME_BROWSER_REPUTATION_REPUTATION_WEB_CONTENTS_OBSERVER_H_
 #define CHROME_BROWSER_REPUTATION_REPUTATION_WEB_CONTENTS_OBSERVER_H_
 
-#include <set>
-
 #include "base/callback_forward.h"
 #include "chrome/browser/reputation/reputation_service.h"
 #include "chrome/browser/reputation/safety_tip_ui.h"
@@ -53,11 +51,7 @@ class ReputationWebContentsObserver
 
   // A ReputationCheckCallback. Called by the reputation service when a
   // reputation result is available.
-  void HandleReputationCheckResult(
-      security_state::SafetyTipStatus safety_tip_status,
-      bool user_ignored,
-      const GURL& url,
-      const GURL& suggested_url);
+  void HandleReputationCheckResult(ReputationCheckResult result);
 
   // A helper method that calls and resets
   // |reputation_check_callback_for_testing_| if it is set.

chrome/browser/ui/views/page_info/safety_tip_page_info_bubble_view_browsertest.cc

@@ -475,6 +475,7 @@ IN_PROC_BROWSER_TEST_P(SafetyTipPageInfoBubbleViewBrowserTest,
   if (ui_status() == UIStatus::kDisabled) {
     return;
   }
+
   auto kNavigatedUrl = GetURL("site1.com");
   TriggerWarning(browser(), kNavigatedUrl, WindowOpenDisposition::CURRENT_TAB);