Bug 1769802: Key JsepTransceiver by UUID instead of a simple integer index. r=mjf

The integer index we're replacing here is based on the order in which
transceivers were added. If we clone the JSEP engine for an sRD that happens to
result in the creation of a transceiver, and at the same time JS calls
addTransceiver, we have a situation where the sRD transceiver is added first
to the cloned JSEP engine, but the addTransceiver transceiver is added first
to the old JSEP engine, resulting in them having the same index. So, let's just
use a proper key for this stuff.

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

Author: docfaraday

dom/media/webrtc/jsapi/PeerConnectionImpl.cpp

@@ -851,8 +851,7 @@ nsresult PeerConnectionImpl::GetDatachannelParameters(
   transportId->clear();
 
   RefPtr<JsepTransceiver> datachannelTransceiver;
-  for (const auto& [id, transceiver] : mJsepSession->GetTransceivers()) {
-    (void)id;  // Lame, but no better way to do this right now.
+  for (const auto& transceiver : mJsepSession->GetTransceivers()) {
     if ((transceiver->GetMediaType() == SdpMediaSection::kApplication) &&
         transceiver->mSendTrack.GetNegotiatedDetails()) {
       datachannelTransceiver = transceiver;
@@ -958,18 +957,8 @@ already_AddRefed<RTCRtpTransceiver> PeerConnectionImpl::AddTransceiver(
     return nullptr;
   }
 
-  RefPtr<JsepTransceiver> jsepTransceiver = new JsepTransceiver(*type);
-
-  RefPtr<RTCRtpTransceiver> transceiver =
-      CreateTransceiver(jsepTransceiver, aInit, aSendTrack, aRv);
-
-  if (aRv.Failed()) {
-    // Would be nice if we could peek at the rv without stealing it, so we
-    // could log...
-    CSFLogError(LOGTAG, "%s: failed", __FUNCTION__);
-    return nullptr;
-  }
-
+  RefPtr<JsepTransceiver> jsepTransceiver =
+      new JsepTransceiver(*type, *mUuidGen);
   jsepTransceiver->SetRtxIsAllowed(mRtxIsAllowed);
 
   // Do this last, since it is not possible to roll back.
@@ -981,6 +970,18 @@ already_AddRefed<RTCRtpTransceiver> PeerConnectionImpl::AddTransceiver(
     return nullptr;
   }
 
+  RefPtr<RTCRtpTransceiver> transceiver = CreateTransceiver(
+      jsepTransceiver->GetUuid(),
+      jsepTransceiver->GetMediaType() == SdpMediaSection::kVideo, aInit,
+      aSendTrack, aRv);
+
+  if (aRv.Failed()) {
+    // Would be nice if we could peek at the rv without stealing it, so we
+    // could log...
+    CSFLogError(LOGTAG, "%s: failed", __FUNCTION__);
+    return nullptr;
+  }
+
   mTransceivers.AppendElement(transceiver);
   return transceiver.forget();
 }
@@ -1073,17 +1074,16 @@ PeerConnectionImpl::CreateDataChannel(
   CSFLogDebug(LOGTAG, "%s: making DOMDataChannel", __FUNCTION__);
 
   RefPtr<JsepTransceiver> dcTransceiver;
-  for (auto& [id, transceiver] : mJsepSession->GetTransceivers()) {
-    (void)id;  // Lame, but no better way to do this right now.
+  for (auto& transceiver : mJsepSession->GetTransceivers()) {
     if (transceiver->GetMediaType() == SdpMediaSection::kApplication) {
       dcTransceiver = transceiver;
       break;
     }
   }
 
   if (!dcTransceiver) {
-    dcTransceiver =
-        new JsepTransceiver(SdpMediaSection::MediaType::kApplication);
+    dcTransceiver = new JsepTransceiver(
+        SdpMediaSection::MediaType::kApplication, *mUuidGen);
     mJsepSession->AddTransceiver(dcTransceiver);
   }
 
@@ -1609,25 +1609,37 @@ PeerConnectionImpl::SetRemoteDescription(int32_t action, const char* aSDP) {
     mPCObserver->OnSetDescriptionError(*buildJSErrorData(result, errorString),
                                        jrv);
   } else {
-    for (const auto& [id, jsepTransceiver] : mJsepSession->GetTransceivers()) {
+    for (const auto& jsepTransceiver : mJsepSession->GetTransceivers()) {
       if (jsepTransceiver->GetMediaType() ==
           SdpMediaSection::MediaType::kApplication) {
         continue;
       }
 
-      if (originalTransceivers.count(id)) {
-        continue;
+      CSFLogDebug(LOGTAG, "%s: Looking for match", __FUNCTION__);
+      RefPtr<RTCRtpTransceiver> transceiver;
+      for (auto& temp : mTransceivers) {
+        if (temp->GetJsepTransceiverId() == jsepTransceiver->GetUuid()) {
+          CSFLogDebug(LOGTAG, "%s: Found match", __FUNCTION__);
+          transceiver = temp;
+          break;
+        }
       }
 
-      dom::RTCRtpTransceiverInit init;
-      init.mDirection = RTCRtpTransceiverDirection::Recvonly;
-      RefPtr<RTCRtpTransceiver> transceiver =
-          CreateTransceiver(jsepTransceiver, init, nullptr, jrv);
-      if (jrv.Failed()) {
-        appendHistory();
-        return NS_ERROR_FAILURE;
+      if (!transceiver) {
+        CSFLogDebug(LOGTAG, "%s: No match, making new", __FUNCTION__);
+        dom::RTCRtpTransceiverInit init;
+        init.mDirection = RTCRtpTransceiverDirection::Recvonly;
+        IgnoredErrorResult rv;
+        transceiver = CreateTransceiver(
+            jsepTransceiver->GetUuid(),
+            jsepTransceiver->GetMediaType() == SdpMediaSection::kVideo, init,
+            nullptr, rv);
+        if (NS_WARN_IF(rv.Failed())) {
+          MOZ_ASSERT(false);
+          break;
+        }
+        mTransceivers.AppendElement(transceiver);
       }
-      mTransceivers.AppendElement(transceiver);
     }
 
     if (wasRestartingIce) {
@@ -3213,8 +3225,7 @@ bool PeerConnectionImpl::ShouldForceProxy() const {
 }
 
 void PeerConnectionImpl::EnsureTransports(const JsepSession& aSession) {
-  for (const auto& [id, transceiver] : aSession.GetTransceivers()) {
-    (void)id;  // Lame, but no better way to do this right now.
+  for (const auto& transceiver : aSession.GetTransceivers()) {
     if (transceiver->HasOwnTransport()) {
       mTransportHandler->EnsureProvisionalTransport(
           transceiver->mTransport.mTransportId,
@@ -3264,8 +3275,7 @@ void PeerConnectionImpl::RemoveRTCDtlsTransportsExcept(
 nsresult PeerConnectionImpl::UpdateTransports(const JsepSession& aSession,
                                               const bool forceIceTcp) {
   std::set<std::string> finalTransports;
-  for (const auto& [id, transceiver] : aSession.GetTransceivers()) {
-    (void)id;  // Lame, but no better way to do this right now.
+  for (const auto& transceiver : aSession.GetTransceivers()) {
     if (transceiver->HasOwnTransport()) {
       finalTransports.insert(transceiver->mTransport.mTransportId);
       UpdateTransport(*transceiver, forceIceTcp);
@@ -3636,7 +3646,7 @@ void PeerConnectionImpl::EnsureIceGathering(bool aDefaultRouteOnly,
 }
 
 already_AddRefed<dom::RTCRtpTransceiver> PeerConnectionImpl::CreateTransceiver(
-    JsepTransceiver* aJsepTransceiver, const RTCRtpTransceiverInit& aInit,
+    const std::string& aId, bool aIsVideo, const RTCRtpTransceiverInit& aInit,
     dom::MediaStreamTrack* aSendTrack, ErrorResult& aRv) {
   PeerConnectionCtx* ctx = PeerConnectionCtx::GetInstance();
   if (!mCall) {
@@ -3649,8 +3659,8 @@ already_AddRefed<dom::RTCRtpTransceiver> PeerConnectionImpl::CreateTransceiver(
   }
 
   RefPtr<RTCRtpTransceiver> transceiver = new RTCRtpTransceiver(
-      mWindow, PrivacyNeeded(), this, mTransportHandler, aJsepTransceiver,
-      mSTSThread.get(), aSendTrack, mCall.get(), mIdGenerator);
+      mWindow, PrivacyNeeded(), this, mTransportHandler, mJsepSession.get(),
+      aId, aIsVideo, mSTSThread.get(), aSendTrack, mCall.get(), mIdGenerator);
 
   transceiver->Init(aInit, aRv);
   if (aRv.Failed()) {

dom/media/webrtc/jsapi/PeerConnectionImpl.h

@@ -126,6 +126,9 @@ class RemoteSourceStreamInfo;
 class PCUuidGenerator : public mozilla::JsepUuidGenerator {
  public:
   virtual bool Generate(std::string* idp) override;
+  virtual mozilla::JsepUuidGenerator* Clone() const override {
+    return new PCUuidGenerator(*this);
+  }
 
  private:
   nsCOMPtr<nsIUUIDGenerator> mGenerator;
@@ -725,7 +728,7 @@ class PeerConnectionImpl final
   nsresult UpdateMediaPipelines();
 
   already_AddRefed<dom::RTCRtpTransceiver> CreateTransceiver(
-      JsepTransceiver* aJsepTransceiver,
+      const std::string& aId, bool aIsVideo,
       const dom::RTCRtpTransceiverInit& aInit,
       dom::MediaStreamTrack* aSendTrack, ErrorResult& aRv);
 

dom/media/webrtc/jsapi/RTCRtpTransceiver.cpp

@@ -156,13 +156,15 @@ NS_INTERFACE_MAP_END
 
 RTCRtpTransceiver::RTCRtpTransceiver(
     nsPIDOMWindowInner* aWindow, bool aPrivacyNeeded, PeerConnectionImpl* aPc,
-    MediaTransportHandler* aTransportHandler, JsepTransceiver* aJsepTransceiver,
+    MediaTransportHandler* aTransportHandler, JsepSession* aJsepSession,
+    const std::string& aTransceiverId, bool aIsVideo,
     nsISerialEventTarget* aStsThread, dom::MediaStreamTrack* aSendTrack,
     WebrtcCallWrapper* aCallWrapper, RTCStatsIdGenerator* aIdGenerator)
     : mWindow(aWindow),
       mPc(aPc),
       mTransportHandler(aTransportHandler),
-      mJsepTransceiver(aJsepTransceiver),
+      mTransceiverId(aTransceiverId),
+      mJsepTransceiver(aJsepSession->GetTransceiver(mTransceiverId)),
       mStsThread(aStsThread),
       mCallWrapper(aCallWrapper),
       mSendTrack(aSendTrack),

dom/media/webrtc/jsapi/RTCRtpTransceiver.h

@@ -12,15 +12,14 @@
 #include "nsTArray.h"
 #include "mozilla/dom/MediaStreamTrack.h"
 #include "ErrorList.h"
-#include "jsep/JsepTransceiver.h"
+#include "jsep/JsepSession.h"
 #include "transport/transportlayer.h"  // For TransportLayer::State
 #include "mozilla/dom/RTCRtpTransceiverBinding.h"
 
 class nsIPrincipal;
 
 namespace mozilla {
 class PeerIdentity;
-class JsepTransceiver;
 class MediaSessionConduit;
 class VideoSessionConduit;
 class AudioSessionConduit;
@@ -58,14 +57,12 @@ class RTCRtpTransceiver : public nsISupports,
   /**
    * |aSendTrack| might or might not be set.
    */
-  RTCRtpTransceiver(nsPIDOMWindowInner* aWindow, bool aPrivacyNeeded,
-                    PeerConnectionImpl* aPc,
-                    MediaTransportHandler* aTransportHandler,
-                    JsepTransceiver* aJsepTransceiver,
-                    nsISerialEventTarget* aStsThread,
-                    MediaStreamTrack* aSendTrack,
-                    WebrtcCallWrapper* aCallWrapper,
-                    RTCStatsIdGenerator* aIdGenerator);
+  RTCRtpTransceiver(
+      nsPIDOMWindowInner* aWindow, bool aPrivacyNeeded, PeerConnectionImpl* aPc,
+      MediaTransportHandler* aTransportHandler, JsepSession* aJsepSession,
+      const std::string& aTransceiverId, bool aIsVideo,
+      nsISerialEventTarget* aStsThread, MediaStreamTrack* aSendTrack,
+      WebrtcCallWrapper* aCallWrapper, RTCStatsIdGenerator* aIdGenerator);
 
   void Init(const RTCRtpTransceiverInit& aInit, ErrorResult& aRv);
 
@@ -145,6 +142,8 @@ class RTCRtpTransceiver : public nsISupports,
 
   MediaSessionConduit* GetConduit() const { return mConduit; }
 
+  const std::string& GetJsepTransceiverId() const { return mTransceiverId; }
+
   // nsISupports
   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
   NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(RTCRtpTransceiver)
@@ -188,7 +187,8 @@ class RTCRtpTransceiver : public nsISupports,
   nsCOMPtr<nsPIDOMWindowInner> mWindow;
   RefPtr<PeerConnectionImpl> mPc;
   RefPtr<MediaTransportHandler> mTransportHandler;
-  const RefPtr<JsepTransceiver> mJsepTransceiver;
+  const std::string mTransceiverId;
+  RefPtr<JsepTransceiver> mJsepTransceiver;
   nsCOMPtr<nsISerialEventTarget> mStsThread;
   // state for webrtc.org that is shared between all transceivers
   RefPtr<WebrtcCallWrapper> mCallWrapper;

dom/media/webrtc/jsep/JsepSession.h

@@ -107,8 +107,7 @@ class JsepSession {
   template <class UnaryFunction>
   void ForEachCodec(UnaryFunction& function) {
     std::for_each(Codecs().begin(), Codecs().end(), function);
-    for (auto& [id, transceiver] : GetTransceivers()) {
-      (void)id;  // Lame, but no better way to do this right now.
+    for (auto& transceiver : GetTransceivers()) {
       transceiver->mSendTrack.ForEachCodec(function);
       transceiver->mRecvTrack.ForEachCodec(function);
     }
@@ -117,16 +116,24 @@ class JsepSession {
   template <class BinaryPredicate>
   void SortCodecs(BinaryPredicate& sorter) {
     std::stable_sort(Codecs().begin(), Codecs().end(), sorter);
-    for (auto& [id, transceiver] : GetTransceivers()) {
-      (void)id;  // Lame, but no better way to do this right now.
+    for (auto& transceiver : GetTransceivers()) {
       transceiver->mSendTrack.SortCodecs(sorter);
       transceiver->mRecvTrack.SortCodecs(sorter);
     }
   }
 
-  virtual const std::map<size_t, RefPtr<JsepTransceiver>>& GetTransceivers()
+  // Returns transceivers in the order they were added.
+  virtual const std::vector<RefPtr<JsepTransceiver>>& GetTransceivers()
       const = 0;
-  virtual std::map<size_t, RefPtr<JsepTransceiver>>& GetTransceivers() = 0;
+  virtual std::vector<RefPtr<JsepTransceiver>>& GetTransceivers() = 0;
+  RefPtr<JsepTransceiver> GetTransceiver(const std::string& aId) const {
+    for (const auto& transceiver : GetTransceivers()) {
+      if (transceiver->GetUuid() == aId) {
+        return transceiver;
+      }
+    }
+    return nullptr;
+  }
   virtual nsresult AddTransceiver(RefPtr<JsepTransceiver> transceiver) = 0;
 
   class Result {
@@ -198,8 +205,7 @@ class JsepSession {
     memset(receiving, 0, sizeof(receiving));
     memset(sending, 0, sizeof(sending));
 
-    for (const auto& [id, transceiver] : GetTransceivers()) {
-      (void)id;  // Lame, but no better way to do this right now.
+    for (const auto& transceiver : GetTransceivers()) {
       if (transceiver->mRecvTrack.GetActive() ||
           transceiver->GetMediaType() == SdpMediaSection::kApplication) {
         receiving[transceiver->mRecvTrack.GetMediaType()]++;