From e16291b3e8885b72320f49feb86573ce598daaf6 Mon Sep 17 00:00:00 2001
From: Andrew Paseltiner <apaseltiner@chromium.org>
Date: Thu, 09 Apr 2026 06:07:33 -0700
Subject: [PATCH] media/midi: Fix data race and premature destruction in MIDI manager

This change addresses a race condition in the Windows MIDI
implementation where MidiManagerWin could be destroyed while
initialization was still running on a background thread.

Key improvements:

1. Updated MidiManager::HasOpenSession to check both clients_ and
   pending_clients_. This prevents MidiService from deleting the
   manager when a renderer closes its session before initialization
   completes, correctly reflecting that a background task is still
   active.
2. Synchronized ~MidiManagerWin by acquiring GetTaskLock() at the
   start of the destructor. This ensures that destruction is serialized
   with InitializeOnTaskRunner and other background tasks, preventing
   concurrent access to port vectors and pointers.
3. Added a regression test ReproduceLifecycleRace in
   midi_manager_unittest.cc to verify that HasOpenSession correctly
   accounts for pending clients.

Fixed: 500018484
Change-Id: Ib7d5bc4deb5e00ac3d3f32ab561d7353d5699351
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7737022
Commit-Queue: Andrew Paseltiner <apaseltiner@chromium.org>
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1612147}
---

diff --git a/media/midi/midi_manager.cc b/media/midi/midi_manager.cc
index 0c7c19a..5fca320 100644
--- a/media/midi/midi_manager.cc
+++ b/media/midi/midi_manager.cc
@@ -164,7 +164,7 @@
 
 bool MidiManager::HasOpenSession() {
   base::AutoLock auto_lock(lock_);
-  return clients_.size() != 0u;
+  return !clients_.empty() || !pending_clients_.empty();
 }
 
 void MidiManager::DispatchSendMidiData(MidiManagerClient* client,
diff --git a/media/midi/midi_manager_unittest.cc b/media/midi/midi_manager_unittest.cc
index 2be4a7b6..9eb48aa 100644
--- a/media/midi/midi_manager_unittest.cc
+++ b/media/midi/midi_manager_unittest.cc
@@ -219,6 +219,8 @@
 
   base::WeakPtr<FakeMidiManagerFactory> factory() { return factory_; }
 
+  MidiService* service() { return service_.get(); }
+
  private:
   base::test::TaskEnvironment env_;
   base::WeakPtr<FakeMidiManagerFactory> factory_;
@@ -337,6 +339,31 @@
   EXPECT_FALSE(test_future.IsReady());
 }
 
+TEST_F(MidiManagerTest, ReproduceLifecycleRace) {
+  base::test::TestFuture<void> test_future;
+  std::unique_ptr<FakeMidiManagerClient> client =
+      std::make_unique<FakeMidiManagerClient>(test_future.GetCallback());
+
+  // Start a session. This will put the client in pending_clients_.
+  StartSession(client.get());
+  ASSERT_TRUE(factory()->manager());
+  EXPECT_EQ(1U, factory()->manager()->GetPendingClientCount());
+  EXPECT_EQ(0U, factory()->manager()->GetClientCount());
+
+  // FIXED: HasOpenSession() now checks both clients_ and pending_clients_,
+  // so it should return true while initialization is ongoing.
+  EXPECT_TRUE(factory()->manager()->HasOpenSession());
+
+  // If we end the session now, EndSession calls HasOpenSession to decide if it
+  // should delete the manager. Since the client is still in pending_clients_
+  // until removed, EndSession correctly removes it.
+  EXPECT_TRUE(service()->EndSession(client.get()));
+
+  // Now that the last client is gone (even from pending_clients_),
+  // HasOpenSession should return false and the manager should be deleted.
+  EXPECT_FALSE(factory()->manager());
+}
+
 class PlatformMidiManagerTest : public ::testing::Test {
  public:
   PlatformMidiManagerTest()
diff --git a/media/midi/midi_manager_win.cc b/media/midi/midi_manager_win.cc
index d4d4550..2898226 100644
--- a/media/midi/midi_manager_win.cc
+++ b/media/midi/midi_manager_win.cc
@@ -723,6 +723,11 @@
   if (instance_id_ == kInvalidInstanceId)
     return;
 
+  // Behind the lock below, we can safely access all members for finalization
+  // even on the I/O thread. This also ensures that no bound task runs on
+  // TaskRunner concurrently while destructing the instance.
+  base::AutoLock lock(*GetTaskLock());
+
   // Unregisters on the I/O thread. OnDevicesChanged() won't be called any more.
   CHECK(thread_runner_->BelongsToCurrentThread());
   base::SystemMonitor::Get()->RemoveDevicesChangedObserver(this);
@@ -738,20 +743,12 @@
 
   // Invalidate instance bound tasks.
   {
-    base::AutoLock lock(*GetInstanceIdLock());
+    base::AutoLock lock_id(*GetInstanceIdLock());
     CHECK_EQ(instance_id_, g_active_instance_id);
     g_active_instance_id = kInvalidInstanceId;
     CHECK_EQ(this, g_manager_instance);
     g_manager_instance = nullptr;
   }
-
-  // Ensures that no bound task runs on TaskRunner so to destruct the instance
-  // safely.
-  // Tasks that did not started yet will do nothing after invalidate the
-  // instance ID above.
-  // Behind the lock below, we can safely access all members for finalization
-  // even on the I/O thread.
-  base::AutoLock lock(*GetTaskLock());
 }
 
 void MidiManagerWin::StartInitialization() {

diff --git a/media/midi/midi_manager_unittest.cc b/media/midi/midi_manager_unittest.cc
index 2be4a7b6..9eb48aa 100644
--- a/media/midi/midi_manager_unittest.cc
+++ b/media/midi/midi_manager_unittest.cc
@@ -219,6 +219,8 @@
 
   base::WeakPtr<FakeMidiManagerFactory> factory() { return factory_; }
 
+  MidiService* service() { return service_.get(); }
+
  private:
   base::test::TaskEnvironment env_;
   base::WeakPtr<FakeMidiManagerFactory> factory_;
@@ -337,6 +339,31 @@
   EXPECT_FALSE(test_future.IsReady());
 }
 
+TEST_F(MidiManagerTest, ReproduceLifecycleRace) {
+  base::test::TestFuture<void> test_future;
+  std::unique_ptr<FakeMidiManagerClient> client =
+      std::make_unique<FakeMidiManagerClient>(test_future.GetCallback());
+
+  // Start a session. This will put the client in pending_clients_.
+  StartSession(client.get());
+  ASSERT_TRUE(factory()->manager());
+  EXPECT_EQ(1U, factory()->manager()->GetPendingClientCount());
+  EXPECT_EQ(0U, factory()->manager()->GetClientCount());
+
+  // FIXED: HasOpenSession() now checks both clients_ and pending_clients_,
+  // so it should return true while initialization is ongoing.
+  EXPECT_TRUE(factory()->manager()->HasOpenSession());
+
+  // If we end the session now, EndSession calls HasOpenSession to decide if it
+  // should delete the manager. Since the client is still in pending_clients_
+  // until removed, EndSession correctly removes it.
+  EXPECT_TRUE(service()->EndSession(client.get()));
+
+  // Now that the last client is gone (even from pending_clients_),
+  // HasOpenSession should return false and the manager should be deleted.
+  EXPECT_FALSE(factory()->manager());
+}
+
 class PlatformMidiManagerTest : public ::testing::Test {
  public:
   PlatformMidiManagerTest()