Destroy CastDeviceListHost during KeyedServices shutdown

This makes MediaNotificationService destroy all the CastDeviceListHosts
that it's instantiated in its KeyedService shutdown. This is necessary
because CastDeviceListHost depends on MediaRouter, another KeyedService.

Bug: 1457757
Change-Id: I453279da77b141ad9cd89310fc8128cc7d2919f2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4672319
Reviewed-by: Tommy Steimel <steimel@chromium.org>
Commit-Queue: Takumi Fujimoto <takumif@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1168361}

chrome/browser/ui/global_media_controls/cast_device_list_host.cc

@@ -88,7 +88,8 @@ CastDeviceListHost::CastDeviceListHost(
     : cast_controller_(std::move(dialog_controller)),
       client_(std::move(client)),
       media_remoting_callback_(std::move(media_remoting_callback)),
-      hide_dialog_callback_(std::move(hide_dialog_callback)) {
+      hide_dialog_callback_(std::move(hide_dialog_callback)),
+      id_(next_id_++) {
   cast_controller_->AddObserver(this);
   cast_controller_->RegisterDestructor(
       base::BindOnce(&CastDeviceListHost::DestroyCastController,
@@ -181,3 +182,5 @@ void CastDeviceListHost::StartCasting(const media_router::UIMediaSink& sink) {
 void CastDeviceListHost::DestroyCastController() {
   cast_controller_.reset();
 }
+
+int CastDeviceListHost::next_id_ = 0;

chrome/browser/ui/global_media_controls/cast_device_list_host.h

@@ -36,17 +36,23 @@ class CastDeviceListHost : public global_media_controls::mojom::DeviceListHost,
   void OnModelUpdated(const media_router::CastDialogModel& model) override;
   void OnCastingStarted() override;
 
+  int id() const { return id_; }
+
  private:
   void StartCasting(const media_router::UIMediaSink& sink);
   void DestroyCastController();
 
+  // Used to generate `id_`.
+  static int next_id_;
+
   std::unique_ptr<media_router::CastDialogController> cast_controller_;
   mojo::Remote<global_media_controls::mojom::DeviceListClient> client_;
   std::vector<media_router::UIMediaSink> sinks_;
   // Called whenever a Media Remoting session is starting.
   MediaRemotingCallback media_remoting_callback_;
   // Called whenever a tab mirroring session starts.
   base::RepeatingClosure hide_dialog_callback_;
+  const int id_;
 };
 
 #endif  // CHROME_BROWSER_UI_GLOBAL_MEDIA_CONTROLS_CAST_DEVICE_LIST_HOST_H_

chrome/browser/ui/global_media_controls/cast_device_list_host_unittest.cc

@@ -22,6 +22,9 @@ using media_router::UIMediaSinkState;
 using testing::_;
 using testing::NiceMock;
 using testing::Return;
+namespace mojom {
+using global_media_controls::mojom::DeviceListClient;
+}  // namespace mojom
 
 namespace {
 
@@ -72,24 +75,29 @@ class CastDeviceListHostTest : public testing::Test {
     testing::Test::SetUp();
     auto dialog_controller = std::make_unique<MockCastDialogController>();
     dialog_controller_ = dialog_controller.get();
-    host_ = std::make_unique<CastDeviceListHost>(
+    host_ = CreateHost(std::move(dialog_controller), &client_receiver_);
+  }
+
+  MOCK_METHOD(void, OnMediaRemotingRequested, ());
+  MOCK_METHOD(void, HideMediaDialog, ());
+
+ protected:
+  std::unique_ptr<CastDeviceListHost> CreateHost(
+      std::unique_ptr<media_router::CastDialogController> dialog_controller,
+      mojo::PendingReceiver<mojom::DeviceListClient>* client_receiver) {
+    return std::make_unique<CastDeviceListHost>(
         std::move(dialog_controller),
-        client_receiver_.InitWithNewPipeAndPassRemote(),
+        client_receiver->InitWithNewPipeAndPassRemote(),
         base::BindRepeating(&CastDeviceListHostTest::OnMediaRemotingRequested,
                             base::Unretained(this)),
         base::BindRepeating(&CastDeviceListHostTest::HideMediaDialog,
                             base::Unretained(this)));
   }
 
-  MOCK_METHOD(void, OnMediaRemotingRequested, ());
-  MOCK_METHOD(void, HideMediaDialog, ());
-
- protected:
   content::BrowserTaskEnvironment task_environment_;
   std::unique_ptr<CastDeviceListHost> host_;
   raw_ptr<MockCastDialogController> dialog_controller_ = nullptr;
-  mojo::PendingReceiver<global_media_controls::mojom::DeviceListClient>
-      client_receiver_;
+  mojo::PendingReceiver<mojom::DeviceListClient> client_receiver_;
 };
 
 TEST_F(CastDeviceListHostTest, StartPresentation) {
@@ -183,3 +191,11 @@ TEST_F(CastDeviceListHostTest, SelectingDeviceClearsIssue) {
   EXPECT_CALL(*dialog_controller_, ClearIssue(issue.id()));
   host_->SelectDevice(sink.id);
 }
+
+TEST_F(CastDeviceListHostTest, GetId) {
+  mojo::PendingReceiver<mojom::DeviceListClient> client_receiver;
+  std::unique_ptr<CastDeviceListHost> host2 = CreateHost(
+      std::make_unique<MockCastDialogController>(), &client_receiver);
+  // IDs should be unique.
+  EXPECT_NE(host_->id(), host2->id());
+}

chrome/browser/ui/global_media_controls/media_notification_service.cc

@@ -7,7 +7,6 @@
 #include <memory>
 
 #include "base/callback_list.h"
-#include "base/metrics/field_trial_params.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/ranges/algorithm.h"
 #include "base/unguessable_token.h"
@@ -25,7 +24,6 @@
 #include "components/global_media_controls/public/media_dialog_delegate.h"
 #include "components/global_media_controls/public/media_item_manager.h"
 #include "components/global_media_controls/public/media_item_producer.h"
-#include "components/global_media_controls/public/media_item_ui.h"
 #include "components/media_message_center/media_notification_item.h"
 #include "components/media_router/browser/media_router_factory.h"
 #include "components/media_router/browser/presentation/start_presentation_context.h"
@@ -241,14 +239,21 @@ MediaNotificationService::~MediaNotificationService() {
 }
 
 void MediaNotificationService::Shutdown() {
-  // `cast_notification_producer_` and
-  // `presentation_request_notification_producer_` depend on MediaRouter,
-  // which is another keyed service. So they must be destroyed here.
+  shutdown_has_started_ = true;
+  // `cast_notification_producer_`,
+  // `presentation_request_notification_producer_` and `host_receivers_`
+  // depend on MediaRouter, which is another keyed service. So they must be
+  // destroyed here.
   if (cast_notification_producer_) {
     item_manager_->RemoveItemProducer(cast_notification_producer_.get());
   }
   cast_notification_producer_.reset();
   presentation_request_notification_producer_.reset();
+  for (const auto& host : host_receivers_) {
+    if (host.second) {
+      host.second->Close();
+    }
+  }
 }
 
 void MediaNotificationService::OnAudioSinkChosen(const std::string& item_id,
@@ -471,7 +476,7 @@ MediaNotificationService::CreateCastDialogControllerForPresentationRequest() {
 
 void MediaNotificationService::CreateCastDeviceListHost(
     std::unique_ptr<media_router::CastDialogController> dialog_controller,
-    mojo::PendingReceiver<mojom::DeviceListHost> host_receiver,
+    mojo::PendingReceiver<mojom::DeviceListHost> host_pending_receiver,
     mojo::PendingRemote<mojom::DeviceListClient> client_remote,
     absl::optional<std::string> session_id) {
   if (!dialog_controller) {
@@ -485,14 +490,19 @@ void MediaNotificationService::CreateCastDeviceListHost(
                 &MediaNotificationService::OnMediaRemotingRequested,
                 weak_ptr_factory_.GetWeakPtr(), session_id.value())
           : base::DoNothing();
-  mojo::MakeSelfOwnedReceiver(
-      std::make_unique<CastDeviceListHost>(
-          std::move(dialog_controller), std::move(client_remote),
-          std::move(media_remoting_callback_),
-          base::BindRepeating(
-              &global_media_controls::MediaItemManager::HideDialog,
-              item_manager_->GetWeakPtr())),
-      std::move(host_receiver));
+  auto host = std::make_unique<CastDeviceListHost>(
+      std::move(dialog_controller), std::move(client_remote),
+      std::move(media_remoting_callback_),
+      base::BindRepeating(&global_media_controls::MediaItemManager::HideDialog,
+                          item_manager_->GetWeakPtr()));
+  int host_id = host->id();
+  mojo::SelfOwnedReceiverRef<global_media_controls::mojom::DeviceListHost>
+      host_receiver = mojo::MakeSelfOwnedReceiver(
+          std::move(host), std::move(host_pending_receiver));
+  host_receiver->set_connection_error_handler(
+      base::BindOnce(&MediaNotificationService::RemoveDeviceListHost,
+                     weak_ptr_factory_.GetWeakPtr(), host_id));
+  host_receivers_.emplace(host_id, std::move(host_receiver));
 }
 
 void MediaNotificationService::set_device_provider_for_testing(
@@ -555,3 +565,12 @@ MediaNotificationService::GetActiveControllableSessionForWebContents(
   }
   return "";
 }
+
+void MediaNotificationService::RemoveDeviceListHost(int host_id) {
+  // If shutdown has started, then we may currently be iterating through
+  // `host_receivers_` so we should not erase from it. `host_receivers_` will
+  // get destroyed soon anyways.
+  if (!shutdown_has_started_) {
+    host_receivers_.erase(host_id);
+  }
+}

chrome/browser/ui/global_media_controls/media_notification_service.h

@@ -24,6 +24,7 @@
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/receiver.h"
+#include "mojo/public/cpp/bindings/self_owned_receiver.h"
 #include "services/metrics/public/cpp/ukm_source_id.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
 
@@ -157,6 +158,8 @@ class MediaNotificationService
   std::string GetActiveControllableSessionForWebContents(
       content::WebContents* web_contents) const;
 
+  void RemoveDeviceListHost(int host);
+
   const raw_ptr<Profile> profile_;
 
   std::unique_ptr<global_media_controls::MediaItemManager> item_manager_;
@@ -181,6 +184,14 @@ class MediaNotificationService
 
   mojo::Receiver<global_media_controls::mojom::DeviceService> receiver_;
 
+  // Maps from hosts' IDs to hosts.
+  std::map<
+      int,
+      mojo::SelfOwnedReceiverRef<global_media_controls::mojom::DeviceListHost>>
+      host_receivers_;
+
+  bool shutdown_has_started_ = false;
+
   base::WeakPtrFactory<MediaNotificationService> weak_ptr_factory_{this};
 };
 

chrome/browser/ui/global_media_controls/media_notification_service_unittest.cc

@@ -556,7 +556,8 @@ TEST_F(MediaNotificationServiceCastTest, GetDeviceListHostForSession) {
   service()->GetDeviceListHostForSession(
       id.ToString(), TakeReceiverAndExpectNoDisconnect(host_remote),
       TakeRemoteAndExpectNoDisconnect(client.receiver()));
-  base::RunLoop().RunUntilIdle();
+  host_remote.FlushForTesting();
+  client.receiver().FlushForTesting();
 }
 
 TEST_F(MediaNotificationServiceCastTest,
@@ -566,7 +567,8 @@ TEST_F(MediaNotificationServiceCastTest,
   service()->GetDeviceListHostForSession(
       "invalid_id", TakeReceiverAndExpectDisconnect(host_remote),
       TakeRemoteAndExpectDisconnect(client.receiver()));
-  base::RunLoop().RunUntilIdle();
+  host_remote.FlushForTesting();
+  client.receiver().FlushForTesting();
 }
 
 TEST_F(MediaNotificationServiceCastTest, GetDeviceListHostForPresentation) {
@@ -577,7 +579,8 @@ TEST_F(MediaNotificationServiceCastTest, GetDeviceListHostForPresentation) {
   service()->GetDeviceListHostForPresentation(
       TakeReceiverAndExpectNoDisconnect(host_remote),
       TakeRemoteAndExpectNoDisconnect(client.receiver()));
-  base::RunLoop().RunUntilIdle();
+  host_remote.FlushForTesting();
+  client.receiver().FlushForTesting();
 }
 
 TEST_F(MediaNotificationServiceCastTest,
@@ -587,7 +590,23 @@ TEST_F(MediaNotificationServiceCastTest,
   service()->GetDeviceListHostForPresentation(
       TakeReceiverAndExpectDisconnect(host_remote),
       TakeRemoteAndExpectDisconnect(client.receiver()));
-  base::RunLoop().RunUntilIdle();
+  host_remote.FlushForTesting();
+  client.receiver().FlushForTesting();
+}
+
+TEST_F(MediaNotificationServiceCastTest, DeleteDeviceListHostOnShutdown) {
+  mojo::Remote<mojom::DeviceListHost> host_remote;
+  MockDeviceListClient client;
+  auto id = SimulatePlayingControllableMediaForWebContents(web_contents());
+  service()->GetDeviceListHostForSession(
+      id.ToString(), TakeReceiverAndExpectDisconnect(host_remote),
+      TakeRemoteAndExpectDisconnect(client.receiver()));
+
+  // Shutdown() should cause a disconnect, fulfilling the expectations in
+  // TakeReceiver...() and TakeRemote...() above.
+  service()->Shutdown();
+  host_remote.FlushForTesting();
+  client.receiver().FlushForTesting();
 }
 
 TEST_F(MediaNotificationServiceCastTest,