From 2f0dc63af5b10d63243a451b8043999f2b680e8c Mon Sep 17 00:00:00 2001
From: Hongchan Choi <hongchan@google.com>
Date: Thu, 21 May 2026 12:19:22 -0700
Subject: [PATCH] [media/midi] Synchronize access to MidiManagerAndroid ports and devices

Synchronize access to all input/output port vectors, port maps,
and device vectors in MidiManagerAndroid using the existing lock_.

Previously, these collections were mutated on the UI thread (inside
AddDevice and OnDetached) but accessed on the IO thread (inside
DispatchSendMidiData) and JNI threads (inside OnReceivedData) without
synchronization. This could lead to concurrent access issues.

We resolve this by:
- Guarding all shared collection accesses in MidiManagerAndroid
  under the internal lock_.
- Using extremely short critical sections (bounds check, vector read,
  or map lookup only) on the real-time paths (DispatchSendMidiData
  and OnReceivedData) to ensure zero real-time overhead.
- Keeping expensive operations, JNI calls, and client notifications
  completely outside of locked sections, which also eliminates nested
  lock acquisitions and prevents deadlocks.
- Adding a multi-threaded concurrency unit test in
  midi_manager_unittest.cc to verify thread safety.

Bug: 514742327
Test: midi_unittests
Change-Id: Ic4ee8d17dee46819888800dbea11a5a9144e1d0d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7861363
Commit-Queue: Hongchan Choi <hongchan@chromium.org>
Reviewed-by: Michael Wilson <mjwilson@chromium.org>
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1634454}
---

diff --git a/media/midi/OWNERS b/media/midi/OWNERS
index 19e2d23..d7c888b8 100644
--- a/media/midi/OWNERS
+++ b/media/midi/OWNERS
@@ -1,6 +1,8 @@
 # Following reviewers should be able to review all code under //media/midi.
 # But it would be a good idea to involve {yukawa,agoode}@chromium.org if
 # they are the original author of files you are touching.
+hongchan@chromium.org
+mjwilson@chromium.org
 toyoshim@chromium.org
 
 per-file *.mojom=set noparent
diff --git a/media/midi/midi_manager_android.cc b/media/midi/midi_manager_android.cc
index bd68160..36ca6e1 100644
--- a/media/midi/midi_manager_android.cc
+++ b/media/midi/midi_manager_android.cc
@@ -76,15 +76,21 @@
                                               uint32_t port_index,
                                               const std::vector<uint8_t>& data,
                                               base::TimeTicks timestamp) {
-  if (port_index >= all_output_ports_.size()) {
-    // |port_index| is provided by a renderer so we can't believe that it is
-    // in the valid range.
-    return;
+  MidiOutputPortAndroid* port = nullptr;
+  {
+    base::AutoLock auto_lock(lock_);
+    if (port_index >= all_output_ports_.size()) {
+      // |port_index| is provided by a renderer so we can't believe that it is
+      // in the valid range.
+      return;
+    }
+    port = all_output_ports_[port_index];
   }
+
   if (GetOutputPortState(port_index) == PortState::CONNECTED) {
     // We treat send call as implicit open.
     // TODO(yhirano): Implement explicit open operation from the renderer.
-    if (all_output_ports_[port_index]->Open()) {
+    if (port->Open()) {
       SetOutputPortState(port_index, PortState::OPENED);
     } else {
       // We cannot open the port. It's useless to send data to such a port.
@@ -99,7 +105,7 @@
   service()->task_service()->PostBoundDelayedTask(
       TaskService::kDefaultRunnerId,
       base::BindOnce(&MidiOutputPortAndroid::Send,
-                     base::Unretained(all_output_ports_[port_index]), data),
+                     base::Unretained(port), data),
       delay);
   service()->task_service()->PostBoundDelayedTask(
       TaskService::kDefaultRunnerId,
@@ -111,9 +117,14 @@
 void MidiManagerAndroid::OnReceivedData(MidiInputPortAndroid* port,
                                         base::span<const uint8_t> data,
                                         base::TimeTicks timestamp) {
-  const auto i = input_port_to_index_.find(port);
-  DCHECK(input_port_to_index_.end() != i);
-  ReceiveMidiData(i->second, data, timestamp);
+  size_t index = 0;
+  {
+    base::AutoLock auto_lock(lock_);
+    const auto i = input_port_to_index_.find(port);
+    DCHECK(input_port_to_index_.end() != i);
+    index = i->second;
+  }
+  ReceiveMidiData(index, data, timestamp);
 }
 
 void MidiManagerAndroid::OnInitialized(JNIEnv* env,
@@ -141,6 +152,7 @@
 
 void MidiManagerAndroid::OnDetached(JNIEnv* env,
                                     const JavaRef<jobject>& raw_device) {
+  base::AutoLock auto_lock(lock_);
   for (auto& device : devices_) {
     if (device->HasRawDevice(env, raw_device)) {
       for (auto& port : device->input_ports()) {
@@ -160,6 +172,7 @@
 }
 
 void MidiManagerAndroid::AddDevice(std::unique_ptr<MidiDeviceAndroid> device) {
+  base::AutoLock auto_lock(lock_);
   for (auto& port : device->input_ports()) {
     // We implicitly open input ports here, because there are no signal
     // from the renderer when to open.
diff --git a/media/midi/midi_manager_android.h b/media/midi/midi_manager_android.h
index 4a46cfa..be774fd 100644
--- a/media/midi/midi_manager_android.h
+++ b/media/midi/midi_manager_android.h
@@ -16,6 +16,7 @@
 #include "base/containers/span.h"
 #include "base/memory/raw_ptr.h"
 #include "base/synchronization/lock.h"
+#include "base/thread_annotations.h"
 #include "base/time/time.h"
 #include "media/midi/midi_export.h"
 #include "media/midi/midi_input_port_android.h"
@@ -34,8 +35,9 @@
 
 // MidiManagerAndroid is a MidiManager subclass for Android M or newer. For
 // older android OSes, we use MidiManagerUsb.
-class MidiManagerAndroid final : public MidiManager,
-                                 public MidiInputPortAndroid::Delegate {
+class MIDI_EXPORT MidiManagerAndroid final
+    : public MidiManager,
+      public MidiInputPortAndroid::Delegate {
  public:
   explicit MidiManagerAndroid(MidiService* service);
   ~MidiManagerAndroid() override;
@@ -66,30 +68,30 @@
   void AddOutputPortAndroid(MidiOutputPortAndroid* port,
                             MidiDeviceAndroid* device);
 
-  // TODO(toyoshim): Remove |lock_| once dynamic instantiation mode is enabled
-  // by default. This protects objects allocated on the I/O thread from doubly
-  // released on the main thread.
+  // Protects objects mutated on the UI thread but accessed on the IO thread.
   base::Lock lock_;
 
   // All ports held in |devices_|. Each device has ownership of ports, but we
   // can store pointers here because a device will keep its ports while it is
   // alive.
   std::vector<raw_ptr<MidiInputPortAndroid, VectorExperimental>>
-      all_input_ports_;
+      all_input_ports_ GUARDED_BY(lock_);
   // A dictionary from a port to its index.
   // input_port_to_index_[all_input_ports_[i]] == i for each valid |i|.
-  absl::flat_hash_map<MidiInputPortAndroid*, size_t> input_port_to_index_;
+  absl::flat_hash_map<MidiInputPortAndroid*, size_t> input_port_to_index_
+      GUARDED_BY(lock_);
 
   // Ditto for output ports.
   std::vector<raw_ptr<MidiOutputPortAndroid, VectorExperimental>>
-      all_output_ports_;
-  absl::flat_hash_map<MidiOutputPortAndroid*, size_t> output_port_to_index_;
+      all_output_ports_ GUARDED_BY(lock_);
+  absl::flat_hash_map<MidiOutputPortAndroid*, size_t> output_port_to_index_
+      GUARDED_BY(lock_);
 
   // `devices_` must be declared after the port index maps so that it is
   // destroyed first (in reverse declaration order). This ensures that MidiPorts
   // are closed and stop receiving callbacks before the maps are destroyed.
   // See https://crbug.com/490254128.
-  std::vector<std::unique_ptr<MidiDeviceAndroid>> devices_;
+  std::vector<std::unique_ptr<MidiDeviceAndroid>> devices_ GUARDED_BY(lock_);
 
   base::android::ScopedJavaGlobalRef<jobject> raw_manager_;
 };
diff --git a/media/midi/midi_manager_unittest.cc b/media/midi/midi_manager_unittest.cc
index 9eb48aa..2d4aff3 100644
--- a/media/midi/midi_manager_unittest.cc
+++ b/media/midi/midi_manager_unittest.cc
@@ -28,6 +28,8 @@
 #endif  // BUILDFLAG(IS_WIN)
 
 #if BUILDFLAG(IS_ANDROID)
+#include "base/android/jni_android.h"
+#include "base/threading/simple_thread.h"
 #include "media/midi/midi_manager_android.h"
 #endif  // BUILDFLAG(IS_ANDROID)
 
@@ -444,6 +446,58 @@
   EndSession();

diff --git a/media/midi/midi_manager_unittest.cc b/media/midi/midi_manager_unittest.cc
index 9eb48aa..2d4aff3 100644
--- a/media/midi/midi_manager_unittest.cc
+++ b/media/midi/midi_manager_unittest.cc
@@ -28,6 +28,8 @@
 #endif  // BUILDFLAG(IS_WIN)
 
 #if BUILDFLAG(IS_ANDROID)
+#include "base/android/jni_android.h"
+#include "base/threading/simple_thread.h"
 #include "media/midi/midi_manager_android.h"
 #endif  // BUILDFLAG(IS_ANDROID)
 
@@ -444,6 +446,58 @@
   EndSession();
 }
 
+#if BUILDFLAG(IS_ANDROID)
+class ConcurrencyTestThread : public base::SimpleThread {
+ public:
+  ConcurrencyTestThread(const std::string& name, base::OnceClosure closure)
+      : base::SimpleThread(name), closure_(std::move(closure)) {}
+  void Run() override { std::move(closure_).Run(); }
+ private:
+  base::OnceClosure closure_;
+};
+
+TEST_F(MidiManagerTest, MidiManagerAndroidConcurrency) {
+  auto manager = std::make_unique<MidiManagerAndroid>(service());
+  std::atomic<bool> stop{false};
+
+  // SendThread: Repeatedly sends MIDI data. Since the list of output ports is
+  // empty, this quickly bounds-checks and exits under the lock, simulating
+  // high-frequency concurrent messaging activity.
+  base::OnceClosure send_task = base::BindOnce(
+      [](MidiManagerAndroid* manager, std::atomic<bool>* stop) {
+        std::vector<uint8_t> data;
+        while (!stop->load()) {
+          manager->DispatchSendMidiData(nullptr, 0, data, base::TimeTicks());
+        }
+      },
+      manager.get(), &stop);
+
+  // DetachThread: Repeatedly simulates detaching a device. Since the list
+  // of devices is empty, this quickly exits under the lock, simulating
+  // concurrent dynamic device-attach/detach JNI events.
+  base::OnceClosure detach_task = base::BindOnce(
+      [](MidiManagerAndroid* manager, std::atomic<bool>* stop) {
+        JNIEnv* env = base::android::AttachCurrentThread();
+        while (!stop->load()) {
+          manager->OnDetached(env, nullptr);
+        }
+      },
+      manager.get(), &stop);
+
+  ConcurrencyTestThread thread1("SendThread", std::move(send_task));
+  ConcurrencyTestThread thread2("DetachThread", std::move(detach_task));
+
+  thread1.Start();
+  thread2.Start();
+
+  base::PlatformThread::Sleep(base::Milliseconds(100));
+  stop.store(true);
+
+  thread1.Join();
+  thread2.Join();
+}
+#endif  // BUILDFLAG(IS_ANDROID)
+
 }  // namespace
 
 }  // namespace midi