Bug 856848 - Fix thread crash r=ehugg,ekr

media/mtransport/runnable_utils.h

@@ -26,14 +26,14 @@ class runnable_args_base : public nsRunnable {
 // which is why it is machine generated). The four templates
 // are:
 //
-// WrapRunnable(o, m, ...) -- wraps a member function m of an object ptr o 
+// WrapRunnable(o, m, ...) -- wraps a member function m of an object ptr o
 // WrapRunnableRet(o, m, ..., r) -- wraps a member function m of an object ptr o
 //                                  the function returns something that can
 //                                  be assigned to *r
 // WrapRunnableNM(f, ...) -- wraps a function f
 // WrapRunnableNMRet(f, ..., r) -- wraps a function f that returns something
 //                                 that can be assigned to *r
-// 
+//
 // All of these template functions return a Runnable* which can be passed
 // to Dispatch().
 #include "runnable_utils_generated.h"
@@ -45,7 +45,12 @@ static inline nsresult RUN_ON_THREAD(nsIEventTarget *thread, nsIRunnable *runnab
     bool on;
     nsresult rv;
     rv = thread->IsOnCurrentThread(&on);
-    MOZ_ASSERT(NS_SUCCEEDED(rv));
+
+    // If the target thread has already shut down, we don't want to assert.
+    if (rv != NS_ERROR_NOT_INITIALIZED) {
+      MOZ_ASSERT(NS_SUCCEEDED(rv));
+    }
+
     NS_ENSURE_SUCCESS(rv, rv);
     if(!on) {
       return thread->Dispatch(runnable_ref, flags);

media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp

@@ -277,7 +277,7 @@ PeerConnectionImpl::PeerConnectionImpl()
   , mWindow(NULL)
   , mIdentity(NULL)
   , mSTSThread(NULL)
-  , mMedia(new PeerConnectionMedia(this))
+  , mMedia(NULL)
   , mNumAudioStreams(0)
   , mNumVideoStreams(0)
   , mHaveDataStream(false) {
@@ -526,6 +526,8 @@ PeerConnectionImpl::Initialize(IPeerConnectionObserver* aObserver,
     return NS_ERROR_FAILURE;
   }
 
+  mMedia = new PeerConnectionMedia(this);
+
   // Connect ICE slots.
   mMedia->SignalIceGatheringCompleted.connect(this, &PeerConnectionImpl::IceGatheringCompleted);
   mMedia->SignalIceCompleted.connect(this, &PeerConnectionImpl::IceCompleted);

media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp

@@ -120,12 +120,17 @@ PeerConnectionImpl* PeerConnectionImpl::CreatePeerConnection()
 }
 
 
+PeerConnectionMedia::PeerConnectionMedia(PeerConnectionImpl *parent)
+    : mParent(parent),
+      mLocalSourceStreamsLock("PeerConnectionMedia.mLocalSourceStreamsLock"),
+      mIceCtx(NULL),
+      mDNSResolver(new mozilla::NrIceResolver()),
+      mMainThread(mParent->GetMainThread()),
+      mSTSThread(mParent->GetSTSThread()) {}
+
 nsresult PeerConnectionMedia::Init(const std::vector<NrIceStunServer>& stun_servers,
                                    const std::vector<NrIceTurnServer>& turn_servers)
 {
-  mMainThread = mParent->GetMainThread();
-  mSTSThread = mParent->GetSTSThread();
-
   // TODO(ekr@rtfm.com): need some way to set not offerer later
   // Looks like a bug in the NrIceCtx API.
   mIceCtx = NrIceCtx::Create("PC:" + mParent->GetHandle(), true);

media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h

@@ -230,12 +230,7 @@ RemoteSourceStreamInfo(already_AddRefed<DOMMediaStream> aMediaStream,
 
 class PeerConnectionMedia : public sigslot::has_slots<> {
  public:
-  PeerConnectionMedia(PeerConnectionImpl *parent)
-      : mParent(parent),
-      mLocalSourceStreamsLock("PeerConnectionMedia.mLocalSourceStreamsLock"),
-      mIceCtx(NULL),
-      mDNSResolver(new mozilla::NrIceResolver()) {}
-
+  PeerConnectionMedia(PeerConnectionImpl *parent);
   ~PeerConnectionMedia() {}
 
   nsresult Init(const std::vector<mozilla::NrIceStunServer>& stun_servers,

media/webrtc/signaling/test/signaling_unittests.cpp

@@ -528,25 +528,24 @@ class ParsedSDP {
 
 class SignalingAgent {
  public:
-  SignalingAgent() : pc(nullptr) {}
+  SignalingAgent() : pc(nullptr) {
+    cfg_.addStunServer("23.21.150.121", 3478);
+
+    pc = sipcc::PeerConnectionImpl::CreatePeerConnection();
+    ASSERT_TRUE(pc);
+  }
 
   ~SignalingAgent() {
-    mozilla::SyncRunnable::DispatchToThread(pc->GetMainThread(),
+    mozilla::SyncRunnable::DispatchToThread(gThread,
       WrapRunnable(this, &SignalingAgent::Close));
   }
 
   void Init_m(nsCOMPtr<nsIThread> thread)
   {
-    pc = sipcc::PeerConnectionImpl::CreatePeerConnection();
-    ASSERT_TRUE(pc);
-
     pObserver = new TestObserver(pc);
     ASSERT_TRUE(pObserver);
 
-    sipcc::IceConfiguration cfg;
-    cfg.addStunServer("23.21.150.121", 3478);
-    ASSERT_EQ(pc->Initialize(pObserver, nullptr, cfg, thread), NS_OK);
-
+    ASSERT_EQ(pc->Initialize(pObserver, nullptr, cfg_, thread), NS_OK);
   }
 
   void Init(nsCOMPtr<nsIThread> thread)
@@ -597,10 +596,12 @@ class SignalingAgent {
 
   void Close()
   {
-    cout << "Close" << endl;
-
-    pc->Close(false);
-    pc = nullptr;
+    if (pc) {
+      cout << "Close" << endl;
+      
+      pc->Close(false);
+      pc = nullptr;
+    }
 
     // Shutdown is synchronous evidently.
     // ASSERT_TRUE(pObserver->WaitForObserverCall());
@@ -849,6 +850,7 @@ public:
   char* offer_;
   char* answer_;
   nsRefPtr<DOMMediaStream> domMediaStream_;
+  sipcc::IceConfiguration cfg_;
 
 private:
   void SDPSanityCheck(std::string sdp, uint32_t flags, bool offer)
@@ -994,6 +996,19 @@ class SignalingAgentTest : public ::testing::Test {
     return true;
   }
 
+  void CreateAgentNoInit() {
+    ScopedDeletePtr<SignalingAgent> agent(new SignalingAgent());
+    agents_.push_back(agent.forget());
+  }
+
+  bool InitAgent(size_t i) {
+    return agents_[i]->InitAllowFail(gThread);
+  }
+
+  SignalingAgent *agent(size_t i) {
+    return agents_[i];
+  }
+
  private:
   std::vector<SignalingAgent *> agents_;
 };
@@ -1972,6 +1987,11 @@ TEST_F(SignalingAgentTest, CreateUntilFailThenWait) {
   PR_Sleep(10000);  // Wait to see if we crash
 }
 
+// Test for bug 856433.
+TEST_F(SignalingAgentTest, CreateNoInit) {
+  CreateAgentNoInit();
+}
+
 /*
  * Test for Bug 843595
  */