Install Breakpad when consent is given.

Previously, on ChromeOS, Breakpad would only be installed if
(a) the device owner had already consented to stats collection at the
time the browser process started up or
(b) the user consented to stats collection in the OOBE flow.

Extend this to install Breakpad if the user consents to metrics
collection in the settings menu. Or if a tast test changes consent via
the autotest private functions.

Consistent with previous behavior, Breakpad is not uninstalled if the
user revokes consent. Rather, we rely on crash_reporter to discard the
report in that case.

This change only affects Breakpad. Crashpad is always installed and
relies on crash_reporter discarding reports if crash collection consent
is not given.

This only affects ChromeOS.

This change isn't perfect. It only installs Breakpad in the main browser
process when the user gives consent in the setting screen. Trying to
install Breakpad in all running processes seems like it would be very
complex, and Breakpad is being deprecated in favor of Crashpad soon, so
I'm just fixing the main browser process here.

BUGS=chromium:1058261, chromium:1058258

TEST=Disabled consent in OOBE, logged in, enabled consent on settings
screen, crashed browser and saw crash files written out.
TEST=Enabled consent in OOBE, crashed browser before logging in, ensured
crash files written out
TEST=tast ... ui.ChromeCrashLoggedIn.browser_breakpad

Fixed: chromium:1058261, chromium:1058258
Change-Id: I8d2ac65b690790f0396d8a67ce9a0c3af114d69c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2145676
Commit-Queue: Ian Barkley-Yeung <iby@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#759078}

chrome/browser/chromeos/BUILD.gn

@@ -2311,6 +2311,8 @@ source_set("chromeos") {
     "system/automatic_reboot_manager.cc",
     "system/automatic_reboot_manager.h",
     "system/automatic_reboot_manager_observer.h",
+    "system/breakpad_consent_watcher.cc",
+    "system/breakpad_consent_watcher.h",
     "system/device_disabling_manager.cc",
     "system/device_disabling_manager.h",
     "system/device_disabling_manager_default_delegate.cc",

chrome/browser/chromeos/chrome_browser_main_chromeos.cc

@@ -111,6 +111,7 @@
 #include "chrome/browser/chromeos/settings/device_settings_service.h"
 #include "chrome/browser/chromeos/settings/shutdown_policy_forwarder.h"
 #include "chrome/browser/chromeos/startup_settings_cache.h"
+#include "chrome/browser/chromeos/system/breakpad_consent_watcher.h"
 #include "chrome/browser/chromeos/system/input_device_settings.h"
 #include "chrome/browser/chromeos/system/user_removal_manager.h"
 #include "chrome/browser/chromeos/system_token_cert_db_initializer.h"
@@ -591,6 +592,19 @@ void ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun() {
   bulk_printers_calculator_factory_ =
       std::make_unique<BulkPrintersCalculatorFactory>();
 
+  // StatsReportingController is created in
+  // ChromeBrowserMainParts::PreCreateThreads, so this must come afterwards.
+  chromeos::StatsReportingController* stats_controller =
+      chromeos::StatsReportingController::Get();
+  // |stats_controller| can be nullptr if ChromeBrowserMainParts's
+  // browser_process_->GetApplicationLocale() returns empty. We're trying to
+  // show an error message in that case, so don't just crash. (See
+  // ChromeBrowserMainParts::PreCreateThreadsImpl()).
+  if (stats_controller != nullptr) {
+    breakpad_consent_watcher_ =
+        system::BreakpadConsentWatcher::Initialize(stats_controller);
+  }
+
   ChromeBrowserMainPartsLinux::PreMainMessageLoopRun();
 }
 

chrome/browser/chromeos/chrome_browser_main_chromeos.h

@@ -84,6 +84,7 @@ class Controller;
 
 namespace system {
 class DarkResumeController;
+class BreakpadConsentWatcher;
 }  // namespace system
 
 // ChromeBrowserMainParts implementation for chromeos specific code.
@@ -194,6 +195,7 @@ class ChromeBrowserMainPartsChromeos : public ChromeBrowserMainPartsLinux {
       login_screen_extensions_storage_cleaner_;
 
   std::unique_ptr<GnubbyNotification> gnubby_notification_;
+  std::unique_ptr<system::BreakpadConsentWatcher> breakpad_consent_watcher_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromeBrowserMainPartsChromeos);
 };

chrome/browser/chromeos/login/wizard_controller.cc

@@ -1231,17 +1231,6 @@ void WizardController::OnResetScreenExit() {
 void WizardController::OnChangedMetricsReportingState(bool enabled) {
   StatsReportingController::Get()->SetEnabled(
       ProfileManager::GetActiveUserProfile(), enabled);
-  if (crash_reporter::IsCrashpadEnabled()) {
-    return;
-  }
-
-  if (!enabled)
-    return;
-#if BUILDFLAG(GOOGLE_CHROME_BRANDING)
-  base::ThreadPool::PostTask(
-      FROM_HERE, {base::MayBlock()},
-      base::BindOnce(&breakpad::InitCrashReporter, std::string()));
-#endif
 }
 
 void WizardController::OnDeviceModificationCanceled() {

chrome/browser/chromeos/system/breakpad_consent_watcher.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 "chrome/browser/chromeos/system/breakpad_consent_watcher.h"
+
+#include <string>
+
+#include "base/bind.h"
+#include "base/logging.h"
+#include "base/memory/ptr_util.h"
+#include "chrome/installer/util/google_update_settings.h"
+#include "components/crash/core/app/breakpad_linux.h"
+#include "components/crash/core/app/crashpad.h"
+
+namespace chromeos {
+namespace system {
+
+BreakpadConsentWatcher::BreakpadConsentWatcher() = default;
+BreakpadConsentWatcher::~BreakpadConsentWatcher() = default;
+
+std::unique_ptr<BreakpadConsentWatcher> BreakpadConsentWatcher::Initialize(
+    StatsReportingController* stat_controller) {
+  DCHECK(stat_controller != nullptr);
+  if (crash_reporter::IsCrashpadEnabled()) {
+    // Crashpad is always installed, regardless of consent. (crash_reporter is
+    // responsible for discarding the reports if consent is off.) Therefore, we
+    // do not need to watch the consent setting.
+    return nullptr;
+  }
+
+  if (breakpad::IsCrashReporterEnabled()) {
+    // Already enabled, no need to enable it again. (If consent is revoked,
+    // crash_reporter will discard any resulting crashes.)
+    return nullptr;
+  }
+
+  auto watcher = base::WrapUnique(new BreakpadConsentWatcher);
+  watcher->subscription_ = stat_controller->AddObserver(
+      base::BindRepeating(BreakpadConsentWatcher::OnConsentChange));
+
+  return watcher;
+}
+
+void BreakpadConsentWatcher::OnConsentChange() {
+  GoogleUpdateSettings::CollectStatsConsentTaskRunner()->PostTask(
+      FROM_HERE,
+      base::BindOnce(
+          &BreakpadConsentWatcher::OnConsentChangeCollectStatsConsentThread));
+}
+
+void BreakpadConsentWatcher::OnConsentChangeCollectStatsConsentThread() {
+  if (breakpad::IsCrashReporterEnabled()) {
+    // No need to enable breakpad twice.
+    return;
+  }
+
+  // Breakpad will check the consent setting in InitCrashReporter. No need to
+  // check it here.
+  breakpad::InitCrashReporter(std::string());
+}
+
+}  // namespace system
+}  // namespace chromeos

chrome/browser/chromeos/system/breakpad_consent_watcher.h

+// 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.
+
+#ifndef CHROME_BROWSER_CHROMEOS_SYSTEM_BREAKPAD_CONSENT_WATCHER_H_
+#define CHROME_BROWSER_CHROMEOS_SYSTEM_BREAKPAD_CONSENT_WATCHER_H_
+
+#include <memory>
+
+#include "chrome/browser/chromeos/settings/stats_reporting_controller.h"
+
+namespace chromeos {
+namespace system {
+
+// Watches the crash reporting consent setting (cros.metrics.reportingEnabled).
+// If crash reporting is enabled, installs Breakpad if it isn't already
+// installed.
+//
+// This class is only created in the main browser process. Unfortunately, that
+// means changing the consent setting only installs Breakpad in the browser
+// process. Renderer and GPU processes started before consent was given will
+// still discard any crashes. This problem will be fixed when we move to
+// Crashpad.
+class BreakpadConsentWatcher {
+ public:
+  ~BreakpadConsentWatcher();
+
+  // Create a BreakpadConsentWatcher. Returns nullptr if this process doesn't
+  // need a BreakpadConsentWatcher. Returning nullptr is NOT indicitive of an
+  // error.
+  static std::unique_ptr<BreakpadConsentWatcher> Initialize(
+      StatsReportingController* stat_controller);
+
+ private:
+  BreakpadConsentWatcher();
+  BreakpadConsentWatcher(const BreakpadConsentWatcher&) = delete;
+  BreakpadConsentWatcher& operator=(const BreakpadConsentWatcher&) = delete;
+
+  // Callback function. Called whenever the crash reporting consent is changed.
+  static void OnConsentChange();
+
+  // Callback function that happens on the
+  // GoogleUpdateSettings::CollectStatsConsentTaskRunner() thread. We must be
+  // on that thread to avoid races. Also, InitCrashReporter() calls blocking
+  // functions so we must be on a blockable thread.
+  // Called whenever the crash reporting consent is changed.
+  static void OnConsentChangeCollectStatsConsentThread();
+
+  std::unique_ptr<StatsReportingController::ObserverSubscription> subscription_;
+};
+
+}  // namespace system
+}  // namespace chromeos
+
+#endif  // CHROME_BROWSER_CHROMEOS_SYSTEM_BREAKPAD_CONSENT_WATCHER_H_