Skip to content

Commit

Permalink
[Merge M103] Reland "Fix deadlock in WebRtcMetronomeAdapter"
Browse files Browse the repository at this point in the history 
This is a reland of commit 2f684b2

Can reland: The build on Windows was fixed with https://webrtc-review.googlesource.com/c/src/+/262811

Original change's description:
> Fix deadlock in WebRtcMetronomeAdapter
>
> This could occur when a listener was being removed at the same time as
> OnTick was called. The problem is that OnTick first requires the
> TickHandle to acquire its is_active_lock, and then the
> WebRtcMetronomeAdapter would acquire its listener lock, while
> RemoveListener acquires the listener lock before cancelling the
> TickHandle which tries to acquire its is_active_lock. This means that
> the two locks could be acquired in different orders, which led to
> occasional hangs and deadlocks.
>
> This fix removes the requirement acquiring the listener lock when
> interacting with the TickHandles, instead adding 1 TickHandle per
> webrtc::Metronome::TickListener. To ensure that the TickListener::OnTick
> was not invoked after it was removed a webrtc::PendingTaskSafety is used
> taking advantage of the fact that the only user of webrtc::Metronome is
> the webrtc::DecodeSynchronizer. The threading model or interface of
> webrtc::Metronome will be clarified or reworked.
>
> The crash has been confirmed with a codeplay (found in the bug report).
>
> Bug: 1325109
> Change-Id: I15d36cde0d93bed370d0210fa4f7f8b08f5f3be0
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3649679
> Reviewed-by: Tomas Gunnarsson <tommi@chromium.org>
> Commit-Queue: Evan Shrubsole <eshr@google.com>
> Cr-Commit-Position: refs/heads/main@{#1004743}

(cherry picked from commit ec178bb)

Bug: 1325109
Change-Id: Ia5b6f3ebad7981f853a8eac8dcf195f0ef6fc0de
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3654082
Reviewed-by: Tomas Gunnarsson <tommi@chromium.org>
Commit-Queue: Evan Shrubsole <eshr@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#1006416}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3678447
Commit-Queue: Tomas Gunnarsson <tommi@chromium.org>
Auto-Submit: Evan Shrubsole <eshr@google.com>
Reviewed-by: Henrik Boström <hbos@chromium.org>
Cr-Commit-Position: refs/branch-heads/5060@{#410}
Cr-Branched-From: b83393d-refs/heads/main@{#1002911}
  • Loading branch information
@eshrubs
eshrubs authored and Chromium LUCI CQ committed May 31, 2022
1 parent b644624 commit 3547524
Show file tree
Hide file tree
Showing 2 changed files with 83 additions and 75 deletions.
45 changes: 33 additions & 12 deletions third_party/blink/renderer/platform/peerconnection/metronome_source_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "base/notreached.h"
#include "base/synchronization/waitable_event.h"
#include "base/task/thread_pool.h"
#include "base/test/bind.h"
#include "base/test/task_environment.h"
#include "base/time/time.h"
#include "testing/gtest/include/gtest/gtest.h"
Expand Down Expand Up @@ -59,17 +60,28 @@ class FakeTaskQueue : public webrtc::TaskQueueBase {

void PostLastTask() {
if (last_task_) {
runner_->PostTask(FROM_HERE,
base::BindOnce(
[](FakeTaskQueue* thiz,
std::unique_ptr<webrtc::QueuedTask> task) {
if (!task->Run())
task.release();
},
base::Unretained(this), std::move(last_task_)));
PostOnRunner(FROM_HERE,
base::BindOnce(
[](FakeTaskQueue* thiz,
std::unique_ptr<webrtc::QueuedTask> task) {
if (!task->Run())
task.release();
},
base::Unretained(this), std::move(last_task_)));
}
}

void PostOnRunner(base::Location from_here, base::OnceClosure cb) {
runner_->PostTask(
FROM_HERE,
base::BindOnce(
[](FakeTaskQueue* thiz, base::OnceClosure cb) {
webrtc::TaskQueueBase::CurrentTaskQueueSetter setter(thiz);
std::move(cb).Run();
},
base::Unretained(this), std::move(cb)));
}

private:
const bool retain_tasks_;
scoped_refptr<base::SequencedTaskRunner> runner_;
Expand Down Expand Up @@ -330,7 +342,10 @@ TEST_F(MetronomeSourceTest, WebRtcMetronomeAdapterAddsHandler) {
auto metronome_adapter = metronome_source_->CreateWebRtcMetronome();

EXPECT_FALSE(metronome_source_->HasListenersForTesting());
metronome_adapter->AddListener(&tick_listener);
fake_queue.PostOnRunner(FROM_HERE, base::BindLambdaForTesting([&] {
metronome_adapter->AddListener(&tick_listener);
}));
task_environment_.RunUntilIdle();
EXPECT_TRUE(metronome_source_->HasListenersForTesting());

// Next tick should trigger callback.
Expand All @@ -342,7 +357,9 @@ TEST_F(MetronomeSourceTest, WebRtcMetronomeAdapterAddsHandler) {
EXPECT_EQ(11, callback_count);

// Removing should not fire callback.
metronome_adapter->RemoveListener(&tick_listener);
fake_queue.PostOnRunner(FROM_HERE, base::BindLambdaForTesting([&] {
metronome_adapter->RemoveListener(&tick_listener);
}));
task_environment_.FastForwardBy(MetronomeSource::Tick());
EXPECT_EQ(11, callback_count);

Expand All @@ -361,7 +378,9 @@ TEST_F(MetronomeSourceTest,
&fake_queue);
auto metronome_adapter = metronome_source_->CreateWebRtcMetronome();

metronome_adapter->AddListener(&tick_listener);
fake_queue.PostOnRunner(FROM_HERE, base::BindLambdaForTesting([&] {
metronome_adapter->AddListener(&tick_listener);
}));
task_environment_.FastForwardBy(MetronomeSource::Tick());
EXPECT_EQ(0, callback_count);
// Now task should have been posted to fake queue. Running it should increase
Expand All @@ -377,7 +396,9 @@ TEST_F(MetronomeSourceTest,
task_environment_.FastForwardBy(MetronomeSource::Tick());
EXPECT_EQ(1, callback_count);

metronome_adapter->RemoveListener(&tick_listener);
fake_queue.PostOnRunner(FROM_HERE, base::BindLambdaForTesting([&] {
metronome_adapter->RemoveListener(&tick_listener);
}));
fake_queue.PostLastTask();
task_environment_.RunUntilIdle();
EXPECT_EQ(1, callback_count);
Expand Down
113 changes: 50 additions & 63 deletions third_party/webrtc_overrides/metronome_source.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,40 +29,33 @@ constexpr base::TimeDelta kMetronomeTick = base::Hertz(64);

namespace {

// Wraps a webrtc::Metronome::TickListener to ensure that OnTick is not called
// after it is removed from the WebRtcMetronomeAdapter, using the Deactive
// method.
class WebRtcMetronomeListenerWrapper
: public base::RefCountedThreadSafe<WebRtcMetronomeListenerWrapper> {
public:
explicit WebRtcMetronomeListenerWrapper(
webrtc::Metronome::TickListener* listener)
: listener_(listener) {}

void Deactivate() {
base::AutoLock auto_lock(lock_);
active_ = false;
}

webrtc::Metronome::TickListener* listener() { return listener_; }

void OnTick() {
base::AutoLock auto_lock(lock_);
if (!active_)
return;
listener_->OnTick();
}

private:
friend class base::RefCountedThreadSafe<WebRtcMetronomeListenerWrapper>;
~WebRtcMetronomeListenerWrapper() = default;

webrtc::Metronome::TickListener* const listener_;

base::Lock lock_;
bool active_ GUARDED_BY(lock_) = true;
// Stores a MetronomeSource::ListenerHandle which handles listening to handle
// ticks, and an atomic flag for cancelling the task attached to the listener.
// When a TickListener invokes, it will check that the cancel flag was not set.
// To avoid a race between the cancel flag and `OnTick` being invoked after
// `RemoveListener`, `RemoveListener` needs to be called from
// `listener->OnTickTaskQueue()`. This is the case for the only user -
// webrtc::DecodeSynchronizer.
//
// TODO(http://crbug.com/1253787): Clarify threading requirements of
// webrtc::Metronome, or change interface.
struct HandleWithCancelation {
HandleWithCancelation(
scoped_refptr<MetronomeSource::ListenerHandle> handle,
rtc::scoped_refptr<webrtc::PendingTaskSafetyFlag> task_safety)
: handle(std::move(handle)), task_safety(std::move(task_safety)) {}

scoped_refptr<MetronomeSource::ListenerHandle> handle;
rtc::scoped_refptr<webrtc::PendingTaskSafetyFlag> task_safety;
};

void InvokeOnTickOnWebRtcTaskQueue(
webrtc::Metronome::TickListener* listener,
rtc::scoped_refptr<webrtc::PendingTaskSafetyFlag> task_safety) {
listener->OnTickTaskQueue()->PostTask(webrtc::ToQueuedTask(
std::move(task_safety), [listener] { listener->OnTick(); }));
}

class WebRtcMetronomeAdapter : public webrtc::Metronome {
public:
explicit WebRtcMetronomeAdapter(
Expand All @@ -78,37 +71,40 @@ class WebRtcMetronomeAdapter : public webrtc::Metronome {
// only be added to the metronome once.
void AddListener(TickListener* listener) override {
DCHECK(listener);
auto task_safety = webrtc::PendingTaskSafetyFlag::Create();
// `listener` can be unretained since the `handle` will be cancelled when
// `listener` is removed.
auto handle = metronome_source_->AddListener(
nullptr, base::BindRepeating(&InvokeOnTickOnWebRtcTaskQueue,
base::Unretained(listener), task_safety));
base::AutoLock auto_lock(lock_);
auto [it, inserted] = listeners_.emplace(
listener,
base::MakeRefCounted<WebRtcMetronomeListenerWrapper>(listener));
std::piecewise_construct, std::forward_as_tuple(listener),
std::forward_as_tuple(std::move(handle), std::move(task_safety)));
DCHECK(inserted);
if (listeners_.size() == 1) {
DCHECK(!tick_handle_);
tick_handle_ = metronome_source_->AddListener(
nullptr,
base::BindRepeating(&WebRtcMetronomeAdapter::OnTick,
base::Unretained(this)),
base::TimeTicks::Min());
}
}

// Removes the tick listener from the metronome. Once this method has returned
// OnTick will never be called again. This method must not be called from
// within OnTick.
void RemoveListener(TickListener* listener) override {
DCHECK(listener);
base::AutoLock auto_lock(lock_);
auto it = listeners_.find(listener);
if (it == listeners_.end()) {
DLOG(WARNING) << __FUNCTION__ << " called with unregistered listener.";
return;
}
it->second->Deactivate();
listeners_.erase(it);
if (listeners_.size() == 0) {
metronome_source_->RemoveListener(std::move(tick_handle_));
scoped_refptr<MetronomeSource::ListenerHandle> handle;
rtc::scoped_refptr<webrtc::PendingTaskSafetyFlag> task_safety;

{
base::AutoLock auto_lock(lock_);
auto it = listeners_.find(listener);
if (it == listeners_.end()) {
DLOG(WARNING) << __FUNCTION__ << " called with unregistered listener.";
return;
}
handle = std::move(it->second.handle);
task_safety = std::move(it->second.task_safety);
listeners_.erase(listener);
}
task_safety->SetNotAlive();
metronome_source_->RemoveListener(std::move(handle));
}

// Returns the current tick period of the metronome.
Expand All @@ -117,19 +113,10 @@ class WebRtcMetronomeAdapter : public webrtc::Metronome {
}

private:
void OnTick() {
base::AutoLock auto_lock(lock_);
for (auto [listener, wrapper] : listeners_) {
listener->OnTickTaskQueue()->PostTask(webrtc::ToQueuedTask(
[the_wrapper = std::move(wrapper)] { the_wrapper->OnTick(); }));
}
}

const scoped_refptr<MetronomeSource> metronome_source_;
base::Lock lock_;
base::flat_map<TickListener*, scoped_refptr<WebRtcMetronomeListenerWrapper>>
listeners_ GUARDED_BY(lock_);
scoped_refptr<MetronomeSource::ListenerHandle> tick_handle_ GUARDED_BY(lock_);
base::flat_map<TickListener*, HandleWithCancelation> listeners_
GUARDED_BY(lock_);
};

} // namespace
Expand Down

0 comments on commit 3547524

Please sign in to comment.