commit 17048012261406343c4e3f82418720b7cf0f4dd7
author Yves Gerey <yvesg@webrtc.org> Fri Jul 26 15:49:52 2019
committer Commit Bot <commit-bot@chromium.org> Fri Jul 26 17:03:29 2019
tree 5ef3f21b33b78427a57d09ac7bced31ea0fe1f13
parent 378cae2543f5ea7d0a95a1c15516da0c5cd15f6c

Prevent concurrent access to AudioSendStream's configuration.

By design:
 * OnPacketAdded() is meant to be called on pacer thread.
 * Reconfigure()   is meant to be called on worker thread.
Thus we guard against race condition on config_ member.

Possible downside: packet filtering based on ssrc might be slowed down.

Bug: webrtc:9849
Change-Id: I734bb9b34b01db160705897adb1b58e866e12639
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/146980
Commit-Queue: Yves Gerey <yvesg@google.com>
Reviewed-by: Oskar Sundbom <ossu@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28691}

audio/audio_send_stream.cc

diff --git a/audio/audio_send_stream.cc b/audio/audio_send_stream.cc
index 8eeacda..c5ec1b5 100644
--- a/audio/audio_send_stream.cc
+++ b/audio/audio_send_stream.cc
@@ -236,6 +236,8 @@
   const auto& channel_send = stream->channel_send_;
   const auto& old_config = stream->config_;
 
+  stream->config_cs_.Enter();
+
   // Configuration parameters which cannot be changed.
   RTC_DCHECK(first_time ||
              old_config.send_transport == new_config.send_transport);
@@ -263,6 +265,9 @@
 
   const ExtensionIds old_ids = FindExtensionIds(old_config.rtp.extensions);
   const ExtensionIds new_ids = FindExtensionIds(new_config.rtp.extensions);
+
+  stream->config_cs_.Leave();
+
   // Audio level indication
   if (first_time || new_ids.audio_level != old_ids.audio_level) {
     channel_send->SetSendAudioLevelIndicationStatus(new_ids.audio_level != 0,
@@ -299,6 +304,7 @@
           stream->rtp_transport_, bandwidth_observer);
     }
   }
+  stream->config_cs_.Enter();
   // MID RTP header extension.
   if ((first_time || new_ids.mid != old_ids.mid ||
        new_config.rtp.mid != old_config.rtp.mid) &&
@@ -321,6 +327,7 @@
     ReconfigureBitrateObserver(stream, new_config);
   }
   stream->config_ = new_config;
+  stream->config_cs_.Leave();
 }
 
 void AudioSendStream::Start() {
@@ -487,7 +494,12 @@
 void AudioSendStream::OnPacketAdded(uint32_t ssrc, uint16_t seq_num) {
   RTC_DCHECK(pacer_thread_checker_.IsCurrent());
   // Only packets that belong to this stream are of interest.
-  if (ssrc == config_.rtp.ssrc) {
+  bool same_ssrc;
+  {
+    rtc::CritScope lock(&config_cs_);
+    same_ssrc = ssrc == config_.rtp.ssrc;
+  }
+  if (same_ssrc) {
     rtc::CritScope lock(&packet_loss_tracker_cs_);
     // TODO(eladalon): This function call could potentially reset the window,
     // setting both PLR and RPLR to unknown. Consider (during upcoming

audio/audio_send_stream.h

diff --git a/audio/audio_send_stream.h b/audio/audio_send_stream.h
index fd65296..37eb89a 100644
--- a/audio/audio_send_stream.h
+++ b/audio/audio_send_stream.h
@@ -153,6 +153,7 @@
   rtc::RaceChecker audio_capture_race_checker_;
   rtc::TaskQueue* worker_queue_;
   const AudioAllocationSettings allocation_settings_;
+  rtc::CriticalSection config_cs_;
   webrtc::AudioSendStream::Config config_;
   rtc::scoped_refptr<webrtc::AudioState> audio_state_;
   const std::unique_ptr<voe::ChannelSendInterface> channel_send_;

audio/audio_send_stream_unittest.cc

diff --git a/audio/audio_send_stream_unittest.cc b/audio/audio_send_stream_unittest.cc
index dcd3667..94bc34c 100644
--- a/audio/audio_send_stream_unittest.cc
+++ b/audio/audio_send_stream_unittest.cc
@@ -11,6 +11,7 @@
 #include "audio/audio_send_stream.h"
 
 #include <string>
+#include <thread>
 #include <utility>
 #include <vector>
 
@@ -679,6 +680,29 @@
   send_stream->Reconfigure(helper.config());
 }
 
+// Allow to check for race conditions under tsan.
+// This mimicks the situation where 'ModuleProcessThread' (pacer thread) is
+// launched by webrtc::RtpTransportControllerSend::RtpTransportControllerSend().
+TEST(AudioSendStreamTest, RaceFree) {
+  ConfigHelper helper(false, false);
+  // Sanity checks: copy-pasted from DontRecreateEncoder test.
+  EXPECT_CALL(*helper.channel_send(), SetEncoderForMock(_, _))
+      .WillOnce(Return());
+
+  EXPECT_CALL(*helper.channel_send(), RegisterCngPayloadType(105, 8000));
+
+  helper.config().send_codec_spec =
+      AudioSendStream::Config::SendCodecSpec(9, kG722Format);
+  helper.config().send_codec_spec->cng_payload_type = 105;
+  auto send_stream = helper.CreateAudioSendStream();
+  std::thread pacer([&]() {
+    send_stream->OnPacketAdded(/*ssrc*/ 0xcafe,
+                               /*seq_num*/ 0xf00d);
+  });
+  send_stream->Reconfigure(helper.config());
+  pacer.join();
+}
+
 TEST(AudioSendStreamTest, ReconfigureTransportCcResetsFirst) {
   ScopedFieldTrials field_trials("WebRTC-Audio-SendSideBwe/Enabled/");
   ConfigHelper helper(false, true);