Bug 1738931 - Unset remote SSRC in a direct task (stable state) to avoid re-entrancy. r=bwc

...rather than by unsetting the old remote ssrc on all other conduits that
already have it set.

With this patch we don't risk re-entrancy into UnsetRemoteSSRC (CallWrapper,
VideoConduit) or SetRemoteSSRCConfig (VideoConduit).

Differential Revision: https://phabricator.services.mozilla.com/D130270

dom/media/webrtc/libwebrtcglue/AudioConduit.cpp

@@ -331,13 +331,13 @@ std::vector<uint32_t> WebrtcAudioConduit::GetLocalSSRCs() const {
   return std::vector<uint32_t>(1, mRecvStreamConfig.rtp.local_ssrc);
 }
 
-bool WebrtcAudioConduit::OverrideRemoteSSRC(uint32_t ssrc) {
+bool WebrtcAudioConduit::OverrideRemoteSSRC(uint32_t aSsrc) {
   MOZ_ASSERT(mCallThread->IsOnCurrentThread());
 
-  if (mRecvStreamConfig.rtp.remote_ssrc == ssrc) {
+  if (mRecvStreamConfig.rtp.remote_ssrc == aSsrc) {
     return true;
   }
-  mRecvStreamConfig.rtp.remote_ssrc = ssrc;
+  mRecvStreamConfig.rtp.remote_ssrc = aSsrc;
 
   const bool wasReceiving = mRecvStreamRunning;
   const bool hadRecvStream = mRecvStream;

dom/media/webrtc/libwebrtcglue/AudioConduit.h

@@ -159,10 +159,10 @@ class WebrtcAudioConduit : public AudioSessionConduit,
    *
    * Call thread only.
    */
-  bool OverrideRemoteSSRC(uint32_t ssrc);
+  bool OverrideRemoteSSRC(uint32_t aSsrc);
 
  public:
-  void UnsetRemoteSSRC(uint32_t ssrc) override {}
+  void UnsetRemoteSSRC(uint32_t aSsrc) override {}
 
   Maybe<webrtc::AudioReceiveStream::Stats> GetReceiverStats() const override;
   Maybe<webrtc::AudioSendStream::Stats> GetSenderStats() const override;

dom/media/webrtc/libwebrtcglue/MediaConduitInterface.h

@@ -142,7 +142,7 @@ class MediaSessionConduit {
   virtual Ssrcs GetLocalSSRCs() const = 0;
 
   virtual Maybe<Ssrc> GetRemoteSSRC() const = 0;
-  virtual void UnsetRemoteSSRC(Ssrc ssrc) = 0;
+  virtual void UnsetRemoteSSRC(Ssrc aSsrc) = 0;
 
   virtual bool HasCodecPluginID(uint64_t aPluginID) const = 0;
 
@@ -358,8 +358,6 @@ class VideoSessionConduit : public MediaSessionConduit {
 
   virtual void DisableSsrcChanges() = 0;
 
-  void UnsetRemoteSSRC(Ssrc ssrc) override = 0;
-
   /**
    * Function to deliver a capture video frame for encoding and transport.
    * If the frame's timestamp is 0, it will be automatcally generated.

dom/media/webrtc/libwebrtcglue/VideoConduit.cpp

@@ -977,33 +977,42 @@ void WebrtcVideoConduit::CreateRecvStream() {
               mRecvStreamConfig.rtp.remote_ssrc);
 }
 
-void WebrtcVideoConduit::SetRemoteSSRCConfig(uint32_t ssrc, uint32_t rtxSsrc) {
+void WebrtcVideoConduit::NotifyUnsetCurrentRemoteSSRC() {
   MOZ_ASSERT(mCallThread->IsOnCurrentThread());
-  mMutex.AssertNotCurrentThreadOwns();
-  // Don't unset (and call back into) ourselves.
+  CSFLogDebug(LOGTAG, "%s (%p): Unsetting SSRC %u in other conduits",
+              __FUNCTION__, this, mRecvStreamConfig.rtp.remote_ssrc);
   mCall->UnregisterConduit(this);
-
-  CSFLogDebug(LOGTAG, "%s: SSRC %u (0x%x)", __FUNCTION__, ssrc, ssrc);
-  mCall->UnsetRemoteSSRC(ssrc);
-
-  // Remote SSRC set -- listen for unsets from other conduits.
+  mCall->UnsetRemoteSSRC(mRecvStreamConfig.rtp.remote_ssrc);
   mCall->RegisterConduit(this);
-
-  mRecvSSRC = mRecvStreamConfig.rtp.remote_ssrc = ssrc;
-  mRecvStreamConfig.rtp.rtx_ssrc = rtxSsrc;
 }
 
-void WebrtcVideoConduit::SetRemoteSSRCAndRestartAsNeeded(uint32_t ssrc,
-                                                         uint32_t rtxSsrc) {
+void WebrtcVideoConduit::SetRemoteSSRCConfig(uint32_t aSsrc,
+                                             uint32_t aRtxSsrc) {
   MOZ_ASSERT(mCallThread->IsOnCurrentThread());
-  mMutex.AssertNotCurrentThreadOwns();
 
-  if (mRecvStreamConfig.rtp.remote_ssrc == ssrc &&
-      mRecvStreamConfig.rtp.rtx_ssrc == rtxSsrc) {
+  CSFLogDebug(LOGTAG, "%s: SSRC %u (0x%x)", __FUNCTION__, aSsrc, aSsrc);
+
+  if (mRecvStreamConfig.rtp.remote_ssrc != aSsrc) {
+    nsCOMPtr<nsIDirectTaskDispatcher> dtd = do_QueryInterface(mCallThread);
+    MOZ_ALWAYS_SUCCEEDS(dtd->DispatchDirectTask(NewRunnableMethod(
+        "WebrtcVideoConduit::NotifyUnsetCurrentRemoteSSRC", this,
+        &WebrtcVideoConduit::NotifyUnsetCurrentRemoteSSRC)));
+  }
+
+  mRecvSSRC = mRecvStreamConfig.rtp.remote_ssrc = aSsrc;
+  mRecvStreamConfig.rtp.rtx_ssrc = aRtxSsrc;
+}
+
+void WebrtcVideoConduit::SetRemoteSSRCAndRestartAsNeeded(uint32_t aSsrc,
+                                                         uint32_t aRtxSsrc) {
+  MOZ_ASSERT(mCallThread->IsOnCurrentThread());
+
+  if (mRecvStreamConfig.rtp.remote_ssrc == aSsrc &&
+      mRecvStreamConfig.rtp.rtx_ssrc == aRtxSsrc) {
     return;
   }
 
-  SetRemoteSSRCConfig(ssrc, rtxSsrc);
+  SetRemoteSSRCConfig(aSsrc, aRtxSsrc);
 
   const bool wasReceiving = mEngineReceiving;
   const bool hadRecvStream = mRecvStream;
@@ -1066,12 +1075,12 @@ void WebrtcVideoConduit::EnsureLocalSSRC() {
   mRecvStreamConfig.rtp.local_ssrc = ssrcs[0];
 }
 
-void WebrtcVideoConduit::UnsetRemoteSSRC(uint32_t ssrc) {
+void WebrtcVideoConduit::UnsetRemoteSSRC(uint32_t aSsrc) {
   MOZ_ASSERT(mCallThread->IsOnCurrentThread());
   mMutex.AssertNotCurrentThreadOwns();
 
-  if (mRecvStreamConfig.rtp.remote_ssrc != ssrc &&
-      mRecvStreamConfig.rtp.rtx_ssrc != ssrc) {
+  if (mRecvStreamConfig.rtp.remote_ssrc != aSsrc &&
+      mRecvStreamConfig.rtp.rtx_ssrc != aSsrc) {
     return;
   }
 
@@ -1079,10 +1088,12 @@ void WebrtcVideoConduit::UnsetRemoteSSRC(uint32_t ssrc) {
   uint32_t our_ssrc = 0;
   do {
     our_ssrc = GenerateRandomSSRC();
-  } while (NS_WARN_IF(our_ssrc == ssrc) ||
-           NS_WARN_IF(std::any_of(
-               ssrcs.begin(), ssrcs.end(),
-               [&](const auto& aSsrc) { return our_ssrc == aSsrc; })));
+  } while (NS_WARN_IF(our_ssrc == aSsrc) ||
+           NS_WARN_IF(std::find(ssrcs.begin(), ssrcs.end(), our_ssrc) !=
+                      ssrcs.end()));
+
+  CSFLogDebug(LOGTAG, "%s (%p): Generated remote SSRC %u", __FUNCTION__, this,
+              our_ssrc);
 
   // There is a (tiny) chance that this new random ssrc will collide with some
   // other conduit's remote ssrc, in which case that conduit will choose a new
@@ -1145,6 +1156,7 @@ MediaConduitErrorCode WebrtcVideoConduit::Init() {
 
 void WebrtcVideoConduit::InitCall() {
   MOZ_ASSERT(mCallThread->IsOnCurrentThread());
+  mCall->RegisterConduit(this);
   mSendPluginCreated = mEncoderFactory->CreatedGmpPluginEvent().Connect(
       GetMainThreadSerialEventTarget(),
       [self = detail::RawPtr(this)](uint64_t aPluginID) {

dom/media/webrtc/libwebrtcglue/VideoConduit.h

@@ -157,9 +157,15 @@ class WebrtcVideoConduit
   Ssrcs GetLocalSSRCs() const override;
   Maybe<Ssrc> GetRemoteSSRC() const override;
 
-  void UnsetRemoteSSRC(uint32_t ssrc) override;
-  void SetRemoteSSRCConfig(uint32_t ssrc, uint32_t rtxSsrc);
-  void SetRemoteSSRCAndRestartAsNeeded(uint32_t ssrc, uint32_t rtxSsrc);
+  // Call thread.
+  void UnsetRemoteSSRC(uint32_t aSsrc) override;
+
+ private:
+  void NotifyUnsetCurrentRemoteSSRC();
+  void SetRemoteSSRCConfig(uint32_t aSsrc, uint32_t aRtxSsrc);
+  void SetRemoteSSRCAndRestartAsNeeded(uint32_t aSsrc, uint32_t aRtxSsrc);
+
+ public:
   // Creating a recv stream or a send stream requires a local ssrc to be
   // configured. This method will generate one if needed.
   void EnsureLocalSSRC();
@@ -317,8 +323,8 @@ class WebrtcVideoConduit
     explicit Control(const RefPtr<AbstractThread>& aCallThread);
   } mControl;
 
-  // WatchManager allowing Mirrors to trigger functions that will update the
-  // webrtc.org configuration.
+  // WatchManager allowing Mirrors and other watch targets to trigger functions
+  // that will update the webrtc.org configuration.
   WatchManager<WebrtcVideoConduit> mWatchManager;
 
   mutable Mutex mMutex;

media/webrtc/signaling/gtest/MockConduit.h

@@ -0,0 +1,60 @@
+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim:set ts=2 sw=2 sts=2 et cindent: */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#ifndef MEDIA_WEBRTC_SIGNALING_GTEST_MOCKCONDUIT_H_
+#define MEDIA_WEBRTC_SIGNALING_GTEST_MOCKCONDUIT_H_
+
+#include "gmock/gmock.h"
+#include "MediaConduitInterface.h"
+
+namespace webrtc {
+std::ostream& operator<<(std::ostream& aStream,
+                         const webrtc::Call::Stats& aObj) {
+  aStream << aObj.ToString(0);
+  return aStream;
+}
+}  // namespace webrtc
+
+namespace mozilla {
+class MockConduit : public MediaSessionConduit {
+ public:
+  MockConduit() = default;
+
+  MOCK_CONST_METHOD0(type, Type());
+  MOCK_METHOD1(SetTransportActive, void(bool));
+  MOCK_METHOD0(SenderRtpSendEvent, MediaEventSourceExc<MediaPacket>&());
+  MOCK_METHOD0(SenderRtcpSendEvent, MediaEventSourceExc<MediaPacket>&());
+  MOCK_METHOD0(ReceiverRtcpSendEvent, MediaEventSourceExc<MediaPacket>&());
+  MOCK_METHOD1(ConnectReceiverRtpEvent,
+               void(MediaEventSourceExc<MediaPacket, webrtc::RTPHeader>&));
+  MOCK_METHOD1(ConnectReceiverRtcpEvent,
+               void(MediaEventSourceExc<MediaPacket>&));
+  MOCK_METHOD1(ConnectSenderRtcpEvent, void(MediaEventSourceExc<MediaPacket>&));
+  MOCK_CONST_METHOD0(LastRtcpReceived, Maybe<DOMHighResTimeStamp>());
+  MOCK_CONST_METHOD1(RtpSendBaseSeqFor, Maybe<uint16_t>(uint32_t));
+  MOCK_CONST_METHOD0(GetNow, DOMHighResTimeStamp());
+  MOCK_CONST_METHOD0(GetTimestampMaker, dom::RTCStatsTimestampMaker&());
+  MOCK_CONST_METHOD0(GetLocalSSRCs, Ssrcs());
+  MOCK_CONST_METHOD0(GetRemoteSSRC, Maybe<Ssrc>());
+  MOCK_METHOD1(UnsetRemoteSSRC, void(Ssrc));
+  MOCK_CONST_METHOD1(HasCodecPluginID, bool(uint64_t));
+  MOCK_METHOD0(RtcpByeEvent, MediaEventSource<void>&());
+  MOCK_METHOD0(RtcpTimeoutEvent, MediaEventSource<void>&());
+  MOCK_METHOD3(SendRtp,
+               bool(const uint8_t*, size_t, const webrtc::PacketOptions&));
+  MOCK_METHOD2(SendSenderRtcp, bool(const uint8_t*, size_t));
+  MOCK_METHOD2(SendReceiverRtcp, bool(const uint8_t*, size_t));
+  MOCK_METHOD2(DeliverPacket, void(rtc::CopyOnWriteBuffer, PacketType));
+  MOCK_METHOD0(Shutdown, void());
+  MOCK_METHOD0(AsAudioSessionConduit, Maybe<RefPtr<AudioSessionConduit>>());
+  MOCK_METHOD0(AsVideoSessionConduit, Maybe<RefPtr<VideoSessionConduit>>());
+  MOCK_CONST_METHOD0(GetCallStats, Maybe<webrtc::Call::Stats>());
+
+  MOCK_CONST_METHOD0(GetUpstreamRtpSources, std::vector<webrtc::RtpSource>());
+};
+}  // namespace mozilla
+
+#endif

media/webrtc/signaling/gtest/videoconduit_unittests.cpp

@@ -20,6 +20,7 @@
 #include "media/base/video_adapter.h"
 
 #include "MockCall.h"
+#include "MockConduit.h"
 
 using namespace mozilla;
 using namespace testing;
@@ -1937,4 +1938,35 @@ TEST_F(VideoConduitTest, TestLocalAndRemoteSsrcCollision) {
             Call()->mVideoSendConfig->rtp.ssrcs[0]);
 }
 
+TEST_F(VideoConduitTest, TestExternalRemoteSsrcCollision) {
+  auto other = MakeRefPtr<MockConduit>();
+  mCallWrapper->RegisterConduit(other);
+
+  // First the mControl update should trigger an UnsetRemoteSSRC(1) from us.
+  // Then we simulate another conduit using that same ssrc, which should trigger
+  // us to generate a fresh ssrc that is not 0 and not 1.
+  {
+    InSequence s;
+    EXPECT_CALL(*other, UnsetRemoteSSRC(1U)).Times(2);
+    EXPECT_CALL(*other, UnsetRemoteSSRC(Not(testing::AnyOf(0U, 1U))));
+  }
+
+  mControl.Update([&](auto& aControl) {
+    aControl.mRemoteSsrc = 1;
+    aControl.mReceiving = true;
+  });
+  EXPECT_TRUE(Call()->mVideoReceiveConfig);
+  EXPECT_EQ(Call()->mVideoReceiveConfig->rtp.remote_ssrc, 1U);
+
+  mozilla::Unused << WaitFor(InvokeAsync(
+      GetCurrentSerialEventTarget(), __func__, [wrapper = mCallWrapper] {
+        wrapper->UnsetRemoteSSRC(1);
+        return GenericPromise::CreateAndResolve(true, __func__);
+      }));
+
+  EXPECT_TRUE(Call()->mVideoReceiveConfig);
+  EXPECT_THAT(Call()->mVideoReceiveConfig->rtp.remote_ssrc,
+              Not(testing::AnyOf(0U, 1U)));
+}
+
 }  // End namespace test.