Fire MaybeSignalReadyToSend in a PostTask when recursive
Speculative fix. Writing the test for it is more complex.
Bug: chromium:1483874
Change-Id: Icfaf1524b0499c609010753e0b6f3cadbd0e98f9
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/321480
Reviewed-by: Per Kjellander <perkj@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40820}
diff --git a/pc/BUILD.gn b/pc/BUILD.gn
index 899b89a..e9a49ca 100644
--- a/pc/BUILD.gn
+++ b/pc/BUILD.gn
@@ -444,6 +444,7 @@
":rtp_transport_internal",
":session_description",
"../api:array_view",
+ "../api/task_queue:pending_task_safety_flag",
"../api/units:timestamp",
"../call:rtp_receiver",
"../call:video_stream_api",
diff --git a/pc/rtp_transport.cc b/pc/rtp_transport.cc
index 32bdf99..653b51f 100644
--- a/pc/rtp_transport.cc
+++ b/pc/rtp_transport.cc
@@ -285,8 +285,18 @@
bool ready_to_send =
rtp_ready_to_send_ && (rtcp_ready_to_send_ || rtcp_mux_enabled_);
if (ready_to_send != ready_to_send_) {
+ if (processing_ready_to_send_) {
+ // Delay ReadyToSend processing until current operation is finished.
+ // Note that this may not cause a signal, since ready_to_send may
+ // have a new value by the time this executes.
+ TaskQueueBase::Current()->PostTask(
+ SafeTask(safety_.flag(), [this] { MaybeSignalReadyToSend(); }));
+ return;
+ }
ready_to_send_ = ready_to_send;
+ processing_ready_to_send_ = true;
SendReadyToSend(ready_to_send);
+ processing_ready_to_send_ = false;
}
}
diff --git a/pc/rtp_transport.h b/pc/rtp_transport.h
index 1afcb5e..456c91c 100644
--- a/pc/rtp_transport.h
+++ b/pc/rtp_transport.h
@@ -17,6 +17,7 @@
#include <string>
#include "absl/types/optional.h"
+#include "api/task_queue/pending_task_safety_flag.h"
#include "call/rtp_demuxer.h"
#include "call/video_receive_stream.h"
#include "modules/rtp_rtcp/include/rtp_header_extension_map.h"
@@ -137,6 +138,9 @@
// Used for identifying the MID for RtpDemuxer.
RtpHeaderExtensionMap header_extension_map_;
+ // Guard against recursive "ready to send" signals
+ bool processing_ready_to_send_ = false;
+ ScopedTaskSafety safety_;
};
} // namespace webrtc
diff --git a/pc/rtp_transport_unittest.cc b/pc/rtp_transport_unittest.cc
index ca748ae..5b6a830 100644
--- a/pc/rtp_transport_unittest.cc
+++ b/pc/rtp_transport_unittest.cc
@@ -10,12 +10,16 @@
#include "pc/rtp_transport.h"
+#include <utility>
+
#include "p2p/base/fake_packet_transport.h"
#include "pc/test/rtp_transport_test_util.h"
#include "rtc_base/buffer.h"
#include "rtc_base/containers/flat_set.h"
+#include "rtc_base/gunit.h"
#include "rtc_base/third_party/sigslot/sigslot.h"
#include "test/gtest.h"
+#include "test/run_loop.h"
namespace webrtc {
@@ -321,4 +325,28 @@
transport.UnregisterRtpDemuxerSink(&observer);
}
+TEST(RtpTransportTest, RecursiveSetSendDoesNotCrash) {
+ const int kShortTimeout = 100;
+ test::RunLoop loop;
+ RtpTransport transport(kMuxEnabled);
+ rtc::FakePacketTransport fake_rtp("fake_rtp");
+ transport.SetRtpPacketTransport(&fake_rtp);
+ TransportObserver observer(&transport);
+ observer.SetActionOnReadyToSend([&](bool ready) {
+ const rtc::PacketOptions options;
+ const int flags = 0;
+ rtc::CopyOnWriteBuffer rtp_data(kRtpData, kRtpLen);
+ transport.SendRtpPacket(&rtp_data, options, flags);
+ });
+ // The fake RTP will have no destination, so will return -1.
+ fake_rtp.SetError(ENOTCONN);
+ fake_rtp.SetWritable(true);
+ // At this point, only the initial ready-to-send is observed.
+ EXPECT_TRUE(observer.ready_to_send());
+ EXPECT_EQ(observer.ready_to_send_signal_count(), 1);
+ // After the wait, the ready-to-send false is observed.
+ EXPECT_EQ_WAIT(observer.ready_to_send_signal_count(), 2, kShortTimeout);
+ EXPECT_FALSE(observer.ready_to_send());
+}
+
} // namespace webrtc
diff --git a/pc/test/rtp_transport_test_util.h b/pc/test/rtp_transport_test_util.h
index 29ffad8..593ee00 100644
--- a/pc/test/rtp_transport_test_util.h
+++ b/pc/test/rtp_transport_test_util.h
@@ -11,6 +11,8 @@
#ifndef PC_TEST_RTP_TRANSPORT_TEST_UTIL_H_
#define PC_TEST_RTP_TRANSPORT_TEST_UTIL_H_
+#include <utility>
+
#include "call/rtp_packet_sink_interface.h"
#include "modules/rtp_rtcp/source/rtp_packet_received.h"
#include "pc/rtp_transport_internal.h"
@@ -65,6 +67,9 @@
}
void OnReadyToSend(bool ready) {
+ if (action_on_ready_to_send_) {
+ action_on_ready_to_send_(ready);
+ }
ready_to_send_signal_count_++;
ready_to_send_ = ready;
}
@@ -73,6 +78,10 @@
int ready_to_send_signal_count() { return ready_to_send_signal_count_; }
+ void SetActionOnReadyToSend(absl::AnyInvocable<void(bool)> action) {
+ action_on_ready_to_send_ = std::move(action);
+ }
+
private:
bool ready_to_send_ = false;
int rtp_count_ = 0;
@@ -81,6 +90,7 @@
int ready_to_send_signal_count_ = 0;
rtc::CopyOnWriteBuffer last_recv_rtp_packet_;
rtc::CopyOnWriteBuffer last_recv_rtcp_packet_;
+ absl::AnyInvocable<void(bool)> action_on_ready_to_send_;
};
} // namespace webrtc
