From f009e38fe0cf7a6d25e0b5964296490cdb72224f Mon Sep 17 00:00:00 2001
From: Guy Hershenbaum <hershi@fb.com>
Date: Mon, 19 Aug 2024 19:06:12 +0300
Subject: [PATCH] Fix AudioSendStream reconfigure - stop processing during
 unconfigured state
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When Reconfiguring there's a call to ResetSenderCongestionControlObjects followed by a later call to
RegisterSenderCongestionControlObjects which happen on the worker thread, while enqueuing packets is
happening on a different thread.
If packets are enqueued in between these calls we get a null dereference of the `rtp_packet_pacer_`
This change fixes it by pausing processing of incoming audio in the interim state

Bug: webrtc:358290775
Change-Id: I77cebfb131545ce2a6fdb26105dd999da3f7c443
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/359080
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Reviewed-by: Jakob Ivarsson‎ <jakobi@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#42815}
---
 audio/channel_send.cc          | 16 ++++++++++++++++
 audio/channel_send_unittest.cc | 19 +++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/audio/channel_send.cc b/audio/channel_send.cc
index b683cc335e..888dfaf7fe 100644
--- a/audio/channel_send.cc
+++ b/audio/channel_send.cc
@@ -313,11 +313,27 @@ class RtpPacketSenderProxy : public RtpPacketSender {
   void EnqueuePackets(
       std::vector<std::unique_ptr<RtpPacketToSend>> packets) override {
     MutexLock lock(&mutex_);
+
+    // Since we allow having an instance with no rtp_packet_pacer_ set we
+    // should handle calls to member functions in this state gracefully rather
+    // than null dereferencing.
+    if (!rtp_packet_pacer_) {
+      RTC_DLOG(LS_WARNING)
+          << "Dropping packets queued while rtp_packet_pacer_ is null.";
+      return;
+    }
     rtp_packet_pacer_->EnqueuePackets(std::move(packets));
   }
 
   void RemovePacketsForSsrc(uint32_t ssrc) override {
     MutexLock lock(&mutex_);
+
+    // Since we allow having an instance with no rtp_packet_pacer_ set we
+    // should handle calls to member functions in this state gracefully rather
+    // than null dereferencing.
+    if (!rtp_packet_pacer_) {
+      return;
+    }
     rtp_packet_pacer_->RemovePacketsForSsrc(ssrc);
   }
 
diff --git a/audio/channel_send_unittest.cc b/audio/channel_send_unittest.cc
index 38198594b5..3535a9908e 100644
--- a/audio/channel_send_unittest.cc
+++ b/audio/channel_send_unittest.cc
@@ -362,6 +362,25 @@ TEST_F(ChannelSendTest, UsedRateIsLargerofLastTwoFrames) {
   EXPECT_EQ(used_rate_3, used_rate_2);
 }
 
+// Test that we gracefully handle packets while the congestion control objects
+// are not configured. This can happen during calls
+// AudioSendStream::ConfigureStream
+TEST_F(ChannelSendTest, EnqueuePacketsGracefullyHandlesNonInitializedPacer) {
+  EXPECT_CALL(transport_, SendRtp).Times(1);
+  channel_->StartSend();
+  channel_->ResetSenderCongestionControlObjects();
+  // This should trigger a packet, but congestion control is not configured
+  // so it should be dropped
+  ProcessNextFrame();
+  ProcessNextFrame();
+
+  channel_->RegisterSenderCongestionControlObjects(&transport_controller_);
+  // Now that we reconfigured the congestion control objects the new frame
+  // should be processed
+  ProcessNextFrame();
+  ProcessNextFrame();
+}
+
 }  // namespace
 }  // namespace voe
 }  // namespace webrtc