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}diff --git a/pc/remoteaudiosource.cc b/pc/remoteaudiosource.cc
index 6fcc2ab..c484095 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::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 @@
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 3759d6f..d35fb04 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 @@
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 @@
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 1bc0c0c..41ba04e 100644
--- a/rtc_base/thread.cc
+++ b/rtc_base/thread.cc
@@ -529,6 +529,7 @@
AutoThread::~AutoThread() {
Stop();
+ DoDestroy();
if (ThreadManager::Instance()->CurrentThread() == this) {
ThreadManager::Instance()->SetCurrentThread(nullptr);
}
@@ -550,6 +551,12 @@
// 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 7e38732..65a1752 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.*