Bug 1175609 - Bring onnegotiationneeded in line with spec. r=mt

--HG--
extra : transplant_source : %D3Z%87AP%CC%BA%AA%0B%27z%A9bh%01%0A3%40%C7U

dom/media/tests/mochitest/test_peerConnection_remoteReofferRollback.html

@@ -14,6 +14,7 @@
   var test;
   runNetworkTest(function (options) {
     test = new PeerConnectionTest(options);
+    var firstNegotiationSize = test.chain.commands.length;
     addRenegotiation(test.chain,
       [
         function PC_LOCAL_ADD_SECOND_STREAM(test) {
@@ -42,6 +43,8 @@
         },
 
         function PC_LOCAL_ROLLBACK(test) {
+          // We haven't negotiated the new stream yet.
+          test.pcLocal.expectNegotiationNeeded();
           return test.setLocalDescription(
               test.pcLocal,
               new RTCSessionDescription({ type: "rollback", sdp: ""}),
@@ -53,7 +56,7 @@
           return test.pcLocal.endOfTrickleIce;
         },
       ],
-      1 // Second PC_REMOTE_SET_REMOTE_DESCRIPTION
+      firstNegotiationSize // Second PC_REMOTE_SET_REMOTE_DESCRIPTION
     );
     test.chain.append(commandsPeerConnectionOfferAnswer);
     test.setMediaConstraints([{audio: true}], [{audio: true}]);

dom/media/tests/mochitest/test_peerConnection_remoteRollback.html

@@ -18,6 +18,8 @@
     test.chain.removeAfter('PC_REMOTE_SET_REMOTE_DESCRIPTION');
     test.chain.append([
         function PC_REMOTE_ROLLBACK(test) {
+          // We still haven't negotiated the tracks
+          test.pcRemote.expectNegotiationNeeded();
           return test.setRemoteDescription(
               test.pcRemote,
               new RTCSessionDescription({ type: "rollback" }),
@@ -25,6 +27,8 @@
         },
 
         function PC_LOCAL_ROLLBACK(test) {
+          // We still haven't negotiated the tracks
+          test.pcLocal.expectNegotiationNeeded();
           return test.setLocalDescription(
               test.pcLocal,
               new RTCSessionDescription({ type: "rollback", sdp: ""}),

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

@@ -393,7 +393,7 @@ PeerConnectionImpl::PeerConnectionImpl(const GlobalObject* aGlobal)
   , mHaveDataStream(false)
   , mAddCandidateErrorCount(0)
   , mTrickle(true) // TODO(ekr@rtfm.com): Use pref
-  , mShouldSuppressNegotiationNeeded(false)
+  , mNegotiationNeeded(false)
 {
 #if !defined(MOZILLA_EXTERNAL_LINKAGE)
   MOZ_ASSERT(NS_IsMainThread());
@@ -2619,8 +2619,10 @@ PeerConnectionImpl::SetSignalingState_m(PCImplSignalingState aSignalingState,
   mSignalingState = aSignalingState;
 
   bool fireNegotiationNeeded = false;
-
   if (mSignalingState == PCImplSignalingState::SignalingStable) {
+    // Either negotiation is done, or we've rolled back. In either case, we
+    // need to re-evaluate whether further negotiation is required.
+    mNegotiationNeeded = false;
     // If we're rolling back a local offer, we might need to remove some
     // transports, but nothing further needs to be done.
     mMedia->ActivateOrRemoveTransports(*mJsepSession);
@@ -2628,16 +2630,16 @@ PeerConnectionImpl::SetSignalingState_m(PCImplSignalingState aSignalingState,
       mMedia->UpdateMediaPipelines(*mJsepSession);
       InitializeDataChannel();
       mMedia->StartIceChecks(*mJsepSession);
-      mShouldSuppressNegotiationNeeded = false;
-      if (!mJsepSession->AllLocalTracksAreAssigned()) {
-        CSFLogInfo(logTag, "Not all local tracks were assigned to an "
-                           "m-section, either because the offerer did not offer"
-                           " to receive enough tracks, or because tracks were "
-                           "added after CreateOffer/Answer, but before "
-                           "offer/answer completed. This requires "
-                           "renegotiation.");
-        fireNegotiationNeeded = true;
-      }
+    }
+
+    if (!mJsepSession->AllLocalTracksAreAssigned()) {
+      CSFLogInfo(logTag, "Not all local tracks were assigned to an "
+                 "m-section, either because the offerer did not offer"
+                 " to receive enough tracks, or because tracks were "
+                 "added after CreateOffer/Answer, but before "
+                 "offer/answer completed. This requires "
+                 "renegotiation.");
+      fireNegotiationNeeded = true;
     }
 
     // Telemetry: record info on the current state of streams/renegotiations/etc
@@ -2655,9 +2657,6 @@ PeerConnectionImpl::SetSignalingState_m(PCImplSignalingState aSignalingState,
         mMaxSending[i] = sending[i];
       }
     }
-
-  } else {
-    mShouldSuppressNegotiationNeeded = true;
   }
 
   if (mSignalingState == PCImplSignalingState::SignalingClosed) {
@@ -2672,6 +2671,8 @@ PeerConnectionImpl::SetSignalingState_m(PCImplSignalingState aSignalingState,
   pco->OnStateChange(PCObserverStateType::SignalingState, rv);
 
   if (fireNegotiationNeeded) {
+    // We don't use MaybeFireNegotiationNeeded here, since content might have
+    // already cased a transition from stable.
     OnNegotiationNeeded();
   }
 }
@@ -3480,11 +3481,41 @@ PeerConnectionImpl::RecordLongtermICEStatistics() {
 void
 PeerConnectionImpl::OnNegotiationNeeded()
 {
-  if (mShouldSuppressNegotiationNeeded) {
+  if (mSignalingState != PCImplSignalingState::SignalingStable) {
+    // We will check whether we need to renegotiate when we reach stable again
     return;
   }
 
-  mShouldSuppressNegotiationNeeded = true;
+  if (mNegotiationNeeded) {
+    return;
+  }
+
+  mNegotiationNeeded = true;
+
+  RUN_ON_THREAD(mThread,
+                WrapRunnableNM(&MaybeFireNegotiationNeeded_static, mHandle),
+                NS_DISPATCH_NORMAL);
+}
+
+/* static */
+void
+PeerConnectionImpl::MaybeFireNegotiationNeeded_static(
+    const std::string& pcHandle)
+{
+  PeerConnectionWrapper wrapper(pcHandle);
+  if (!wrapper.impl()) {
+    return;
+  }
+
+  wrapper.impl()->MaybeFireNegotiationNeeded();
+}
+
+void
+PeerConnectionImpl::MaybeFireNegotiationNeeded()
+{
+  if (!mNegotiationNeeded) {
+    return;
+  }
 
   RefPtr<PeerConnectionObserver> pco = do_QueryObjectReferent(mPCObserver);
   if (!pco) {

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

@@ -705,6 +705,8 @@ private:
   void RecordLongtermICEStatistics();
 
   void OnNegotiationNeeded();
+  static void MaybeFireNegotiationNeeded_static(const std::string& pcHandle);
+  void MaybeFireNegotiationNeeded();
 
   // Timecard used to measure processing time. This should be the first class
   // attribute so that we accurately measure the time required to instantiate
@@ -797,7 +799,7 @@ private:
 
   bool mTrickle;
 
-  bool mShouldSuppressNegotiationNeeded;
+  bool mNegotiationNeeded;
 
   // storage for Telemetry data
   uint16_t mMaxReceiving[SdpMediaSection::kMediaTypes];