Commit 96c5b055 authored by Guido Urdaneta's avatar Guido Urdaneta Committed by Commit Bot

Revert "Address race between device notifications and enumerations in AudioDeviceListenerMac"

This reverts commit 01694e80.

Reason for revert: 
Does not fix the bug it intended to fix.
Reverting will facilitate merging of an alternative patch.


Original change's description:
> 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}

TBR=guidou@chromium.org,olka@chromium.org,ossu@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 911311
Change-Id: Ic05d999b32ddb308649384ae29c1721d595e6347
Reviewed-on: https://chromium-review.googlesource.com/c/1373840Reviewed-by: default avatarGuido Urdaneta <guidou@chromium.org>
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615908}
parent 9c9cda0c
......@@ -12,9 +12,6 @@
#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"
......@@ -46,8 +43,6 @@ 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)
......@@ -58,28 +53,13 @@ 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
......@@ -188,7 +168,6 @@ 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 =
......@@ -209,37 +188,15 @@ 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());
it_key->second->MarkAsInactive();
did_mark_as_deleted = true;
source_listeners_.erase(it_key);
}
}
}
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
......@@ -7,6 +7,7 @@
#include <CoreAudio/AudioHardware.h>
#include <map>
#include <memory>
#include <utility>
......@@ -50,7 +51,6 @@ 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_;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment