Bug 1496245: Fix bugs where transceivers could be associated with m-sections of another type. r=mjf,jib
authorByron Campen [:bwc] <docfaraday@gmail.com>
Thu, 25 Oct 2018 15:53:20 +0000
changeset 491349 d7641a700ff64f760f0f8483a458f9870418141f
parent 491348 ae2b0a2cd8a752d2e0783baaf7be8a1d69ea385c
child 491350 f8e052dc4f52301738cf54b951e0b2564d04f59d
push id247
push userfmarier@mozilla.com
push dateSat, 27 Oct 2018 01:06:44 +0000
reviewersmjf, jib
bugs1496245
milestone65.0a1
Bug 1496245: Fix bugs where transceivers could be associated with m-sections of another type. r=mjf,jib Bug 1496245 - Part 0: web-platform-test for rollback followed by SRD with different media type. Bug 1496245 - Part 1: Allow SRD(offer) to stomp un-negotiated level mappings on our transceivers. Bug 1496245 - Part 2: Don't create extra datachannel transceivers, just reuse (and possibly re-enable) the one we have. Differential Revision: https://phabricator.services.mozilla.com/D8259
media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
media/webrtc/signaling/src/jsep/JsepTransceiver.h
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
testing/web-platform/tests/webrtc/RTCRtpTransceiver.https.html
--- a/media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
+++ b/media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
@@ -117,16 +117,21 @@ JsepSessionImpl::AddTransceiver(RefPtr<J
       }
 
       transceiver->mSendTrack.UpdateTrackIds(std::vector<std::string>(), trackId);
     }
   } else {
     // Datachannel transceivers should always be sendrecv. Just set it instead
     // of asserting.
     transceiver->mJsDirection = SdpDirectionAttribute::kSendrecv;
+#ifdef DEBUG
+    for (auto& transceiver : mTransceivers) {
+      MOZ_ASSERT(transceiver->GetMediaType() != SdpMediaSection::kApplication);
+    }
+#endif
   }
 
   transceiver->mSendTrack.PopulateCodecs(mSupportedCodecs.values);
   transceiver->mRecvTrack.PopulateCodecs(mSupportedCodecs.values);
   // We do not set mLevel yet, we do that either on createOffer, or setRemote
 
   mTransceivers.push_back(transceiver);
   return NS_OK;
@@ -223,44 +228,42 @@ JsepSessionImpl::AddAudioVideoRtpExtensi
                          direction);
 }
 
 nsresult
 JsepSessionImpl::CreateOfferMsection(const JsepOfferOptions& options,
                                      JsepTransceiver& transceiver,
                                      Sdp* local)
 {
-  JsepTrack& sendTrack(transceiver.mSendTrack);
-  JsepTrack& recvTrack(transceiver.mRecvTrack);
-
   SdpMediaSection::Protocol protocol(
-      SdpHelper::GetProtocolForMediaType(sendTrack.GetMediaType()));
+      SdpHelper::GetProtocolForMediaType(transceiver.GetMediaType()));
 
   const Sdp* answer(GetAnswer());
   const SdpMediaSection* lastAnswerMsection = nullptr;
 
   if (answer &&
       (local->GetMediaSectionCount() < answer->GetMediaSectionCount())) {
     lastAnswerMsection =
       &answer->GetMediaSection(local->GetMediaSectionCount());
     // Use the protocol the answer used, even if it is not what we would have
     // used.
     protocol = lastAnswerMsection->GetProtocol();
   }
 
   SdpMediaSection* msection = &local->AddMediaSection(
-      sendTrack.GetMediaType(),
+      transceiver.GetMediaType(),
       transceiver.mJsDirection,
       0,
       protocol,
       sdp::kIPv4,
       "0.0.0.0");
 
   // Some of this stuff (eg; mid) sticks around even if disabled
   if (lastAnswerMsection) {
+    MOZ_ASSERT(lastAnswerMsection->GetMediaType() == transceiver.GetMediaType());
     nsresult rv = mSdpHelper.CopyStickyParams(*lastAnswerMsection, msection);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
   if (transceiver.IsStopped()) {
     SdpHelper::DisableMsection(local, msection);
     return NS_OK;
   }
@@ -273,18 +276,18 @@ JsepSessionImpl::CreateOfferMsection(con
     // Set RTCP-MUX.
     msection->GetAttributeList().SetAttribute(
         new SdpFlagAttribute(SdpAttribute::kRtcpMuxAttribute));
   }
 
   nsresult rv = AddTransportAttributes(msection, SdpSetupAttribute::kActpass);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  sendTrack.AddToOffer(mSsrcGenerator, msection);
-  recvTrack.AddToOffer(mSsrcGenerator, msection);
+  transceiver.mSendTrack.AddToOffer(mSsrcGenerator, msection);
+  transceiver.mRecvTrack.AddToOffer(mSsrcGenerator, msection);
 
   AddExtmap(msection);
 
   if (lastAnswerMsection && lastAnswerMsection->GetPort()) {
     MOZ_ASSERT(transceiver.IsAssociated());
     MOZ_ASSERT(transceiver.GetMid() ==
                lastAnswerMsection->GetAttributeList().GetMid());
   } else {
@@ -627,16 +630,17 @@ JsepSessionImpl::CreateAnswer(const Jsep
 }
 
 nsresult
 JsepSessionImpl::CreateAnswerMsection(const JsepAnswerOptions& options,
                                       JsepTransceiver& transceiver,
                                       const SdpMediaSection& remoteMsection,
                                       Sdp* sdp)
 {
+  MOZ_ASSERT(transceiver.GetMediaType() == remoteMsection.GetMediaType());
   SdpDirectionAttribute::Direction direction =
     reverse(remoteMsection.GetDirection()) & transceiver.mJsDirection;
   SdpMediaSection& msection =
       sdp->AddMediaSection(remoteMsection.GetMediaType(),
                            direction,
                            9,
                            remoteMsection.GetProtocol(),
                            sdp::kIPv4,
@@ -963,16 +967,21 @@ JsepSessionImpl::SetRemoteDescription(Js
           SdpAttribute::kIceOptionsAttribute)) {
     iceOptions = parsed->GetAttributeList().GetIceOptions().mValues;
   }
 
   // Save in case we need to rollback.
   if (type == kJsepSdpOffer) {
     mOldTransceivers.clear();
     for (const auto& transceiver : mTransceivers) {
+      if (!transceiver->IsNegotiated()) {
+        // We chose a level for this transceiver, but never negotiated it.
+        // Discard this state.
+        transceiver->ClearLevel();
+      }
       mOldTransceivers.push_back(new JsepTransceiver(*transceiver));
     }
   }
 
   // TODO(bug 1095780): Note that we create remote tracks even when
   // They contain only codecs we can't negotiate or other craziness.
   rv = UpdateTransceiversFromRemoteDescription(*parsed);
   NS_ENSURE_SUCCESS(rv, rv);
@@ -1487,17 +1496,19 @@ JsepSessionImpl::GetTransceiverForMid(co
 
   return nullptr;
 }
 
 JsepTransceiver*
 JsepSessionImpl::GetTransceiverForLocal(size_t level)
 {
   if (JsepTransceiver* transceiver = GetTransceiverForLevel(level)) {
-    if (WasMsectionDisabledLastNegotiation(level) && transceiver->IsStopped()) {
+    if (WasMsectionDisabledLastNegotiation(level) &&
+        transceiver->IsStopped() &&
+        transceiver->GetMediaType() != SdpMediaSection::kApplication) {
       // Attempt to recycle. If this fails, the old transceiver stays put.
       transceiver->Disassociate();
       JsepTransceiver* newTransceiver = FindUnassociatedTransceiver(
           transceiver->GetMediaType(), false);
       if (newTransceiver) {
         newTransceiver->SetLevel(level);
         transceiver->ClearLevel();
         return newTransceiver;
@@ -1639,16 +1650,21 @@ JsepSessionImpl::WasMsectionDisabledLast
 }
 
 JsepTransceiver*
 JsepSessionImpl::FindUnassociatedTransceiver(
     SdpMediaSection::MediaType type, bool magic)
 {
   // Look through transceivers that are not mapped to an m-section
   for (RefPtr<JsepTransceiver>& transceiver : mTransceivers) {
+    if (type == SdpMediaSection::kApplication &&
+        type == transceiver->GetMediaType()) {
+      transceiver->RestartDatachannelTransceiver();
+      return transceiver.get();
+    }
     if (!transceiver->IsStopped() &&
         !transceiver->HasLevel() &&
         (!magic || transceiver->HasAddTrackMagic()) &&
         (transceiver->GetMediaType() == type)) {
       return transceiver.get();
     }
   }
 
--- a/media/webrtc/signaling/src/jsep/JsepTransceiver.h
+++ b/media/webrtc/signaling/src/jsep/JsepTransceiver.h
@@ -54,16 +54,19 @@ class JsepTransceiver {
       mRemoved(orig.mRemoved),
       mNegotiated(orig.mNegotiated)
     {}
 
     NS_INLINE_DECL_THREADSAFE_REFCOUNTING(JsepTransceiver);
 
     void Rollback(JsepTransceiver& oldTransceiver)
     {
+      MOZ_ASSERT(oldTransceiver.GetMediaType() == GetMediaType());
+      MOZ_ASSERT(!oldTransceiver.HasLevel() ||
+                 oldTransceiver.GetLevel() == GetLevel());
       mTransport = oldTransceiver.mTransport;
       mLevel = oldTransceiver.mLevel;
       mBundleLevel = oldTransceiver.mBundleLevel;
       mRecvTrack = oldTransceiver.mRecvTrack;
 
       // stop() caused by a disabled m-section in a remote offer cannot be
       // rolled back.
       if (!IsStopped()) {
@@ -104,18 +107,16 @@ class JsepTransceiver {
       MOZ_ASSERT(!HasLevel());
       MOZ_ASSERT(!IsStopped());
 
       mLevel = level;
     }
 
     void ClearLevel()
     {
-      MOZ_ASSERT(mStopped);
-      MOZ_ASSERT(!IsAssociated());
       mLevel = SIZE_MAX;
     }
 
     size_t GetLevel() const
     {
       MOZ_ASSERT(HasLevel());
       return mLevel;
     }
@@ -125,16 +126,22 @@ class JsepTransceiver {
       mStopped = true;
     }
 
     bool IsStopped() const
     {
       return mStopped;
     }
 
+    void RestartDatachannelTransceiver()
+    {
+      MOZ_RELEASE_ASSERT(GetMediaType() == SdpMediaSection::kApplication);
+      mStopped = false;
+    }
+
     void SetRemoved()
     {
       mRemoved = true;
     }
 
     bool IsRemoved() const
     {
       return mRemoved;
@@ -198,16 +205,17 @@ class JsepTransceiver {
     bool IsNegotiated() const
     {
       return mNegotiated;
     }
 
     // Convenience function
     SdpMediaSection::MediaType GetMediaType() const
     {
+      MOZ_ASSERT(mRecvTrack.GetMediaType() == mSendTrack.GetMediaType());
       return mRecvTrack.GetMediaType();
     }
 
     bool HasOwnTransport() const
     {
       if (mTransport.mComponents &&
           (!HasBundleLevel() || (GetLevel() == BundleLevel()))) {
         return true;
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ -327,17 +327,16 @@ PeerConnectionImpl::PeerConnectionImpl(c
   , mAllowIceLoopback(false)
   , mAllowIceLinkLocal(false)
   , mForceIceTcp(false)
   , mMedia(nullptr)
   , mUuidGen(MakeUnique<PCUuidGenerator>())
   , mIceRestartCount(0)
   , mIceRollbackCount(0)
   , mHaveConfiguredCodecs(false)
-  , mHaveDataStream(false)
   , mAddCandidateErrorCount(0)
   , mTrickle(true) // TODO(ekr@rtfm.com): Use pref
   , mPrivateWindow(false)
   , mActiveOnWindow(false)
   , mPacketDumpEnabled(false)
   , mPacketDumpFlagsMutex("Packet dump flags mutex")
   , listenPort(0)
   , connectPort(0)
@@ -1202,21 +1201,32 @@ PeerConnectionImpl::CreateDataChannel(co
     aType == DataChannelConnection::PARTIAL_RELIABLE_REXMIT ? aMaxNum :
     (aType == DataChannelConnection::PARTIAL_RELIABLE_TIMED ? aMaxTime : 0),
     nullptr, nullptr, aExternalNegotiated, aStream
   );
   NS_ENSURE_TRUE(dataChannel,NS_ERROR_FAILURE);
 
   CSFLogDebug(LOGTAG, "%s: making DOMDataChannel", __FUNCTION__);
 
-  if (!mHaveDataStream) {
-    mJsepSession->AddTransceiver(
-        new JsepTransceiver(SdpMediaSection::MediaType::kApplication));
-    mHaveDataStream = true;
+  RefPtr<JsepTransceiver> dcTransceiver;
+  for (auto& transceiver : mJsepSession->GetTransceivers()) {
+    if (transceiver->GetMediaType() == SdpMediaSection::kApplication) {
+      dcTransceiver = transceiver;
+      break;
+    }
   }
+
+  if (!dcTransceiver) {
+    dcTransceiver =
+      new JsepTransceiver(SdpMediaSection::MediaType::kApplication);
+    mJsepSession->AddTransceiver(dcTransceiver);
+  }
+
+  dcTransceiver->RestartDatachannelTransceiver();
+
   RefPtr<nsDOMDataChannel> retval;
   rv = NS_NewDOMDataChannel(dataChannel.forget(), mWindow,
 			    getter_AddRefs(retval));
   if (NS_FAILED(rv)) {
     return rv;
   }
   retval.forget(aRetval);
   return NS_OK;
@@ -1267,18 +1277,16 @@ PeerConnectionImpl::NotifyDataChannel(al
   MOZ_ASSERT(channel);
   CSFLogDebug(LOGTAG, "%s: channel: %p", __FUNCTION__, channel.get());
 
   RefPtr<nsDOMDataChannel> domchannel;
   nsresult rv = NS_NewDOMDataChannel(channel.forget(),
                                      mWindow, getter_AddRefs(domchannel));
   NS_ENSURE_SUCCESS_VOID(rv);
 
-  mHaveDataStream = true;
-
   RefPtr<PeerConnectionObserver> pco = do_QueryObjectReferent(mPCObserver);
   if (!pco) {
     return;
   }
 
   RUN_ON_THREAD(mThread,
                 WrapRunnableNM(NotifyDataChannel_m,
                                domchannel.forget(),
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ -706,18 +706,16 @@ private:
 
   // Start time of ICE, used for telemetry
   mozilla::TimeStamp mIceStartTime;
   // Start time of call used for Telemetry
   mozilla::TimeStamp mStartTime;
 
   bool mHaveConfiguredCodecs;
 
-  bool mHaveDataStream;
-
   unsigned int mAddCandidateErrorCount;
 
   bool mTrickle;
 
   bool mPrivateWindow;
 
   // Whether this PeerConnection is being counted as active by mWindow
   bool mActiveOnWindow;
--- a/testing/web-platform/tests/webrtc/RTCRtpTransceiver.https.html
+++ b/testing/web-platform/tests/webrtc/RTCRtpTransceiver.https.html
@@ -1793,16 +1793,95 @@
     offer = await pc.createOffer();
     await pc.setLocalDescription(offer);
     pc.getTransceivers()[0].stop();
     await pc.setLocalDescription({type: "rollback"});
 
     hasProps(pc.getTransceivers(), [{ stopped: true }]);
   };
 
+  const checkRollbackAndSetRemoteOfferWithDifferentType = async t => {
+    const pc1 = new RTCPeerConnection();
+    t.add_cleanup(() => pc1.close());
+
+    const audioStream = await navigator.mediaDevices.getUserMedia({audio: true});
+    t.add_cleanup(() => stopTracks(audioStream));
+    const audioTrack = audioStream.getAudioTracks()[0];
+    pc1.addTrack(audioTrack, audioStream);
+
+    const pc2 = new RTCPeerConnection();
+    t.add_cleanup(() => pc2.close());
+
+    const videoStream = await navigator.mediaDevices.getUserMedia({video: true});
+    t.add_cleanup(() => stopTracks(videoStream));
+    const videoTrack = videoStream.getVideoTracks()[0];
+    pc2.addTrack(videoTrack, videoStream);
+
+    await pc1.setLocalDescription(await pc1.createOffer());
+    await pc1.setLocalDescription({type: "rollback"});
+
+    hasProps(pc1.getTransceivers(),
+      [
+        {
+          receiver: {track: {kind: "audio"}},
+          sender: {track: audioTrack},
+          direction: "sendrecv",
+          mid: null,
+          currentDirection: null,
+          stopped: false
+        }
+      ]);
+
+    hasProps(pc2.getTransceivers(),
+      [
+        {
+          receiver: {track: {kind: "video"}},
+          sender: {track: videoTrack},
+          direction: "sendrecv",
+          mid: null,
+          currentDirection: null,
+          stopped: false
+        }
+      ]);
+
+    await offerAnswer(pc2, pc1);
+
+    hasPropsAndUniqueMids(pc1.getTransceivers(),
+      [
+        {
+          receiver: {track: {kind: "audio"}},
+          sender: {track: audioTrack},
+          direction: "sendrecv",
+          mid: null,
+          currentDirection: null,
+          stopped: false
+        },
+        {
+          receiver: {track: {kind: "video"}},
+          sender: {track: null},
+          direction: "recvonly",
+          currentDirection: "recvonly",
+          stopped: false
+        }
+      ]);
+
+    hasPropsAndUniqueMids(pc2.getTransceivers(),
+      [
+        {
+          receiver: {track: {kind: "video"}},
+          sender: {track: videoTrack},
+          direction: "sendrecv",
+          currentDirection: "sendonly",
+          stopped: false
+        }
+      ]);
+
+    await offerAnswer(pc1, pc2);
+  };
+
   const checkRemoteRollback = async t => {
     const pc1 = new RTCPeerConnection();
     t.add_cleanup(() => pc1.close());
 
     const stream = await navigator.mediaDevices.getUserMedia({audio: true});
     t.add_cleanup(() => stopTracks(stream));
     const track = stream.getAudioTracks()[0];
     pc1.addTrack(track, stream);
@@ -2265,13 +2344,14 @@ const tests = [
   checkStop,
   checkStopAfterCreateOffer,
   checkStopAfterSetLocalOffer,
   checkStopAfterSetRemoteOffer,
   checkStopAfterCreateAnswer,
   checkStopAfterSetLocalAnswer,
   checkStopAfterClose,
   checkLocalRollback,
+  checkRollbackAndSetRemoteOfferWithDifferentType,
   checkRemoteRollback,
   checkMsectionReuse
 ].forEach(test => promise_test(test, test.name));
 
 </script>