PeerConnectionFactory: delay deletion of owned threads.

Since PeerConnection holds a ref to its creating PeerConnectionFactory, it's
possible for ~PeerConnectionFactory() to be run on its signaling thread.
Deleting a thread from within that thread is sad times, so don't do it.

It would be nicer to avoid having PeerConnection hold a ref to the factory,
and instead require the user to keep the factory alive.  Unfortunately that
changes the contract on PeerConnection{,Factory} and it's unclear how to vet
existing callers for safety.

BUG=3100
R=juberti@webrtc.org, noahric@google.com

Review URL: https://webrtc-codereview.appspot.com/11289004

git-svn-id: http://webrtc.googlecode.com/svn/trunk@5933 4adac7df-926f-26a2-2b94-8c16560cd09d

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_);
   }
 }
 

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<PeerConnectionFactoryInterface> factory(
+      webrtc::CreatePeerConnectionFactory());
+  NullPeerConnectionObserver observer;
+  talk_base::scoped_refptr<PeerConnectionInterface> 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;
+}