diff --git a/talk/app/webrtc/peerconnectionfactory.cc b/talk/app/webrtc/peerconnectionfactory.cc index dc14bfb567..89f62a6a81 100644 --- a/talk/app/webrtc/peerconnectionfactory.cc +++ b/talk/app/webrtc/peerconnectionfactory.cc @@ -183,12 +183,33 @@ PeerConnectionFactory::PeerConnectionFactory( // ASSERT(default_adm != NULL); } +// Deletes |thread| if it is not the current thread, else causes it to +// exit and delete itself after completing the currently processing +// message. +// +// NOTE: this is required because: +// 1) PeerConnection holds a ref on PeerConnectionFactory; and +// 2) PeerConnectionFactory may own the signaling & worker threads; and +// 3) PeerConnection is always destroyed on the signaling thread. +// As a result, if the last ref on PeerConnectionFactory is held by +// PeerConnection, a naive "delete signaling_thread_;" in +// ~PeerConnectionFactory() would result in deadlock. See +// https://code.google.com/p/webrtc/issues/detail?id=3100 for history. +static void DeleteOrRelease(talk_base::Thread* thread) { + if (thread->IsCurrent()) { + thread->Release(); // Causes thread to delete itself after Quit(). + thread->Quit(); + } else { + delete thread; // Calls thread->Stop() implicitly. + } +} + PeerConnectionFactory::~PeerConnectionFactory() { signaling_thread_->Clear(this); signaling_thread_->Send(this, MSG_TERMINATE_FACTORY); if (owns_ptrs_) { - delete signaling_thread_; - delete worker_thread_; + DeleteOrRelease(signaling_thread_); + DeleteOrRelease(worker_thread_); } } diff --git a/talk/app/webrtc/peerconnectionfactory_unittest.cc b/talk/app/webrtc/peerconnectionfactory_unittest.cc index 9f9d7881a8..4d21e1b26f 100644 --- a/talk/app/webrtc/peerconnectionfactory_unittest.cc +++ b/talk/app/webrtc/peerconnectionfactory_unittest.cc @@ -340,3 +340,24 @@ TEST_F(PeerConnectionFactoryTest, LocalRendering) { EXPECT_TRUE(capturer->CaptureFrame()); EXPECT_EQ(2, local_renderer.num_rendered_frames()); } + +// Test that no deadlock or ASSERT triggers on releasing the last +// reference to a PeerConnectionFactory (regression test for +// https://code.google.com/p/webrtc/issues/detail?id=3100). +TEST(PeerConnectionFactory2Test, ThreadTeardown) { + talk_base::scoped_refptr factory( + webrtc::CreatePeerConnectionFactory()); + NullPeerConnectionObserver observer; + talk_base::scoped_refptr pc( + factory->CreatePeerConnection( + webrtc::PeerConnectionInterface::IceServers(), + NULL, + NULL, + &observer)); + factory = NULL; + // Now |pc| holds the last ref to the factory (and thus its + // threads). If the next line, which causes |pc| to be freed, + // doesn't ASSERT (in Debug) or deadlock (in Release) then the test + // is successful. + pc = NULL; +}