Change ownership of BulkPrintersCalculatorFactory

It was found that BulkPrintersCalculatorFactory is leaking state between
tests and causing flakiness.  Change ownership from a singleton to an
object owned by chrome_browser_main_parts_chromeos to guarantee that only
one ever exists during tests and in production.  To accommodate this, adjust
users of the factory to handle a null factory which will occur during unit
tests and shutdown.

Bug: chromium:1015949, chromium:966561
Change-Id: I61eb5bc9e37363e95d26e8b07ae0cb0d5087c275
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1888517
Commit-Queue: Sean Kau <skau@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Nikita Podguzov <nikitapodguzov@chromium.org>
Reviewed-by: Bailey Berro <baileyberro@chromium.org>
Cr-Commit-Position: refs/heads/master@{#721562}

chrome/browser/chromeos/chrome_browser_main_chromeos.cc

@@ -100,6 +100,7 @@
 #include "chrome/browser/chromeos/power/power_metrics_reporter.h"
 #include "chrome/browser/chromeos/power/process_data_collector.h"
 #include "chrome/browser/chromeos/power/renderer_freezer.h"
+#include "chrome/browser/chromeos/printing/bulk_printers_calculator_factory.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/chromeos/resource_reporter/resource_reporter.h"
 #include "chrome/browser/chromeos/scheduler_configuration_manager.h"
@@ -580,6 +581,11 @@ void ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun() {
   session_termination_manager_ =
       std::make_unique<chromeos::SessionTerminationManager>();
 
+  // This should be in PreProfileInit but it needs to be created before the
+  // policy connector is started.
+  bulk_printers_calculator_factory_ =
+      std::make_unique<BulkPrintersCalculatorFactory>();
+
   ChromeBrowserMainPartsLinux::PreMainMessageLoopRun();
 }
 
@@ -1057,6 +1063,9 @@ void ChromeBrowserMainPartsChromeos::PostMainMessageLoopRun() {
   if (NetworkCertLoader::IsInitialized())
     NetworkCertLoader::Get()->set_is_shutting_down();
 
+  // Tear down BulkPrintersCalculators while we still have threads.
+  bulk_printers_calculator_factory_.reset();
+
   CHECK(g_browser_process);
   CHECK(g_browser_process->platform_part());
 

chrome/browser/chromeos/chrome_browser_main_chromeos.h

@@ -39,6 +39,7 @@ class CrosvmMetrics;
 namespace chromeos {
 
 class ArcKioskAppManager;
+class BulkPrintersCalculatorFactory;
 class CrosUsbDetector;
 class DemoModeResourcesRemover;
 class DiscoverManager;
@@ -173,6 +174,9 @@ class ChromeBrowserMainPartsChromeos : public ChromeBrowserMainPartsLinux {
   std::unique_ptr<chromeos::system::DarkResumeController>
       dark_resume_controller_;
 
+  std::unique_ptr<chromeos::BulkPrintersCalculatorFactory>
+      bulk_printers_calculator_factory_;
+
   std::unique_ptr<SessionTerminationManager> session_termination_manager_;
 
   // Set when PreProfileInit() is called. If PreMainMessageLoopRun() exits

chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc

@@ -44,6 +44,7 @@
 #include "chrome/browser/chromeos/policy/scheduled_update_checker/device_scheduled_update_checker.h"
 #include "chrome/browser/chromeos/policy/server_backed_state_keys_broker.h"
 #include "chrome/browser/chromeos/policy/tpm_auto_update_mode_policy_handler.h"
+#include "chrome/browser/chromeos/printing/bulk_printers_calculator_factory.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/browser/chromeos/settings/device_settings_service.h"
 #include "chrome/browser/chromeos/system/timezone_util.h"
@@ -251,9 +252,14 @@ void BrowserPolicyConnectorChromeOS::Init(
           chromeos::NetworkHandler::Get()->network_state_handler(),
           content::ServiceManagerConnection::GetForProcess()->GetConnector());
 
+  chromeos::BulkPrintersCalculatorFactory* calculator_factory =
+      chromeos::BulkPrintersCalculatorFactory::Get();
+  DCHECK(calculator_factory)
+      << "Policy connector initialized before the bulk printers factory";
   device_cloud_external_data_policy_handlers_.push_back(
       std::make_unique<policy::DeviceNativePrintersExternalDataHandler>(
-          GetPolicyService()));
+          GetPolicyService(), calculator_factory->GetForDevice()));
+
   device_cloud_external_data_policy_handlers_.push_back(
       std::make_unique<policy::DeviceWallpaperImageExternalDataHandler>(
           local_state, GetPolicyService()));

chrome/browser/chromeos/policy/external_data_handlers/device_native_printers_external_data_handler.cc

@@ -8,47 +8,42 @@
 
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/chromeos/printing/bulk_printers_calculator.h"
-#include "chrome/browser/chromeos/printing/bulk_printers_calculator_factory.h"
 #include "components/policy/policy_constants.h"
 
 namespace policy {
 
-namespace {
-
-base::WeakPtr<chromeos::BulkPrintersCalculator> GetBulkPrintersCalculator() {
-  return chromeos::BulkPrintersCalculatorFactory::Get()->GetForDevice();
-}
-
-}  // namespace
-
 DeviceNativePrintersExternalDataHandler::
-    DeviceNativePrintersExternalDataHandler(PolicyService* policy_service)
-    : device_native_printers_observer_(
+    DeviceNativePrintersExternalDataHandler(
+        PolicyService* policy_service,
+        base::WeakPtr<chromeos::BulkPrintersCalculator> calculator)
+    : calculator_(calculator),
+      device_native_printers_observer_(
           std::make_unique<DeviceCloudExternalDataPolicyObserver>(
               policy_service,
               key::kDeviceNativePrinters,
               this)) {}
 
 DeviceNativePrintersExternalDataHandler::
-    ~DeviceNativePrintersExternalDataHandler() {
-  chromeos::BulkPrintersCalculatorFactory::Get()->Shutdown();
-}
+    ~DeviceNativePrintersExternalDataHandler() = default;
 
 void DeviceNativePrintersExternalDataHandler::OnDeviceExternalDataSet(
     const std::string& policy) {
-  GetBulkPrintersCalculator()->ClearData();
+  if (calculator_)
+    calculator_->ClearData();
 }
 
 void DeviceNativePrintersExternalDataHandler::OnDeviceExternalDataCleared(
     const std::string& policy) {
-  GetBulkPrintersCalculator()->ClearData();
+  if (calculator_)
+    calculator_->ClearData();
 }
 
 void DeviceNativePrintersExternalDataHandler::OnDeviceExternalDataFetched(
     const std::string& policy,
     std::unique_ptr<std::string> data,
     const base::FilePath& file_path) {
-  GetBulkPrintersCalculator()->SetData(std::move(data));
+  if (calculator_)
+    calculator_->SetData(std::move(data));
 }
 
 void DeviceNativePrintersExternalDataHandler::Shutdown() {

chrome/browser/chromeos/policy/external_data_handlers/device_native_printers_external_data_handler.h

@@ -8,8 +8,13 @@
 #include <memory>
 #include <string>
 
+#include "base/memory/weak_ptr.h"
 #include "chrome/browser/chromeos/policy/external_data_handlers/device_cloud_external_data_policy_handler.h"
 
+namespace chromeos {
+class BulkPrintersCalculator;
+}  // namespace chromeos
+
 namespace policy {
 
 class PolicyService;
@@ -17,8 +22,9 @@ class PolicyService;
 class DeviceNativePrintersExternalDataHandler
     : public DeviceCloudExternalDataPolicyHandler {
  public:
-  explicit DeviceNativePrintersExternalDataHandler(
-      PolicyService* policy_service);
+  DeviceNativePrintersExternalDataHandler(
+      PolicyService* policy_service,
+      base::WeakPtr<chromeos::BulkPrintersCalculator> device_calculator);
   ~DeviceNativePrintersExternalDataHandler() override;
 
   // DeviceCloudExternalDataPolicyHandler:
@@ -30,6 +36,8 @@ class DeviceNativePrintersExternalDataHandler
   void Shutdown() override;
 
  private:
+  base::WeakPtr<chromeos::BulkPrintersCalculator> calculator_;
+
   std::unique_ptr<DeviceCloudExternalDataPolicyObserver>
       device_native_printers_observer_;
 

chrome/browser/chromeos/policy/external_data_handlers/device_native_printers_external_data_handler_unittest.cc

@@ -9,7 +9,6 @@
 
 #include "base/test/task_environment.h"
 #include "chrome/browser/chromeos/printing/bulk_printers_calculator.h"
-#include "chrome/browser/chromeos/printing/bulk_printers_calculator_factory.h"
 #include "chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h"
 #include "chromeos/printing/printer_configuration.h"
 #include "chromeos/settings/cros_settings_names.h"
@@ -70,11 +69,10 @@ class DeviceNativePrintersExternalDataHandlerTest : public testing::Test {
     EXPECT_CALL(policy_service_,
                 RemoveObserver(policy::POLICY_DOMAIN_CHROME, testing::_))
         .Times(1);
+    external_printers_ = chromeos::BulkPrintersCalculator::Create();
     device_native_printers_external_data_handler_ =
         std::make_unique<DeviceNativePrintersExternalDataHandler>(
-            &policy_service_);
-    external_printers_ =
-        chromeos::BulkPrintersCalculatorFactory::Get()->GetForDevice();
+            &policy_service_, external_printers_->AsWeakPtr());
     external_printers_->SetAccessMode(
         chromeos::BulkPrintersCalculator::ALL_ACCESS);
   }
@@ -88,7 +86,7 @@ class DeviceNativePrintersExternalDataHandlerTest : public testing::Test {
   MockPolicyService policy_service_;
   std::unique_ptr<DeviceNativePrintersExternalDataHandler>
       device_native_printers_external_data_handler_;
-  base::WeakPtr<chromeos::BulkPrintersCalculator> external_printers_;
+  std::unique_ptr<chromeos::BulkPrintersCalculator> external_printers_;
 };
 
 TEST_F(DeviceNativePrintersExternalDataHandlerTest, OnDataFetched) {

chrome/browser/chromeos/policy/external_data_handlers/native_printers_external_data_handler.cc

@@ -18,7 +18,11 @@ namespace {
 
 base::WeakPtr<chromeos::BulkPrintersCalculator> GetBulkPrintersCalculator(
     const std::string& user_id) {
-  return chromeos::BulkPrintersCalculatorFactory::Get()->GetForAccountId(
+  auto* factory = chromeos::BulkPrintersCalculatorFactory::Get();
+  if (!factory) {
+    return nullptr;
+  }
+  return factory->GetForAccountId(
       CloudExternalDataPolicyHandler::GetAccountId(user_id));
 }
 
@@ -40,13 +44,19 @@ NativePrintersExternalDataHandler::~NativePrintersExternalDataHandler() =
 void NativePrintersExternalDataHandler::OnExternalDataSet(
     const std::string& policy,
     const std::string& user_id) {
-  GetBulkPrintersCalculator(user_id)->ClearData();
+  auto calculator = GetBulkPrintersCalculator(user_id);
+  if (calculator) {
+    calculator->ClearData();
+  }
 }
 
 void NativePrintersExternalDataHandler::OnExternalDataCleared(
     const std::string& policy,
     const std::string& user_id) {
-  GetBulkPrintersCalculator(user_id)->ClearData();
+  auto calculator = GetBulkPrintersCalculator(user_id);
+  if (calculator) {
+    calculator->ClearData();
+  }
 }
 
 void NativePrintersExternalDataHandler::OnExternalDataFetched(
@@ -54,12 +64,18 @@ void NativePrintersExternalDataHandler::OnExternalDataFetched(
     const std::string& user_id,
     std::unique_ptr<std::string> data,
     const base::FilePath& file_path) {
-  GetBulkPrintersCalculator(user_id)->SetData(std::move(data));
+  auto calculator = GetBulkPrintersCalculator(user_id);
+  if (calculator) {
+    calculator->SetData(std::move(data));
+  }
 }
 
 void NativePrintersExternalDataHandler::RemoveForAccountId(
     const AccountId& account_id) {
-  chromeos::BulkPrintersCalculatorFactory::Get()->RemoveForUserId(account_id);
+  auto* factory = chromeos::BulkPrintersCalculatorFactory::Get();
+  if (factory) {
+    factory->RemoveForUserId(account_id);
+  }
 }
 
 }  // namespace policy

chrome/browser/chromeos/printing/bulk_printers_calculator_factory.cc

@@ -11,10 +11,16 @@
 
 namespace chromeos {
 
+namespace {
+
+// This class is owned by ChromeBrowserMainPartsChromeos.
+static BulkPrintersCalculatorFactory* g_bulk_printers_factory = nullptr;
+
+}  // namespace
+
 // static
 BulkPrintersCalculatorFactory* BulkPrintersCalculatorFactory::Get() {
-  static base::NoDestructor<BulkPrintersCalculatorFactory> instance;
-  return instance.get();
+  return g_bulk_printers_factory;
 }
 
 base::WeakPtr<BulkPrintersCalculator>
@@ -36,19 +42,33 @@ void BulkPrintersCalculatorFactory::RemoveForUserId(
 base::WeakPtr<BulkPrintersCalculator>
 BulkPrintersCalculatorFactory::GetForDevice() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  if (device_printers_)
-    return device_printers_->AsWeakPtr();
+  if (shutdown_) {
+    return nullptr;
+  }
+
+  if (!device_printers_)
     device_printers_ = BulkPrintersCalculator::Create();
   return device_printers_->AsWeakPtr();
 }
 
 void BulkPrintersCalculatorFactory::Shutdown() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(!shutdown_);
+  shutdown_ = true;
   printers_by_user_.clear();
   device_printers_.reset();
 }
 
-BulkPrintersCalculatorFactory::BulkPrintersCalculatorFactory() = default;
-BulkPrintersCalculatorFactory::~BulkPrintersCalculatorFactory() = default;
+BulkPrintersCalculatorFactory::BulkPrintersCalculatorFactory() {
+  // Only one factory should exist.
+  DCHECK(!g_bulk_printers_factory);
+  g_bulk_printers_factory = this;
+}
+
+BulkPrintersCalculatorFactory::~BulkPrintersCalculatorFactory() {
+  // Ensure that an instance was created sometime in the past.
+  DCHECK(g_bulk_printers_factory);
+  g_bulk_printers_factory = nullptr;
+}
 
 }  // namespace chromeos

chrome/browser/chromeos/printing/bulk_printers_calculator_factory.h

@@ -26,6 +26,7 @@ class BulkPrintersCalculatorFactory {
   static BulkPrintersCalculatorFactory* Get();
 
   BulkPrintersCalculatorFactory();
+  ~BulkPrintersCalculatorFactory();
 
   // Returns a WeakPtr to the BulkPrintersCalculator registered for
   // |account_id|.
@@ -35,10 +36,10 @@ class BulkPrintersCalculatorFactory {
   base::WeakPtr<BulkPrintersCalculator> GetForAccountId(
       const AccountId& account_id);
 
-  // Returns a WeakPtr to the BulkPrintersCalculator registered for the device.
+  // Returns a pointer to the BulkPrintersCalculator registered for the device.
   // If requested BulkPrintersCalculator does not exist, the object is
   // created and registered. The returned object remains valid until Shutdown is
-  // called. It never returns nullptr.
+  // called. Returns nullptr if called after Shutdown or during unit tests.
   base::WeakPtr<BulkPrintersCalculator> GetForDevice();
 
   // Deletes the BulkPrintersCalculator registered for |account_id|.
@@ -48,8 +49,7 @@ class BulkPrintersCalculatorFactory {
   void Shutdown();
 
  private:
-  ~BulkPrintersCalculatorFactory();
-
+  bool shutdown_ = false;
   std::map<AccountId, std::unique_ptr<BulkPrintersCalculator>>
       printers_by_user_;
   std::unique_ptr<BulkPrintersCalculator> device_printers_;

chrome/browser/chromeos/printing/enterprise_printers_provider.cc

@@ -48,27 +48,36 @@ class EnterprisePrintersProviderImpl : public EnterprisePrintersProvider,
     // initialization of pref_change_registrar
     pref_change_registrar_.Init(profile->GetPrefs());
 
+    auto* factory = BulkPrintersCalculatorFactory::Get();
+    if (!factory) {
+      DVLOG(1) << "Factory is null.  Policies are unbound.  This is only "
+                  "expected in unit tests";
+      return;
+    }
+
     // Get instance of BulkPrintersCalculator for device policies.
-    device_printers_ = BulkPrintersCalculatorFactory::Get()->GetForDevice();
+    device_printers_ = factory->GetForDevice();
     if (device_printers_) {
       devices_binder_ =
           CalculatorsPoliciesBinder::DeviceBinder(settings, device_printers_);
       device_printers_->AddObserver(this);
       RecalculateCompleteFlagForDevicePrinters();
     }
+
     // Calculate account_id_ and get instance of BulkPrintersCalculator for user
     // policies.
     const user_manager::User* user =
         ProfileHelper::Get()->GetUserByProfile(profile);
     if (user) {
       account_id_ = user->GetAccountId();
-      user_printers_ =
-          BulkPrintersCalculatorFactory::Get()->GetForAccountId(account_id_);
+      user_printers_ = factory->GetForAccountId(account_id_);
+      // Binds instances of BulkPrintersCalculator to policies.
       profile_binder_ = CalculatorsPoliciesBinder::UserBinder(
           profile->GetPrefs(), user_printers_);
       user_printers_->AddObserver(this);
       RecalculateCompleteFlagForUserPrinters();
     }
+
     // Binds policy with recommended printers (deprecated). This method calls
     // indirectly RecalculateCurrentPrintersList() that prepares the first
     // version of final list of printers.
@@ -81,7 +90,6 @@ class EnterprisePrintersProviderImpl : public EnterprisePrintersProvider,
       device_printers_->RemoveObserver(this);
     if (user_printers_) {
       user_printers_->RemoveObserver(this);
-      BulkPrintersCalculatorFactory::Get()->RemoveForUserId(account_id_);
     }
   }
 
@@ -98,9 +106,9 @@ class EnterprisePrintersProviderImpl : public EnterprisePrintersProvider,
 
   // BulkPrintersCalculator::Observer implementation
   void OnPrintersChanged(const BulkPrintersCalculator* sender) override {
-    if (sender == device_printers_.get()) {
+    if (device_printers_ && sender == device_printers_.get()) {
       RecalculateCompleteFlagForDevicePrinters();
-    } else {
+    } else if (user_printers_ && sender == user_printers_.get()) {
       RecalculateCompleteFlagForUserPrinters();
     }
     RecalculateCurrentPrintersList();
@@ -149,6 +157,11 @@ class EnterprisePrintersProviderImpl : public EnterprisePrintersProvider,
   // These three methods calculate resultant list of printers and complete flag.
 
   void RecalculateCompleteFlagForUserPrinters() {
+    if (!user_printers_) {
+      user_printers_is_complete_ = true;
+      return;
+    }
+
     user_printers_is_complete_ =
         user_printers_->IsComplete() &&
         (user_printers_->IsDataPolicySet() ||
@@ -156,6 +169,11 @@ class EnterprisePrintersProviderImpl : public EnterprisePrintersProvider,
   }
 
   void RecalculateCompleteFlagForDevicePrinters() {
+    if (!device_printers_) {
+      device_printers_is_complete_ = true;
+      return;
+    }
+
     device_printers_is_complete_ =
         device_printers_->IsComplete() &&
         (device_printers_->IsDataPolicySet() ||

chrome/browser/chromeos/printing/synced_printers_manager_unittest.cc

@@ -15,6 +15,7 @@
 #include "base/scoped_observer.h"
 #include "base/strings/string_util.h"
 #include "base/time/time.h"
+#include "chrome/browser/chromeos/printing/bulk_printers_calculator_factory.h"
 #include "chrome/browser/chromeos/printing/printers_sync_bridge.h"
 #include "chrome/browser/chromeos/printing/synced_printers_manager_factory.h"
 #include "chrome/common/pref_names.h"
@@ -107,6 +108,9 @@ class SyncedPrintersManagerTest : public testing::Test {
   // Must outlive |manager_|.
   TestingProfile profile_;
 
+  // TODO(https://crbug.com/1030127): Remove this dependency after enterprise
+  // printers are removed from this class.
+  BulkPrintersCalculatorFactory bulk_factory_;
   std::unique_ptr<SyncedPrintersManager> manager_;
 };