Life time of BulkPrintersCalculator objects

Instances of BulkPrintersCalculator class dispensed by
BulkPrintersCalculatorFactory cause random crashes in test environment.
Sometimes, BulkPrintersCalculatorFactory is not cleared and it may hold
pointers to BulkPrintersCalculator created in previous tests. These
objects are used by two different modules (policies and printing) and
there is no simple rule when they should be deleted, because in many
tests different parts of the real chrome environment are available.
I have decided to bind their lifetime to objects responsible for
handling external policies.

BUG=chromium:966561
TEST=on my workstation

Change-Id: I894e67ab9c1fe61da2ab67832d6e6d65d157719e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1722196
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Sean Kau <skau@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Auto-Submit: Piotr Pawliczek <pawliczek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#683614}

chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc

@@ -55,9 +55,6 @@
 #include "chrome/browser/chromeos/policy/external_data_handlers/print_servers_external_data_handler.h"
 #include "chrome/browser/chromeos/policy/external_data_handlers/user_avatar_image_external_data_handler.h"
 #include "chrome/browser/chromeos/policy/external_data_handlers/wallpaper_image_external_data_handler.h"
-#include "chrome/browser/chromeos/printing/bulk_printers_calculator_factory.h"
-#include "chrome/browser/chromeos/printing/print_servers_provider.h"
-#include "chrome/browser/chromeos/printing/print_servers_provider_factory.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/chromeos/session_length_limiter.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
@@ -444,7 +441,6 @@ void ChromeUserManagerImpl::Shutdown() {
   }
   multi_profile_user_controller_.reset();
   cloud_external_data_policy_handlers_.clear();
-  BulkPrintersCalculatorFactory::Get()->ShutdownProfiles();
   registrar_.RemoveAll();
 }
 

chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc

@@ -43,7 +43,6 @@
 #include "chrome/browser/chromeos/policy/remote_commands/affiliated_remote_commands_invalidator.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"
@@ -294,7 +293,6 @@ void BrowserPolicyConnectorChromeOS::Shutdown() {
        device_cloud_external_data_policy_handlers_) {
     device_cloud_external_data_policy_handler->Shutdown();
   }
-  chromeos::BulkPrintersCalculatorFactory::Get()->ShutdownForDevice();
 
   ChromeBrowserPolicyConnector::Shutdown();
 }

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

@@ -16,7 +16,8 @@ namespace policy {
 namespace {
 
 base::WeakPtr<chromeos::BulkPrintersCalculator> GetBulkPrintersCalculator() {
-  return chromeos::BulkPrintersCalculatorFactory::Get()->GetForDevice();
+  return chromeos::BulkPrintersCalculatorFactory::Get()->GetForDevice(
+      /*create_if_not_exists=*/true);
 }
 
 }  // namespace
@@ -30,7 +31,9 @@ DeviceNativePrintersExternalDataHandler::
               this)) {}
 
 DeviceNativePrintersExternalDataHandler::
-    ~DeviceNativePrintersExternalDataHandler() = default;
+    ~DeviceNativePrintersExternalDataHandler() {
+  chromeos::BulkPrintersCalculatorFactory::Get()->Shutdown();
+}
 
 void DeviceNativePrintersExternalDataHandler::OnDeviceExternalDataSet(
     const std::string& policy) {
@@ -50,8 +53,7 @@ void DeviceNativePrintersExternalDataHandler::OnDeviceExternalDataFetched(
 }
 
 void DeviceNativePrintersExternalDataHandler::Shutdown() {
-  if (device_native_printers_observer_)
-    device_native_printers_observer_.reset();
+  device_native_printers_observer_.reset();
 }
 
 }  // namespace policy

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

@@ -74,7 +74,7 @@ class DeviceNativePrintersExternalDataHandlerTest : public testing::Test {
         std::make_unique<DeviceNativePrintersExternalDataHandler>(
             &policy_service_);
     external_printers_ =
-        chromeos::BulkPrintersCalculatorFactory::Get()->GetForDevice();
+        chromeos::BulkPrintersCalculatorFactory::Get()->GetForDevice(true);
     external_printers_->SetAccessMode(
         chromeos::BulkPrintersCalculator::ALL_ACCESS);
   }

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

@@ -19,7 +19,8 @@ namespace {
 base::WeakPtr<chromeos::BulkPrintersCalculator> GetBulkPrintersCalculator(
     const std::string& user_id) {
   return chromeos::BulkPrintersCalculatorFactory::Get()->GetForAccountId(
-      CloudExternalDataPolicyHandler::GetAccountId(user_id));
+      CloudExternalDataPolicyHandler::GetAccountId(user_id),
+      /*create_if_not_exists=*/true);
 }
 
 }  // namespace

chrome/browser/chromeos/printing/bulk_printers_calculator_factory.cc

@@ -4,7 +4,7 @@
 
 #include "chrome/browser/chromeos/printing/bulk_printers_calculator_factory.h"
 
-#include "base/lazy_instance.h"
+#include "base/no_destructor.h"
 #include "chrome/browser/chromeos/printing/bulk_printers_calculator.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/profiles/profile.h"
@@ -13,39 +13,34 @@
 
 namespace chromeos {
 
-namespace {
-
-base::LazyInstance<BulkPrintersCalculatorFactory>::DestructorAtExit
-    g_printers_factory = LAZY_INSTANCE_INITIALIZER;
-
-}  // namespace
-
 // static
 BulkPrintersCalculatorFactory* BulkPrintersCalculatorFactory::Get() {
-  return g_printers_factory.Pointer();
+  static base::NoDestructor<BulkPrintersCalculatorFactory> instance;
+  return instance.get();
 }
 
 base::WeakPtr<BulkPrintersCalculator>
-BulkPrintersCalculatorFactory::GetForAccountId(const AccountId& account_id) {
+BulkPrintersCalculatorFactory::GetForAccountId(const AccountId& account_id,
+                                               bool create_if_not_exists) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  auto found = printers_by_user_.find(account_id);
-  if (found != printers_by_user_.end()) {
-    return found->second->AsWeakPtr();
-  }
-
-  printers_by_user_[account_id] = BulkPrintersCalculator::Create();
+  auto it = printers_by_user_.find(account_id);
+  if (it != printers_by_user_.end())
+    return it->second->AsWeakPtr();
+  if (!create_if_not_exists)
+    return nullptr;
+  printers_by_user_.emplace(account_id, BulkPrintersCalculator::Create());
   return printers_by_user_[account_id]->AsWeakPtr();
 }
 
 base::WeakPtr<BulkPrintersCalculator>
-BulkPrintersCalculatorFactory::GetForProfile(Profile* profile) {
+BulkPrintersCalculatorFactory::GetForProfile(Profile* profile,
+                                             bool create_if_not_exists) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   const user_manager::User* user =
       ProfileHelper::Get()->GetUserByProfile(profile);
   if (!user)
     return nullptr;
-
-  return GetForAccountId(user->GetAccountId());
+  return GetForAccountId(user->GetAccountId(), create_if_not_exists);
 }
 
 void BulkPrintersCalculatorFactory::RemoveForUserId(
@@ -55,20 +50,19 @@ void BulkPrintersCalculatorFactory::RemoveForUserId(
 }
 
 base::WeakPtr<BulkPrintersCalculator>
-BulkPrintersCalculatorFactory::GetForDevice() {
+BulkPrintersCalculatorFactory::GetForDevice(bool create_if_not_exists) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  if (!device_printers_)
-    device_printers_ = BulkPrintersCalculator::Create();
+  if (device_printers_)
+    return device_printers_->AsWeakPtr();
+  if (!create_if_not_exists)
+    return nullptr;
+  device_printers_ = BulkPrintersCalculator::Create();
   return device_printers_->AsWeakPtr();
 }
 
-void BulkPrintersCalculatorFactory::ShutdownProfiles() {
+void BulkPrintersCalculatorFactory::Shutdown() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   printers_by_user_.clear();
-}
-
-void BulkPrintersCalculatorFactory::ShutdownForDevice() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   device_printers_.reset();
 }
 

chrome/browser/chromeos/printing/bulk_printers_calculator_factory.h

@@ -8,7 +8,6 @@
 #include <map>
 #include <memory>
 
-#include "base/lazy_instance.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/sequence_checker.h"
@@ -20,40 +19,51 @@ namespace chromeos {
 
 class BulkPrintersCalculator;
 
-// Dispenses BulkPrintersCalculator objects based on account id.  Access to this
-// object should be sequenced.
+// Dispenses BulkPrintersCalculator objects based on account id or for device
+// context.  Access to this object should be sequenced.
 class BulkPrintersCalculatorFactory {
  public:
+  // It never returns nullptr.
   static BulkPrintersCalculatorFactory* Get();
 
+  BulkPrintersCalculatorFactory();
+
   // Returns a WeakPtr to the BulkPrintersCalculator registered for
-  // |account_id|. If an BulkPrintersCalculator does not exist, one will be
-  // created for |account_id|. The returned object remains valid until
-  // RemoveForUserId or Shutdown is called.
+  // |account_id|.
+  // If requested BulkPrintersCalculator does not exist, the output depends on
+  // the given parameter |create_if_not_exists|. If it is true, the object is
+  // created and registered, otherwise nullptr is returned.
+  // The returned object remains valid until RemoveForUserId or Shutdown is
+  // called.
   base::WeakPtr<BulkPrintersCalculator> GetForAccountId(
-      const AccountId& account_id);
+      const AccountId& account_id,
+      bool create_if_not_exists);
 
   // Returns a WeakPtr to the BulkPrintersCalculator registered for |profile|
-  // which could be null if |profile| does not map to a valid AccountId. The
-  // returned object remains valid until RemoveForUserId or Shutdown is called.
-  base::WeakPtr<BulkPrintersCalculator> GetForProfile(Profile* profile);
+  // which could be nullptr if |profile| does not map to a valid AccountId.
+  // If requested BulkPrintersCalculator does not exist, the output depends on
+  // the given parameter |create_if_not_exists|. If it is true, the object is
+  // created and registered, otherwise nullptr is returned.
+  // The returned object remains valid until RemoveForUserId or Shutdown is
+  // called.
+  base::WeakPtr<BulkPrintersCalculator> GetForProfile(
+      Profile* profile,
+      bool create_if_not_exists);
 
   // Returns a WeakPtr to the BulkPrintersCalculator registered for the device.
-  base::WeakPtr<BulkPrintersCalculator> GetForDevice();
+  // If requested BulkPrintersCalculator does not exist, the output depends on
+  // the given parameter |create_if_not_exists|. If it is true, the object is
+  // created and registered, otherwise nullptr is returned.
+  // The returned object remains valid until Shutdown is called.
+  base::WeakPtr<BulkPrintersCalculator> GetForDevice(bool create_if_not_exists);
 
   // Deletes the BulkPrintersCalculator registered for |account_id|.
   void RemoveForUserId(const AccountId& account_id);
 
-  // Tear down all BulkPrintersCalculator created for users/profiles.
-  void ShutdownProfiles();
-
-  // Tear down BulkPrintersCalculator created for the device.
-  void ShutdownForDevice();
+  // Tear down all BulkPrintersCalculator objects.
+  void Shutdown();
 
  private:
-  friend struct base::LazyInstanceTraitsBase<BulkPrintersCalculatorFactory>;
-
-  BulkPrintersCalculatorFactory();
   ~BulkPrintersCalculatorFactory();
 
   std::map<AccountId, std::unique_ptr<BulkPrintersCalculator>>

chrome/browser/chromeos/printing/calculators_policies_binder.cc

@@ -60,7 +60,8 @@ class CalculatorsPoliciesBinderImpl : public CalculatorsPoliciesBinder {
       : settings_(settings), profile_(profile) {
     pref_change_registrar_.Init(profile->GetPrefs());
     // Bind device policies to corresponding instance of BulkPrintersCalculator.
-    device_printers_ = BulkPrintersCalculatorFactory::Get()->GetForDevice();
+    device_printers_ = BulkPrintersCalculatorFactory::Get()->GetForDevice(
+        /*create_if_not_exists=*/false);
     if (device_printers_ && ++(BindingsCount()[device_printers_.get()]) == 1) {
       BindSettings(kDeviceNativePrintersAccessMode,
                    &CalculatorsPoliciesBinderImpl::UpdateDeviceAccessMode);
@@ -70,8 +71,9 @@ class CalculatorsPoliciesBinderImpl : public CalculatorsPoliciesBinder {
                    &CalculatorsPoliciesBinderImpl::UpdateDeviceWhitelist);
     }
     // Bind user policies to corresponding instance of BulkPrintersCalculator.
-    user_printers_ =
-        BulkPrintersCalculatorFactory::Get()->GetForProfile(profile);
+    user_printers_ = BulkPrintersCalculatorFactory::Get()->GetForProfile(
+        profile,
+        /*create_if_not_exists=*/false);
     if (user_printers_ && ++(BindingsCount()[user_printers_.get()]) == 1) {
       BindPref(prefs::kRecommendedNativePrintersAccessMode,
                &CalculatorsPoliciesBinderImpl::UpdateUserAccessMode);

chrome/browser/chromeos/printing/enterprise_printers_provider.cc

@@ -52,14 +52,16 @@ class EnterprisePrintersProviderImpl : public EnterprisePrintersProvider,
       // Binds instances of BulkPrintersCalculator to policies.
       policies_binder_ = CalculatorsPoliciesBinder::Create(settings, profile);
       // Get instance of BulkPrintersCalculator for device policies.
-      device_printers_ = BulkPrintersCalculatorFactory::Get()->GetForDevice();
+      device_printers_ = BulkPrintersCalculatorFactory::Get()->GetForDevice(
+          /*create_if_not_exists=*/false);
       if (device_printers_) {
         device_printers_->AddObserver(this);
         RecalculateCompleteFlagForDevicePrinters();
       }
       // Get instance of BulkPrintersCalculator for user policies.
-      user_printers_ =
-          BulkPrintersCalculatorFactory::Get()->GetForProfile(profile);
+      user_printers_ = BulkPrintersCalculatorFactory::Get()->GetForProfile(
+          profile,
+          /*create_if_not_exists=*/false);
       if (user_printers_) {
         user_printers_->AddObserver(this);
         RecalculateCompleteFlagForUserPrinters();