Cleanup CBCM cloud reporting earlier during browser shutdown

CloudReporting observers ProfileManager and few KeyedServices. It needs
to be turned down before Profile system so that these observers can be
removed properly.

Adding a browser test to verify the issue which waits until cloud reporting
fully launched and shutdown.

Bug: 1047619, 1047886
Change-Id: I28620dd0f8265bb10c4139eb24bad24dcaf94ea0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2033435Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Commit-Queue: Owen Min <zmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#738250}

chrome/browser/browser_process_impl.cc

@@ -186,6 +186,7 @@
 #if !defined(OS_ANDROID) && !defined(OS_CHROMEOS)
 #include "chrome/browser/first_run/upgrade_util.h"
 #include "chrome/browser/notifications/notification_ui_manager.h"
+#include "chrome/browser/policy/chrome_browser_cloud_management_controller.h"
 #include "chrome/browser/ui/user_manager.h"
 #endif
 
@@ -362,6 +363,17 @@ void BrowserProcessImpl::StartTearDown() {
   plugins_resource_service_.reset();
 #endif
 
+#if !defined(OS_ANDROID) && !defined(OS_CHROMEOS)
+  // Initial cleanup for ChromeBrowserCloudManagement, shutdown components that
+  // depend on profile and notification system. For example, ProfileManager
+  // observer and KeyServices observer need to be removed before profiles.
+  if (browser_policy_connector_ &&
+      browser_policy_connector_->chrome_browser_cloud_management_controller()) {
+    browser_policy_connector_->chrome_browser_cloud_management_controller()
+        ->ShutDown();
+  }
+#endif
+
   system_notification_helper_.reset();
 
 #if !defined(OS_CHROMEOS)

chrome/browser/enterprise_reporting/report_scheduler.cc

@@ -51,6 +51,9 @@ ReportScheduler::ReportScheduler(
 }
 
 ReportScheduler::~ReportScheduler() {
+  // Stop observing ProfileManager if we are tracking stale profiles.
+  if (stale_profiles_)
+    g_browser_process->profile_manager()->RemoveObserver(this);
   if (IsReportingEnabled() && stale_profiles_) {
     base::UmaHistogramExactLinear("Enterprise.CloudReportingStaleProfileCount",
                                   stale_profiles_->size(),

chrome/browser/enterprise_reporting/report_scheduler_browsertest.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 "base/command_line.h"
+#include "base/run_loop.h"
+#include "base/test/scoped_feature_list.h"
+#include "build/branding_buildflags.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/chrome_browser_main.h"
+#include "chrome/browser/chrome_browser_main_extra_parts.h"
+#include "chrome/browser/policy/browser_dm_token_storage.h"
+#include "chrome/browser/policy/chrome_browser_cloud_management_controller.h"
+#include "chrome/browser/policy/chrome_browser_policy_connector.h"
+#include "chrome/browser/policy/fake_browser_dm_token_storage.h"
+#include "chrome/common/chrome_features.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "components/prefs/pref_service.h"
+
+namespace enterprise_reporting {
+
+class ChromeBrowserExtraSetUp : public ChromeBrowserMainExtraParts {
+ public:
+  explicit ChromeBrowserExtraSetUp(
+      policy::ChromeBrowserCloudManagementController::Observer* observer)
+      : observer_(observer) {}
+  void PreMainMessageLoopStart() override {
+    g_browser_process->browser_policy_connector()
+        ->chrome_browser_cloud_management_controller()
+        ->AddObserver(observer_);
+  }
+
+ private:
+  policy::ChromeBrowserCloudManagementController::Observer* observer_;
+};
+
+class ReportSchedulerTest
+    : public InProcessBrowserTest,
+      public policy::ChromeBrowserCloudManagementController::Observer {
+ public:
+  ReportSchedulerTest() {
+    policy::BrowserDMTokenStorage::SetForTesting(&storage_);
+
+    scoped_feature_list_.InitAndEnableFeature(
+        features::kEnterpriseReportingInBrowser);
+  }
+  ~ReportSchedulerTest() override = default;
+
+  void SetUpOnMainThread() override {
+    g_browser_process->local_state()->SetBoolean(prefs::kCloudReportingEnabled,
+                                                 true);
+  }
+
+  void CreatedBrowserMainParts(content::BrowserMainParts* parts) override {
+    static_cast<ChromeBrowserMainParts*>(parts)->AddParts(
+        new ChromeBrowserExtraSetUp(this));
+  }
+
+  void TearDownOnMainThread() override {
+    g_browser_process->browser_policy_connector()
+        ->chrome_browser_cloud_management_controller()
+        ->RemoveObserver(this);
+  }
+
+  // policy::ChromeBrowserCloudManagementController::Observer
+  void OnCloudReportingLaunched() override {
+    has_cloud_reporting_launched = true;
+    if (run_loop_)
+      run_loop_->Quit();
+  }
+
+  void WaitForCloudReportingLaunching() {
+    if (has_cloud_reporting_launched)
+      return;
+    run_loop_ = std::make_unique<base::RunLoop>();
+    run_loop_->Run();
+  }
+
+#if !BUILDFLAG(GOOGLE_CHROME_BRANDING)
+  void SetUpDefaultCommandLine(base::CommandLine* command_line) override {
+    InProcessBrowserTest::SetUpDefaultCommandLine(command_line);
+    command_line->AppendSwitch(::switches::kEnableChromeBrowserCloudManagement);
+  }
+#endif
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+  policy::FakeBrowserDMTokenStorage storage_;
+  std::unique_ptr<base::RunLoop> run_loop_;
+  bool has_cloud_reporting_launched = false;
+};
+
+// Verify that the cloud reporting can be launched and shutdown with the browser
+// without crash.
+IN_PROC_BROWSER_TEST_F(ReportSchedulerTest, LaunchTest) {
+  WaitForCloudReportingLaunching();
+}
+
+}  // namespace enterprise_reporting

chrome/browser/policy/chrome_browser_cloud_management_controller.cc

@@ -343,6 +343,11 @@ void ChromeBrowserCloudManagementController::OnClientError(
     UnenrollBrowser();
 }
 
+void ChromeBrowserCloudManagementController::ShutDown() {
+  if (report_scheduler_)
+    report_scheduler_.reset();
+}
+
 void ChromeBrowserCloudManagementController::NotifyPolicyRegisterFinished(
     bool succeeded) {
   for (auto& observer : observers_) {
@@ -356,6 +361,12 @@ void ChromeBrowserCloudManagementController::NotifyBrowserUnenrolled(
     observer.OnBrowserUnenrolled(succeeded);
 }
 
+void ChromeBrowserCloudManagementController::NotifyCloudReportingLaunched() {
+  for (auto& observer : observers_) {
+    observer.OnCloudReportingLaunched();
+  }
+}
+
 bool ChromeBrowserCloudManagementController::GetEnrollmentTokenAndClientId(
     std::string* enrollment_token,
     std::string* client_id) {
@@ -447,6 +458,8 @@ void ChromeBrowserCloudManagementController::CreateReportScheduler() {
   auto generator = std::make_unique<enterprise_reporting::ReportGenerator>();
   report_scheduler_ = std::make_unique<enterprise_reporting::ReportScheduler>(
       cloud_policy_client_.get(), std::move(generator));
+
+  NotifyCloudReportingLaunched();
 }
 
 }  // namespace policy

chrome/browser/policy/chrome_browser_cloud_management_controller.h

@@ -66,6 +66,9 @@ class ChromeBrowserCloudManagementController
 
     // Called when the browser has been unenrolled.
     virtual void OnBrowserUnenrolled(bool succeeded) {}
+
+    // Called when the cloud reporting is launched.
+    virtual void OnCloudReportingLaunched() {}
   };
 
   // Directory name under the user-data-dir where the policy data is stored.
@@ -100,9 +103,13 @@ class ChromeBrowserCloudManagementController
   void OnRegistrationStateChanged(CloudPolicyClient* client) override;
   void OnClientError(CloudPolicyClient* client) override;
 
+  // Early cleanup during browser shutdown process
+  void ShutDown();
+
  protected:
   void NotifyPolicyRegisterFinished(bool succeeded);
   void NotifyBrowserUnenrolled(bool succeeded);
+  void NotifyCloudReportingLaunched();
 
  private:
   bool GetEnrollmentTokenAndClientId(std::string* enrollment_token,

chrome/test/BUILD.gn

@@ -922,6 +922,7 @@ if (!is_android) {
       "../browser/download/download_frame_policy_browsertest.cc",
       "../browser/download/download_started_animation_browsertest.cc",
       "../browser/download/save_page_browsertest.cc",
+      "../browser/enterprise_reporting/report_scheduler_browsertest.cc",
       "../browser/fast_shutdown_browsertest.cc",
       "../browser/favicon/content_favicon_driver_browsertest.cc",
       "../browser/first_run/first_run_browsertest.cc",
@@ -2518,6 +2519,7 @@ if (!is_android) {
         "../browser/ui/webui/signin/user_manager_ui_browsertest.cc",
 
         # chromeos does not support machine level user cloud policies
+        "../browser/enterprise_reporting/report_scheduler_browsertest.cc",
         "../browser/policy/cloud/chrome_browser_cloud_management_browsertest.cc",
 
         # TODO(1026473): Re-enable these when linux-chromeos-rel breakage is