From 5dd42fd849062373cab757605824254fc16db6b6 Mon Sep 17 00:00:00 2001
From: deadbeef <deadbeef@webrtc.org>
Date: Mon, 2 May 2016 16:20:01 -0700
Subject: [PATCH] Fixing a segfault that can occur when changing the track of
 an RtpSender.

The reference to the old track needs to be kept alive until SetAudioSend/
SetSource is called, because otherwise it could be deleted while the audio/
video engine is still trying to use the track.

BUG=webrtc:5796

Review-Url: https://codereview.webrtc.org/1894283002
Cr-Commit-Position: refs/heads/master@{#12598}
---
 webrtc/api/rtpsender.cc                  |  6 +++++
 webrtc/api/rtpsenderreceiver_unittest.cc | 29 ++++++++++++++++++++----
 2 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/webrtc/api/rtpsender.cc b/webrtc/api/rtpsender.cc
index 58cb18c6cd..360b6868fd 100644
--- a/webrtc/api/rtpsender.cc
+++ b/webrtc/api/rtpsender.cc
@@ -122,6 +122,9 @@ bool AudioRtpSender::SetTrack(MediaStreamTrackInterface* track) {
 
   // Attach to new track.
   bool prev_can_send_track = can_send_track();
+  // Keep a reference to the old track to keep it alive until we call
+  // SetAudioSend.
+  rtc::scoped_refptr<AudioTrackInterface> old_track = track_;
   track_ = audio_track;
   if (track_) {
     cached_track_enabled_ = track_->enabled();
@@ -276,6 +279,9 @@ bool VideoRtpSender::SetTrack(MediaStreamTrackInterface* track) {
 
   // Attach to new track.
   bool prev_can_send_track = can_send_track();
+  // Keep a reference to the old track to keep it alive until we call
+  // SetSource.
+  rtc::scoped_refptr<VideoTrackInterface> old_track = track_;
   track_ = video_track;
   if (track_) {
     cached_track_enabled_ = track_->enabled();
diff --git a/webrtc/api/rtpsenderreceiver_unittest.cc b/webrtc/api/rtpsenderreceiver_unittest.cc
index 0ec3cfe470..f8b968f549 100644
--- a/webrtc/api/rtpsenderreceiver_unittest.cc
+++ b/webrtc/api/rtpsenderreceiver_unittest.cc
@@ -28,6 +28,7 @@
 
 using ::testing::_;
 using ::testing::Exactly;
+using ::testing::InvokeWithoutArgs;
 using ::testing::Return;
 
 static const char kStreamLabel1[] = "local_stream_1";
@@ -420,7 +421,14 @@ TEST_F(RtpSenderReceiverTest, AudioSenderTrackSetToNull) {
       new AudioRtpSender(track, kStreamLabel1, &audio_provider_, nullptr);
   sender->SetSsrc(kAudioSsrc);
 
-  EXPECT_CALL(audio_provider_, SetAudioSend(kAudioSsrc, false, _, _)).Times(1);
+  // Expect that SetAudioSend will be called before the reference to the track
+  // is released.
+  EXPECT_CALL(audio_provider_, SetAudioSend(kAudioSsrc, false, _, nullptr))
+      .Times(1)
+      .WillOnce(InvokeWithoutArgs([&track] {
+        EXPECT_LT(2, track->AddRef());
+        track->Release();
+      }));
   EXPECT_TRUE(sender->SetTrack(nullptr));
 
   // Make sure it's SetTrack that called methods on the provider, and not the
@@ -429,14 +437,25 @@ TEST_F(RtpSenderReceiverTest, AudioSenderTrackSetToNull) {
 }
 
 TEST_F(RtpSenderReceiverTest, VideoSenderTrackSetToNull) {
-  AddVideoTrack();
-  EXPECT_CALL(video_provider_, SetSource(kVideoSsrc, video_track_.get()));
+  rtc::scoped_refptr<VideoTrackSourceInterface> source(
+      FakeVideoTrackSource::Create());
+  rtc::scoped_refptr<VideoTrackInterface> track =
+      VideoTrack::Create(kVideoTrackId, source);
+  EXPECT_CALL(video_provider_, SetSource(kVideoSsrc, track.get()));
   EXPECT_CALL(video_provider_, SetVideoSend(kVideoSsrc, true, _));
   rtc::scoped_refptr<VideoRtpSender> sender =
-      new VideoRtpSender(video_track_, kStreamLabel1, &video_provider_);
+      new VideoRtpSender(track, kStreamLabel1, &video_provider_);
   sender->SetSsrc(kVideoSsrc);
 
-  EXPECT_CALL(video_provider_, SetSource(kVideoSsrc, nullptr)).Times(1);
+  // Expect that SetSource will be called before the reference to the track
+  // is released.
+  EXPECT_CALL(video_provider_, SetSource(kVideoSsrc, nullptr))
+      .Times(1)
+      .WillOnce(InvokeWithoutArgs([&track] {
+        EXPECT_LT(2, track->AddRef());
+        track->Release();
+        return true;
+      }));
   EXPECT_CALL(video_provider_, SetVideoSend(kVideoSsrc, false, _)).Times(1);
   EXPECT_TRUE(sender->SetTrack(nullptr));