From 8c09594f13179278e04d4a558cf90c12725db66c Mon Sep 17 00:00:00 2001
From: Yuwei Huang <[email protected]>
Date: Thu, 23 Jul 2026 17:44:10 -0700
Subject: [PATCH] [remoting] Use COM ref-counting for audio device change detector

DefaultAudioDeviceChangeDetector implements IMMNotificationClient but
stubbed AddRef()/Release() and was owned by a unique_ptr, so system
references could not keep it alive while notifications were pending
after AudioCapturerWin released it.

Convert the class to a Microsoft::WRL::RuntimeClass with ClassicCom
flags so it gets standard COM reference counting, hold it via ComPtr in
AudioCapturerWin, and move unregistration to an explicit Unregister()
method that the owner calls before releasing its reference. Add unit
tests covering registration, GetAndReset, and late notification safety.

Bug: 515470739
Change-Id: I8b249149e6e54b85daf058d675bca29082e6d416
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8140581
Commit-Queue: Yuwei Huang <[email protected]>
Auto-Submit: Yuwei Huang <[email protected]>
Reviewed-by: Joe Downing <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1667543}
---
diff --git a/remoting/host/audio_capturer_win.cc b/remoting/host/audio_capturer_win.cc
index 252412b2..2a17bdea 100644
--- a/remoting/host/audio_capturer_win.cc
+++ b/remoting/host/audio_capturer_win.cc
@@ -87,7 +87,10 @@
 void AudioCapturerWin::Deinitialize() {
   DCHECK(thread_checker_.CalledOnValidThread());
   wave_format_ex_.Reset(nullptr);
-  default_device_detector_.reset();
+  if (default_device_detector_) {
+    default_device_detector_->Unregister();
+  }
+  default_device_detector_.Reset();
   audio_capture_client_.Reset();
   if (audio_client_) {
     audio_client_->Stop();
@@ -113,7 +116,8 @@
   }
 
   default_device_detector_ =
-      std::make_unique<DefaultAudioDeviceChangeDetector>(mm_device_enumerator);
+      Microsoft::WRL::Make<DefaultAudioDeviceChangeDetector>(
+          mm_device_enumerator);
 
   // Get the audio endpoint.
   hr = mm_device_enumerator->GetDefaultAudioEndpoint(eRender, eConsole,
diff --git a/remoting/host/audio_capturer_win.h b/remoting/host/audio_capturer_win.h
index 168b922..396b2b5d 100644
--- a/remoting/host/audio_capturer_win.h
+++ b/remoting/host/audio_capturer_win.h
@@ -77,7 +77,8 @@
   Microsoft::WRL::ComPtr<IAudioClient> audio_client_;
   Microsoft::WRL::ComPtr<IMMDevice> mm_device_;
 
-  std::unique_ptr<DefaultAudioDeviceChangeDetector> default_device_detector_;
+  Microsoft::WRL::ComPtr<DefaultAudioDeviceChangeDetector>
+      default_device_detector_;
 
   HRESULT last_capture_error_;
 
diff --git a/remoting/host/win/default_audio_device_change_detector.cc b/remoting/host/win/default_audio_device_change_detector.cc
index dbd76f5..4115415 100644
--- a/remoting/host/win/default_audio_device_change_detector.cc
+++ b/remoting/host/win/default_audio_device_change_detector.cc
@@ -4,8 +4,7 @@
 
 #include "remoting/host/win/default_audio_device_change_detector.h"
 
-#include <unknwn.h>
-
+#include "base/check.h"
 #include "base/logging.h"
 
 namespace remoting {
@@ -21,10 +20,18 @@
     LOG(WARNING) << "Failed to register IMMNotificationClient, we may not be "
                     "able to detect the new default audio device. Error "
                  << hr;
+    return;
   }
+  registered_ = true;
 }
 
-DefaultAudioDeviceChangeDetector::~DefaultAudioDeviceChangeDetector() {
+DefaultAudioDeviceChangeDetector::~DefaultAudioDeviceChangeDetector() = default;
+
+void DefaultAudioDeviceChangeDetector::Unregister() {
+  if (!registered_) {
+    return;
+  }
+  registered_ = false;
   enumerator_->UnregisterEndpointNotificationCallback(this);
 }
 
@@ -49,16 +56,6 @@
   return S_OK;
 }
 
-HRESULT DefaultAudioDeviceChangeDetector::QueryInterface(REFIID iid,
-                                                         void** object) {
-  if (iid == IID_IUnknown || iid == __uuidof(IMMNotificationClient)) {
-    *object = static_cast<IMMNotificationClient*>(this);
-    return S_OK;
-  }
-  *object = nullptr;
-  return E_NOINTERFACE;
-}
-
 HRESULT DefaultAudioDeviceChangeDetector::OnDeviceAdded(LPCWSTR pwstrDeviceId) {
   return S_OK;
 }
@@ -80,12 +77,4 @@
   return S_OK;
 }
 
-ULONG DefaultAudioDeviceChangeDetector::AddRef() {
-  return 1;
-}
-
-ULONG DefaultAudioDeviceChangeDetector::Release() {
-  return 1;
-}
-
 }  // namespace remoting
diff --git a/remoting/host/win/default_audio_device_change_detector.h b/remoting/host/win/default_audio_device_change_detector.h
index b8cb7d8..d04dfab 100644
--- a/remoting/host/win/default_audio_device_change_detector.h
+++ b/remoting/host/win/default_audio_device_change_detector.h
@@ -6,23 +6,39 @@
 #define REMOTING_HOST_WIN_DEFAULT_AUDIO_DEVICE_CHANGE_DETECTOR_H_
 
 #include <mmdeviceapi.h>
+
 #include <wrl/client.h>
+#include <wrl/implements.h>
 
 #include "base/synchronization/lock.h"
 
 namespace remoting {
 
 // An IMMNotificationClient implementation to detect the change of the default
-// audio output device on the system. It registers itself into the input
-// IMMDeviceEnumerator in constructor and unregisters in destructor.
-// This class does not use the default ref-counting memory management method
-// provided by IUnknown: calling DefaultAudioDeviceChangeDetector::Release()
-// won't delete the object.
-class DefaultAudioDeviceChangeDetector final : public IMMNotificationClient {
+// audio output device on the system. It registers itself with the input
+// IMMDeviceEnumerator in the constructor. The owner must call Unregister()
+// before releasing its reference so that the enumerator releases its own
+// reference to this object. Instances must be created with
+// Microsoft::WRL::Make and held in a Microsoft::WRL::ComPtr so that the
+// instance is kept alive while the system holds outstanding references.
+class DefaultAudioDeviceChangeDetector final
+    : public Microsoft::WRL::RuntimeClass<
+          Microsoft::WRL::RuntimeClassFlags<Microsoft::WRL::ClassicCom>,
+          IMMNotificationClient> {
  public:
   explicit DefaultAudioDeviceChangeDetector(
       const Microsoft::WRL::ComPtr<IMMDeviceEnumerator>& enumerator);
-  ~DefaultAudioDeviceChangeDetector();
+
+  DefaultAudioDeviceChangeDetector(const DefaultAudioDeviceChangeDetector&) =
+      delete;
+  DefaultAudioDeviceChangeDetector& operator=(
+      const DefaultAudioDeviceChangeDetector&) = delete;
+
+  ~DefaultAudioDeviceChangeDetector() override;
+
+  // Unregisters this object from the IMMDeviceEnumerator. Must be called by
+  // the owner before releasing its reference.
+  void Unregister();
 
   bool GetAndReset();
 
@@ -32,8 +48,6 @@
                                            ERole role,
                                            LPCWSTR pwstrDefaultDevice) override;
 
-  HRESULT __stdcall QueryInterface(REFIID iid, void** object) override;
-
   // No-ops overrides.
   HRESULT __stdcall OnDeviceAdded(LPCWSTR pwstrDeviceId) override;
   HRESULT __stdcall OnDeviceRemoved(LPCWSTR pwstrDeviceId) override;
@@ -41,10 +55,9 @@
                                          DWORD dwNewState) override;
   HRESULT __stdcall OnPropertyValueChanged(LPCWSTR pwstrDeviceId,
                                            const PROPERTYKEY key) override;
-  ULONG __stdcall AddRef() override;
-  ULONG __stdcall Release() override;
 
   const Microsoft::WRL::ComPtr<IMMDeviceEnumerator> enumerator_;
+  bool registered_ = false;
   bool changed_ = false;
   base::Lock lock_;
 };
diff --git a/remoting/host/win/default_audio_device_change_detector_unittest.cc b/remoting/host/win/default_audio_device_change_detector_unittest.cc
new file mode 100644
index 0000000..be9c675
--- /dev/null
+++ b/remoting/host/win/default_audio_device_change_detector_unittest.cc
@@ -0,0 +1,150 @@
+// Copyright 2026 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "remoting/host/win/default_audio_device_change_detector.h"
+
+#include <mmdeviceapi.h>
+
+#include <wrl/client.h>
+#include <wrl/implements.h>
+
+#include "base/win/scoped_com_initializer.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace remoting {
+
+namespace {
+
+class FakeMMDeviceEnumerator final
+    : public Microsoft::WRL::RuntimeClass<
+          Microsoft::WRL::RuntimeClassFlags<Microsoft::WRL::ClassicCom>,
+          IMMDeviceEnumerator> {
+ public:
+  FakeMMDeviceEnumerator() = default;
+
+  FakeMMDeviceEnumerator(const FakeMMDeviceEnumerator&) = delete;
+  FakeMMDeviceEnumerator& operator=(const FakeMMDeviceEnumerator&) = delete;
+
+  ~FakeMMDeviceEnumerator() override = default;
+
+  Microsoft::WRL::ComPtr<IMMNotificationClient> client() const {
+    return client_;
+  }
+
+  // IMMDeviceEnumerator implementation.
+  HRESULT __stdcall EnumAudioEndpoints(
+      EDataFlow dataFlow,
+      DWORD dwStateMask,
+      IMMDeviceCollection** ppDevices) override;
+  HRESULT __stdcall GetDefaultAudioEndpoint(EDataFlow dataFlow,
+                                            ERole role,
+                                            IMMDevice** ppEndpoint) override;
+  HRESULT __stdcall GetDevice(LPCWSTR pwstrId, IMMDevice** ppDevice) override;
+  HRESULT __stdcall RegisterEndpointNotificationCallback(
+      IMMNotificationClient* pClient) override;
+  HRESULT __stdcall UnregisterEndpointNotificationCallback(
+      IMMNotificationClient* pClient) override;
+
+ private:
+  Microsoft::WRL::ComPtr<IMMNotificationClient> client_;
+};
+
+HRESULT FakeMMDeviceEnumerator::EnumAudioEndpoints(
+    EDataFlow dataFlow,
+    DWORD dwStateMask,
+    IMMDeviceCollection** ppDevices) {
+  return E_NOTIMPL;
+}
+
+HRESULT FakeMMDeviceEnumerator::GetDefaultAudioEndpoint(
+    EDataFlow dataFlow,
+    ERole role,
+    IMMDevice** ppEndpoint) {
+  return E_NOTIMPL;
+}
+
+HRESULT FakeMMDeviceEnumerator::GetDevice(LPCWSTR pwstrId,
+                                          IMMDevice** ppDevice) {
+  return E_NOTIMPL;
+}
+
+HRESULT FakeMMDeviceEnumerator::RegisterEndpointNotificationCallback(
+    IMMNotificationClient* pClient) {
+  client_ = pClient;
+  return S_OK;
+}
+
+HRESULT FakeMMDeviceEnumerator::UnregisterEndpointNotificationCallback(
+    IMMNotificationClient* pClient) {
+  if (client_.Get() != pClient) {
+    return E_INVALIDARG;
+  }
+  client_.Reset();
+  return S_OK;
+}
+
+}  // namespace
+
+class DefaultAudioDeviceChangeDetectorTest : public testing::Test {
+ public:
+  DefaultAudioDeviceChangeDetectorTest() = default;
+  ~DefaultAudioDeviceChangeDetectorTest() override = default;
+
+ protected:
+  base::win::ScopedCOMInitializer com_init_;
+};
+
+TEST_F(DefaultAudioDeviceChangeDetectorTest, RegistersAndUnregisters) {
+  auto enumerator = Microsoft::WRL::Make<FakeMMDeviceEnumerator>();
+  ASSERT_FALSE(enumerator->client());
+
+  auto detector =
+      Microsoft::WRL::Make<DefaultAudioDeviceChangeDetector>(enumerator);
+  EXPECT_EQ(enumerator->client().Get(),
+            static_cast<IMMNotificationClient*>(detector.Get()));
+
+  detector->Unregister();
+  EXPECT_FALSE(enumerator->client());
+
+  // A second call is a no-op.
+  detector->Unregister();
+}
+
+TEST_F(DefaultAudioDeviceChangeDetectorTest, GetAndReset) {
+  auto enumerator = Microsoft::WRL::Make<FakeMMDeviceEnumerator>();
+  auto detector =
+      Microsoft::WRL::Make<DefaultAudioDeviceChangeDetector>(enumerator);
+
+  EXPECT_FALSE(detector->GetAndReset());
+
+  enumerator->client()->OnDefaultDeviceChanged(eRender, eConsole, nullptr);
+  EXPECT_TRUE(detector->GetAndReset());
+  EXPECT_FALSE(detector->GetAndReset());
+
+  detector->Unregister();
+}
+
+TEST_F(DefaultAudioDeviceChangeDetectorTest,
+       OutlivesOwnerWhileSystemHoldsReference) {
+  auto enumerator = Microsoft::WRL::Make<FakeMMDeviceEnumerator>();
+  auto detector =
+      Microsoft::WRL::Make<DefaultAudioDeviceChangeDetector>(enumerator);
+
+  // Simulate the system queuing a notification, which takes an additional
+  // reference on the registered client.
+  Microsoft::WRL::ComPtr<IMMNotificationClient> pending = enumerator->client();
+  ASSERT_TRUE(pending);
+
+  // The owner unregisters the detector and releases its reference, as
+  // AudioCapturerWin::Deinitialize() does.
+  detector->Unregister();
+  detector.Reset();
+
+  // The detector must remain alive while `pending` holds a reference, so a
+  // late notification is delivered to a valid object.
+  EXPECT_EQ(pending->OnDefaultDeviceChanged(eRender, eConsole, nullptr), S_OK);
+  pending.Reset();
+}
+
+}  // namespace remoting
diff --git a/remoting/host/win/BUILD.gn b/remoting/host/win/BUILD.gn
index 65ea109..c5aca2b 100644
--- a/remoting/host/win/BUILD.gn
+++ b/remoting/host/win/BUILD.gn
@@ -235,6 +235,7 @@
   testonly = true
 
   sources = [
+    "default_audio_device_change_detector_unittest.cc",
     "desktop_event_handler_unittest.cc",
     "event_trace_data.cc",
     "event_trace_data.h",

diff --git a/remoting/host/win/default_audio_device_change_detector_unittest.cc b/remoting/host/win/default_audio_device_change_detector_unittest.cc
new file mode 100644
index 0000000..be9c675
--- /dev/null
+++ b/remoting/host/win/default_audio_device_change_detector_unittest.cc
@@ -0,0 +1,150 @@
+// Copyright 2026 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "remoting/host/win/default_audio_device_change_detector.h"
+
+#include <mmdeviceapi.h>
+
+#include <wrl/client.h>
+#include <wrl/implements.h>
+
+#include "base/win/scoped_com_initializer.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace remoting {
+
+namespace {
+
+class FakeMMDeviceEnumerator final
+    : public Microsoft::WRL::RuntimeClass<
+          Microsoft::WRL::RuntimeClassFlags<Microsoft::WRL::ClassicCom>,
+          IMMDeviceEnumerator> {
+ public:
+  FakeMMDeviceEnumerator() = default;
+
+  FakeMMDeviceEnumerator(const FakeMMDeviceEnumerator&) = delete;
+  FakeMMDeviceEnumerator& operator=(const FakeMMDeviceEnumerator&) = delete;
+
+  ~FakeMMDeviceEnumerator() override = default;
+
+  Microsoft::WRL::ComPtr<IMMNotificationClient> client() const {
+    return client_;
+  }
+
+  // IMMDeviceEnumerator implementation.
+  HRESULT __stdcall EnumAudioEndpoints(
+      EDataFlow dataFlow,
+      DWORD dwStateMask,
+      IMMDeviceCollection** ppDevices) override;
+  HRESULT __stdcall GetDefaultAudioEndpoint(EDataFlow dataFlow,
+                                            ERole role,
+                                            IMMDevice** ppEndpoint) override;
+  HRESULT __stdcall GetDevice(LPCWSTR pwstrId, IMMDevice** ppDevice) override;
+  HRESULT __stdcall RegisterEndpointNotificationCallback(
+      IMMNotificationClient* pClient) override;
+  HRESULT __stdcall UnregisterEndpointNotificationCallback(
+      IMMNotificationClient* pClient) override;
+
+ private:
+  Microsoft::WRL::ComPtr<IMMNotificationClient> client_;
+};
+
+HRESULT FakeMMDeviceEnumerator::EnumAudioEndpoints(
+    EDataFlow dataFlow,
+    DWORD dwStateMask,
+    IMMDeviceCollection** ppDevices) {
+  return E_NOTIMPL;
+}
+
+HRESULT FakeMMDeviceEnumerator::GetDefaultAudioEndpoint(
+    EDataFlow dataFlow,
+    ERole role,
+    IMMDevice** ppEndpoint) {
+  return E_NOTIMPL;
+}
+
+HRESULT FakeMMDeviceEnumerator::GetDevice(LPCWSTR pwstrId,
+                                          IMMDevice** ppDevice) {
+  return E_NOTIMPL;
+}
+
+HRESULT FakeMMDeviceEnumerator::RegisterEndpointNotificationCallback(
+    IMMNotificationClient* pClient) {
+  client_ = pClient;
+  return S_OK;
+}
+
+HRESULT FakeMMDeviceEnumerator::UnregisterEndpointNotificationCallback(
+    IMMNotificationClient* pClient) {
+  if (client_.Get() != pClient) {
+    return E_INVALIDARG;
+  }
+  client_.Reset();
+  return S_OK;
+}
+
+}  // namespace
+
+class DefaultAudioDeviceChangeDetectorTest : public testing::Test {
+ public:
+  DefaultAudioDeviceChangeDetectorTest() = default;
+  ~DefaultAudioDeviceChangeDetectorTest() override = default;
+
+ protected:
+  base::win::ScopedCOMInitializer com_init_;
+};
+
+TEST_F(DefaultAudioDeviceChangeDetectorTest, RegistersAndUnregisters) {
+  auto enumerator = Microsoft::WRL::Make<FakeMMDeviceEnumerator>();
+  ASSERT_FALSE(enumerator->client());
+
+  auto detector =
+      Microsoft::WRL::Make<DefaultAudioDeviceChangeDetector>(enumerator);
+  EXPECT_EQ(enumerator->client().Get(),
+            static_cast<IMMNotificationClient*>(detector.Get()));
+
+  detector->Unregister();
+  EXPECT_FALSE(enumerator->client());
+
+  // A second call is a no-op.
+  detector->Unregister();
+}
+
+TEST_F(DefaultAudioDeviceChangeDetectorTest, GetAndReset) {
+  auto enumerator = Microsoft::WRL::Make<FakeMMDeviceEnumerator>();
+  auto detector =
+      Microsoft::WRL::Make<DefaultAudioDeviceChangeDetector>(enumerator);
+
+  EXPECT_FALSE(detector->GetAndReset());
+
+  enumerator->client()->OnDefaultDeviceChanged(eRender, eConsole, nullptr);
+  EXPECT_TRUE(detector->GetAndReset());
+  EXPECT_FALSE(detector->GetAndReset());
+
+  detector->Unregister();
+}
+
+TEST_F(DefaultAudioDeviceChangeDetectorTest,
+       OutlivesOwnerWhileSystemHoldsReference) {
+  auto enumerator = Microsoft::WRL::Make<FakeMMDeviceEnumerator>();
+  auto detector =
+      Microsoft::WRL::Make<DefaultAudioDeviceChangeDetector>(enumerator);
+
+  // Simulate the system queuing a notification, which takes an additional
+  // reference on the registered client.
+  Microsoft::WRL::ComPtr<IMMNotificationClient> pending = enumerator->client();
+  ASSERT_TRUE(pending);
+
+  // The owner unregisters the detector and releases its reference, as
+  // AudioCapturerWin::Deinitialize() does.
+  detector->Unregister();
+  detector.Reset();
+
+  // The detector must remain alive while `pending` holds a reference, so a
+  // late notification is delivered to a valid object.
+  EXPECT_EQ(pending->OnDefaultDeviceChanged(eRender, eConsole, nullptr), S_OK);
+  pending.Reset();
+}
+
+}  // namespace remoting