commit ac75f4ce572eba615d713c40a538e4d060630fd9
parent 8e6e4b84c679556e3402c412e5a9452ed920db8e
Author: Dan Baker <dbaker@mozilla.com>
Date: Mon, 1 Dec 2025 20:41:19 -0700
Bug 2000941 - Vendor libwebrtc from 426f0590b6
Upstream commit: https://webrtc.googlesource.com/src/+/426f0590b6e4ae4541ac30fe898e5fdfcde4a7d5
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}
Diffstat:
2 files changed, 52 insertions(+), 22 deletions(-)
diff --git a/third_party/libwebrtc/README.mozilla.last-vendor b/third_party/libwebrtc/README.mozilla.last-vendor
@@ -1,4 +1,4 @@
# ./mach python dom/media/webrtc/third_party_build/vendor-libwebrtc.py --from-local /Users/danielbaker/elm/.moz-fast-forward/moz-libwebrtc --commit mozpatches libwebrtc
-libwebrtc updated from /Users/danielbaker/elm/.moz-fast-forward/moz-libwebrtc commit mozpatches on 2025-12-02T03:38:48.183160+00:00.
+libwebrtc updated from /Users/danielbaker/elm/.moz-fast-forward/moz-libwebrtc commit mozpatches on 2025-12-02T03:41:05.959801+00:00.
# base of lastest vendoring
-34e3cb8457
+426f0590b6
diff --git a/third_party/libwebrtc/pc/test/mock_peer_connection_observers.h b/third_party/libwebrtc/pc/test/mock_peer_connection_observers.h
@@ -350,42 +350,72 @@ class MockSetSessionDescriptionObserver : public SetSessionDescriptionObserver {
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 FakeSetRemoteDescriptionObserver
- : public SetRemoteDescriptionObserverInterface {
+class FakeSetLocalDescriptionObserver
+ : public SetLocalDescriptionObserverInterface,
+ public FakeDescriptionObserver {
public:
- bool called() const { return error_.has_value(); }
- RTCError& error() {
- RTC_DCHECK(error_.has_value());
- return *error_;
- }
+ FakeSetLocalDescriptionObserver() = default;
- // SetRemoteDescriptionObserverInterface implementation.
- void OnSetRemoteDescriptionComplete(RTCError error) override {
- error_ = std::move(error);
+ private:
+ void OnSetLocalDescriptionComplete(RTCError error) override {
+ OnCallback(std::move(error));
}
+};
+
+class FakeSetRemoteDescriptionObserver
+ : public SetRemoteDescriptionObserverInterface,
+ public FakeDescriptionObserver {
+ public:
+ 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 {
