Convert VmCameraMicManager from a KeyedService to a singleton

MediaClientImpl needs to register itself as an observer of
VmCameraMicManager. Depending on whether the user has customized a flag
or not, MediaClientImpl is either constructed before or after
`UserSessionInitializer::OnUserSessionStarted(/*is_primary_user*/=true)`
is run. The reason seems to be that 1) MediaClientImpl is constructed in
a task queue; 2) Customizing a flag causes the browser to be restarted
after the user signs in, which changes the execution characteristics.

My last attempt (CL 2586286) to fix the registration does not work if
the user has customized flags. So, this CL makes VmCameraMicManager
a singleton which is always available so that MediaClientImpl can always
reliably do the registration in its ctor.

Bug: b/167491603
Change-Id: I2b73b59216851f0b8b2533120317e54c98382b45
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2589061
Commit-Queue: Jason Lin <lxj@google.com>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#836941}

chrome/browser/chromeos/BUILD.gn

@@ -916,8 +916,6 @@ source_set("chromeos") {
     "camera_detector.h",
     "camera_mic/vm_camera_mic_manager.cc",
     "camera_mic/vm_camera_mic_manager.h",
-    "camera_mic/vm_camera_mic_manager_factory.cc",
-    "camera_mic/vm_camera_mic_manager_factory.h",
     "camera_presence_notifier.cc",
     "camera_presence_notifier.h",
     "cert_provisioning/cert_provisioning_common.cc",

chrome/browser/chromeos/camera_mic/vm_camera_mic_manager.cc

@@ -10,12 +10,12 @@
 #include "ash/public/cpp/vm_camera_mic_constants.h"
 #include "base/bind.h"
 #include "base/feature_list.h"
+#include "base/no_destructor.h"
 #include "base/notreached.h"
 #include "base/strings/string16.h"
 #include "base/system/sys_info.h"
 #include "base/time/time.h"
 #include "chrome/app/vector_icons/vector_icons.h"
-#include "chrome/browser/chromeos/camera_mic/vm_camera_mic_manager_factory.h"
 #include "chrome/browser/chromeos/plugin_vm/plugin_vm_util.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/notifications/notification_display_service.h"
@@ -64,26 +64,26 @@ constexpr VmCameraMicManager::NotificationType
 constexpr VmCameraMicManager::NotificationType
     VmCameraMicManager::kCameraWithMicNotification;
 
-VmCameraMicManager* VmCameraMicManager::GetForProfile(Profile* profile) {
-  return VmCameraMicManagerFactory::GetForProfile(profile);
+VmCameraMicManager* VmCameraMicManager::Get() {
+  static base::NoDestructor<VmCameraMicManager> manager;
+  return manager.get();
 }
 
-VmCameraMicManager::VmCameraMicManager(Profile* profile)
-    : profile_(profile),
-      crostini_vm_notification_observer_(
-          profile,
-          base::BindRepeating(OpenCrostiniSettings)),
-      plugin_vm_notification_observer_(
-          profile,
-          base::BindRepeating(OpenPluginVmSettings)),
-      observer_timer_(
+VmCameraMicManager::VmCameraMicManager()
+    : observer_timer_(
           FROM_HERE,
           kObserverTimerDelay,
           base::BindRepeating(&VmCameraMicManager::NotifyActiveChanged,
                               // Unretained because the timer cannot
                               // live longer than the manager.
-                              base::Unretained(this))) {
-  DCHECK(ProfileHelper::IsPrimaryProfile(profile));
+                              base::Unretained(this))) {}
+
+void VmCameraMicManager::OnPrimaryUserSessionStarted(Profile* primary_profile) {
+  primary_profile_ = primary_profile;
+  crostini_vm_notification_observer_.Initialize(
+      primary_profile_, base::BindRepeating(OpenCrostiniSettings));
+  plugin_vm_notification_observer_.Initialize(
+      primary_profile_, base::BindRepeating(OpenPluginVmSettings));
 
   for (VmType vm : {VmType::kCrostiniVm, VmType::kPluginVm}) {
     notification_map_[vm] = {};
@@ -95,10 +95,13 @@ VmCameraMicManager::VmCameraMicManager(Profile* profile)
       media::ShouldUseCrosCameraService()) {
     // OnActiveClientChange() will be called automatically after the
     // subscription, so there is no need to get the current status here.
-    camera_observation_.Observe(media::CameraHalDispatcherImpl::GetInstance());
+    media::CameraHalDispatcherImpl::GetInstance()->AddActiveClientObserver(
+        this);
   }
 }
 
+// The class is supposed to be used as a singleton with `base::NoDestructor`, so
+// we do not do clean up (e.g. deregister as observers) here.
 VmCameraMicManager::~VmCameraMicManager() = default;
 
 void VmCameraMicManager::SetActive(VmType vm, DeviceType device, bool active) {
@@ -164,9 +167,8 @@ void VmCameraMicManager::OnActiveClientChange(
       type == cros::mojom::CameraClientType::PLUGINVM) {
     content::GetUIThreadTaskRunner({})->PostTask(
         FROM_HERE,
-        base::BindOnce(&VmCameraMicManager::SetActive,
-                       weak_ptr_factory_.GetWeakPtr(), VmType::kPluginVm,
-                       DeviceType::kCamera, is_active));
+        base::BindOnce(&VmCameraMicManager::SetActive, base::Unretained(this),
+                       VmType::kPluginVm, DeviceType::kCamera, is_active));
   }
 }
 
@@ -264,9 +266,9 @@ void VmCameraMicManager::OpenNotification(VmType vm, NotificationType type) {
       base::MakeRefCounted<message_center::ThunkNotificationDelegate>(
           std::move(notification_observer_)));
 
-  NotificationDisplayService::GetForProfile(profile_)->Display(
-      NotificationHandler::Type::TRANSIENT, notification,
-      /*metadata=*/nullptr);
+  NotificationDisplayService::GetForProfile(primary_profile_)
+      ->Display(NotificationHandler::Type::TRANSIENT, notification,
+                /*metadata=*/nullptr);
 }
 
 void VmCameraMicManager::CloseNotification(VmType vm, NotificationType type) {
@@ -275,17 +277,21 @@ void VmCameraMicManager::CloseNotification(VmType vm, NotificationType type) {
           features::kVmCameraMicIndicatorsAndNotifications)) {
     return;
   }
-  NotificationDisplayService::GetForProfile(profile_)->Close(
-      NotificationHandler::Type::TRANSIENT, GetNotificationId(vm, type));
+  NotificationDisplayService::GetForProfile(primary_profile_)
+      ->Close(NotificationHandler::Type::TRANSIENT,
+              GetNotificationId(vm, type));
 }
 
-VmCameraMicManager::VmNotificationObserver::VmNotificationObserver(
-    Profile* profile,
-    OpenSettingsFunction open_settings)
-    : profile_{profile}, open_settings_{open_settings} {}
-
+VmCameraMicManager::VmNotificationObserver::VmNotificationObserver() = default;
 VmCameraMicManager::VmNotificationObserver::~VmNotificationObserver() = default;
 
+void VmCameraMicManager::VmNotificationObserver::Initialize(
+    Profile* profile,
+    OpenSettingsFunction open_settings) {
+  profile_ = profile;
+  open_settings_ = std::move(open_settings);
+}
+
 base::WeakPtr<message_center::NotificationObserver>
 VmCameraMicManager::VmNotificationObserver::GetWeakPtr() {
   return weak_ptr_factory_.GetWeakPtr();

chrome/browser/chromeos/camera_mic/vm_camera_mic_manager.h

@@ -25,12 +25,12 @@ class Profile;
 namespace chromeos {
 
 // This class manages camera/mic access (and the access notifications) for VMs
-// (crostini and parallels for now). Like all the VMs, it is only available for
-// the primary and non-incognito profile. We might need to change this if we
-// extend this class to support the browser, in which case we will also need to
-// make the notification ids different for different profiles.
-class VmCameraMicManager : public KeyedService,
-                           public media::CameraActiveClientObserver {
+// (crostini and parallels for now). All of the notifications are sent to the
+// primary profile since all VMs support only the primary profile. We might need
+// to change this if we extend this class to support the browser, in which case
+// we will also need to make the notification ids different for different
+// profiles.
+class VmCameraMicManager : public media::CameraActiveClientObserver {
  public:
   enum class VmType { kCrostiniVm, kPluginVm };
 
@@ -45,12 +45,13 @@ class VmCameraMicManager : public KeyedService,
     virtual void OnVmCameraMicActiveChanged(VmCameraMicManager*) {}
   };
 
-  // Return nullptr if the profile is non-primary or incognito.
-  static VmCameraMicManager* GetForProfile(Profile* profile);
+  static VmCameraMicManager* Get();
 
-  explicit VmCameraMicManager(Profile* profile);
+  VmCameraMicManager();
   ~VmCameraMicManager() override;
 
+  void OnPrimaryUserSessionStarted(Profile* primary_profile);
+
   VmCameraMicManager(const VmCameraMicManager&) = delete;
   VmCameraMicManager& operator=(const VmCameraMicManager&) = delete;
 
@@ -88,10 +89,11 @@ class VmCameraMicManager : public KeyedService,
    public:
     using OpenSettingsFunction = base::RepeatingCallback<void(Profile*)>;
 
-    VmNotificationObserver(Profile* profile,
-                           OpenSettingsFunction open_settings);
+    VmNotificationObserver();
     ~VmNotificationObserver();
 
+    void Initialize(Profile* profile, OpenSettingsFunction open_settings);
+
     base::WeakPtr<NotificationObserver> GetWeakPtr();
 
     // message_center::NotificationObserver:
@@ -99,7 +101,7 @@ class VmCameraMicManager : public KeyedService,
                const base::Optional<base::string16>& reply) override;
 
    private:
-    Profile* const profile_;
+    Profile* profile_ = nullptr;
     OpenSettingsFunction open_settings_;
     base::WeakPtrFactory<VmNotificationObserver> weak_ptr_factory_{this};
   };
@@ -116,22 +118,13 @@ class VmCameraMicManager : public KeyedService,
   void OpenNotification(VmType vm, NotificationType type);
   void CloseNotification(VmType vm, NotificationType type);
 
-  Profile* const profile_;
+  Profile* primary_profile_ = nullptr;
   VmNotificationObserver crostini_vm_notification_observer_;
   VmNotificationObserver plugin_vm_notification_observer_;
   NotificationMap notification_map_;
 
   base::RetainingOneShotTimer observer_timer_;
   base::ObserverList<Observer> observers_;
-
-  base::ScopedObservation<
-      media::CameraHalDispatcherImpl,
-      media::CameraActiveClientObserver,
-      &media::CameraHalDispatcherImpl::AddActiveClientObserver,
-      &media::CameraHalDispatcherImpl::RemoveActiveClientObserver>
-      camera_observation_{this};
-
-  base::WeakPtrFactory<VmCameraMicManager> weak_ptr_factory_{this};
 };
 
 }  // namespace chromeos

chrome/browser/chromeos/camera_mic/vm_camera_mic_manager_factory.cc

-// Copyright 2020 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "chrome/browser/chromeos/camera_mic/vm_camera_mic_manager_factory.h"
-
-#include "chrome/browser/chromeos/camera_mic/vm_camera_mic_manager.h"
-#include "chrome/browser/chromeos/profiles/profile_helper.h"
-#include "chrome/browser/profiles/profile.h"
-#include "components/keyed_service/content/browser_context_dependency_manager.h"
-
-namespace chromeos {
-
-VmCameraMicManager* VmCameraMicManagerFactory::GetForProfile(Profile* profile) {
-  return static_cast<VmCameraMicManager*>(
-      GetInstance()->GetServiceForBrowserContext(profile, true));
-}
-
-VmCameraMicManagerFactory* VmCameraMicManagerFactory::GetInstance() {
-  static base::NoDestructor<VmCameraMicManagerFactory> factory;
-  return factory.get();
-}
-
-VmCameraMicManagerFactory::VmCameraMicManagerFactory()
-    : BrowserContextKeyedServiceFactory(
-          "VmCameraMicManager",
-          BrowserContextDependencyManager::GetInstance()) {}
-
-VmCameraMicManagerFactory::~VmCameraMicManagerFactory() = default;
-
-// BrowserContextKeyedServiceFactory:
-KeyedService* VmCameraMicManagerFactory::BuildServiceInstanceFor(
-    content::BrowserContext* context) const {
-  Profile* profile = Profile::FromBrowserContext(context);
-  if (ProfileHelper::IsPrimaryProfile(profile))
-    return new VmCameraMicManager(profile);
-  return nullptr;
-}
-
-}  // namespace chromeos

chrome/browser/chromeos/camera_mic/vm_camera_mic_manager_factory.h

-// Copyright 2020 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef CHROME_BROWSER_CHROMEOS_CAMERA_MIC_VM_CAMERA_MIC_MANAGER_FACTORY_H_
-#define CHROME_BROWSER_CHROMEOS_CAMERA_MIC_VM_CAMERA_MIC_MANAGER_FACTORY_H_
-
-#include "base/no_destructor.h"
-#include "components/keyed_service/content/browser_context_keyed_service_factory.h"
-
-class Profile;
-
-namespace chromeos {
-
-class VmCameraMicManager;
-
-class VmCameraMicManagerFactory : public BrowserContextKeyedServiceFactory {
- public:
-  static VmCameraMicManager* GetForProfile(Profile* profile);
-
-  static VmCameraMicManagerFactory* GetInstance();
-
- private:
-  friend class base::NoDestructor<VmCameraMicManagerFactory>;
-
-  VmCameraMicManagerFactory();
-
-  ~VmCameraMicManagerFactory() override;
-
-  // BrowserContextKeyedServiceFactory:
-  KeyedService* BuildServiceInstanceFor(
-      content::BrowserContext* context) const override;
-};
-
-}  // namespace chromeos
-
-#endif  // CHROME_BROWSER_CHROMEOS_CAMERA_MIC_VM_CAMERA_MIC_MANAGER_FACTORY_H_

chrome/browser/chromeos/camera_mic/vm_camera_mic_manager_unittest.cc

@@ -12,7 +12,6 @@
 #include "base/bind.h"
 #include "base/containers/flat_map.h"
 #include "base/test/scoped_feature_list.h"
-#include "chrome/browser/chromeos/camera_mic/vm_camera_mic_manager_factory.h"
 #include "chrome/browser/chromeos/login/users/mock_user_manager.h"
 #include "chrome/browser/notifications/notification_display_service.h"
 #include "chrome/browser/notifications/notification_display_service_factory.h"
@@ -124,8 +123,8 @@ class VmCameraMicManagerTest : public testing::Test {
                   return std::make_unique<FakeNotificationDisplayService>();
                 })));
 
-    vm_camera_mic_manager_ =
-        VmCameraMicManagerFactory::GetForProfile(&testing_profile_);
+    vm_camera_mic_manager_ = std::make_unique<VmCameraMicManager>();
+    vm_camera_mic_manager_->OnPrimaryUserSessionStarted(&testing_profile_);
   }
 
   void SetActive(const ActiveMap& active_map) {
@@ -145,7 +144,7 @@ class VmCameraMicManagerTest : public testing::Test {
   base::test::ScopedFeatureList scoped_feature_list_;
 
   FakeNotificationDisplayService* fake_display_service_;
-  VmCameraMicManager* vm_camera_mic_manager_;
+  std::unique_ptr<VmCameraMicManager> vm_camera_mic_manager_;
 
   DISALLOW_COPY_AND_ASSIGN(VmCameraMicManagerTest);
 };

chrome/browser/chromeos/login/session/user_session_initializer.cc

@@ -13,7 +13,7 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browser_process_platform_part_chromeos.h"
 #include "chrome/browser/chromeos/arc/session/arc_service_launcher.h"
-#include "chrome/browser/chromeos/camera_mic/vm_camera_mic_manager_factory.h"
+#include "chrome/browser/chromeos/camera_mic/vm_camera_mic_manager.h"
 #include "chrome/browser/chromeos/child_accounts/child_status_reporting_service_factory.h"
 #include "chrome/browser/chromeos/child_accounts/child_user_service_factory.h"
 #include "chrome/browser/chromeos/child_accounts/family_user_metrics_service_factory.h"
@@ -213,11 +213,7 @@ void UserSessionInitializer::OnUserSessionStarted(bool is_primary_user) {
     if (plugin_vm_manager)
       plugin_vm_manager->OnPrimaryUserSessionStarted();
 
-    VmCameraMicManagerFactory::GetForProfile(primary_profile_);
-
-    auto* media_client_impl = MediaClientImpl::Get();
-    if (media_client_impl)
-      media_client_impl->OnPrimaryUserSessionStarted(primary_profile_);
+    VmCameraMicManager::Get()->OnPrimaryUserSessionStarted(primary_profile_);
   }
 }
 

chrome/browser/ui/ash/media_client_impl.cc

@@ -141,6 +141,8 @@ MediaClientImpl::MediaClientImpl() {
   MediaCaptureDevicesDispatcher::GetInstance()->AddObserver(this);
   BrowserList::AddObserver(this);
 
+  chromeos::VmCameraMicManager::Get()->AddObserver(this);
+
   DCHECK(!g_media_client);
   g_media_client = this;
 }
@@ -154,10 +156,7 @@ MediaClientImpl::~MediaClientImpl() {
   MediaCaptureDevicesDispatcher::GetInstance()->RemoveObserver(this);
   BrowserList::RemoveObserver(this);
 
-  auto* vm_camera_mic_manager = chromeos::VmCameraMicManager::GetForProfile(
-      ProfileManager::GetPrimaryUserProfile());
-  if (vm_camera_mic_manager)
-    vm_camera_mic_manager->RemoveObserver(this);
+  chromeos::VmCameraMicManager::Get()->RemoveObserver(this);
 }
 
 // static
@@ -179,18 +178,6 @@ void MediaClientImpl::InitForTesting(ash::MediaController* controller) {
   media_controller_->SetClient(this);
 }
 
-void MediaClientImpl::OnPrimaryUserSessionStarted(Profile* primary_profile) {
-  // Skip when not on real device (i.e. tests and emulator on dev box). The
-  // reason is that some tests cause this function to be run more than once,
-  // which results in a crash.
-  if (base::SysInfo::IsRunningOnChromeOS()) {
-    auto* vm_camera_mic_manager =
-        chromeos::VmCameraMicManager::GetForProfile(primary_profile);
-    if (vm_camera_mic_manager)
-      vm_camera_mic_manager->AddObserver(this);
-  }
-}
-
 void MediaClientImpl::HandleMediaNextTrack() {
   HandleMediaAction(ui::VKEY_MEDIA_NEXT_TRACK);
 }

chrome/browser/ui/ash/media_client_impl.h

@@ -29,8 +29,6 @@ class MediaClientImpl : public ash::MediaClient,
   // Tests can provide a mock mojo interface for the ash controller.
   void InitForTesting(ash::MediaController* controller);
 
-  void OnPrimaryUserSessionStarted(Profile* primary_prifle);
-
   // Returns a pointer to the singleton MediaClient, or nullptr if none exists.
   static MediaClientImpl* Get();