Bug 864255: Move more of PeerConnectionMedia shutdown to occur synchronously r=ekr

dom/media/PeerConnection.js

@@ -83,7 +83,7 @@ GlobalPCList.prototype = {
         this._list[winID].forEach(function(pcref) {
           let pc = pcref.get();
           if (pc !== null) {
-            pc._pc.close(false);
+            pc._pc.close();
             delete pc._observer;
             pc._pc = null;
           }
@@ -92,8 +92,10 @@ GlobalPCList.prototype = {
       }
     } else if (topic == "profile-change-net-teardown" ||
                topic == "network:offline-about-to-go-offline") {
-      // Delete all peerconnections on shutdown - synchronously (we need
-      // them to be done deleting transports before we return)!
+      // Delete all peerconnections on shutdown - mostly synchronously (we
+      // need them to be done deleting transports and streams before we
+      // return)!  All socket operations must be queued to STS thread
+      // before we return to here.
       // Also kill them if "Work Offline" is selected - more can be created
       // while offline, but attempts to connect them should fail.
       let array;
@@ -101,7 +103,7 @@ GlobalPCList.prototype = {
         array.forEach(function(pcref) {
           let pc = pcref.get();
           if (pc !== null) {
-            pc._pc.close(true);
+            pc._pc.close();
             delete pc._observer;
             pc._pc = null;
           }

dom/media/bridge/IPeerConnection.idl

@@ -67,7 +67,7 @@ interface IPeerConnectionObserver : nsISupports
   void foundIceCandidate(in string candidate);
 };
 
-[scriptable, uuid(121ff773-949b-48b9-83b2-9a4ef908833c)]
+[scriptable, uuid(80b98a4f-c629-4e81-b738-a4608f6a4cd3)]
 interface IPeerConnection : nsISupports
 {
   const unsigned long kHintAudio = 0x00000001;
@@ -136,7 +136,7 @@ interface IPeerConnection : nsISupports
   void addIceCandidate(in string candidate, in string mid, in unsigned short level);
 
   /* Puts the SIPCC engine back to 'kIdle', shuts down threads, deletes state */
-  void close(in bool isSynchronous);
+  void close();
 
   /* Attributes */
   readonly attribute string localDescription;

media/webrtc/signaling/src/media-conduit/MediaConduitInterface.h

@@ -167,6 +167,7 @@ public:
    * and attaches the new to the Conduit.
    */
   virtual MediaConduitErrorCode AttachRenderer(RefPtr<VideoRenderer> aRenderer) = 0;
+  virtual void DetachRenderer() = 0;
 
   /**
    * Function to deliver a capture video frame for encoding and transport

media/webrtc/signaling/src/media-conduit/VideoConduit.cpp

@@ -29,7 +29,7 @@ mozilla::RefPtr<VideoSessionConduit> VideoSessionConduit::Create()
   {
     CSFLogError(logTag,  "%s VideoConduit Init Failed ", __FUNCTION__);
     delete obj;
-    return NULL;
+    return nullptr;
   }
   CSFLogDebug(logTag,  "%s Successfully created VideoConduit ", __FUNCTION__);
   return obj;
@@ -51,14 +51,16 @@ WebrtcVideoConduit::~WebrtcVideoConduit()
   {
     mPtrViECapture->DisconnectCaptureDevice(mCapId);
     mPtrViECapture->ReleaseCaptureDevice(mCapId);
-    mPtrExtCapture = NULL;
+    mPtrExtCapture = nullptr;
     mPtrViECapture->Release();
   }
 
   //Deal with External Renderer
   if(mPtrViERender)
   {
-    mPtrViERender->StopRender(mChannel);
+    if(mRenderer) {
+      mPtrViERender->StopRender(mChannel);
+    }
     mPtrViERender->RemoveRenderer(mChannel);
     mPtrViERender->Release();
   }
@@ -113,7 +115,7 @@ MediaConduitErrorCode WebrtcVideoConduit::Init()
       CSFLogError(logTag,  "%s: could not get Java environment", __FUNCTION__);
       return kMediaConduitSessionNotInited;
   }
-  jvm->AttachCurrentThread(&env, NULL);
+  jvm->AttachCurrentThread(&env, nullptr);
 
   webrtc::VideoEngine::SetAndroidObjects(jvm, (void*)context);
 
@@ -289,25 +291,37 @@ WebrtcVideoConduit::AttachRenderer(mozilla::RefPtr<VideoRenderer> aVideoRenderer
     MOZ_ASSERT(PR_FALSE);
     return kMediaConduitInvalidRenderer;
   }
-  //Assign the new renderer - overwrites if there is already one
-  mRenderer = aVideoRenderer;
 
   //Start Rendering if we haven't already
-  if(!mEngineRendererStarted)
+  if(!mRenderer)
   {
+    mRenderer = aVideoRenderer; // must be done before StartRender()
+
     if(mPtrViERender->StartRender(mChannel) == -1)
     {
       CSFLogError(logTag, "%s Starting the Renderer Failed %d ", __FUNCTION__,
                                                       mPtrViEBase->LastError());
-      mRenderer = NULL;
+      mRenderer = nullptr;
       return kMediaConduitRendererFail;
     }
-    mEngineRendererStarted = true;
+  } else {
+    //Assign the new renderer - overwrites if there is already one
+    mRenderer = aVideoRenderer;
   }
 
   return kMediaConduitNoError;
 }
 
+void
+WebrtcVideoConduit::DetachRenderer()
+{
+  if(mRenderer)
+  {
+    mPtrViERender->StopRender(mChannel);
+    mRenderer = nullptr;
+  }
+}
+
 MediaConduitErrorCode
 WebrtcVideoConduit::AttachTransport(mozilla::RefPtr<TransportInterface> aTransport)
 {

media/webrtc/signaling/src/media-conduit/VideoConduit.h

@@ -60,7 +60,8 @@ public:
    * Note: Multiple invocations of this API shall remove an existing renderer
    * and attaches the new to the Conduit.
    */
-   MediaConduitErrorCode AttachRenderer(mozilla::RefPtr<VideoRenderer> aVideoRenderer);
+  virtual MediaConduitErrorCode AttachRenderer(mozilla::RefPtr<VideoRenderer> aVideoRenderer);
+  virtual void DetachRenderer();
 
   /**
    * APIs used by the registered external transport to this Conduit to
@@ -150,7 +151,6 @@ public:
                       mRenderer(NULL),
                       mEngineTransmitting(false),
                       mEngineReceiving(false),
-                      mEngineRendererStarted(false),
                       mChannel(-1),
                       mCapId(-1),
                       mCurSendCodecConfig(NULL)
@@ -207,7 +207,6 @@ private:
   // Engine state we are concerned with.
   bool mEngineTransmitting; //If true ==> Transmit Sub-system is up and running
   bool mEngineReceiving;    // if true ==> Receive Sus-sysmtem up and running
-  bool mEngineRendererStarted; // If true ==> Rendering Sub-system is up and running
 
   int mChannel; // Video Channel for this conduit
   int mCapId;   // Capturer for this conduit

media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp

@@ -112,10 +112,12 @@ nsresult MediaPipeline::Init_s() {
 }
 
 
-// Disconnect us from the transport so that we can cleanly destruct
-// the pipeline on the main thread.
+// Disconnect us from the transport so that we can cleanly destruct the
+// pipeline on the main thread.  ShutdownMedia_m() must have already been
+// called
 void MediaPipeline::ShutdownTransport_s() {
   ASSERT_ON_THREAD(sts_thread_);
+  MOZ_ASSERT(!stream_); // verifies that ShutdownMedia_m() has run
 
   disconnect_all();
   transport_->Detach();
@@ -1020,6 +1022,7 @@ nsresult MediaPipelineReceiveVideo::Init() {
   listener_->AddSelf(new VideoSegment());
 #endif
 
+  // Always happens before we can DetachMediaStream()
   static_cast<VideoSessionConduit *>(conduit_.get())->
       AttachRenderer(renderer_);
 

media/webrtc/signaling/src/mediapipeline/MediaPipeline.h

@@ -101,19 +101,13 @@ class MediaPipeline : public sigslot::has_slots<> {
     MOZ_ASSERT(!stream_);  // Check that we have shut down already.
   }
 
-
-
-  // Must be called on the STS thread. Must be called
-  // before ShutdownMedia_m.
+  // Must be called on the STS thread.  Must be called after ShutdownMedia_m().
   void ShutdownTransport_s();
 
   // Must be called on the main thread.
   void ShutdownMedia_m() {
     ASSERT_ON_THREAD(main_thread_);
 
-    MOZ_ASSERT(!rtp_transport_);
-    MOZ_ASSERT(!rtcp_transport_);
-
     if (stream_) {
       DetachMediaStream();
     }
@@ -318,9 +312,7 @@ class MediaPipelineTransmit : public MediaPipeline {
   virtual void DetachMediaStream() {
     ASSERT_ON_THREAD(main_thread_);
     stream_->RemoveListener(listener_);
-    // Remove our reference so that when the MediaStreamGraph
-    // releases the listener, it will be destroyed.
-    listener_ = nullptr;
+    // Let the listener be destroyed with the pipeline (or later).
     stream_ = nullptr;
   }
 
@@ -434,9 +426,6 @@ class MediaPipelineReceiveAudio : public MediaPipelineReceive {
     ASSERT_ON_THREAD(main_thread_);
     listener_->EndTrack();
     stream_->RemoveListener(listener_);
-    // Remove our reference so that when the MediaStreamGraph
-    // releases the listener, it will be destroyed.
-    listener_ = nullptr;
     stream_ = nullptr;
   }
 
@@ -500,15 +489,12 @@ class MediaPipelineReceiveVideo : public MediaPipelineReceive {
     ASSERT_ON_THREAD(main_thread_);
 
     listener_->EndTrack();
-
-    conduit_ = nullptr;  // Force synchronous destruction so we
-                         // stop generating video.
-
+    // stop generating video and thus stop invoking the PipelineRenderer
+    // and PipelineListener - the renderer has a raw ptr to the Pipeline to
+    // avoid cycles, and the render callbacks are invoked from a different
+    // thread so simple null-checks would cause TSAN bugs without locks.
+    static_cast<VideoSessionConduit*>(conduit_.get())->DetachRenderer();
     stream_->RemoveListener(listener_);
-    // Remove our reference so that when the MediaStreamGraph
-    // releases the listener, it will be destroyed.
-    listener_ = nullptr;
-
     stream_ = nullptr;
   }
 

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

@@ -297,7 +297,7 @@ PeerConnectionImpl::~PeerConnectionImpl()
   }
 
   CSFLogInfo(logTag, "%s: PeerConnectionImpl destructor invoked", __FUNCTION__);
-  CloseInt(false);
+  CloseInt();
 
 #ifdef MOZILLA_INTERNAL_API
   // Deregister as an NSS Shutdown Object
@@ -1202,17 +1202,17 @@ PeerConnectionImpl::CheckApiState(bool assert_ice_ready) const
 }
 
 NS_IMETHODIMP
-PeerConnectionImpl::Close(bool aIsSynchronous)
+PeerConnectionImpl::Close()
 {
   CSFLogDebug(logTag, "%s", __FUNCTION__);
   PC_AUTO_ENTER_API_CALL_NO_CHECK();
 
-  return CloseInt(aIsSynchronous);
+  return CloseInt();
 }
 
 
 nsresult
-PeerConnectionImpl::CloseInt(bool aIsSynchronous)
+PeerConnectionImpl::CloseInt()
 {
   PC_AUTO_ENTER_API_CALL_NO_CHECK();
 
@@ -1228,7 +1228,7 @@ PeerConnectionImpl::CloseInt(bool aIsSynchronous)
   }
 #endif
 
-  ShutdownMedia(aIsSynchronous);
+  ShutdownMedia();
 
   // DataConnection will need to stay alive until all threads/runnables exit
 
@@ -1236,7 +1236,7 @@ PeerConnectionImpl::CloseInt(bool aIsSynchronous)
 }
 
 void
-PeerConnectionImpl::ShutdownMedia(bool aIsSynchronous)
+PeerConnectionImpl::ShutdownMedia()
 {
   PC_AUTO_ENTER_API_CALL_NO_CHECK();
 

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

@@ -281,7 +281,7 @@ private:
   NS_IMETHODIMP CreateAnswerInt(MediaConstraints& constraints);
   NS_IMETHODIMP EnsureDataConnection(uint16_t aNumstreams);
 
-  nsresult CloseInt(bool aIsSynchronous);
+  nsresult CloseInt();
   void ChangeReadyState(ReadyState aReadyState);
   nsresult CheckApiState(bool assert_ice_ready) const;
   void CheckThread() const {
@@ -303,8 +303,8 @@ private:
   void virtualDestroyNSSReference() MOZ_FINAL;
 #endif
 
-  // Shut down media. Called on any thread.
-  void ShutdownMedia(bool isSynchronous);
+  // Shut down media - called on main thread only
+  void ShutdownMedia();
 
   // ICE callbacks run on the right thread.
   nsresult IceStateChange_m(IceState aState);

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

@@ -83,6 +83,9 @@ void LocalSourceStreamInfo::DetachMedia_m()
        ++it) {
     it->second->ShutdownMedia_m();
   }
+  mAudioTracks.Clear();
+  mVideoTracks.Clear();
+  mMediaStream = nullptr;
 }
 
 void RemoteSourceStreamInfo::DetachTransport_s()
@@ -108,6 +111,7 @@ void RemoteSourceStreamInfo::DetachMedia_m()
        ++it) {
     it->second->ShutdownMedia_m();
   }
+  mMediaStream = nullptr;
 }
 
 PeerConnectionImpl* PeerConnectionImpl::CreatePeerConnection()
@@ -297,7 +301,16 @@ PeerConnectionMedia::SelfDestruct()
 
   CSFLogDebug(logTag, "%s: ", __FUNCTION__);
 
-  // Shutdown the transport.
+  // Shut down the media
+  for (uint32_t i=0; i < mLocalSourceStreams.Length(); ++i) {
+    mLocalSourceStreams[i]->DetachMedia_m();
+  }
+
+  for (uint32_t i=0; i < mRemoteSourceStreams.Length(); ++i) {
+    mRemoteSourceStreams[i]->DetachMedia_m();
+  }
+
+  // Shutdown the transport (async)
   RUN_ON_THREAD(mSTSThread, WrapRunnable(
       this, &PeerConnectionMedia::ShutdownMediaTransport_s),
                 NS_DISPATCH_NORMAL);
@@ -308,19 +321,9 @@ PeerConnectionMedia::SelfDestruct()
 void
 PeerConnectionMedia::SelfDestruct_m()
 {
-  ASSERT_ON_THREAD(mMainThread);
-
   CSFLogDebug(logTag, "%s: ", __FUNCTION__);
 
-  // Shut down the media
-  for (uint32_t i=0; i < mLocalSourceStreams.Length(); ++i) {
-    mLocalSourceStreams[i]->DetachMedia_m();
-  }
-
-  for (uint32_t i=0; i < mRemoteSourceStreams.Length(); ++i) {
-    mRemoteSourceStreams[i]->DetachMedia_m();
-  }
-
+  ASSERT_ON_THREAD(mMainThread);
   mLocalSourceStreams.Clear();
   mRemoteSourceStreams.Clear();
 

media/webrtc/signaling/test/signaling_unittests.cpp

@@ -599,7 +599,7 @@ class SignalingAgent {
     if (pc) {
       cout << "Close" << endl;
 
-      pc->Close(false);
+      pc->Close();
       pc = nullptr;
     }