From 3b80aace615be848a6e9dd47f60ab449b711f16e Mon Sep 17 00:00:00 2001
From: Steve Anton <steveanton@webrtc.org>
Date: Thu, 19 Oct 2017 10:17:12 -0700
Subject: [PATCH] Fix flaky memory leak in RemoteAudioSource

Bug: webrtc:8405
Change-Id: Ie7c89214323678c6ea34e344bb1a5a33ad46b3f0
Reviewed-on: https://webrtc-review.googlesource.com/13401
Commit-Queue: Steve Anton <steveanton@webrtc.org>
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#20362}
---
 pc/remoteaudiosource.cc                       | 27 +++++++------------
 pc/remoteaudiosource.h                        |  7 ++---
 rtc_base/thread.cc                            |  7 +++++
 ...eerconnection_unittests.gtest-memcheck.txt |  3 ---
 4 files changed, 21 insertions(+), 23 deletions(-)

diff --git a/pc/remoteaudiosource.cc b/pc/remoteaudiosource.cc
index 6fcc2aba82..c484095fbf 100644
--- a/pc/remoteaudiosource.cc
+++ b/pc/remoteaudiosource.cc
@@ -22,22 +22,6 @@
 
 namespace webrtc {
 
-class RemoteAudioSource::MessageHandler : public rtc::MessageHandler {
- public:
-  explicit MessageHandler(RemoteAudioSource* source) : source_(source) {}
-
- private:
-  ~MessageHandler() override {}
-
-  void OnMessage(rtc::Message* msg) override {
-    source_->OnMessage(msg);
-    delete this;
-  }
-
-  const rtc::scoped_refptr<RemoteAudioSource> source_;
-  RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(MessageHandler);
-};
-
 class RemoteAudioSource::Sink : public AudioSinkInterface {
  public:
   explicit Sink(RemoteAudioSource* source) : source_(source) {}
@@ -148,7 +132,13 @@ void RemoteAudioSource::OnData(const AudioSinkInterface::Data& audio) {
 void RemoteAudioSource::OnAudioChannelGone() {
   // Called when the audio channel is deleted.  It may be the worker thread
   // in libjingle or may be a different worker thread.
-  main_thread_->Post(RTC_FROM_HERE, new MessageHandler(this));
+  // This object needs to live long enough for the cleanup logic in OnMessage to
+  // run, so take a reference to it as the data. Sometimes the message may not
+  // be processed (because the thread was destroyed shortly after this call),
+  // but that is fine because the thread destructor will take care of destroying
+  // the message data which will release the reference on RemoteAudioSource.
+  main_thread_->Post(RTC_FROM_HERE, this, 0,
+                     new rtc::ScopedRefMessageData<RemoteAudioSource>(this));
 }
 
 void RemoteAudioSource::OnMessage(rtc::Message* msg) {
@@ -156,6 +146,9 @@ void RemoteAudioSource::OnMessage(rtc::Message* msg) {
   sinks_.clear();
   state_ = MediaSourceInterface::kEnded;
   FireOnChanged();
+  // Will possibly delete this RemoteAudioSource since it is reference counted
+  // in the message.
+  delete msg->pdata;
 }
 
 }  // namespace webrtc
diff --git a/pc/remoteaudiosource.h b/pc/remoteaudiosource.h
index 3759d6fbc3..d35fb04ed5 100644
--- a/pc/remoteaudiosource.h
+++ b/pc/remoteaudiosource.h
@@ -18,6 +18,7 @@
 #include "api/notifier.h"
 #include "pc/channel.h"
 #include "rtc_base/criticalsection.h"
+#include "rtc_base/messagehandler.h"
 
 namespace rtc {
 struct Message;
@@ -27,7 +28,8 @@ class Thread;
 namespace webrtc {
 
 // This class implements the audio source used by the remote audio track.
-class RemoteAudioSource : public Notifier<AudioSourceInterface> {
+class RemoteAudioSource : public Notifier<AudioSourceInterface>,
+                          rtc::MessageHandler {
  public:
   // Creates an instance of RemoteAudioSource.
   static rtc::scoped_refptr<RemoteAudioSource> Create(
@@ -61,8 +63,7 @@ class RemoteAudioSource : public Notifier<AudioSourceInterface> {
   void OnData(const AudioSinkInterface::Data& audio);
   void OnAudioChannelGone();
 
-  class MessageHandler;
-  void OnMessage(rtc::Message* msg);
+  void OnMessage(rtc::Message* msg) override;
 
   AudioObserverList audio_observers_;
   rtc::CriticalSection sink_lock_;
diff --git a/rtc_base/thread.cc b/rtc_base/thread.cc
index 1bc0c0cd85..41ba04e5ce 100644
--- a/rtc_base/thread.cc
+++ b/rtc_base/thread.cc
@@ -529,6 +529,7 @@ AutoThread::AutoThread() : Thread(SocketServer::CreateDefault()) {
 
 AutoThread::~AutoThread() {
   Stop();
+  DoDestroy();
   if (ThreadManager::Instance()->CurrentThread() == this) {
     ThreadManager::Instance()->SetCurrentThread(nullptr);
   }
@@ -550,6 +551,12 @@ AutoSocketServerThread::~AutoSocketServerThread() {
   // P2PTransportChannelPingTest, relying on the message posted in
   // cricket::Connection::Destroy.
   ProcessMessages(0);
+  // Stop and destroy the thread before clearing it as the current thread.
+  // Sometimes there are messages left in the MessageQueue that will be
+  // destroyed by DoDestroy, and sometimes the destructors of the message and/or
+  // its contents rely on this thread still being set as the current thread.
+  Stop();
+  DoDestroy();
   rtc::ThreadManager::Instance()->SetCurrentThread(old_thread_);
   if (old_thread_) {
     MessageQueueManager::Add(old_thread_);
diff --git a/tools_webrtc/valgrind/gtest_exclude/peerconnection_unittests.gtest-memcheck.txt b/tools_webrtc/valgrind/gtest_exclude/peerconnection_unittests.gtest-memcheck.txt
index 7e38732185..65a1752090 100644
--- a/tools_webrtc/valgrind/gtest_exclude/peerconnection_unittests.gtest-memcheck.txt
+++ b/tools_webrtc/valgrind/gtest_exclude/peerconnection_unittests.gtest-memcheck.txt
@@ -1,10 +1,7 @@
 # Tests that are failing when run under memcheck.
 # https://code.google.com/p/webrtc/issues/detail?id=4387
 DtmfSenderTest.*
-PeerConnectionCryptoUnitTest.*
 PeerConnectionEndToEndTest.*
-PeerConnectionIceUnitTest.*
 PeerConnectionIntegrationTest.*
 PeerConnectionInterfaceTest.*
-PeerConnectionRtpTest.*
 RTCStatsIntegrationTest.*