Fix threading issue in "fake" (test) DescriptionObservers
This fixes a problem affecting PeerConnectionWrapper and other
classes using FakeSetLocalDescriptionObserver and
FakeSetRemoteDescriptionObserver whereby callbacks would arrive on
a different thread than the test thread. This has caused some flake
in tests, but mostly has been masked by other synchronous blocking
calls between the test thread and signaling thread.
The pattern was that the fake observer object was being polled from
the test thread until an operation completed on the signaling thread,
leading to tsan errors such as detected here:
https://chromium-swarm.appspot.com/task?id=735c37c9a2b00011&o=true&w=true
Brief (and trimmed) example stacks:
WARNING: ThreadSanitizer: data race (pid=259249)
Write of size 1 at 0x721000023d70 by thread T1:
...
#2 FakeSetLocalDescriptionObserver::OnSetLocalDescriptionComplete(...)
#3 SdpOfferAnswerHandler::DoSetLocalDescription(...)
...
#5 rtc_operations_chain_internal::OperationWithFunctor<...>::Run()
#6 ChainOperation<(lambda at ../../pc/sdp_offer_answer.cc:1678:7)>
#7 SdpOfferAnswerHandler::SetLocalDescription(...)
...
Previous read of size 1 at 0x721000023d70 by main thread:
...
#1 called pc/test/mock_peer_connection_observers.h:356:39
#2 operator() pc/peer_connection_wrapper.cc:183:3
#3 operator() test/wait_until.h:84:24
...
#6 WaitUntil(FunctionView<bool ()>, WaitUntilSettings)
#7 WaitUntil<(lambda at ../../pc/peer_connection_wrapper.cc:183:3)>
#8 PeerConnectionWrapper::SetLocalDescription(...)
...
Bug: none
Change-Id: Iadc4634fe78d5d1b252b1a3c05d7ae9c5b76922c
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/409340
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#45651}
diff --git a/pc/test/mock_peer_connection_observers.h b/pc/test/mock_peer_connection_observers.h
index 4e948e0..001534a 100644
--- a/pc/test/mock_peer_connection_observers.h
+++ b/pc/test/mock_peer_connection_observers.h
@@ -350,42 +350,72 @@
std::string error_;
};
-class FakeSetLocalDescriptionObserver
- : public SetLocalDescriptionObserverInterface {
+// Base implementation class for fake local/remote description
+// observer classes. Handles the case where the usage of the observer class
+// is not on the same thread as the callback comes in on. In that case
+// a task is posted to the original test thread to set the error variable.
+// This is to be compatible with polling `WaitUntil` loops that poll the
+// `called()` state from the test thread. If the callback were to be
+// allowed to change the called() state, then we'd be checking and modifying
+// the state of the `error_` variable on two different thread without
+// synchronization, which is a problem.
+class FakeDescriptionObserver {
public:
- bool called() const { return error_.has_value(); }
+ FakeDescriptionObserver() : thread_(Thread::Current()) {
+ RTC_DCHECK(thread_);
+ }
+
+ bool called() const {
+ RTC_DCHECK_RUN_ON(thread_);
+ return error_.has_value();
+ }
+
RTCError& error() {
+ RTC_DCHECK_RUN_ON(thread_);
RTC_DCHECK(error_.has_value());
return *error_;
}
- // SetLocalDescriptionObserverInterface implementation.
- void OnSetLocalDescriptionComplete(RTCError error) override {
- error_ = std::move(error);
+ protected:
+ void OnCallback(RTCError error) {
+ if (Thread::Current() == thread_) {
+ RTC_DCHECK_RUN_ON(thread_);
+ error_ = std::move(error);
+ } else {
+ thread_->PostTask([this, error = std::move(error)]() {
+ RTC_DCHECK_RUN_ON(thread_);
+ error_ = std::move(error);
+ });
+ }
}
private:
- // Set on complete, on success this is set to an RTCError::OK() error.
- std::optional<RTCError> error_;
+ Thread* const thread_;
+ std::optional<RTCError> error_ RTC_GUARDED_BY(thread_);
+};
+
+class FakeSetLocalDescriptionObserver
+ : public SetLocalDescriptionObserverInterface,
+ public FakeDescriptionObserver {
+ public:
+ FakeSetLocalDescriptionObserver() = default;
+
+ private:
+ void OnSetLocalDescriptionComplete(RTCError error) override {
+ OnCallback(std::move(error));
+ }
};
class FakeSetRemoteDescriptionObserver
- : public SetRemoteDescriptionObserverInterface {
+ : public SetRemoteDescriptionObserverInterface,
+ public FakeDescriptionObserver {
public:
- bool called() const { return error_.has_value(); }
- RTCError& error() {
- RTC_DCHECK(error_.has_value());
- return *error_;
- }
-
- // SetRemoteDescriptionObserverInterface implementation.
- void OnSetRemoteDescriptionComplete(RTCError error) override {
- error_ = std::move(error);
- }
+ FakeSetRemoteDescriptionObserver() = default;
private:
- // Set on complete, on success this is set to an RTCError::OK() error.
- std::optional<RTCError> error_;
+ void OnSetRemoteDescriptionComplete(RTCError error) override {
+ OnCallback(std::move(error));
+ }
};
class MockDataChannelObserver : public DataChannelObserver {
