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}

pc/BUILD.gn

@@ -444,6 +444,7 @@ rtc_source_set("rtp_transport") {
     ":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",

pc/rtp_transport.cc

@@ -285,8 +285,18 @@ void RtpTransport::MaybeSignalReadyToSend() {
   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;
   }
 }
 

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 @@ class RtpTransport : public RtpTransportInternal {
 
   // 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

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 @@ TEST(RtpTransportTest, DontSignalUnhandledRtpPayloadType) {
   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

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 @@ class TransportObserver : public RtpPacketSinkInterface {
   }
 
   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 @@ class TransportObserver : public RtpPacketSinkInterface {
 
   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 @@ class TransportObserver : public RtpPacketSinkInterface {
   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