Update TriggerManager to consider user preferences and incognito state.

TriggerManager will now decide whether data collection can begin, and
whether a report can be sent, based on user preferences and incognito
state.

This also required moving the kSafeBrowsingExtendedReportingOptInAllowed
pref into component safe_browsing_prefs.

Bug: 730646
Change-Id: I9d84a550d389cc54eb1dfc2630066ca9fd6c5b20
Reviewed-on: https://chromium-review.googlesource.com/537392Reviewed-by: Tim Volodine <timvolodine@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Reviewed-by: Elliot Glaysher <erg@chromium.org>
Reviewed-by: Dominic Battré <battre@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Reviewed-by: Jialiu Lin <jialiul@chromium.org>
Commit-Queue: Luke Z <lpz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481947}

chrome/browser/policy/configuration_policy_handler_list_factory.cc

@@ -48,6 +48,7 @@
 #include "components/policy/core/common/schema.h"
 #include "components/policy/policy_constants.h"
 #include "components/prefs/pref_value_map.h"
+#include "components/safe_browsing/common/safe_browsing_prefs.h"
 #include "components/search_engines/default_search_policy_handler.h"
 #include "components/signin/core/common/signin_pref_names.h"
 #include "components/spellcheck/spellcheck_build_features.h"

chrome/browser/prefs/browser_prefs.cc

@@ -99,6 +99,7 @@
 #include "components/prefs/pref_service.h"
 #include "components/proxy_config/pref_proxy_config_tracker_impl.h"
 #include "components/rappor/rappor_service_impl.h"
+#include "components/safe_browsing/common/safe_browsing_prefs.h"
 #include "components/search_engines/template_url_prepopulate_data.h"
 #include "components/ssl_config/ssl_config_service_manager.h"
 #include "components/startup_metric_utils/browser/startup_metric_utils.h"
@@ -483,6 +484,7 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {
   ProtocolHandlerRegistry::RegisterProfilePrefs(registry);
   PushMessagingAppIdentifier::RegisterProfilePrefs(registry);
   RegisterBrowserUserPrefs(registry);
+  safe_browsing::RegisterProfilePrefs(registry);
   SessionStartupPref::RegisterProfilePrefs(registry);
   TemplateURLPrepopulateData::RegisterProfilePrefs(registry);
   translate::LanguageModel::RegisterProfilePrefs(registry);

chrome/browser/profiles/profile.cc

@@ -147,21 +147,10 @@ void Profile::RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {
       prefs::kSafeBrowsingEnabled,
       true,
       user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
-  registry->RegisterBooleanPref(prefs::kSafeBrowsingExtendedReportingEnabled,
-                                false);
-  registry->RegisterBooleanPref(prefs::kSafeBrowsingScoutReportingEnabled,
-                                false);
-  registry->RegisterBooleanPref(prefs::kSafeBrowsingScoutGroupSelected, false);
-  registry->RegisterBooleanPref(
-      prefs::kSafeBrowsingSawInterstitialExtendedReporting, false);
-  registry->RegisterBooleanPref(
-      prefs::kSafeBrowsingSawInterstitialScoutReporting, false);
   registry->RegisterBooleanPref(prefs::kSafeBrowsingProceedAnywayDisabled,
                                 false);
   registry->RegisterBooleanPref(prefs::kSSLErrorOverrideAllowed, true);
   registry->RegisterDictionaryPref(prefs::kSafeBrowsingIncidentsSent);
-  registry->RegisterBooleanPref(
-      prefs::kSafeBrowsingExtendedReportingOptInAllowed, true);
   // This pref is intentionally outside the above #if. That flag corresponds
   // to the Notifier extension and does not gate the launcher page.
   // TODO(skare): Remove or rename ENABLE_GOOGLE_NOW: http://crbug.com/459827.

chrome/browser/safe_browsing/safe_browsing_blocking_page.cc

@@ -63,7 +63,7 @@ class SafeBrowsingBlockingPageFactoryImpl
         Profile::FromBrowserContext(web_contents->GetBrowserContext())
             ->GetPrefs();
     bool is_extended_reporting_opt_in_allowed =
-        prefs->GetBoolean(prefs::kSafeBrowsingExtendedReportingOptInAllowed);
+        IsExtendedReportingOptInAllowed(*prefs);
     bool is_proceed_anyway_disabled =
         prefs->GetBoolean(prefs::kSafeBrowsingProceedAnywayDisabled);
 
@@ -116,14 +116,18 @@ SafeBrowsingBlockingPage::SafeBrowsingBlockingPage(
           CreateControllerClient(web_contents, unsafe_resources, ui_manager),
           display_options),
       threat_details_in_progress_(false) {
-  // Start computing threat details. They will be sent only
-  // if the user opts-in on the blocking page later.
-  // If there's more than one malicious resources, it means the user
-  // clicked through the first warning, so we don't prepare additional
-  // reports.
+  // Make sure the safe browsing service is available - it may not be when
+  // shutting down.
+  if (!g_browser_process->safe_browsing_service())
+    return;
+
+  // Start computing threat details. Trigger Manager will decide if it's safe to
+  // begin collecting data at this time. The report will be sent only if the
+  // user opts-in on the blocking page later.
+  // If there's more than one malicious resources, it means the user clicked
+  // through the first warning, so we don't prepare additional reports.
   if (unsafe_resources.size() == 1 &&
-      ShouldReportThreatDetails(unsafe_resources[0].threat_type) &&
-      sb_error_ui()->CanShowExtendedReportingOption()) {
+      ShouldReportThreatDetails(unsafe_resources[0].threat_type)) {
     Profile* profile =
         Profile::FromBrowserContext(web_contents->GetBrowserContext());
     threat_details_in_progress_ =
@@ -132,7 +136,8 @@ SafeBrowsingBlockingPage::SafeBrowsingBlockingPage(
             ->StartCollectingThreatDetails(
                 web_contents, unsafe_resources[0], profile->GetRequestContext(),
                 HistoryServiceFactory::GetForProfile(
-                    profile, ServiceAccessType::EXPLICIT_ACCESS));
+                    profile, ServiceAccessType::EXPLICIT_ACCESS),
+                sb_error_ui()->get_error_display_options());
   }
 }
 
@@ -192,18 +197,23 @@ void SafeBrowsingBlockingPage::FinishThreatDetails(const base::TimeDelta& delay,
   if (!threat_details_in_progress_)
     return;
 
-  const bool enabled = sb_error_ui()->is_extended_reporting_enabled() &&
-                       sb_error_ui()->is_extended_reporting_opt_in_allowed();
-  if (!enabled)
+  // Make sure the safe browsing service is available - it may not be when
+  // shutting down.
+  if (!g_browser_process->safe_browsing_service())
     return;
 
-  controller()->metrics_helper()->RecordUserInteraction(
-      security_interstitials::MetricsHelper::EXTENDED_REPORTING_IS_ENABLED);
-  // Finish the malware details collection, send it over.
-  g_browser_process->safe_browsing_service()
-      ->trigger_manager()
-      ->FinishCollectingThreatDetails(web_contents(), delay, did_proceed,
-                                      num_visits);
+  // Finish computing threat details. TriggerManager will decide if its safe to
+  // send the report.
+  bool report_sent = g_browser_process->safe_browsing_service()
+                         ->trigger_manager()
+                         ->FinishCollectingThreatDetails(
+                             web_contents(), delay, did_proceed, num_visits,
+                             sb_error_ui()->get_error_display_options());
+
+  if (report_sent) {
+    controller()->metrics_helper()->RecordUserInteraction(
+        security_interstitials::MetricsHelper::EXTENDED_REPORTING_IS_ENABLED);
+  }
 }
 
 // static

chrome/browser/safe_browsing/safe_browsing_service.h

@@ -236,6 +236,7 @@ class SafeBrowsingService : public base::RefCountedThreadSafe<
   friend class SafeBrowsingBlockingQuietPageTest;
   friend class SafeBrowsingServerTest;
   friend class SafeBrowsingServiceTest;
+  friend class SafeBrowsingUIManagerTest;
   friend class SafeBrowsingURLRequestContextGetter;
   friend class TestSafeBrowsingService;
   friend class TestSafeBrowsingServiceFactory;

chrome/browser/safe_browsing/ui_manager_unittest.cc

@@ -6,9 +6,10 @@
 
 #include "base/run_loop.h"
 #include "chrome/browser/safe_browsing/safe_browsing_blocking_page.h"
-#include "chrome/browser/safe_browsing/safe_browsing_service.h"
+#include "chrome/browser/safe_browsing/test_safe_browsing_service.h"
 #include "chrome/browser/safe_browsing/ui_manager.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
+#include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/safe_browsing/common/safe_browsing_prefs.h"
 #include "components/safe_browsing_db/util.h"
@@ -76,9 +77,25 @@ class SafeBrowsingUIManagerTest : public ChromeRenderViewHostTestHarness {
     SetThreadBundleOptions(content::TestBrowserThreadBundle::REAL_IO_THREAD);
     ChromeRenderViewHostTestHarness::SetUp();
     SafeBrowsingUIManager::CreateWhitelistForTesting(web_contents());
+
+    safe_browsing::TestSafeBrowsingServiceFactory sb_service_factory;
+    auto* safe_browsing_service =
+        sb_service_factory.CreateSafeBrowsingService();
+    TestingBrowserProcess::GetGlobal()->SetSafeBrowsingService(
+        safe_browsing_service);
+    g_browser_process->safe_browsing_service()->Initialize();
+    // A profile was created already but SafeBrowsingService wasn't around to
+    // get notified of it, so include that notification now.
+    safe_browsing_service->AddPrefService(
+        Profile::FromBrowserContext(web_contents()->GetBrowserContext())
+            ->GetPrefs());
   }
 
-  void TearDown() override { ChromeRenderViewHostTestHarness::TearDown(); }
+  void TearDown() override {
+    TestingBrowserProcess::GetGlobal()->safe_browsing_service()->ShutDown();
+    TestingBrowserProcess::GetGlobal()->SetSafeBrowsingService(nullptr);
+    ChromeRenderViewHostTestHarness::TearDown();
+  }
 
   bool IsWhitelisted(security_interstitials::UnsafeResource resource) {
     return ui_manager_->IsWhitelisted(resource);

chrome/browser/ssl/ssl_browser_tests.cc

@@ -63,6 +63,7 @@
 #include "components/network_time/network_time_test_utils.h"
 #include "components/network_time/network_time_tracker.h"
 #include "components/prefs/testing_pref_service.h"
+#include "components/safe_browsing/common/safe_browsing_prefs.h"
 #include "components/security_interstitials/core/controller_client.h"
 #include "components/security_interstitials/core/metrics_helper.h"
 #include "components/security_state/core/security_state.h"

chrome/common/pref_names.cc

@@ -422,11 +422,6 @@ const char kSafeBrowsingProceedAnywayDisabled[] =
 // A dictionary mapping incident types to a dict of incident key:digest pairs.
 const char kSafeBrowsingIncidentsSent[] = "safebrowsing.incidents_sent";
 
-// Boolean that tells us whether users are given the option to opt in to Safe
-// Browsing extended reporting.
-const char kSafeBrowsingExtendedReportingOptInAllowed[] =
-    "safebrowsing.extended_reporting_opt_in_allowed";
-
 // Boolean that is true when the SSL interstitial should allow users to
 // proceed anyway. Otherwise, proceeding is not possible.
 const char kSSLErrorOverrideAllowed[] = "ssl.error_override_allowed";

chrome/common/pref_names.h

@@ -173,7 +173,6 @@ extern const char kDataSaverEnabled[];
 extern const char kSafeBrowsingEnabled[];
 extern const char kSafeBrowsingProceedAnywayDisabled[];
 extern const char kSafeBrowsingIncidentsSent[];
-extern const char kSafeBrowsingExtendedReportingOptInAllowed[];
 extern const char kSSLErrorOverrideAllowed[];
 extern const char kIncognitoModeAvailability[];
 extern const char kSearchSuggestEnabled[];

components/safe_browsing/common/safe_browsing_prefs.cc

@@ -7,6 +7,7 @@
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
+#include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
 
 namespace {
@@ -139,6 +140,8 @@ void RecordExtendedReportingPrefChanged(
 }  // namespace
 
 namespace prefs {
+const char kSafeBrowsingExtendedReportingOptInAllowed[] =
+    "safebrowsing.extended_reporting_opt_in_allowed";
 const char kSafeBrowsingExtendedReportingEnabled[] =
     "safebrowsing.extended_reporting_enabled";
 const char kSafeBrowsingScoutReportingEnabled[] =
@@ -294,6 +297,10 @@ void InitializeSafeBrowsingPrefs(PrefService* prefs) {
   }
 }
 
+bool IsExtendedReportingOptInAllowed(const PrefService& prefs) {
+  return prefs.GetBoolean(prefs::kSafeBrowsingExtendedReportingOptInAllowed);
+}
+
 bool IsExtendedReportingEnabled(const PrefService& prefs) {
   return prefs.GetBoolean(GetExtendedReportingPrefName(prefs));
 }
@@ -345,6 +352,21 @@ void RecordExtendedReportingMetrics(const PrefService& prefs) {
   }
 }
 
+void RegisterProfilePrefs(PrefRegistrySimple* registry) {
+  // TODO(lpz): Move other safe browsing prefs here from c/b/profiles/profile.cc
+  registry->RegisterBooleanPref(prefs::kSafeBrowsingExtendedReportingEnabled,
+                                false);
+  registry->RegisterBooleanPref(prefs::kSafeBrowsingScoutReportingEnabled,
+                                false);
+  registry->RegisterBooleanPref(prefs::kSafeBrowsingScoutGroupSelected, false);
+  registry->RegisterBooleanPref(
+      prefs::kSafeBrowsingSawInterstitialExtendedReporting, false);
+  registry->RegisterBooleanPref(
+      prefs::kSafeBrowsingSawInterstitialScoutReporting, false);
+  registry->RegisterBooleanPref(
+      prefs::kSafeBrowsingExtendedReportingOptInAllowed, true);
+}
+
 void SetExtendedReportingPrefAndMetric(
     PrefService* prefs,
     bool value,

components/safe_browsing/common/safe_browsing_prefs.h

@@ -9,9 +9,15 @@
 
 #include "base/feature_list.h"
 
+class PrefRegistrySimple;
 class PrefService;
 
 namespace prefs {
+// Boolean that tells us whether users are given the option to opt in to Safe
+// Browsing extended reporting. This is exposed as a preference that can be
+// overridden by enterprise policy.
+extern const char kSafeBrowsingExtendedReportingOptInAllowed[];
+
 // Boolean that tell us whether Safe Browsing extended reporting is enabled.
 extern const char kSafeBrowsingExtendedReportingEnabled[];
 
@@ -113,6 +119,10 @@ const char* GetExtendedReportingPrefName(const PrefService& prefs);
 // TODO: this is temporary (crbug.com/662944)
 void InitializeSafeBrowsingPrefs(PrefService* prefs);
 
+// Returns whether the user is able to modify the Safe Browsing Extended
+// Reporting opt-in.
+bool IsExtendedReportingOptInAllowed(const PrefService& prefs);
+
 // Returns whether Safe Browsing Extended Reporting is currently enabled.
 // This should be used to decide if any of the reporting preferences are set,
 // regardless of which specific one is set.
@@ -124,6 +134,9 @@ bool IsScout(const PrefService& prefs);
 // Updates UMA metrics about Safe Browsing Extended Reporting states.
 void RecordExtendedReportingMetrics(const PrefService& prefs);
 
+// Registers user preferences related to Safe Browsing.
+void RegisterProfilePrefs(PrefRegistrySimple* registry);
+
 // Sets the currently active Safe Browsing Extended Reporting preference to the
 // specified value. The |location| indicates the UI where the change was
 // made.

components/safe_browsing/triggers/BUILD.gn

@@ -11,10 +11,12 @@ source_set("triggers") {
   ]
   public_deps = [
     "//components/security_interstitials/content:security_interstitial_page",
+    "//components/security_interstitials/core:core",
     "//content/public/browser:browser",
   ]
   deps = [
     "//base:base",
+    "//components/prefs:prefs",
     "//components/safe_browsing:safe_browsing",
     "//components/safe_browsing/browser:browser",
   ]
@@ -29,6 +31,7 @@ source_set("unit_tests") {
     ":triggers",
     "//base",
     "//chrome/test:test_support",
+    "//components/prefs:test_support",
     "//components/safe_browsing/browser:browser",
     "//content/test:test_support",
     "//testing/gtest",

components/safe_browsing/triggers/DEPS

 include_rules = [
   "+content/public/test",
+  "+components/prefs",
 ]

components/safe_browsing/triggers/trigger_manager.cc

@@ -4,26 +4,67 @@
 
 #include "components/safe_browsing/triggers/trigger_manager.h"
 
+#include "components/prefs/pref_service.h"
 #include "components/safe_browsing/base_ui_manager.h"
 #include "components/safe_browsing/browser/threat_details.h"
+#include "components/safe_browsing/common/safe_browsing_prefs.h"
 #include "components/security_interstitials/content/unsafe_resource.h"
+#include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "net/url_request/url_request_context_getter.h"
 
 namespace safe_browsing {
 
+namespace {
+
+bool CanStartDataCollection(const SBErrorOptions& error_display_options) {
+  // We start data collection as long as user is not incognito and is able to
+  // change the Extended Reporting opt-in. We don't require them to be opted-in
+  // to SBER to begin collecting data, since they may be able to change the
+  // setting while data collection is running (eg: on a security interstitial).
+  return !error_display_options.is_off_the_record &&
+         error_display_options.is_extended_reporting_opt_in_allowed;
+}
+
+bool CanSendReport(const SBErrorOptions& error_display_options) {
+  // Reports are only sent for non-incoginito users who are allowed to modify
+  // the Extended Reporting setting and have opted-in to Extended Reporting.
+  return !error_display_options.is_off_the_record &&
+         error_display_options.is_extended_reporting_opt_in_allowed &&
+         error_display_options.is_extended_reporting_enabled;
+}
+
+}  // namespace
+
 TriggerManager::TriggerManager(BaseUIManager* ui_manager)
     : ui_manager_(ui_manager) {}
 
 TriggerManager::~TriggerManager() {}
 
+// static
+SBErrorOptions TriggerManager::GetSBErrorDisplayOptions(
+    const PrefService& pref_service,
+    const content::WebContents& web_contents) {
+  return SBErrorOptions(/*is_main_frame_load_blocked=*/false,
+                        IsExtendedReportingOptInAllowed(pref_service),
+                        web_contents.GetBrowserContext()->IsOffTheRecord(),
+                        IsExtendedReportingEnabled(pref_service),
+                        /*is_scout_reporting_enabled=*/false,
+                        /*is_proceed_anyway_disabled=*/false,
+                        /*help_center_article_link=*/std::string());
+}
+
 bool TriggerManager::StartCollectingThreatDetails(
     content::WebContents* web_contents,
     const security_interstitials::UnsafeResource& resource,
     net::URLRequestContextGetter* request_context_getter,
-    history::HistoryService* history_service) {
+    history::HistoryService* history_service,
+    const SBErrorOptions& error_display_options) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
+  if (!CanStartDataCollection(error_display_options))
+    return false;
+
   // Ensure we're not already collecting data on this tab.
   if (base::ContainsKey(data_collectors_map_, web_contents))
     return false;
@@ -38,27 +79,36 @@ bool TriggerManager::FinishCollectingThreatDetails(
     content::WebContents* web_contents,
     const base::TimeDelta& delay,
     bool did_proceed,
-    int num_visits) {
+    int num_visits,
+    const SBErrorOptions& error_display_options) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   // Make sure there's a data collector running on this tab.
   if (!base::ContainsKey(data_collectors_map_, web_contents))
     return false;
 
+  // Determine whether a report should be sent.
+  bool should_send_report = CanSendReport(error_display_options);
+
   // Find the data collector and tell it to finish collecting data, and then
   // remove it from our map. We release ownership of the data collector here but
   // it will live until the end of the FinishCollection call because it
   // implements RefCountedThreadSafe.
-  scoped_refptr<ThreatDetails> threat_details =
-      data_collectors_map_[web_contents];
-  content::BrowserThread::PostDelayedTask(
-      content::BrowserThread::IO, FROM_HERE,
-      base::BindOnce(&ThreatDetails::FinishCollection, threat_details,
-                     did_proceed, num_visits),
-      delay);
+  if (should_send_report) {
+    scoped_refptr<ThreatDetails> threat_details =
+        data_collectors_map_[web_contents];
+    content::BrowserThread::PostDelayedTask(
+        content::BrowserThread::IO, FROM_HERE,
+        base::BindOnce(&ThreatDetails::FinishCollection, threat_details,
+                       did_proceed, num_visits),
+        delay);
+  }
+
+  // Regardless of whether the report got sent, clean up the data collector on
+  // this tab.
   data_collectors_map_.erase(web_contents);
 
-  return true;
+  return should_send_report;
 }
 
 }  // namespace safe_browsing

components/safe_browsing/triggers/trigger_manager.h

@@ -10,8 +10,11 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "components/security_interstitials/content/unsafe_resource.h"
+#include "components/security_interstitials/core/base_safe_browsing_error_ui.h"
 #include "content/public/browser/web_contents.h"
 
+class PrefService;
+
 namespace history {
 class HistoryService;
 }
@@ -32,6 +35,9 @@ class ThreatDetails;
 using DataCollectorsMap =
     std::unordered_map<content::WebContents*, scoped_refptr<ThreatDetails>>;
 
+using SBErrorOptions =
+    security_interstitials::BaseSafeBrowsingErrorUI::SBErrorDisplayOptions;
+
 // This class manages SafeBrowsing data-reporting triggers. Triggers are
 // activated for users opted-in to Extended Reporting and when security-related
 // data collection is required.
@@ -44,28 +50,45 @@ class TriggerManager {
   TriggerManager(BaseUIManager* ui_manager);
   ~TriggerManager();
 
+  // Returns a SBErrorDisplayOptions struct containing user state that is
+  // relevant for TriggerManager to decide whether to start/finish data
+  // collection. Looks at incognito state from |web_contents|, and opt-ins from
+  // |pref_service|. Only the fields needed by TriggerManager will be set.
+  static SBErrorOptions GetSBErrorDisplayOptions(
+      const PrefService& pref_service,
+      const content::WebContents& web_contents);
+
   // Begins collecting a ThreatDetails report on the specified |web_contents|.
   // |resource| is the unsafe resource that cause the collection to occur.
   // |request_context_getter| is used to retrieve data from the HTTP cache.
   // |history_service| is used to get data about redirects.
+  // |error_display_options| contains the current state of relevant user
+  // preferences. We use this object for interop with WebView, in Chrome it
+  // should be created by TriggerManager::GetSBErrorDisplayOptions().
   // Returns true if the collection began, or false if it didn't.
   bool StartCollectingThreatDetails(
       content::WebContents* web_contents,
       const security_interstitials::UnsafeResource& resource,
       net::URLRequestContextGetter* request_context_getter,
-      history::HistoryService* history_service);
+      history::HistoryService* history_service,
+      const SBErrorOptions& error_display_options);
 
   // Completes the collection of a ThreatDetails report on the specified
   // |web_contents| and sends the report. |delay| can be used to wait a period
   // of time before finishing the report. |did_proceed| indicates whether the
   // user proceeded through the security interstitial associated with this
   // report. |num_visits| is how many times the user has visited the site
-  // before.
-  // Returns true if the report was completed and sent, or false otherwise.
-  bool FinishCollectingThreatDetails(content::WebContents* web_contents,
-                                     const base::TimeDelta& delay,
-                                     bool did_proceed,
-                                     int num_visits);
+  // before. |error_display_options| contains the current state of relevant user
+  // preferences. We use this object for interop with WebView, in Chrome it
+  // should be created by TriggerManager::GetSBErrorDisplayOptions().
+  // Returns true if the report was completed and sent, or false otherwise (eg:
+  // the user was not opted-in to extended reporting after collection began).
+  bool FinishCollectingThreatDetails(
+      content::WebContents* web_contents,
+      const base::TimeDelta& delay,
+      bool did_proceed,
+      int num_visits,
+      const SBErrorOptions& error_display_options);
 
  private:
   friend class TriggerManagerTest;

components/safe_browsing/triggers/trigger_manager_unittest.cc

@@ -3,6 +3,10 @@
 // found in the LICENSE file.
 
 #include "components/safe_browsing/triggers/trigger_manager.h"
+
+#include "base/stl_util.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/testing_pref_service.h"
 #include "components/safe_browsing/browser/threat_details.h"
 #include "content/public/test/test_browser_context.h"
 #include "content/public/test/test_browser_thread_bundle.h"
@@ -38,7 +42,6 @@ class MockThreatDetailsFactory : public ThreatDetailsFactory {
       net::URLRequestContextGetter* request_context_getter,
       history::HistoryService* history_service) override {
     MockThreatDetails* threat_details = new MockThreatDetails();
-    EXPECT_CALL(*threat_details, FinishCollection(_, _)).Times(1);
     return threat_details;
   }
 };
@@ -50,21 +53,50 @@ class TriggerManagerTest : public ::testing::Test {
 
   void SetUp() override {
     ThreatDetails::RegisterFactory(&mock_threat_details_factory_);
+
+    // Register any prefs that are needed by the trigger manager. By default,
+    // enable Safe Browsing and Extended Reporting.
+    safe_browsing::RegisterProfilePrefs(pref_service_.registry());
+    SetPref(prefs::kSafeBrowsingExtendedReportingOptInAllowed, true);
+    SetPref(prefs::kSafeBrowsingScoutReportingEnabled, true);
+    SetPref(prefs::kSafeBrowsingScoutGroupSelected, true);
+  }
+
+  void SetPref(const std::string& pref, bool value) {
+    pref_service_.SetBoolean(pref, value);
   }
 
   content::WebContents* CreateWebContents() {
+    DCHECK(!browser_context_.IsOffTheRecord())
+        << "CreateWebContents() should not be called after "
+           "CreateIncognitoWebContents()";
+    return web_contents_factory_.CreateWebContents(&browser_context_);
+  }
+
+  content::WebContents* CreateIncognitoWebContents() {
+    browser_context_.set_is_off_the_record(true);
     return web_contents_factory_.CreateWebContents(&browser_context_);
   }
 
   bool StartCollectingThreatDetails(content::WebContents* web_contents) {
+    SBErrorOptions options =
+        TriggerManager::GetSBErrorDisplayOptions(pref_service_, *web_contents);
     return trigger_manager_.StartCollectingThreatDetails(
         web_contents, security_interstitials::UnsafeResource(), nullptr,
-        nullptr);
+        nullptr, options);
   }
 
-  bool FinishCollectingThreatDetails(content::WebContents* web_contents) {
+  bool FinishCollectingThreatDetails(content::WebContents* web_contents,
+                                     bool expect_report_sent) {
+    if (expect_report_sent) {
+      MockThreatDetails* threat_details = static_cast<MockThreatDetails*>(
+          trigger_manager_.data_collectors_map_[web_contents].get());
+      EXPECT_CALL(*threat_details, FinishCollection(_, _)).Times(1);
+    }
+    SBErrorOptions options =
+        TriggerManager::GetSBErrorDisplayOptions(pref_service_, *web_contents);
     return trigger_manager_.FinishCollectingThreatDetails(
-        web_contents, base::TimeDelta(), false, 0);
+        web_contents, base::TimeDelta(), false, 0, options);
   }
 
   const DataCollectorsMap& data_collectors_map() {
@@ -77,6 +109,7 @@ class TriggerManagerTest : public ::testing::Test {
   content::TestBrowserThreadBundle thread_bundle_;
   content::TestBrowserContext browser_context_;
   content::TestWebContentsFactory web_contents_factory_;
+  TestingPrefServiceSimple pref_service_;
 
   DISALLOW_COPY_AND_ASSIGN(TriggerManagerTest);
 };
@@ -87,7 +120,7 @@ TEST_F(TriggerManagerTest, StartAndFinishCollectingThreatDetails) {
   content::WebContents* web_contents1 = CreateWebContents();
   EXPECT_TRUE(StartCollectingThreatDetails(web_contents1));
   EXPECT_THAT(data_collectors_map(), UnorderedElementsAre(Key(web_contents1)));
-  EXPECT_TRUE(FinishCollectingThreatDetails(web_contents1));
+  EXPECT_TRUE(FinishCollectingThreatDetails(web_contents1, true));
   EXPECT_TRUE(data_collectors_map().empty());
 
   // More complex scenarios can happen, where collection happens on two
@@ -98,9 +131,9 @@ TEST_F(TriggerManagerTest, StartAndFinishCollectingThreatDetails) {
   EXPECT_TRUE(StartCollectingThreatDetails(web_contents2));
   EXPECT_THAT(data_collectors_map(),
               UnorderedElementsAre(Key(web_contents1), Key(web_contents2)));
-  EXPECT_TRUE(FinishCollectingThreatDetails(web_contents2));
+  EXPECT_TRUE(FinishCollectingThreatDetails(web_contents2, true));
   EXPECT_THAT(data_collectors_map(), UnorderedElementsAre(Key(web_contents1)));
-  EXPECT_TRUE(FinishCollectingThreatDetails(web_contents1));
+  EXPECT_TRUE(FinishCollectingThreatDetails(web_contents1, true));
   EXPECT_TRUE(data_collectors_map().empty());
 
   // Calling Start twice with the same WebContents is an error, and will return
@@ -109,17 +142,102 @@ TEST_F(TriggerManagerTest, StartAndFinishCollectingThreatDetails) {
   EXPECT_THAT(data_collectors_map(), UnorderedElementsAre(Key(web_contents1)));
   EXPECT_FALSE(StartCollectingThreatDetails(web_contents1));
   EXPECT_THAT(data_collectors_map(), UnorderedElementsAre(Key(web_contents1)));
-  EXPECT_TRUE(FinishCollectingThreatDetails(web_contents1));
+  EXPECT_TRUE(FinishCollectingThreatDetails(web_contents1, true));
   EXPECT_TRUE(data_collectors_map().empty());
 
   // Calling Finish twice with the same WebContents is an error, and will return
   // false the second time. It's basically a no-op.
   EXPECT_TRUE(StartCollectingThreatDetails(web_contents1));
   EXPECT_THAT(data_collectors_map(), UnorderedElementsAre(Key(web_contents1)));
-  EXPECT_TRUE(FinishCollectingThreatDetails(web_contents1));
+  EXPECT_TRUE(FinishCollectingThreatDetails(web_contents1, true));
+  EXPECT_TRUE(data_collectors_map().empty());
+  EXPECT_FALSE(FinishCollectingThreatDetails(web_contents1, false));
+  EXPECT_TRUE(data_collectors_map().empty());
+}
+
+TEST_F(TriggerManagerTest, NoDataCollection_Incognito) {
+  // Data collection will not begin and no reports will be sent when incognito.
+  content::WebContents* web_contents = CreateIncognitoWebContents();
+  EXPECT_FALSE(StartCollectingThreatDetails(web_contents));
+  EXPECT_TRUE(data_collectors_map().empty());
+  EXPECT_FALSE(FinishCollectingThreatDetails(web_contents, false));
+  EXPECT_TRUE(data_collectors_map().empty());
+}
+
+TEST_F(TriggerManagerTest, NoDataCollection_SBEROptInDisallowed) {
+  // Data collection will not begin and no reports will be sent when the user is
+  // not allowed to opt-in to SBER.
+  SetPref(prefs::kSafeBrowsingExtendedReportingOptInAllowed, false);
+  content::WebContents* web_contents = CreateWebContents();
+  EXPECT_FALSE(StartCollectingThreatDetails(web_contents));
+  EXPECT_TRUE(data_collectors_map().empty());
+  EXPECT_FALSE(FinishCollectingThreatDetails(web_contents, false));
+  EXPECT_TRUE(data_collectors_map().empty());
+}
+
+TEST_F(TriggerManagerTest, NoDataCollection_IncognitoAndSBEROptInDisallowed) {
+  // Data collection will not begin and no reports will be sent when the user is
+  // not allowed to opt-in to SBER and is also incognito.
+  SetPref(prefs::kSafeBrowsingExtendedReportingOptInAllowed, false);
+  content::WebContents* web_contents = CreateIncognitoWebContents();
+  EXPECT_FALSE(StartCollectingThreatDetails(web_contents));
+  EXPECT_TRUE(data_collectors_map().empty());
+  EXPECT_FALSE(FinishCollectingThreatDetails(web_contents, false));
+  EXPECT_TRUE(data_collectors_map().empty());
+}
+
+TEST_F(TriggerManagerTest, UserOptedOutOfSBER_DataCollected_NoReportSent) {
+  // When the user is opted-out of SBER then data collection will begin but no
+  // report will be sent when data collection ends.
+  SetPref(prefs::kSafeBrowsingScoutReportingEnabled, false);
+  content::WebContents* web_contents = CreateWebContents();
+  EXPECT_TRUE(StartCollectingThreatDetails(web_contents));
+  EXPECT_THAT(data_collectors_map(), UnorderedElementsAre(Key(web_contents)));
+  EXPECT_FALSE(FinishCollectingThreatDetails(web_contents, false));
+  EXPECT_TRUE(data_collectors_map().empty());
+}
+
+TEST_F(TriggerManagerTest, UserOptsOutOfSBER_DataCollected_NoReportSent) {
+  // If the user opts-out of Extended Reporting while data is being collected
+  // then no report is sent. Note that the test fixture opts the user into
+  // Extended Reporting by default.
+  content::WebContents* web_contents = CreateWebContents();
+  EXPECT_TRUE(StartCollectingThreatDetails(web_contents));
+  EXPECT_THAT(data_collectors_map(), UnorderedElementsAre(Key(web_contents)));
+
+  SetPref(prefs::kSafeBrowsingScoutReportingEnabled, false);
+
+  EXPECT_FALSE(FinishCollectingThreatDetails(web_contents, false));
   EXPECT_TRUE(data_collectors_map().empty());
-  EXPECT_FALSE(FinishCollectingThreatDetails(web_contents1));
+}
+
+TEST_F(TriggerManagerTest, UserOptsInToSBER_DataCollected_ReportSent) {
+  // When the user is opted-out of SBER then data collection will begin. If they
+  // opt-in to SBER while data collection is in progress then the report will
+  // also be sent.
+  SetPref(prefs::kSafeBrowsingScoutReportingEnabled, false);
+  content::WebContents* web_contents = CreateWebContents();
+  EXPECT_TRUE(StartCollectingThreatDetails(web_contents));
+  EXPECT_THAT(data_collectors_map(), UnorderedElementsAre(Key(web_contents)));
+
+  SetPref(prefs::kSafeBrowsingScoutReportingEnabled, true);
+
+  EXPECT_TRUE(FinishCollectingThreatDetails(web_contents, true));
   EXPECT_TRUE(data_collectors_map().empty());
 }
 
+TEST_F(TriggerManagerTest,
+       SBEROptInBecomesDisallowed_DataCollected_NoReportSent) {
+  // If the user loses the ability to opt-in to SBER in the middle of data
+  // collection then the report will not be sent.
+  content::WebContents* web_contents = CreateWebContents();
+  EXPECT_TRUE(StartCollectingThreatDetails(web_contents));
+  EXPECT_THAT(data_collectors_map(), UnorderedElementsAre(Key(web_contents)));
+
+  // Remove the ability to opt-in to SBER.
+  SetPref(prefs::kSafeBrowsingExtendedReportingOptInAllowed, false);
+
+  EXPECT_FALSE(FinishCollectingThreatDetails(web_contents, false));
+  EXPECT_TRUE(data_collectors_map().empty());
+}
 }  // namespace safe_browsing

components/security_interstitials/core/base_safe_browsing_error_ui.h

@@ -99,6 +99,10 @@ class BaseSafeBrowsingErrorUI {
     return display_options_.help_center_article_link;
   }
 
+  const SBErrorDisplayOptions& get_error_display_options() const {
+    return display_options_;
+  }
+
   // Checks if we should even show the extended reporting option. We don't show
   // it in incognito mode or if kSafeBrowsingExtendedReportingOptInAllowed
   // preference is disabled.