Generate and upload enterprise reports following a browser update.

- ReportGenerator can now optionally generate a report lacking full
  profile info.
- BrowserReportGenerator and ReportGenerator are now stateless.
- ReportScheduler now observes BuildState and generates/updates a
  minimal report for desktop Chrome when an update is available. If a
  trigger to generate a report arrives while a report is in-flight, its
  handling is deferred until the previous completes.

BUG=1043624
R=zmin@chromium.org

Change-Id: I7864ee467bd681869ba0a57015c718cd811dc4e6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2036072
Commit-Queue: Greg Thompson <grt@chromium.org>
Reviewed-by: Owen Min <zmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746415}

chrome/browser/enterprise_reporting/browser_report_generator.cc

@@ -10,6 +10,7 @@
 #include "base/files/file_path.h"
 #include "base/path_service.h"
 #include "base/strings/utf_string_conversions.h"
+#include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile_attributes_entry.h"
 #include "chrome/browser/profiles/profile_attributes_storage.h"
@@ -45,18 +46,16 @@ BrowserReportGenerator::BrowserReportGenerator() = default;
 BrowserReportGenerator::~BrowserReportGenerator() = default;
 
 void BrowserReportGenerator::Generate(ReportCallback callback) {
-  DCHECK(!callback_);
-  callback_ = std::move(callback);
-
   auto report = std::make_unique<em::BrowserReport>();
   GenerateBasicInfos(report.get());
   GenerateProfileInfos(report.get());
 
   // std::move is required here because the function completes the report
   // asynchronously.
-  GeneratePluginsIfNeeded(std::move(report));
+  GeneratePluginsIfNeeded(std::move(callback), std::move(report));
 }
 
+// static
 void BrowserReportGenerator::GenerateBasicInfos(em::BrowserReport* report) {
 #if !defined(OS_CHROMEOS)
   report->set_browser_version(version_info::GetVersionNumber());
@@ -66,6 +65,7 @@ void BrowserReportGenerator::GenerateBasicInfos(em::BrowserReport* report) {
   report->set_executable_path(GetExecutablePath());
 }
 
+// static
 void BrowserReportGenerator::GenerateProfileInfos(em::BrowserReport* report) {
   for (auto* entry : g_browser_process->profile_manager()
                          ->GetProfileAttributesStorage()
@@ -86,18 +86,20 @@ void BrowserReportGenerator::GenerateProfileInfos(em::BrowserReport* report) {
 }
 
 void BrowserReportGenerator::GeneratePluginsIfNeeded(
+    ReportCallback callback,
     std::unique_ptr<em::BrowserReport> report) {
 #if defined(OS_CHROMEOS) || !BUILDFLAG(ENABLE_PLUGINS)
-  std::move(callback_).Run(std::move(report));
+  std::move(callback).Run(std::move(report));
 #else
-  content::PluginService::GetInstance()->GetPlugins(
-      base::BindOnce(&BrowserReportGenerator::OnPluginsReady,
-                     weak_ptr_factory_.GetWeakPtr(), std::move(report)));
+  content::PluginService::GetInstance()->GetPlugins(base::BindOnce(
+      &BrowserReportGenerator::OnPluginsReady, weak_ptr_factory_.GetWeakPtr(),
+      std::move(callback), std::move(report)));
 #endif
 }
 
 #if BUILDFLAG(ENABLE_PLUGINS)
 void BrowserReportGenerator::OnPluginsReady(
+    ReportCallback callback,
     std::unique_ptr<em::BrowserReport> report,
     const std::vector<content::WebPluginInfo>& plugins) {
   for (content::WebPluginInfo plugin : plugins) {
@@ -108,7 +110,7 @@ void BrowserReportGenerator::OnPluginsReady(
     plugin_info->set_description(base::UTF16ToUTF8(plugin.desc));
   }
 
-  std::move(callback_).Run(std::move(report));
+  std::move(callback).Run(std::move(report));
 }
 #endif  // BUILDFLAG(ENABLE_PLUGINS)
 

chrome/browser/enterprise_reporting/browser_report_generator.h

@@ -21,9 +21,7 @@ struct WebPluginInfo;
 
 namespace enterprise_reporting {
 
-/**
- * A report generator that collects Browser related information.
- */
+// A report generator that collects Browser related information.
 class BrowserReportGenerator {
  public:
   using ReportCallback =
@@ -32,31 +30,34 @@ class BrowserReportGenerator {
   BrowserReportGenerator();
   ~BrowserReportGenerator();
 
-  // Generate a BrowserReport with following fields.
+  // Generates a BrowserReport with the following fields:
   // - browser_version, channel, executable_path
   // - user profiles: id, name, is_full_report (always be false).
   // - plugins: name, version, filename, description.
   void Generate(ReportCallback callback);
 
  private:
-  // Generate browser_version, channel, executable_path info in the given
+  // Generates browser_version, channel, executable_path info in the given
   // report instance.
-  void GenerateBasicInfos(em::BrowserReport* report);
+  static void GenerateBasicInfos(em::BrowserReport* report);
 
-  // Generate user profiles info in the given report instance.
-  void GenerateProfileInfos(em::BrowserReport* report);
+  // Generates user profiles info in the given report instance.
+  static void GenerateProfileInfos(em::BrowserReport* report);
 
-  // Generate plugin info in the given report instance, if needed. It requires
-  // the ownership of report instance to pass into ReportCallback method.
-  void GeneratePluginsIfNeeded(std::unique_ptr<em::BrowserReport> report);
+  // Generates plugin info in the given report instance, if needed. Passes
+  // |report| to |callback| either asynchronously when the plugin info is
+  // available, or synchronously otherwise.
+  void GeneratePluginsIfNeeded(ReportCallback callback,
+                               std::unique_ptr<em::BrowserReport> report);
 
 #if BUILDFLAG(ENABLE_PLUGINS)
-  void OnPluginsReady(std::unique_ptr<em::BrowserReport> report,
+  // Populates |report| with the plugin info in |plugins|, then passes the
+  // report to |callback|.
+  void OnPluginsReady(ReportCallback callback,
+                      std::unique_ptr<em::BrowserReport> report,
                       const std::vector<content::WebPluginInfo>& plugins);
 #endif
 
-  ReportCallback callback_;
-
   base::WeakPtrFactory<BrowserReportGenerator> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(BrowserReportGenerator);

chrome/browser/enterprise_reporting/report_generator.cc

@@ -7,7 +7,7 @@
 #include <utility>
 
 #include "base/bind.h"
-#include "base/files/file_path.h"
+#include "base/logging.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
@@ -33,28 +33,31 @@ ReportGenerator::ReportGenerator() = default;
 
 ReportGenerator::~ReportGenerator() = default;
 
-void ReportGenerator::Generate(ReportCallback callback) {
-  DCHECK(!callback_);
-  callback_ = std::move(callback);
-  CreateBasicRequest();
+void ReportGenerator::Generate(bool with_profiles, ReportCallback callback) {
+  CreateBasicRequest(std::make_unique<ReportRequest>(), with_profiles,
+                     std::move(callback));
 }
 
 void ReportGenerator::SetMaximumReportSizeForTesting(size_t size) {
   report_request_queue_generator_.SetMaximumReportSizeForTesting(size);
 }
 
-void ReportGenerator::CreateBasicRequest() {
+void ReportGenerator::CreateBasicRequest(
+    std::unique_ptr<ReportRequest> basic_request,
+    bool with_profiles,
+    ReportCallback callback) {
 #if defined(OS_CHROMEOS)
-  SetAndroidAppInfos();
+  SetAndroidAppInfos(basic_request.get());
 #else
-  basic_request_.set_computer_name(this->GetMachineName());
-  basic_request_.set_os_user_name(GetOSUserName());
-  basic_request_.set_serial_number(GetSerialNumber());
-  basic_request_.set_allocated_os_report(GetOSReport().release());
+  basic_request->set_computer_name(this->GetMachineName());
+  basic_request->set_os_user_name(GetOSUserName());
+  basic_request->set_serial_number(GetSerialNumber());
+  basic_request->set_allocated_os_report(GetOSReport().release());
 #endif
 
   browser_report_generator_.Generate(base::BindOnce(
-      &ReportGenerator::OnBrowserReportReady, weak_ptr_factory_.GetWeakPtr()));
+      &ReportGenerator::OnBrowserReportReady, weak_ptr_factory_.GetWeakPtr(),
+      with_profiles, std::move(callback), std::move(basic_request)));
 }
 
 std::unique_ptr<em::OSReport> ReportGenerator::GetOSReport() {
@@ -84,8 +87,9 @@ std::string ReportGenerator::GetSerialNumber() {
 
 #if defined(OS_CHROMEOS)
 
-void ReportGenerator::SetAndroidAppInfos() {
-  basic_request_.clear_android_app_infos();
+void ReportGenerator::SetAndroidAppInfos(ReportRequest* basic_request) {
+  DCHECK(basic_request);
+  basic_request->clear_android_app_infos();
 
   // Android application is only supported for primary profile.
   Profile* primary_profile =
@@ -106,7 +110,7 @@ void ReportGenerator::SetAndroidAppInfos() {
 
   AndroidAppInfoGenerator generator;
   for (std::string app_id : prefs->GetAppIds()) {
-    em::AndroidAppInfo* app_info = basic_request_.add_android_app_infos();
+    em::AndroidAppInfo* app_info = basic_request->add_android_app_infos();
     generator.Generate(prefs, app_id)->Swap(app_info);
   }
 }
@@ -114,11 +118,24 @@ void ReportGenerator::SetAndroidAppInfos() {
 #endif
 
 void ReportGenerator::OnBrowserReportReady(
+    bool with_profiles,
+    ReportCallback callback,
+    std::unique_ptr<ReportRequest> basic_request,
     std::unique_ptr<em::BrowserReport> browser_report) {
-  basic_request_.set_allocated_browser_report(browser_report.release());
-  ReportRequests requests =
-      report_request_queue_generator_.Generate(basic_request_);
-  std::move(callback_).Run(std::move(requests));
+  basic_request->set_allocated_browser_report(browser_report.release());
+
+  if (with_profiles) {
+    // Generate a queue of requests containing detailed profile information.
+    std::move(callback).Run(
+        report_request_queue_generator_.Generate(*basic_request));
+    return;
+  }
+
+  // Return a queue containing only the basic request and browser report without
+  // detailed profile information.
+  ReportRequests requests;
+  requests.push(std::move(basic_request));
+  std::move(callback).Run(std::move(requests));
 }
 
 }  // namespace enterprise_reporting

chrome/browser/enterprise_reporting/report_generator.h

@@ -30,13 +30,19 @@ class ReportGenerator {
   ReportGenerator();
   virtual ~ReportGenerator();
 
-  virtual void Generate(ReportCallback callback);
+  // Asynchronously generates a queue of report requests, providing them to
+  // |callback| when ready. If |with_profiles| is true, full details are
+  // included for all loaded profiles; otherwise, only profile name and path
+  // are included.
+  virtual void Generate(bool with_profiles, ReportCallback callback);
 
   void SetMaximumReportSizeForTesting(size_t size);
 
  protected:
   // Creates a basic request that will be used by all Profiles.
-  void CreateBasicRequest();
+  void CreateBasicRequest(std::unique_ptr<ReportRequest> basic_request,
+                          bool with_profiles,
+                          ReportCallback callback);
 
   // Returns an OS report contains basic OS information includes OS name, OS
   // architecture and OS version.
@@ -55,17 +61,17 @@ class ReportGenerator {
 #if defined(OS_CHROMEOS)
   // Collect the Android application information installed on primary profile,
   // and set it to |basic_request_|.
-  virtual void SetAndroidAppInfos();
+  virtual void SetAndroidAppInfos(ReportRequest* basic_request);
 #endif
 
  private:
-  void OnBrowserReportReady(std::unique_ptr<em::BrowserReport> browser_report);
+  void OnBrowserReportReady(bool with_profiles,
+                            ReportCallback callback,
+                            std::unique_ptr<ReportRequest> basic_request,
+                            std::unique_ptr<em::BrowserReport> browser_report);
 
   ReportRequestQueueGenerator report_request_queue_generator_;
   BrowserReportGenerator browser_report_generator_;
-  ReportCallback callback_;
-  // Basic information that is shared among requests.
-  ReportRequest basic_request_;
 
   base::WeakPtrFactory<ReportGenerator> weak_ptr_factory_{this};
 

chrome/browser/enterprise_reporting/report_generator_unittest.cc

@@ -15,6 +15,7 @@
 #include "build/build_config.h"
 #include "chrome/browser/enterprise_reporting/report_request_definition.h"
 #include "chrome/browser/profiles/profile_attributes_storage.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile_manager.h"
 #include "components/account_id/account_id.h"
@@ -194,20 +195,24 @@ class ReportGeneratorTest : public ::testing::Test {
     plugin_service->RefreshPlugins();
   }
 
-  std::vector<std::unique_ptr<ReportRequest>> GenerateRequests() {
+  std::vector<std::unique_ptr<ReportRequest>> GenerateRequests(
+      bool with_profiles) {
     histogram_tester_ = std::make_unique<base::HistogramTester>();
     base::RunLoop run_loop;
     std::vector<std::unique_ptr<ReportRequest>> rets;
-    generator_.Generate(base::BindLambdaForTesting(
-        [&run_loop, &rets](ReportGenerator::ReportRequests requests) {
-          while (!requests.empty()) {
-            rets.push_back(std::move(requests.front()));
-            requests.pop();
-          }
-          run_loop.Quit();
-        }));
+    generator_.Generate(
+        with_profiles,
+        base::BindLambdaForTesting(
+            [&run_loop, &rets](ReportGenerator::ReportRequests requests) {
+              while (!requests.empty()) {
+                rets.push_back(std::move(requests.front()));
+                requests.pop();
+              }
+              run_loop.Quit();
+            }));
     run_loop.Run();
-    VerifyMetrics(rets);
+    if (with_profiles)
+      VerifyMetrics(rets);  // Only generated for reports with profiles.
     return rets;
   }
 
@@ -228,10 +233,7 @@ class ReportGeneratorTest : public ::testing::Test {
       std::string actual_profile_name = actual_profile_info.name();
 
       // Verify that the profile id is set as profile path.
-      EXPECT_EQ(profile_manager_.profiles_dir()
-                    .AppendASCII(actual_profile_name)
-                    .AsUTF8Unsafe(),
-                actual_profile_info.id());
+      EXPECT_EQ(GetProfilePath(actual_profile_name), actual_profile_info.id());
 
       EXPECT_TRUE(actual_profile_info.has_is_full_report());
 
@@ -258,6 +260,23 @@ class ReportGeneratorTest : public ::testing::Test {
   base::HistogramTester* histogram_tester() { return histogram_tester_.get(); }
 
  private:
+  // Returns a Profile's path, preferring the path of the active profile for
+  // user |profile_name| and falling back to a generated path based on it.
+  std::string GetProfilePath(const std::string& profile_name) {
+    // Find active Profile and return its path.
+    for (auto* profile :
+         profile_manager_.profile_manager()->GetLoadedProfiles()) {
+      if (profile->GetProfileUserName() == profile_name)
+        return profile->GetPath().AsUTF8Unsafe();
+    }
+
+    // No active profile, profile path must be generated by us, return path
+    // with same generator.
+    return profile_manager_.profiles_dir()
+        .AppendASCII(profile_name)
+        .AsUTF8Unsafe();
+  }
+
   ReportGenerator generator_;
 
   content::BrowserTaskEnvironment task_environment_;
@@ -271,7 +290,57 @@ TEST_F(ReportGeneratorTest, GenerateBasicReport) {
   auto profile_names = CreateProfiles(/*number*/ 2, kIdle);
   CreatePlugin();
 
-  auto requests = GenerateRequests();
+  auto requests = GenerateRequests(/*with_profiles=*/true);
+  EXPECT_EQ(1u, requests.size());
+
+  auto* basic_request = requests[0].get();
+
+  // In the ChromeOsUserReportRequest for Chrome OS, these fields are not
+  // existing. Therefore, they are skipped according to current environment.
+#if !defined(OS_CHROMEOS)
+  EXPECT_NE(std::string(), basic_request->computer_name());
+  EXPECT_NE(std::string(), basic_request->os_user_name());
+  VerifySerialNumber(basic_request->serial_number());
+
+  EXPECT_TRUE(basic_request->has_os_report());
+  auto& os_report = basic_request->os_report();
+  EXPECT_NE(std::string(), os_report.name());
+  EXPECT_NE(std::string(), os_report.arch());
+  EXPECT_NE(std::string(), os_report.version());
+#endif
+
+  EXPECT_TRUE(basic_request->has_browser_report());
+  auto& browser_report = basic_request->browser_report();
+#if defined(OS_CHROMEOS)
+  EXPECT_FALSE(browser_report.has_browser_version());
+  EXPECT_FALSE(browser_report.has_channel());
+#else
+  EXPECT_NE(std::string(), browser_report.browser_version());
+  EXPECT_TRUE(browser_report.has_channel());
+#endif
+  EXPECT_NE(std::string(), browser_report.executable_path());
+
+#if defined(OS_CHROMEOS)
+  EXPECT_EQ(0, browser_report.plugins_size());
+#else
+  // There might be other plugins like PDF plugin, however, our fake plugin
+  // should be the first one in the report.
+  EXPECT_LE(1, browser_report.plugins_size());
+  EXPECT_EQ(kPluginName, browser_report.plugins(0).name());
+  EXPECT_EQ(kPluginVersion, browser_report.plugins(0).version());
+  EXPECT_EQ(kPluginDescription, browser_report.plugins(0).description());
+  EXPECT_EQ(kPluginFileName, browser_report.plugins(0).filename());
+#endif
+
+  VerifyProfileReport(/*active_profile_names*/ std::set<std::string>(),
+                      profile_names, browser_report);
+}
+
+TEST_F(ReportGeneratorTest, GenerateWithoutProfiles) {
+  auto profile_names = CreateProfiles(/*number*/ 2, kActive);
+  CreatePlugin();
+
+  auto requests = GenerateRequests(/*with_profiles=*/false);
   EXPECT_EQ(1u, requests.size());
 
   auto* basic_request = requests[0].get();
@@ -334,7 +403,7 @@ TEST_F(ReportGeneratorTest, ReportArcAppInChromeOS) {
 
   // Verify the Arc application information in the report is same as the test
   // data.
-  auto requests = GenerateRequests();
+  auto requests = GenerateRequests(/*with_profiles=*/true);
   EXPECT_EQ(1u, requests.size());
 
   ReportRequest* request = requests.front().get();
@@ -346,7 +415,7 @@ TEST_F(ReportGeneratorTest, ReportArcAppInChromeOS) {
 
   // Generate the Arc application information again and make sure the report
   // remains the same.
-  requests = GenerateRequests();
+  requests = GenerateRequests(/*with_profiles=*/true);
   EXPECT_EQ(1u, requests.size());
 
   request = requests.front().get();
@@ -375,7 +444,7 @@ TEST_F(ReportGeneratorTest, ArcPlayStoreDisabled) {
   // No Arc application information is reported after the Arc Play Store
   // support for given profile is disabled.
   primary_profile.GetPrefs()->SetBoolean(arc::prefs::kArcEnabled, false);
-  auto requests = GenerateRequests();
+  auto requests = GenerateRequests(/*with_profiles=*/true);
   EXPECT_EQ(1u, requests.size());
 
   ReportRequest* request = requests.front().get();

chrome/browser/enterprise_reporting/report_scheduler.cc

@@ -18,6 +18,7 @@
 #include "chrome/browser/policy/browser_dm_token_storage.h"
 #include "chrome/browser/policy/chrome_browser_policy_connector.h"
 #include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/upgrade_detector/build_state.h"
 #include "chrome/common/pref_names.h"
 #include "components/policy/core/common/cloud/cloud_policy_client.h"
 #include "components/policy/core/common/cloud/device_management_service.h"
@@ -40,6 +41,16 @@ bool IsReportingEnabled() {
       prefs::kCloudReportingEnabled);
 }
 
+// Returns true if this build should generate basic reports when an update is
+// detected.
+constexpr bool ShouldReportUpdates() {
+#if defined(OS_CHROMEOS)
+  return false;
+#else
+  return true;
+#endif
+}
+
 }  // namespace
 
 ReportScheduler::ReportScheduler(
@@ -59,6 +70,8 @@ ReportScheduler::~ReportScheduler() {
                                   stale_profiles_->size(),
                                   kMaximumTrackedProfiles);
   }
+  if (ShouldReportUpdates())
+    g_browser_process->GetBuildState()->RemoveObserver(this);
 }
 
 bool ReportScheduler::IsNextReportScheduledForTesting() const {
@@ -74,6 +87,14 @@ void ReportScheduler::OnDMTokenUpdated() {
   OnReportEnabledPrefChanged();
 }
 
+void ReportScheduler::OnUpdate(const BuildState* build_state) {
+  DCHECK(ShouldReportUpdates());
+  // A new version has been detected on the machine and a restart is now needed
+  // for it to take effect. Send a basic report (without profile info)
+  // immediately.
+  GenerateAndUploadReport(kTriggerUpdate);
+}
+
 void ReportScheduler::RegisterPrefObserver() {
   pref_change_registrar_.Init(g_browser_process->local_state());
   pref_change_registrar_.Add(
@@ -86,7 +107,7 @@ void ReportScheduler::RegisterPrefObserver() {
 
 void ReportScheduler::OnReportEnabledPrefChanged() {
   if (!IsReportingEnabled()) {
-    StopRequestTimer();
+    Stop();
     return;
   }
 
@@ -95,16 +116,26 @@ void ReportScheduler::OnReportEnabledPrefChanged() {
   // initialized, and will keep valid during whole life-cycle.
 #if !defined(OS_CHROMEOS)
   if (!SetupBrowserPolicyClientRegistration()) {
-    StopRequestTimer();
+    Stop();
     return;
   }
 #endif
 
-  Start();
+  // Start the periodic report timer.
+  Start(g_browser_process->local_state()->GetTime(kLastUploadTimestamp));
+
+  if (ShouldReportUpdates()) {
+    // Watch for browser updates if not already doing so.
+    auto* build_state = g_browser_process->GetBuildState();
+    if (!build_state->HasObserver(this))
+      build_state->AddObserver(this);
+  }
 }
 
-void ReportScheduler::StopRequestTimer() {
+void ReportScheduler::Stop() {
   request_timer_.Stop();
+  if (ShouldReportUpdates())
+    g_browser_process->GetBuildState()->RemoveObserver(this);
 }
 
 bool ReportScheduler::SetupBrowserPolicyClientRegistration() {
@@ -127,36 +158,58 @@ bool ReportScheduler::SetupBrowserPolicyClientRegistration() {
   return true;
 }
 
-void ReportScheduler::Start() {
-  // The |next_upload_time| is based on the |lastUploadTimestamp| in the
-  // |local_state|, after that, it's 24 hours for each succeeded upload.
-  base::Time next_upload_time =
-      g_browser_process->local_state()->GetTime(kLastUploadTimestamp) +
-      kDefaultUploadInterval;
+void ReportScheduler::Start(base::Time last_upload_time) {
+  // The next report is triggered 24h after the previous was uploaded.
+  const base::Time next_upload_time = last_upload_time + kDefaultUploadInterval;
   if (VLOG_IS_ON(1)) {
     base::TimeDelta first_request_delay = next_upload_time - base::Time::Now();
     VLOG(1) << "Schedule the first report in about "
             << first_request_delay.InHours() << " hour(s) and "
             << first_request_delay.InMinutes() % 60 << " minute(s).";
   }
-  request_timer_.Start(
-      FROM_HERE, next_upload_time,
-      base::BindRepeating(&ReportScheduler::GenerateAndUploadReport,
-                          base::Unretained(this)));
+  request_timer_.Start(FROM_HERE, next_upload_time,
+                       base::BindOnce(&ReportScheduler::GenerateAndUploadReport,
+                                      base::Unretained(this), kTriggerTimer));
 }
 
-void ReportScheduler::GenerateAndUploadReport() {
-  VLOG(1) << "Generating enterprise report.";
-  report_generator_->Generate(base::BindOnce(
-      &ReportScheduler::OnReportGenerated, base::Unretained(this)));
+void ReportScheduler::GenerateAndUploadReport(ReportTrigger trigger) {
+  if (active_trigger_ != kTriggerNone) {
+    // A report is already being generated. Remember this trigger to be handled
+    // once the current report completes.
+    pending_triggers_ |= trigger;
+    return;
+  }
+
+  active_trigger_ = trigger;
+  bool with_profiles = true;
+  switch (trigger) {
+    case kTriggerNone:
+      NOTREACHED();
+      FALLTHROUGH;
+    case kTriggerTimer:
+      VLOG(1) << "Generating enterprise report.";
+      break;
+    case kTriggerUpdate:
+      VLOG(1) << "Generating basic enterprise report upon update.";
+      with_profiles = false;
+      break;
+  }
+
+  report_generator_->Generate(
+      with_profiles, base::BindOnce(&ReportScheduler::OnReportGenerated,
+                                    base::Unretained(this)));
 }
 
 void ReportScheduler::OnReportGenerated(
     ReportGenerator::ReportRequests requests) {
+  DCHECK_NE(active_trigger_, kTriggerNone);
   if (requests.empty()) {
     SYSLOG(ERROR)
         << "No cloud report can be generated. Likely the report is too large.";
-    // We can't generate any report, stop the reporting.
+    // Do not restart the periodic report timer, as it's likely that subsequent
+    // attempts to generate full reports would also fail.
+    active_trigger_ = kTriggerNone;
+    RunPendingTriggers();
     return;
   }
   VLOG(1) << "Uploading enterprise report.";
@@ -170,6 +223,7 @@ void ReportScheduler::OnReportGenerated(
 }
 
 void ReportScheduler::OnReportUploaded(ReportUploader::ReportStatus status) {
+  DCHECK_NE(active_trigger_, kTriggerNone);
   VLOG(1) << "The enterprise report upload result " << status << ".";
   switch (status) {
     case ReportUploader::kSuccess:
@@ -182,21 +236,32 @@ void ReportScheduler::OnReportUploaded(ReportUploader::ReportStatus status) {
     case ReportUploader::kTransientError:
       // Stop retrying and schedule the next report to avoid stale report.
       // Failure count is not reset so retry delay remains.
-      {
-        base::Time now = base::Time::Now();
+      if (active_trigger_ == kTriggerTimer) {
+        const base::Time now = base::Time::Now();
         g_browser_process->local_state()->SetTime(kLastUploadTimestamp, now);
-        if (IsReportingEnabled()) {
-          request_timer_.Start(
-              FROM_HERE, now + kDefaultUploadInterval,
-              base::BindRepeating(&ReportScheduler::GenerateAndUploadReport,
-                                  base::Unretained(this)));
-        }
+        if (IsReportingEnabled())
+          Start(now);
       }
       break;
     case ReportUploader::kPersistentError:
       // No future upload until Chrome relaunch or pref change event.
       break;
   }
+
+  active_trigger_ = kTriggerNone;
+  RunPendingTriggers();
+}
+
+void ReportScheduler::RunPendingTriggers() {
+  DCHECK_EQ(active_trigger_, kTriggerNone);
+  if (!pending_triggers_)
+    return;
+
+  // Timer-triggered reports are a superset of those triggered by an update, so
+  // favor them and consider that they serve both purposes.
+  uint32_t pending_triggers = std::exchange(pending_triggers_, 0);
+  GenerateAndUploadReport(
+      (pending_triggers & kTriggerTimer) != 0 ? kTriggerTimer : kTriggerUpdate);
 }
 
 void ReportScheduler::TrackStaleProfiles() {

chrome/browser/enterprise_reporting/report_scheduler.h

@@ -5,18 +5,19 @@
 #ifndef CHROME_BROWSER_ENTERPRISE_REPORTING_REPORT_SCHEDULER_H_
 #define CHROME_BROWSER_ENTERPRISE_REPORTING_REPORT_SCHEDULER_H_
 
+#include <stdint.h>
 #include <memory>
 #include <queue>
 #include <string>
 
 #include "base/containers/flat_set.h"
 #include "base/macros.h"
-#include "build/build_config.h"
 #include "chrome/browser/enterprise_reporting/notification/extension_request_observer_factory.h"
 #include "chrome/browser/enterprise_reporting/report_generator.h"
 #include "chrome/browser/enterprise_reporting/report_uploader.h"
 #include "chrome/browser/profiles/profile_manager_observer.h"
 #include "chrome/browser/ui/views/relaunch_notification/wall_clock_timer.h"
+#include "chrome/browser/upgrade_detector/build_state_observer.h"
 #include "components/prefs/pref_change_registrar.h"
 
 namespace policy {
@@ -25,9 +26,12 @@ class CloudPolicyClient;
 
 namespace enterprise_reporting {
 
-// Schedules the next report and handles retry in case of error. It also cancels
-// all pending uploads if the report policy is turned off.
-class ReportScheduler : public ProfileManagerObserver {
+// Schedules report generation and upload every 24 hours and upon browser update
+// for desktop Chrome while cloud reporting is enabled via administrative
+// policy. If either of these triggers fires while a report is being generated,
+// processing is deferred until the existing processing completes.
+class ReportScheduler : public ProfileManagerObserver,
+                        public BuildStateObserver {
  public:
   ReportScheduler(policy::CloudPolicyClient* client,
                   std::unique_ptr<ReportGenerator> report_generator);
@@ -43,7 +47,18 @@ class ReportScheduler : public ProfileManagerObserver {
 
   void OnDMTokenUpdated();
 
+  // BuildStateObserver:
+  void OnUpdate(const BuildState* build_state) override;
+
  private:
+  // The trigger leading to report generation. Values are bitmasks in the
+  // |pending_triggers_| bitfield.
+  enum ReportTrigger : uint32_t {
+    kTriggerNone = 0,          // No trigger.
+    kTriggerTimer = 1U << 0,   // The periodic timer expired.
+    kTriggerUpdate = 1U << 1,  // An update was detected.
+  };
+
   // Observes CloudReportingEnabled policy.
   void RegisterPrefObserver();
 
@@ -51,25 +66,31 @@ class ReportScheduler : public ProfileManagerObserver {
   // policy value check during startup.
   void OnReportEnabledPrefChanged();
 
-  // Stop |request_timer_| if it is existing.
-  void StopRequestTimer();
+  // Stops the periodic timer and the update observer.
+  void Stop();
 
   // Register |cloud_policy_client_| with dm token and client id for desktop
   // browser only. (Chrome OS doesn't need this step here.)
   bool SetupBrowserPolicyClientRegistration();
 
-  // Schedules the first update request.
-  void Start();
+  // Starts the periodic timer based on the last time a report was uploaded.
+  void Start(base::Time last_upload_time);
 
-  // Generates a report and uploads it.
-  void GenerateAndUploadReport();
+  // Starts report generation in response to |trigger|.
+  void GenerateAndUploadReport(ReportTrigger trigger);
 
-  // Callback once report is generated.
+  // Continues processing a report (contained in the |requests| collection) by
+  // sending it to the uploader.
   void OnReportGenerated(ReportGenerator::ReportRequests requests);
 
-  // Callback once report upload request is finished.
+  // Finishes processing following report upload. |status| indicates the result
+  // of the attempted upload.
   void OnReportUploaded(ReportUploader::ReportStatus status);
 
+  // Initiates report generation for any triggers that arrived during generation
+  // of another report.
+  void RunPendingTriggers();
+
   // Tracks profiles that miss at least one report.
   void TrackStaleProfiles();
 
@@ -92,6 +113,14 @@ class ReportScheduler : public ProfileManagerObserver {
 
   ExtensionRequestObserverFactory extension_request_observer_factory_;
 
+  // The trigger responsible for initiating active report generation.
+  ReportTrigger active_trigger_ = kTriggerNone;
+
+  // The set of triggers that have fired while processing a report (a bitfield
+  // of ReportTrigger values). They will be handled following completion of the
+  // in-process report.
+  uint32_t pending_triggers_ = 0;
+
   DISALLOW_COPY_AND_ASSIGN(ReportScheduler);
 };
 

chrome/browser/upgrade_detector/build_state.cc

@@ -47,6 +47,11 @@ void BuildState::RemoveObserver(const BuildStateObserver* observer) {
   observers_.RemoveObserver(observer);
 }
 
+bool BuildState::HasObserver(const BuildStateObserver* observer) const {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  return observers_.HasObserver(observer);
+}
+
 void BuildState::NotifyObserversOnUpdate() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   for (auto& observer : observers_)

chrome/browser/upgrade_detector/build_state.h

@@ -70,6 +70,7 @@ class BuildState {
 
   void AddObserver(BuildStateObserver* observer);
   void RemoveObserver(const BuildStateObserver* observer);
+  bool HasObserver(const BuildStateObserver* observer) const;
 
  private:
   void NotifyObserversOnUpdate();