Remove NOTIFICATION_PROFILE_CREATED from OwnerSettingsServiceChromeOS

ProfileManagerObserver::OnProfileAdded() can serve as the signal that
the Profile is fully created.

This patch also adjusts OwnerSettingsServiceChromeOSFactory to make
it more obvious that off the record profiles (both guest and incognito)
don't get a service instance.

TBR=bartfab@chromium.org,rdevlin.cronin@chromium.org

Bug: 268984
Change-Id: I2048d85828863cc9611b724c5b7c5c0b114089ec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1794273Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#695736}

chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager_browsertest.cc

@@ -92,6 +92,7 @@ class ArcKioskAppManagerTest : public InProcessBrowserTest {
   }
 
   void TearDownOnMainThread() override {
+    owner_settings_service_.reset();
     settings_helper_.RestoreRealDeviceSettingsProvider();
   }
 

chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc

@@ -269,6 +269,7 @@ class KioskAppManagerTest : public InProcessBrowserTest {
   }
 
   void TearDownOnMainThread() override {
+    owner_settings_service_.reset();
     settings_helper_.RestoreRealDeviceSettingsProvider();
   }
 

chrome/browser/chromeos/login/kiosk_browsertest.cc

@@ -521,6 +521,7 @@ class KioskTest : public OobeBaseTest {
   }
 
   void TearDownOnMainThread() override {
+    owner_settings_service_.reset();
     settings_helper_.RestoreRealDeviceSettingsProvider();
     AppLaunchController::SetNetworkTimeoutCallbackForTesting(NULL);
     AppLaunchSigninScreen::SetUserManagerForTesting(NULL);

chrome/browser/chromeos/ownership/fake_owner_settings_service.cc

@@ -31,8 +31,7 @@ FakeOwnerSettingsService::FakeOwnerSettingsService(
       set_management_settings_result_(true),
       settings_provider_(provider) {}
 
-FakeOwnerSettingsService::~FakeOwnerSettingsService() {
-}
+FakeOwnerSettingsService::~FakeOwnerSettingsService() = default;
 
 bool FakeOwnerSettingsService::IsOwner() {
   return !InstallAttributes::Get()->IsEnterpriseManaged() &&
@@ -46,13 +45,4 @@ bool FakeOwnerSettingsService::Set(const std::string& setting,
   return true;
 }
 
-void FakeOwnerSettingsService::Observe(
-    int type,
-    const content::NotificationSource& source,
-    const content::NotificationDetails& details) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  if (!ignore_profile_creation_notifications_)
-    OwnerSettingsServiceChromeOS::Observe(type, source, details);
-}
-
 }  // namespace chromeos

chrome/browser/chromeos/ownership/fake_owner_settings_service.h

@@ -34,10 +34,6 @@ class FakeOwnerSettingsService : public OwnerSettingsServiceChromeOS {
     set_management_settings_result_ = success;
   }
 
-  void set_ignore_profile_creation_notification(bool ignore) {
-    ignore_profile_creation_notifications_ = ignore;
-  }
-
   const ManagementSettings& last_settings() const {
     return last_settings_;
   }
@@ -46,19 +42,10 @@ class FakeOwnerSettingsService : public OwnerSettingsServiceChromeOS {
   bool IsOwner() override;
   bool Set(const std::string& setting, const base::Value& value) override;
 
-  // NotificationObserver:
-  void Observe(int type,
-               const content::NotificationSource& source,
-               const content::NotificationDetails& details) override;
-
  private:
   bool set_management_settings_result_ = true;
   ManagementSettings last_settings_;
   StubCrosSettingsProvider* settings_provider_;
-  // Creating TestingProfiles after constructing a FakeOwnerSettingsService
-  // causes the underlying OwnerSettingsServiceChromeOS::Observe to be called,
-  // which can be bad in tests.
-  bool ignore_profile_creation_notifications_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(FakeOwnerSettingsService);
 };

chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc

@@ -19,13 +19,14 @@
 #include "base/stl_util.h"
 #include "base/task/post_task.h"
 #include "base/threading/thread_checker.h"
-#include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/login/session/user_session_manager.h"
 #include "chrome/browser/chromeos/ownership/owner_settings_service_chromeos_factory.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/browser/chromeos/settings/device_settings_provider.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "chromeos/constants/chromeos_switches.h"
 #include "chromeos/tpm/install_attributes.h"
 #include "chromeos/tpm/tpm_token_loader.h"
@@ -35,9 +36,6 @@
 #include "components/user_manager/user_manager.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/browser/notification_details.h"
-#include "content/public/browser/notification_service.h"
-#include "content/public/browser/notification_source.h"
 #include "content/public/common/content_switches.h"
 #include "crypto/nss_key_util.h"
 #include "crypto/nss_util.h"
@@ -204,10 +202,6 @@ OwnerSettingsServiceChromeOS::OwnerSettingsServiceChromeOS(
   if (device_settings_service_)
     device_settings_service_->AddObserver(this);
 
-  registrar_.Add(this,
-                 chrome::NOTIFICATION_PROFILE_CREATED,
-                 content::Source<Profile>(profile_));
-
   if (!user_manager::UserManager::IsInitialized()) {
     // interactive_ui_tests does not set user manager.
     waiting_for_easy_unlock_operation_finshed_ = false;
@@ -217,11 +211,18 @@ OwnerSettingsServiceChromeOS::OwnerSettingsServiceChromeOS(
   UserSessionManager::GetInstance()->WaitForEasyUnlockKeyOpsFinished(
       base::Bind(&OwnerSettingsServiceChromeOS::OnEasyUnlockKeyOpsFinished,
                  weak_factory_.GetWeakPtr()));
+  // The ProfileManager may be null in unit tests.
+  if (g_browser_process->profile_manager())
+    g_browser_process->profile_manager()->AddObserver(this);
 }
 
 OwnerSettingsServiceChromeOS::~OwnerSettingsServiceChromeOS() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  // The ProfileManager may be null in unit tests.
+  if (g_browser_process->profile_manager())
+    g_browser_process->profile_manager()->RemoveObserver(this);
+
   if (device_settings_service_)
     device_settings_service_->RemoveObserver(this);
 
@@ -349,20 +350,12 @@ bool OwnerSettingsServiceChromeOS::CommitTentativeDeviceSettings(
   return true;
 }
 
-void OwnerSettingsServiceChromeOS::Observe(
-    int type,
-    const content::NotificationSource& source,
-    const content::NotificationDetails& details) {
+void OwnerSettingsServiceChromeOS::OnProfileAdded(Profile* profile) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK_EQ(chrome::NOTIFICATION_PROFILE_CREATED, type);
-
-  Profile* profile = content::Source<Profile>(source).ptr();
-  if (profile != profile_) {
-    NOTREACHED();
+  if (profile != profile_)
     return;
-  }
 
-  waiting_for_profile_creation_ = false;
+  g_browser_process->profile_manager()->RemoveObserver(this);
   ReloadKeypair();
 }
 
@@ -693,11 +686,17 @@ void OwnerSettingsServiceChromeOS::ReloadKeypairImpl(const base::Callback<
          const scoped_refptr<PrivateKey>& private_key)>& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  if (waiting_for_profile_creation_ || waiting_for_tpm_token_ ||
-      waiting_for_easy_unlock_operation_finshed_) {
+  // The profile may not be fully created yet: abort, and wait till it is. The
+  // ProfileManager may be null in unit tests, in which case we can assume the
+  // profile is valid.
+  if (g_browser_process->profile_manager() &&
+      !g_browser_process->profile_manager()->IsValidProfile(profile_)) {
     return;
   }
 
+  if (waiting_for_tpm_token_ || waiting_for_easy_unlock_operation_finshed_)
+    return;
+
   bool rv = base::PostTask(
       FROM_HERE, {BrowserThread::IO},
       base::BindOnce(&LoadPrivateKeyOnIOThread, owner_key_util_,

chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h

@@ -13,14 +13,13 @@
 #include "base/macros.h"
 #include "base/values.h"
 #include "chrome/browser/chromeos/settings/device_settings_service.h"
+#include "chrome/browser/profiles/profile_manager_observer.h"
 #include "chromeos/dbus/session_manager/session_manager_client.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/ownership/owner_key_util.h"
 #include "components/ownership/owner_settings_service.h"
 #include "components/policy/core/common/cloud/cloud_policy_constants.h"
 #include "components/policy/proto/chrome_device_policy.pb.h"
-#include "content/public/browser/notification_observer.h"
-#include "content/public/browser/notification_registrar.h"
 
 class Profile;
 
@@ -42,7 +41,7 @@ namespace chromeos {
 // TODO (ygorshenin@): move write path for device settings here
 // (crbug.com/230018).
 class OwnerSettingsServiceChromeOS : public ownership::OwnerSettingsService,
-                                     public content::NotificationObserver,
+                                     public ProfileManagerObserver,
                                      public SessionManagerClient::Observer,
                                      public DeviceSettingsService::Observer {
  public:
@@ -78,10 +77,8 @@ class OwnerSettingsServiceChromeOS : public ownership::OwnerSettingsService,
   bool CommitTentativeDeviceSettings(
       std::unique_ptr<enterprise_management::PolicyData> policy) override;
 
-  // NotificationObserver implementation:
-  void Observe(int type,
-               const content::NotificationSource& source,
-               const content::NotificationDetails& details) override;
+  // ProfileManagerObserver:
+  void OnProfileAdded(Profile* profile) override;
 
   // SessionManagerClient::Observer:
   void OwnerKeySet(bool success) override;
@@ -165,9 +162,6 @@ class OwnerSettingsServiceChromeOS : public ownership::OwnerSettingsService,
   // User ID this service instance belongs to.
   std::string user_id_;
 
-  // Whether profile still needs to be initialized.
-  bool waiting_for_profile_creation_ = true;
-
   // Whether TPM token still needs to be initialized.
   bool waiting_for_tpm_token_ = true;
 
@@ -185,8 +179,6 @@ class OwnerSettingsServiceChromeOS : public ownership::OwnerSettingsService,
   std::unique_ptr<enterprise_management::ChromeDeviceSettingsProto>
       tentative_settings_;
 
-  content::NotificationRegistrar registrar_;
-
   base::WeakPtrFactory<OwnerSettingsServiceChromeOS> weak_factory_{this};
 
   base::WeakPtrFactory<OwnerSettingsServiceChromeOS> store_settings_factory_{

chrome/browser/chromeos/ownership/owner_settings_service_chromeos_factory.cc

@@ -37,11 +37,10 @@ DeviceSettingsService* GetDeviceSettingsService() {
 OwnerSettingsServiceChromeOSFactory::OwnerSettingsServiceChromeOSFactory()
     : BrowserContextKeyedServiceFactory(
           "OwnerSettingsService",
-          BrowserContextDependencyManager::GetInstance()) {
-}
+          BrowserContextDependencyManager::GetInstance()) {}
 
-OwnerSettingsServiceChromeOSFactory::~OwnerSettingsServiceChromeOSFactory() {
-}
+OwnerSettingsServiceChromeOSFactory::~OwnerSettingsServiceChromeOSFactory() =
+    default;
 
 // static
 OwnerSettingsServiceChromeOS*
@@ -87,18 +86,29 @@ void OwnerSettingsServiceChromeOSFactory::SetOwnerKeyUtilForTesting(
   owner_key_util_ = owner_key_util;
 }
 
-// static
-KeyedService* OwnerSettingsServiceChromeOSFactory::BuildInstanceFor(
-    content::BrowserContext* browser_context) {
-  Profile* profile = static_cast<Profile*>(browser_context);
-  if (profile->IsGuestSession() || ProfileHelper::IsSigninProfile(profile) ||
+content::BrowserContext*
+OwnerSettingsServiceChromeOSFactory::GetBrowserContextToUse(
+    content::BrowserContext* context) const {
+  Profile* profile = Profile::FromBrowserContext(context);
+  if (profile->IsOffTheRecord() || ProfileHelper::IsSigninProfile(profile) ||
       ProfileHelper::IsLockScreenAppProfile(profile)) {
     return nullptr;
   }
 
+  return context;
+}
+
+bool OwnerSettingsServiceChromeOSFactory::ServiceIsCreatedWithBrowserContext()
+    const {
+  return true;
+}
+
+KeyedService* OwnerSettingsServiceChromeOSFactory::BuildServiceInstanceFor(
+    content::BrowserContext* context) const {
   // If g_stub_cros_settings_provider_for_testing_ is set, we treat the current
   // user as the owner, and write settings directly to the stubbed provider.
   // This is done using the FakeOwnerSettingsService.
+  Profile* profile = Profile::FromBrowserContext(context);
   if (g_stub_cros_settings_provider_for_testing_ != nullptr) {
     return new FakeOwnerSettingsService(
         g_stub_cros_settings_provider_for_testing_, profile,
@@ -111,14 +121,4 @@ KeyedService* OwnerSettingsServiceChromeOSFactory::BuildInstanceFor(
       GetInstance()->GetOwnerKeyUtil());
 }
 
-bool OwnerSettingsServiceChromeOSFactory::ServiceIsCreatedWithBrowserContext()
-    const {
-  return true;
-}
-
-KeyedService* OwnerSettingsServiceChromeOSFactory::BuildServiceInstanceFor(
-    content::BrowserContext* context) const {
-  return BuildInstanceFor(context);
-}
-
 }  // namespace chromeos

chrome/browser/chromeos/ownership/owner_settings_service_chromeos_factory.h

@@ -53,9 +53,9 @@ class OwnerSettingsServiceChromeOSFactory
   OwnerSettingsServiceChromeOSFactory();
   ~OwnerSettingsServiceChromeOSFactory() override;
 
-  static KeyedService* BuildInstanceFor(content::BrowserContext* context);
-
   // BrowserContextKeyedServiceFactory overrides:
+  content::BrowserContext* GetBrowserContextToUse(
+      content::BrowserContext* context) const override;
   bool ServiceIsCreatedWithBrowserContext() const override;
   KeyedService* BuildServiceInstanceFor(
       content::BrowserContext* browser_context) const override;

chrome/browser/chromeos/policy/status_collector/child_status_collector_browsertest.cc

@@ -214,8 +214,6 @@ class BaseChildStatusCollectorTest : public testing::Test {
     TestingChildStatusCollector::RegisterProfilePrefs(
         profile_pref_service_.registry());
 
-    owner_settings_service_.set_ignore_profile_creation_notification(true);
-
     TestingBrowserProcess::GetGlobal()->SetLocalState(&local_state_);
 
     // Use FakeUpdateEngineClient.

chrome/browser/chromeos/policy/status_collector/device_status_collector_browsertest.cc

@@ -421,8 +421,6 @@ class DeviceStatusCollectorTest : public testing::Test {
     TestingDeviceStatusCollector::RegisterProfilePrefs(
         profile_pref_service_.registry());
 
-    owner_settings_service_.set_ignore_profile_creation_notification(true);
-
     // Set up a fake local state for KioskAppManager.
     TestingBrowserProcess::GetGlobal()->SetLocalState(&local_state_);
     chromeos::KioskAppManager::RegisterPrefs(local_state_.registry());

chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_apitest_chromeos.cc

@@ -39,19 +39,14 @@ class BluetoothLowEnergyApiTestChromeOs : public PlatformAppBrowserTest {
   }
 
   void TearDownOnMainThread() override {
-    PlatformAppBrowserTest::TearDownOnMainThread();
+    owner_settings_service_.reset();
     settings_helper_.RestoreRealDeviceSettingsProvider();
+    PlatformAppBrowserTest::TearDownOnMainThread();
     user_manager_enabler_.reset();
     fake_user_manager_ = nullptr;
   }
 
  protected:
-  chromeos::FakeChromeUserManager* fake_user_manager_;
-  std::unique_ptr<user_manager::ScopedUserManager> user_manager_enabler_;
-
-  chromeos::ScopedCrosSettingsTestHelper settings_helper_;
-  std::unique_ptr<chromeos::FakeOwnerSettingsService> owner_settings_service_;
-
   void EnterKioskSession() {
     fake_user_manager_ = new chromeos::FakeChromeUserManager();
     user_manager_enabler_ = std::make_unique<user_manager::ScopedUserManager>(
@@ -72,6 +67,12 @@ class BluetoothLowEnergyApiTestChromeOs : public PlatformAppBrowserTest {
   chromeos::KioskAppManager* manager() const {
     return chromeos::KioskAppManager::Get();
   }
+
+  chromeos::FakeChromeUserManager* fake_user_manager_;
+  std::unique_ptr<user_manager::ScopedUserManager> user_manager_enabler_;
+
+  chromeos::ScopedCrosSettingsTestHelper settings_helper_;
+  std::unique_ptr<chromeos::FakeOwnerSettingsService> owner_settings_service_;
 };
 
 IN_PROC_BROWSER_TEST_F(BluetoothLowEnergyApiTestChromeOs,