Revert "Add full Profile report into enterprise report."

This reverts commit 578f4505.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 678372 as the
culprit for failures in the build cycles as shown on:
https://analysis.chromium.org/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtLzU3OGY0NTA1NDMwMTA1YjUwMDM3NmRmMTY4NjI0ZDU5YmE0OGFhNWEM

Sample Failed Build: https://ci.chromium.org/buildbot/chromium.memory/Mac%20ASan%2064%20Tests%20%281%29/54995

Sample Failed Step: unit_tests

Original change's description:
> Add full Profile report into enterprise report.
> 
> Full profile reports are append into the report for all activated profiles.
> Multiple requests are created if total report size exceeds the limitation.
> 
> Bug: 956237
> Change-Id: Iee36c34857346ca2a6aa2dbbfe7c09301fd3d129
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1702770
> Commit-Queue: Owen Min <zmin@chromium.org>
> Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#678372}


Change-Id: Ic93aa2db887c88ebafac36a46f4fa8684eb6decb
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 956237
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1706886
Cr-Commit-Position: refs/heads/master@{#678498}

chrome/browser/enterprise_reporting/profile_report_generator.cc

@@ -36,13 +36,12 @@ void ProfileReportGenerator::set_policies_enabled(bool enabled) {
 void ProfileReportGenerator::MaybeGenerate(const base::FilePath& path,
                                            const std::string& name,
                                            ReportCallback callback) {
-  DCHECK(!callback_);
   callback_ = std::move(callback);
 
   profile_ = g_browser_process->profile_manager()->GetProfileByPath(path);
 
   if (!profile_) {
-    CheckReportStatus();
+    CheckReportStatusAsync();
     return;
   }
 
@@ -67,7 +66,7 @@ void ProfileReportGenerator::MaybeGenerate(const base::FilePath& path,
     GetExtensionPolicyInfo();
   }
 
-  CheckReportStatus();
+  CheckReportStatusAsync();
   return;
 }
 
@@ -134,4 +133,10 @@ void ProfileReportGenerator::CheckReportStatus() {
   std::move(callback_).Run(std::move(report_));
 }
 
+void ProfileReportGenerator::CheckReportStatusAsync() {
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::BindOnce(&ProfileReportGenerator::CheckReportStatus,
+                                weak_ptr_factory_.GetWeakPtr()));
+}
+
 }  // namespace enterprise_reporting

chrome/browser/enterprise_reporting/profile_report_generator.h

@@ -60,6 +60,7 @@ class ProfileReportGenerator {
   void OnPluginsLoaded(const std::vector<content::WebPluginInfo>& plugins);
 
   void CheckReportStatus();
+  void CheckReportStatusAsync();
 
   Profile* profile_;
   base::Value policies_;

chrome/browser/enterprise_reporting/report_generator.cc

@@ -29,9 +29,6 @@ namespace em = enterprise_management;
 namespace enterprise_reporting {
 namespace {
 
-const size_t kMaximumReportSize =
-    5000000;  // The report size limitation is 5mb.
-
 std::string GetChromePath() {
   base::FilePath path;
   base::PathService::Get(base::DIR_EXE, &path);
@@ -40,31 +37,15 @@ std::string GetChromePath() {
 
 }  // namespace
 
-ReportGenerator::ReportGenerator()
-    : maximum_report_size_(kMaximumReportSize), weak_ptr_factory_(this) {}
-
+ReportGenerator::ReportGenerator() = default;
 ReportGenerator::~ReportGenerator() = default;
 
-void ReportGenerator::Generate(ReportCallback callback) {
-  DCHECK(!callback_);
-  callback_ = std::move(callback);
+std::vector<std::unique_ptr<em::ChromeDesktopReportRequest>>
+ReportGenerator::Generate() {
   CreateBasicRequest();
-
-  if (basic_request_size_ > maximum_report_size_) {
-    // Basic request is already too large so we can't upload any valid report.
-    // Skip all Profiles and response an empty request list.
-    GetNextProfileReport(
-        basic_request_.browser_report().profile_info_list_size());
-    return;
-  }
-
   requests_.push_back(
       std::make_unique<em::ChromeDesktopReportRequest>(basic_request_));
-  GetNextProfileReport(0);
-}
-
-void ReportGenerator::SetMaximumReportSizeForTesting(size_t size) {
-  maximum_report_size_ = size;
+  return std::move(requests_);
 }
 
 void ReportGenerator::CreateBasicRequest() {
@@ -77,7 +58,6 @@ void ReportGenerator::CreateBasicRequest() {
     basic_request_.mutable_browser_report()->add_profile_info_list()->Swap(
         profile.get());
   }
-  basic_request_size_ = basic_request_.ByteSizeLong();
 }
 
 std::unique_ptr<em::OSReport> ReportGenerator::GetOSReport() {
@@ -127,57 +107,4 @@ ReportGenerator::GetProfiles() {
   return profiles;
 }
 
-void ReportGenerator::GetNextProfileReport(int profile_index) {
-  if (profile_index >=
-      basic_request_.browser_report().profile_info_list_size()) {
-    std::move(callback_).Run(std::move(requests_));
-    return;
-  }
-
-  auto basic_profile_report =
-      basic_request_.browser_report().profile_info_list(profile_index);
-  profile_report_generator_.MaybeGenerate(
-      base::FilePath::FromUTF8Unsafe(basic_profile_report.id()),
-      basic_profile_report.name(),
-      base::BindOnce(&ReportGenerator::OnProfileReportReady,
-                     weak_ptr_factory_.GetWeakPtr(), profile_index));
-}
-
-void ReportGenerator::OnProfileReportReady(
-    int profile_index,
-    std::unique_ptr<em::ChromeUserProfileInfo> profile_report) {
-  // Move to the next Profile if Profile is not loaded and there is no full
-  // report.
-  if (!profile_report) {
-    GetNextProfileReport(profile_index + 1);
-    return;
-  }
-
-  size_t profile_report_size = profile_report->ByteSizeLong();
-  size_t current_request_size = requests_.back()->ByteSizeLong();
-
-  if (current_request_size + profile_report_size <= maximum_report_size_) {
-    // The new full Profile report can be appended into the current request.
-    requests_.back()
-        ->mutable_browser_report()
-        ->mutable_profile_info_list(profile_index)
-        ->Swap(profile_report.get());
-  } else if (basic_request_size_ + profile_report_size <=
-             maximum_report_size_) {
-    // The new full Profile report is too big to be appended into the current
-    // request, move it to the next request if possible.
-    requests_.push_back(
-        std::make_unique<em::ChromeDesktopReportRequest>(basic_request_));
-    requests_.back()
-        ->mutable_browser_report()
-        ->mutable_profile_info_list(profile_index)
-        ->Swap(profile_report.get());
-  }
-  // Else: The new full Profile report is too big to be uploaded, skip this
-  // Profile report.
-  // TODO(crbug.com/956237): Record this event with UMA metrics.
-
-  GetNextProfileReport(profile_index + 1);
-}
-
 }  // namespace enterprise_reporting

chrome/browser/enterprise_reporting/report_generator.h

@@ -10,7 +10,6 @@
 #include <vector>
 
 #include "base/macros.h"
-#include "chrome/browser/enterprise_reporting/profile_report_generator.h"
 #include "components/policy/proto/device_management_backend.pb.h"
 
 namespace em = enterprise_management;
@@ -19,15 +18,10 @@ namespace enterprise_reporting {
 
 class ReportGenerator {
  public:
-  using ReportCallback = base::OnceCallback<void(
-      std::vector<std::unique_ptr<em::ChromeDesktopReportRequest>>)>;
-
   ReportGenerator();
   ~ReportGenerator();
 
-  void Generate(ReportCallback callback);
-
-  void SetMaximumReportSizeForTesting(size_t size);
+  std::vector<std::unique_ptr<em::ChromeDesktopReportRequest>> Generate();
 
  protected:
   // Creates a basic request that will be used by all Profiles.
@@ -56,24 +50,10 @@ class ReportGenerator {
   std::vector<std::unique_ptr<em::ChromeUserProfileInfo>> GetProfiles();
 
  private:
-  void GetNextProfileReport(int profile_index);
-  void OnProfileReportReady(
-      int profile_index,
-      std::unique_ptr<em::ChromeUserProfileInfo> profile_report);
-
-  ProfileReportGenerator profile_report_generator_;
-
-  ReportCallback callback_;
-
   std::vector<std::unique_ptr<em::ChromeDesktopReportRequest>> requests_;
 
   // Basic information that is shared among requests.
   em::ChromeDesktopReportRequest basic_request_;
-  size_t basic_request_size_;
-
-  size_t maximum_report_size_;
-
-  base::WeakPtrFactory<ReportGenerator> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ReportGenerator);
 };

chrome/browser/enterprise_reporting/report_generator_unittest.cc

@@ -6,30 +6,23 @@
 
 #include <set>
 
-#include "base/run_loop.h"
+#include "base/logging.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
-#include "base/test/bind_test_util.h"
 #include "build/build_config.h"
 #include "chrome/browser/profiles/profile_attributes_storage.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile_manager.h"
 #include "components/account_id/account_id.h"
-#include "content/public/browser/plugin_service.h"
 #include "content/public/test/test_browser_thread_bundle.h"
-#include "extensions/browser/extension_registry.h"
-#include "extensions/common/extension_builder.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace em = enterprise_management;
 
 namespace enterprise_reporting {
 namespace {
-
 #if !defined(OS_CHROMEOS)
-constexpr char kProfile[] = "Profile";
-
 // We only upload serial number on Windows.
 void VerifySerialNumber(const std::string& serial_number) {
 #if defined(OS_WIN)
@@ -39,14 +32,7 @@ void VerifySerialNumber(const std::string& serial_number) {
 #endif
 }
 
-// Verify the name is in the set. Remove the name from the set afterwards.
-void FindAndRemoveProfileName(std::set<std::string>* names,
-                              const std::string& name) {
-  auto it = names->find(name);
-  EXPECT_NE(names->end(), it);
-  names->erase(it);
-}
-
+const char kProfile1[] = "Profile";
 #endif
 }  // namespace
 
@@ -57,14 +43,7 @@ class ReportGeneratorTest : public ::testing::Test {
       : profile_manager_(TestingBrowserProcess::GetGlobal()) {}
   ~ReportGeneratorTest() override = default;
 
-  void SetUp() override {
-    ASSERT_TRUE(profile_manager_.SetUp());
-
-    profile_manager_.CreateGuestProfile();
-    profile_manager_.CreateSystemProfile();
-
-    content::PluginService::GetInstance()->Init();
-  }
+  void SetUp() override { ASSERT_TRUE(profile_manager_.SetUp()); }
 
   // Creates |number| of Profiles. Returns the set of their names. The profile
   // names begin with Profile|start_index|. Profile instances are created if
@@ -76,7 +55,7 @@ class ReportGeneratorTest : public ::testing::Test {
     std::set<std::string> profile_names;
     for (int i = start_index; i < number; i++) {
       std::string profile_name =
-          std::string(kProfile) + base::NumberToString(i);
+          std::string(kProfile1) + base::NumberToString(i);
       if (is_active) {
         profile_manager_.CreateTestingProfile(profile_name);
       } else {
@@ -87,63 +66,14 @@ class ReportGeneratorTest : public ::testing::Test {
       }
       profile_names.insert(profile_name);
     }
+    profile_manager_.CreateGuestProfile();
+    profile_manager_.CreateSystemProfile();
     return profile_names;
   }
 
-  std::vector<std::unique_ptr<em::ChromeDesktopReportRequest>>
-  GenerateRequests() {
-    base::RunLoop run_loop;
-    std::vector<std::unique_ptr<em::ChromeDesktopReportRequest>> rets;
-    generator_.Generate(base::BindLambdaForTesting(
-        [&run_loop,
-         &rets](std::vector<std::unique_ptr<em::ChromeDesktopReportRequest>>
-                    requests) {
-          rets = std::move(requests);
-          run_loop.Quit();
-        }));
-    run_loop.Run();
-    return rets;
-  }
-
-  // Verify the profile report matches actual Profile setup.
-  void VerifyProfileReport(const std::set<std::string>& active_profiles_names,
-                           const std::set<std::string>& inactive_profiles_names,
-                           const em::BrowserReport& actual_browser_report) {
-    int expected_profile_number =
-        active_profiles_names.size() + inactive_profiles_names.size();
-    EXPECT_EQ(expected_profile_number,
-              actual_browser_report.profile_info_list_size());
-
-    auto mutable_active_profiles_names(active_profiles_names);
-    auto mutable_inactive_profiles_names(inactive_profiles_names);
-    for (int i = 0; i < expected_profile_number; i++) {
-      auto actual_profile_info = actual_browser_report.profile_info_list(i);
-      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_TRUE(actual_profile_info.has_is_full_report());
-
-      // Activate profiles have full report while the inactive ones don't.
-      if (actual_profile_info.is_full_report())
-        FindAndRemoveProfileName(&mutable_active_profiles_names,
-                                 actual_profile_name);
-      else
-        FindAndRemoveProfileName(&mutable_inactive_profiles_names,
-                                 actual_profile_name);
-    }
-  }
-
   TestingProfileManager* profile_manager() { return &profile_manager_; }
-  ReportGenerator* generator() { return &generator_; }
 
  private:
-  ReportGenerator generator_;
-
   content::TestBrowserThreadBundle thread_bundle_;
   TestingProfileManager profile_manager_;
 
@@ -151,9 +81,11 @@ class ReportGeneratorTest : public ::testing::Test {
 };
 
 TEST_F(ReportGeneratorTest, GenerateBasicReport) {
-  auto profile_names = CreateProfiles(/*number*/ 2, /*is_active=*/false);
+  int profile_number = 2;
+  auto profile_names = CreateProfiles(profile_number, /*is_active=*/false);
 
-  auto requests = GenerateRequests();
+  ReportGenerator generator;
+  auto requests = generator.Generate();
   EXPECT_EQ(1u, requests.size());
 
   auto* basic_request = requests[0].get();
@@ -173,93 +105,27 @@ TEST_F(ReportGeneratorTest, GenerateBasicReport) {
   EXPECT_NE(std::string(), browser_report.executable_path());
   EXPECT_TRUE(browser_report.has_channel());
 
-  VerifyProfileReport(/*active_profile_names*/ std::set<std::string>(),
-                      profile_names, browser_report);
-}
-
-TEST_F(ReportGeneratorTest, GenerateActiveProfiles) {
-  auto inactive_profiles_names =
-      CreateProfiles(/*number*/ 2, /*is_active=*/false);
-  auto active_profiles_names =
-      CreateProfiles(/*number*/ 2, /*is_active=*/true, /*start_index*/ 2);
+  EXPECT_EQ(profile_number, browser_report.profile_info_list_size());
+  for (int i = 0; i < profile_number; i++) {
+    auto profile_info = browser_report.profile_info_list(i);
+    std::string profile_name = profile_info.name();
 
-  auto requests = GenerateRequests();
-  EXPECT_EQ(1u, requests.size());
-
-  VerifyProfileReport(active_profiles_names, inactive_profiles_names,
-                      requests[0]->browser_report());
-}
-
-TEST_F(ReportGeneratorTest, BasicReportIsTooBig) {
-  CreateProfiles(/*number*/ 2, /*is_active=*/false);
-
-  // Set a super small limitation.
-  generator()->SetMaximumReportSizeForTesting(5);
-
-  // Because the limitation is so small, no request can be created.
-  auto requests = GenerateRequests();
-  EXPECT_EQ(0u, requests.size());
-}
-
-TEST_F(ReportGeneratorTest, ReportSeparation) {
-  auto profile_names = CreateProfiles(/*number*/ 2, /*is_active=*/true);
-
-  // Set the limitation just below the size of the report so that it needs to be
-  // separated into two requests later.
-  auto requests = GenerateRequests();
-  EXPECT_EQ(1u, requests.size());
-  generator()->SetMaximumReportSizeForTesting(requests[0]->ByteSizeLong() - 30);
-
-  std::set<std::string> first_request_profiles, second_request_profiles;
-  first_request_profiles.insert(
-      requests[0]->browser_report().profile_info_list(0).name());
-  second_request_profiles.insert(
-      requests[0]->browser_report().profile_info_list(1).name());
-
-  requests = GenerateRequests();
-
-  // The first profile is activated in the first request only while the second
-  // profile is activated in the second request.
-  EXPECT_EQ(2u, requests.size());
-  VerifyProfileReport(first_request_profiles, second_request_profiles,
-                      requests[0]->browser_report());
-  VerifyProfileReport(second_request_profiles, first_request_profiles,
-                      requests[1]->browser_report());
-}
-
-TEST_F(ReportGeneratorTest, ProfileReportIsTooBig) {
-  TestingProfile* first_profile =
-      profile_manager()->CreateTestingProfile(kProfile);
-  std::set<std::string> first_profile_name = {kProfile};
-
-  // Add more things into the Profile to make the report bigger.
-  extensions::ExtensionRegistry* extension_registry =
-      extensions::ExtensionRegistry::Get(first_profile);
-
-  std::string extension_name =
-      "a super super super super super super super super super super super "
-      "super super super super super super long extension name";
-  extension_registry->AddEnabled(extensions::ExtensionBuilder(extension_name)
-                                     .SetID("abcdefghijklmnoabcdefghijklmnoab")
-                                     .Build());
-
-  // Set the limitation just below the size of the report.
-  auto requests = GenerateRequests();
-  EXPECT_EQ(1u, requests.size());
-  generator()->SetMaximumReportSizeForTesting(requests[0]->ByteSizeLong() - 30);
-
-  // Add a smaller Profile.
-  auto second_profile_name = CreateProfiles(/*number*/ 1, /*is_active=*/true);
+    // Verify that the profile id is set as profile path.
+    EXPECT_EQ(profile_manager()
+                  ->profiles_dir()
+                  .AppendASCII(profile_name)
+                  .AsUTF8Unsafe(),
+              profile_info.id());
 
-  requests = GenerateRequests();
+    // Verify that the basic report does not contain any full Profile report.
+    EXPECT_FALSE(profile_info.is_full_report());
 
-  EXPECT_EQ(1u, requests.size());
-  // Only the second Profile is activated while the first one is too big to be
-  // reported.
-  VerifyProfileReport(second_profile_name, first_profile_name,
-                      requests[0]->browser_report());
+    // Verify the profile name is one of the profiles that were created above.
+    auto it = profile_names.find(profile_name);
+    EXPECT_NE(profile_names.end(), it);
+    profile_names.erase(it);
+  }
 }
-
 #endif
 
 }  // namespace enterprise_reporting