Address race between device notifications and enumerations in AudioDeviceListenerMac

Originally AudioDeviceListenerMac assumed that once CoreAudio reports that a device does not exist,
it was no longer possible to get notifications related to that device, so it was safe to remove
the listener for that device.
However, since notifications can come from multiple threads and the corresponding callbacks make
a thread jump, we have observed that it is indeed possible to get notifications about removed devices
which can lead to a crash if the listener for that notification has been removed.
To address this issue, this CL does not remove listeners when the device is removed, but instead
marks the listener as deleted and posts a task to remove listeners that have been marked as deleted
for several seconds, at a time when we can confidently no longer expect notifications from the removed device.

Bug: 911311
Change-Id: I07cc9c8e24a0cfdfadcc04d0e8bd33e14ab61422
Reviewed-on: https://chromium-review.googlesource.com/c/1363187
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Reviewed-by: Oskar Sundbom <ossu@chromium.org>
Reviewed-by: Olga Sharonova <olka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614391}

media/audio/mac/audio_device_listener_mac.cc

@@ -12,6 +12,9 @@
 #include "base/mac/mac_logging.h"
 #include "base/optional.h"
 #include "base/single_thread_task_runner.h"
+#include "base/stl_util.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "base/time/time.h"
 #include "media/audio/audio_manager.h"
 #include "media/audio/mac/core_audio_util_mac.h"
 #include "media/base/bind_to_current_loop.h"
@@ -43,6 +46,8 @@ const AudioObjectPropertyAddress kPropertyInputSourceChanged = {
 
 class AudioDeviceListenerMac::PropertyListener {
  public:
+  static constexpr base::TimeDelta kPruneTime =
+      base::TimeDelta::FromSeconds(15);
   PropertyListener(AudioObjectID monitored_object,
                    const AudioObjectPropertyAddress* property,
                    base::RepeatingClosure callback)
@@ -53,13 +58,28 @@ class AudioDeviceListenerMac::PropertyListener {
   AudioObjectID monitored_object() const { return monitored_object_; }
   const base::RepeatingClosure& callback() const { return callback_; }
   const AudioObjectPropertyAddress* property() const { return address_; }
+  base::TimeTicks deletion_time() const { return deletion_time_; }
+  bool IsActive() const { return deletion_time_.is_null(); }
+  void MarkAsInactive() { deletion_time_ = base::TimeTicks::Now(); }
+  void MarkAsActive() { deletion_time_ = base::TimeTicks(); }
 
  private:
   AudioObjectID monitored_object_;
   const AudioObjectPropertyAddress* address_;
   base::RepeatingClosure callback_;
+  // Stores a deletion time, used for pruning the listener after it is
+  // deregistered from the OS. This helps prevent a race between destroying the
+  // listener after the OS reports the device is removed, and notifications
+  // about the same device that could originate concurrently on another thread.
+  // This can for example, when unplugging a device with input and output
+  // sources, where the noification about source changes might arrive after the
+  // OS reports the device as removed (and therefore marked as deleted). A null
+  // |deletion_time_| means the listener has not been marked as deleted.
+  base::TimeTicks deletion_time_;
 };
 
+constexpr base::TimeDelta AudioDeviceListenerMac::PropertyListener::kPruneTime;
+
 // Callback from the system when an event occurs; this must be called on the
 // MessageLoop that created the AudioManager.
 // static
@@ -168,6 +188,7 @@ void AudioDeviceListenerMac::UpdateSourceListeners() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   std::vector<AudioObjectID> device_ids =
       core_audio_mac::GetAllAudioDeviceIDs();
+  bool did_mark_as_deleted = false;
   for (bool is_input : {true, false}) {
     for (auto device_id : device_ids) {
       const AudioObjectPropertyAddress* property_address =
@@ -188,15 +209,37 @@ void AudioDeviceListenerMac::UpdateSourceListeners() {
           } else {
             source_listener.reset();
           }
+        } else {
+          it_key->second->MarkAsActive();
         }
       } else if (is_monitored) {
         // Stop monitoring if the device has no source but is currently being
         // monitored.
         RemovePropertyListener(it_key->second.get());
-        source_listeners_.erase(it_key);
+        it_key->second->MarkAsInactive();
+        did_mark_as_deleted = true;
       }
     }
   }
+  if (did_mark_as_deleted) {
+    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE,
+        base::BindOnce(&AudioDeviceListenerMac::PruneDeletedListeners,
+                       weak_factory_.GetWeakPtr()),
+        2 * PropertyListener::kPruneTime);
+  }
+}
+
+void AudioDeviceListenerMac::PruneDeletedListeners() {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  base::EraseIf(source_listeners_, [](const auto& elem) {
+    if (elem.second->IsActive())
+      return false;
+
+    base::TimeDelta elapsed =
+        base::TimeTicks::Now() - elem.second->deletion_time();
+    return elapsed >= PropertyListener::kPruneTime;
+  });
 }
 
 }  // namespace media

media/audio/mac/audio_device_listener_mac.h

@@ -7,7 +7,6 @@
 
 #include <CoreAudio/AudioHardware.h>
 
-#include <map>
 #include <memory>
 #include <utility>
 
@@ -51,6 +50,7 @@ class MEDIA_EXPORT AudioDeviceListenerMac {
   void RemovePropertyListener(PropertyListener* property_listener);
   void OnDevicesAddedOrRemoved();
   void UpdateSourceListeners();
+  void PruneDeletedListeners();
 
   base::RepeatingClosure listener_cb_;
   std::unique_ptr<PropertyListener> default_output_listener_;