MidiManager: Fix a potential race on macOS

CompleteInitialization can be posted at a place after DeleteSoon's
task, and it can potentially cause a UAF crash. This happens only on
browser's shutdown sequence, and it won't practically. But just in case
and to make ASAN bots happy:)

Bug: 880665, 672793
Change-Id: I8435290b4df7068d456368624935d3007a2c52d7
Reviewed-on: https://chromium-review.googlesource.com/1238297
Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594667}

media/midi/midi_manager.cc

@@ -104,8 +104,12 @@ void MidiManager::Shutdown() {
       static_cast<Sample>(SendReceiveUsage::MAX) + 1);
 
   // Detach all clients so that they do not call MidiManager methods any more.
+  for (auto* client : pending_clients_)
+    client->Detach();
+  pending_clients_.clear();
   for (auto* client : clients_)
     client->Detach();
+  clients_.clear();
 
   session_thread_runner_ = nullptr;
 }
@@ -129,8 +133,12 @@ void MidiManager::StartSession(MidiManagerClient* client) {
     if (initialization_state_ == InitializationState::COMPLETED) {
       // Platform dependent initialization was already finished for previously
       // initialized clients.
-      if (result_ == Result::OK)
-        AddInitialPorts(client);
+      if (result_ == Result::OK) {
+        for (const auto& info : input_ports_)
+          client->AddInputPort(info);
+        for (const auto& info : output_ports_)
+          client->AddOutputPort(info);
+      }
 
       // Complete synchronously with |result_|;
       clients_.insert(client);
@@ -187,17 +195,6 @@ bool MidiManager::HasOpenSession() {
   return clients_.size() != 0u;
 }
 
-void MidiManager::AccumulateMidiBytesSent(MidiManagerClient* client, size_t n) {
-  base::AutoLock auto_lock(lock_);
-  data_sent_ = true;
-  if (clients_.find(client) == clients_.end())
-    return;
-
-  // Continue to hold lock_ here in case another thread is currently doing
-  // EndSession.
-  client->AccumulateMidiBytesSent(n);
-}
-
 void MidiManager::DispatchSendMidiData(MidiManagerClient* client,
                                        uint32_t port_index,
                                        const std::vector<uint8_t>& data,
@@ -210,22 +207,37 @@ void MidiManager::StartInitialization() {
 }
 
 void MidiManager::CompleteInitialization(Result result) {
-  bool complete_asynchronously = false;
-  {
-    base::AutoLock auto_lock(lock_);
-    if (session_thread_runner_) {
-      if (session_thread_runner_->BelongsToCurrentThread()) {
-        complete_asynchronously = true;
-      } else {
-        session_thread_runner_->PostTask(
-            FROM_HERE,
-            base::BindOnce(&MidiManager::CompleteInitializationOnSessionThread,
-                           base::Unretained(this), result));
-      }
+  TRACE_EVENT0("midi", "MidiManager::CompleteInitialization");
+  ReportUsage(Usage::INITIALIZED);
+
+  base::AutoLock auto_lock(lock_);
+  UMA_HISTOGRAM_ENUMERATION("Media.Midi.InputPorts", input_ports_.size(),
+                            kMaxUmaDevices + 1);
+  UMA_HISTOGRAM_ENUMERATION("Media.Midi.OutputPorts", output_ports_.size(),
+                            kMaxUmaDevices + 1);
+
+  if (!session_thread_runner_)
+    return;
+  DCHECK(session_thread_runner_->BelongsToCurrentThread());
+
+  DCHECK(!finalized_);
+  DCHECK(clients_.empty());
+  DCHECK_EQ(initialization_state_, InitializationState::STARTED);
+  initialization_state_ = InitializationState::COMPLETED;
+  result_ = result;
+
+  for (auto* client : pending_clients_) {
+    if (result_ == Result::OK) {
+      for (const auto& info : input_ports_)
+        client->AddInputPort(info);
+      for (const auto& info : output_ports_)
+        client->AddOutputPort(info);
     }
+
+    clients_.insert(client);
+    client->CompleteStartSession(result_);
   }
-  if (complete_asynchronously)
-    CompleteInitializationOnSessionThread(result);
+  pending_clients_.clear();
 }
 
 void MidiManager::AddInputPort(const MidiPortInfo& info) {
@@ -260,6 +272,23 @@ void MidiManager::SetOutputPortState(uint32_t port_index, PortState state) {
     client->SetOutputPortState(port_index, state);
 }
 
+mojom::PortState MidiManager::GetOutputPortState(uint32_t port_index) {
+  base::AutoLock auto_lock(lock_);
+  DCHECK_LT(port_index, output_ports_.size());
+  return output_ports_[port_index].state;
+}
+
+void MidiManager::AccumulateMidiBytesSent(MidiManagerClient* client, size_t n) {
+  base::AutoLock auto_lock(lock_);
+  data_sent_ = true;
+  if (clients_.find(client) == clients_.end())
+    return;
+
+  // Continue to hold lock_ here in case another thread is currently doing
+  // EndSession.
+  client->AccumulateMidiBytesSent(n);
+}
+
 void MidiManager::ReceiveMidiData(uint32_t port_index,
                                   const uint8_t* data,
                                   size_t length,
@@ -271,38 +300,4 @@ void MidiManager::ReceiveMidiData(uint32_t port_index,
     client->ReceiveMidiData(port_index, data, length, timestamp);
 }
 
-void MidiManager::CompleteInitializationOnSessionThread(Result result) {
-  TRACE_EVENT0("midi", "MidiManager::CompleteInitialization");
-  ReportUsage(Usage::INITIALIZED);
-
-  base::AutoLock auto_lock(lock_);
-  UMA_HISTOGRAM_ENUMERATION("Media.Midi.InputPorts", input_ports_.size(),
-                            kMaxUmaDevices + 1);
-  UMA_HISTOGRAM_ENUMERATION("Media.Midi.OutputPorts", output_ports_.size(),
-                            kMaxUmaDevices + 1);
-
-  DCHECK(clients_.empty());
-  DCHECK_EQ(initialization_state_, InitializationState::STARTED);
-  initialization_state_ = InitializationState::COMPLETED;
-  result_ = result;
-
-  for (auto* client : pending_clients_) {
-    if (result_ == Result::OK)
-      AddInitialPorts(client);
-
-    clients_.insert(client);
-    client->CompleteStartSession(result_);
-  }
-  pending_clients_.clear();
-}
-
-void MidiManager::AddInitialPorts(MidiManagerClient* client) {
-  lock_.AssertAcquired();
-
-  for (const auto& info : input_ports_)
-    client->AddInputPort(info);
-  for (const auto& info : output_ports_)
-    client->AddOutputPort(info);
-}
-
 }  // namespace midi

media/midi/midi_manager.h

@@ -14,6 +14,7 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.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_port_info.h"
@@ -108,10 +109,6 @@ class MIDI_EXPORT MidiManager {
   // Returns true if there is at least one client that keep a session open.
   bool HasOpenSession();
 
-  // Invoke AccumulateMidiBytesSent() for |client| safely. If the session was
-  // already closed, do nothing.
-  void AccumulateMidiBytesSent(MidiManagerClient* client, size_t n);
-
   // DispatchSendMidiData() is called when MIDI data should be sent to the MIDI
   // system.
   // This method is supposed to return immediately and should not block.
@@ -145,31 +142,34 @@ class MIDI_EXPORT MidiManager {
   virtual void Finalize() {}
 
   // Called from a platform dependent implementation of StartInitialization().
-  // The method can be called on any thread, and it invokes
-  // CompleteInitializationOnSessionThread() on the thread that ran
-  // StartSession() and distributes |result| to MIDIManagerClient objects in
+  // The method distributes |result| to MIDIManagerClient objects in
   // |pending_clients_|.
   void CompleteInitialization(mojom::Result result);
 
-  // The following methods can be called on any thread.
+  // The following five methods can be called on any thread to notify clients of
+  // status changes on ports, or to obtain port status.
   void AddInputPort(const MidiPortInfo& info);
   void AddOutputPort(const MidiPortInfo& info);
   void SetInputPortState(uint32_t port_index, mojom::PortState state);
   void SetOutputPortState(uint32_t port_index, mojom::PortState state);
+  mojom::PortState GetOutputPortState(uint32_t port_index);
 
-  // Dispatches to all clients.
+  // Invoke AccumulateMidiBytesSent() for |client| safely. If the session was
+  // already closed, do nothing. Can be called on any thread.
+  void AccumulateMidiBytesSent(MidiManagerClient* client, size_t n);
+
+  // Dispatches to all clients. Can be called on any thread.
   void ReceiveMidiData(uint32_t port_index,
                        const uint8_t* data,
                        size_t length,
                        base::TimeTicks time);
 
+  // Only for testing.
   size_t clients_size_for_testing() const { return clients_.size(); }
   size_t pending_clients_size_for_testing() const {
     return pending_clients_.size();
   }
 
-  const MidiPortInfoList& input_ports() const { return input_ports_; }
-  const MidiPortInfoList& output_ports() const { return output_ports_; }
   MidiService* service() { return service_; }
 
  private:
@@ -179,11 +179,9 @@ class MIDI_EXPORT MidiManager {
     COMPLETED,
   };
 
-  void CompleteInitializationOnSessionThread(mojom::Result result);
-  void AddInitialPorts(MidiManagerClient* client);
-
   // Keeps track of all clients who wish to receive MIDI data.
-  std::set<MidiManagerClient*> clients_;
+  // TODO(toyoshim): Enable GUARDED_BY once a testing function is fixed.
+  std::set<MidiManagerClient*> clients_;  // GUARDED_BY(lock_);
 
   // Keeps track of all clients who are waiting for CompleteStartSession().
   std::set<MidiManagerClient*> pending_clients_;
@@ -203,12 +201,12 @@ class MIDI_EXPORT MidiManager {
   mojom::Result result_;
 
   // Keeps all MidiPortInfo.
-  MidiPortInfoList input_ports_;
-  MidiPortInfoList output_ports_;
+  MidiPortInfoList input_ports_ GUARDED_BY(lock_);
+  MidiPortInfoList output_ports_ GUARDED_BY(lock_);
 
   // Tracks if actual data transmission happens.
-  bool data_sent_;
-  bool data_received_;
+  bool data_sent_ GUARDED_BY(lock_);
+  bool data_received_ GUARDED_BY(lock_);
 
   // Protects members above.
   base::Lock lock_;

media/midi/midi_manager_android.cc

@@ -97,8 +97,7 @@ void MidiManagerAndroid::DispatchSendMidiData(MidiManagerClient* client,
     // in the valid range.
     return;
   }
-  DCHECK_EQ(output_ports().size(), all_output_ports_.size());
-  if (output_ports()[port_index].state == PortState::CONNECTED) {
+  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()) {
@@ -120,7 +119,7 @@ void MidiManagerAndroid::DispatchSendMidiData(MidiManagerClient* client,
       delay);
   service()->task_service()->PostBoundDelayedTask(
       TaskService::kDefaultRunnerId,
-      base::BindOnce(&MidiManager::AccumulateMidiBytesSent,
+      base::BindOnce(&MidiManagerAndroid::AccumulateMidiBytesSent,
                      base::Unretained(this), client, data.size()),
       delay);
 }
@@ -145,13 +144,19 @@ void MidiManagerAndroid::OnInitialized(
         env, env->GetObjectArrayElement(devices, i));
     AddDevice(std::make_unique<MidiDeviceAndroid>(env, raw_device, this));
   }
-  CompleteInitialization(Result::OK);
+  service()->task_service()->PostBoundTask(
+      TaskService::kDefaultRunnerId,
+      base::BindOnce(&MidiManagerAndroid::CompleteInitialization,
+                     base::Unretained(this), Result::OK));
 }
 
 void MidiManagerAndroid::OnInitializationFailed(
     JNIEnv* env,
     const JavaParamRef<jobject>& caller) {
-  CompleteInitialization(Result::INITIALIZATION_ERROR);
+  service()->task_service()->PostBoundTask(
+      TaskService::kDefaultRunnerId,
+      base::BindOnce(&MidiManagerAndroid::CompleteInitialization,
+                     base::Unretained(this), Result::INITIALIZATION_ERROR));
 }
 
 void MidiManagerAndroid::OnAttached(JNIEnv* env,

media/midi/midi_manager_mac.cc

@@ -38,7 +38,7 @@ const size_t kCoreMIDIMaxPacketListSize = 65536;
 const size_t kEstimatedMaxPacketDataSize = kCoreMIDIMaxPacketListSize / 2;
 
 enum {
-  kDefaultRunnerNotUsedOnMac = TaskService::kDefaultRunnerId,
+  kSessionTaskRunner = TaskService::kDefaultRunnerId,
   kClientTaskRunner,
 };
 
@@ -160,7 +160,7 @@ void MidiManagerMac::InitializeCoreMIDI() {
   OSStatus result = MIDIClientCreate(CFSTR("Chrome"), ReceiveMidiNotifyDispatch,
                                      this, &client);
   if (result != noErr || client == 0u)
-    return CompleteInitialization(Result::INITIALIZATION_ERROR);
+    return CompleteCoreMIDIInitialization(Result::INITIALIZATION_ERROR);
 
   {
     base::AutoLock lock(midi_client_lock_);
@@ -173,11 +173,11 @@ void MidiManagerMac::InitializeCoreMIDI() {
   result = MIDIInputPortCreate(client, CFSTR("MIDI Input"), ReadMidiDispatch,
                                this, &midi_input_);
   if (result != noErr || midi_input_ == 0u)
-    return CompleteInitialization(Result::INITIALIZATION_ERROR);
+    return CompleteCoreMIDIInitialization(Result::INITIALIZATION_ERROR);
 
   result = MIDIOutputPortCreate(client, CFSTR("MIDI Output"), &midi_output_);
   if (result != noErr || midi_output_ == 0u)
-    return CompleteInitialization(Result::INITIALIZATION_ERROR);
+    return CompleteCoreMIDIInitialization(Result::INITIALIZATION_ERROR);
 
   // Following loop may miss some newly attached devices, but such device will
   // be captured by ReceiveMidiNotifyDispatch callback.
@@ -207,7 +207,14 @@ void MidiManagerMac::InitializeCoreMIDI() {
   for (size_t i = 0u; i < sources_.size(); ++i)
     MIDIPortConnectSource(midi_input_, sources_[i], reinterpret_cast<void*>(i));
 
-  CompleteInitialization(Result::OK);
+  CompleteCoreMIDIInitialization(Result::OK);
+}
+
+void MidiManagerMac::CompleteCoreMIDIInitialization(mojom::Result result) {
+  service()->task_service()->PostBoundTask(
+      kSessionTaskRunner,
+      base::BindOnce(&MidiManagerMac::CompleteInitialization,
+                     base::Unretained(this), result));
 }
 
 // static

media/midi/midi_manager_mac.h

@@ -40,6 +40,10 @@ class MIDI_EXPORT MidiManagerMac final : public MidiManager {
   // StartInitialization().
   void InitializeCoreMIDI();
 
+  // Completes CoreMIDI initialization and asks the thread that ran
+  // StartInitialization() to call CompleteStartSession() safely.
+  void CompleteCoreMIDIInitialization(mojom::Result result);
+
   // CoreMIDI callback for MIDI notification.
   // Receives MIDI related event notifications from CoreMIDI.
   static void ReceiveMidiNotifyDispatch(const MIDINotification* message,

media/midi/midi_manager_usb.cc

@@ -7,6 +7,7 @@
 #include <memory>
 #include <utility>
 
+#include "base/bind.h"
 #include "base/logging.h"
 #include "base/strings/stringprintf.h"
 #include "media/midi/midi_service.h"
@@ -26,7 +27,6 @@ MidiManagerUsb::~MidiManagerUsb() {
   // TODO(toyoshim): Remove following DCHECKs once the dynamic instantiation
   // mode is enabled by default.
   base::AutoLock lock(lock_);
-  DCHECK(initialize_callback_.is_null());
   DCHECK(device_factory_);
   DCHECK(devices_.empty());
   DCHECK(output_streams_.empty());
@@ -38,8 +38,7 @@ void MidiManagerUsb::StartInitialization() {
   bool result = service()->task_service()->BindInstance();
   DCHECK(result);
 
-  Initialize(base::BindOnce(&MidiManager::CompleteInitialization,
-                            base::Unretained(this)));
+  Initialize();
 }
 
 void MidiManagerUsb::Finalize() {
@@ -49,7 +48,6 @@ void MidiManagerUsb::Finalize() {
   // TODO(toyoshim): Remove following code once the dynamic instantiation mode
   // is enabled by default.
   base::AutoLock lock(lock_);
-  initialize_callback_ = Callback();
   devices_.clear();
   output_streams_.clear();
   input_stream_.reset();
@@ -59,9 +57,7 @@ void MidiManagerUsb::Finalize() {
   // which the constructor runs.
 }
 
-void MidiManagerUsb::Initialize(Callback callback) {
-  initialize_callback_ = std::move(callback);
-
+void MidiManagerUsb::Initialize() {
   // This is safe because EnumerateDevices cancels the operation on destruction.
   device_factory_->EnumerateDevices(
       this, base::BindOnce(&MidiManagerUsb::OnEnumerateDevicesDone,
@@ -140,19 +136,21 @@ void MidiManagerUsb::OnReceivedData(size_t jack_index,
 
 void MidiManagerUsb::OnEnumerateDevicesDone(bool result,
                                             UsbMidiDevice::Devices* devices) {
-  if (!result) {
-    std::move(initialize_callback_).Run(Result::INITIALIZATION_ERROR);
-    return;
-  }
-  input_stream_.reset(new UsbMidiInputStream(this));
-  devices->swap(devices_);
-  for (size_t i = 0; i < devices_.size(); ++i) {
-    if (!AddPorts(devices_[i].get(), static_cast<int>(i))) {
-      std::move(initialize_callback_).Run(Result::INITIALIZATION_ERROR);
-      return;
+  if (result) {
+    input_stream_.reset(new UsbMidiInputStream(this));
+    devices->swap(devices_);
+    for (size_t i = 0; i < devices_.size(); ++i) {
+      if (!AddPorts(devices_[i].get(), static_cast<int>(i))) {
+        result = false;
+        break;
+      }
     }
   }
-  std::move(initialize_callback_).Run(Result::OK);
+  service()->task_service()->PostBoundTask(
+      TaskService::kDefaultRunnerId,
+      base::BindOnce(&MidiManager::CompleteInitialization,
+                     base::Unretained(this),
+                     result ? Result::OK : Result::INITIALIZATION_ERROR));
 }
 
 bool MidiManagerUsb::AddPorts(UsbMidiDevice* device, int device_id) {

media/midi/midi_manager_usb.h

@@ -12,8 +12,6 @@
 #include <utility>
 #include <vector>
 
-#include "base/bind.h"
-#include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/containers/hash_tables.h"
 #include "base/macros.h"
@@ -69,14 +67,12 @@ class USB_MIDI_EXPORT MidiManagerUsb : public MidiManager,
   const UsbMidiInputStream* input_stream() const { return input_stream_.get(); }
 
  private:
-  using Callback = base::OnceCallback<void(mojom::Result)>;
-
   // Initializes this object.
-  // When the initialization finishes, |callback| will be called with the
-  // result.
+  // When the initialization finishes, CompleteInitialization will be called
+  // with the result on the same thread, but asynchronously.
   // When this factory is destroyed during the operation, the operation
-  // will be canceled silently (i.e. |callback| will not be called).
-  void Initialize(Callback callback);
+  // will be canceled silently (i.e. CompleteInitialization will not be called).
+  void Initialize();
 
   void OnEnumerateDevicesDone(bool result, UsbMidiDevice::Devices* devices);
   bool AddPorts(UsbMidiDevice* device, int device_id);
@@ -91,8 +87,6 @@ class USB_MIDI_EXPORT MidiManagerUsb : public MidiManager,
   std::vector<std::unique_ptr<UsbMidiOutputStream>> output_streams_;
   std::unique_ptr<UsbMidiInputStream> input_stream_;
 
-  Callback initialize_callback_;
-
   // A map from <endpoint_number, cable_number> to the index of input jacks.
   base::hash_map<std::pair<int, int>, size_t> input_jack_dictionary_;
 

media/midi/midi_manager_winrt.cc

@@ -50,7 +50,7 @@ using mojom::PortState;
 using mojom::Result;
 
 enum {
-  kDefaultRunnerNotUsedOnWinrt = TaskService::kDefaultRunnerId,
+  kDefaultTaskRunner = TaskService::kDefaultRunnerId,
   kComTaskRunner
 };
 
@@ -825,7 +825,10 @@ void MidiManagerWinrt::InitializeOnComRunner() {
   bool preload_success = base::win::ResolveCoreWinRTDelayload() &&
                          ScopedHString::ResolveCoreWinRTStringDelayload();
   if (!preload_success) {
-    CompleteInitialization(Result::INITIALIZATION_ERROR);
+    service()->task_service()->PostBoundTask(
+        kDefaultTaskRunner,
+        base::BindOnce(&MidiManagerWinrt::CompleteInitialization,
+                       base::Unretained(this), Result::INITIALIZATION_ERROR));
     return;
   }
 
@@ -836,7 +839,10 @@ void MidiManagerWinrt::InitializeOnComRunner() {
         port_manager_out_->StartWatcher())) {
     port_manager_in_->StopWatcher();
     port_manager_out_->StopWatcher();
-    CompleteInitialization(Result::INITIALIZATION_ERROR);
+    service()->task_service()->PostBoundTask(
+        kDefaultTaskRunner,
+        base::BindOnce(&MidiManagerWinrt::CompleteInitialization,
+                       base::Unretained(this), Result::INITIALIZATION_ERROR));
   }
 }
 
@@ -869,8 +875,12 @@ void MidiManagerWinrt::OnPortManagerReady() {
   DCHECK(service()->task_service()->IsOnTaskRunner(kComTaskRunner));
   DCHECK(port_manager_ready_count_ < 2);
 
-  if (++port_manager_ready_count_ == 2)
-    CompleteInitialization(Result::OK);
+  if (++port_manager_ready_count_ == 2) {
+    service()->task_service()->PostBoundTask(
+        kDefaultTaskRunner,
+        base::BindOnce(&MidiManagerWinrt::CompleteInitialization,
+                       base::Unretained(this), Result::OK));
+  }
 }
 
 }  // namespace midi

media/midi/midi_service.cc

@@ -34,9 +34,7 @@ MidiService::MidiService(std::unique_ptr<ManagerFactory> factory)
 
 MidiService::~MidiService() {
   base::AutoLock lock(lock_);
-
-  manager_.reset();
-
+  DCHECK(!manager_);
   base::AutoLock threads_lock(threads_lock_);
   threads_.clear();
 }