Track pending ICE restarts independently for different media sections.

webrtc/api/webrtcsession_unittest.cc

Index: webrtc/api/webrtcsession_unittest.cc
diff --git a/webrtc/api/webrtcsession_unittest.cc b/webrtc/api/webrtcsession_unittest.cc
index 8bbcb58dd8699ed81127f64c960a43843d5c2c6e..f763f908175a3d77d5c0e51cf36c3c1d77938afb 100644
--- a/webrtc/api/webrtcsession_unittest.cc
+++ b/webrtc/api/webrtcsession_unittest.cc
@@ -759,6 +759,34 @@ class WebRtcSessionTest
     EXPECT_TRUE(expect_equal);
   }
 
+  // Compares ufrag/password only for the specified media type.
+  void CompareIceUfragAndPassword(const cricket::SessionDescription* desc1,
+                                  const cricket::SessionDescription* desc2,
+                                  cricket::MediaType media_type,
+                                  bool expect_equal) {

[pthatcher1, 2016/02/17 06:46:58]

Can you wrap this in something with the name "Expect" in it, such as:

void ExpectEqualIceUfragAndPwd(...) {
  CompareUfragAndPwd(..., true);
}


void ExpectNotEqualIceUfragAndPwdl(...) {
  CompareUfragAndPwd(..., false);
}

That would just make the test cases more readable.

[Taylor Brandstetter, 2016/02/17 21:43:50]

Again, just copy/pasted code. But I'll fix it...

+    if (desc1->contents().size() != desc2->contents().size()) {
+      EXPECT_FALSE(expect_equal);

[pthatcher1, 2016/02/17 06:46:58]

The test errors will be easier to read if you write it like this:

if (expect_equal) {
  EXPECT_EQ(desc1->contents().size(), desc2->contents());
} else {
  EXPECT_NE(desc1->contents().size(), desc2->contents());
}

Same below.

[Taylor Brandstetter, 2016/02/17 21:43:50]

I just made this function return a bool. I think it's cleaner that way.

+      return;
+    }
+
+    const cricket::ContentInfo* cinfo =
+        cricket::GetFirstMediaContent(desc1->contents(), media_type);
+    const cricket::TransportDescription* transport_desc1 =
+        desc1->GetTransportDescriptionByName(cinfo->name);
+    const cricket::TransportDescription* transport_desc2 =
+        desc2->GetTransportDescriptionByName(cinfo->name);
+    if (!transport_desc1 || !transport_desc2) {
+      EXPECT_FALSE(expect_equal);
+      return;
+    }
+    if (transport_desc1->ice_pwd != transport_desc2->ice_pwd ||
+        transport_desc1->ice_ufrag != transport_desc2->ice_ufrag) {
+      EXPECT_FALSE(expect_equal);
+      return;
+    }
+    EXPECT_TRUE(expect_equal);
+  }
+
   void RemoveIceUfragPwdLines(const SessionDescriptionInterface* current_desc,
                               std::string *sdp) {
     const cricket::SessionDescription* desc = current_desc->description();
@@ -812,6 +840,24 @@ class WebRtcSessionTest
     }
   }
 
+  // Modifies ufrag/pwd for specified |media_type|, by directly modifying the
+  // description.
+  void ModifyIceUfragPwd(SessionDescriptionInterface* current_desc,
+                         cricket::MediaType media_type,
+                         const std::string& modified_ice_ufrag,
+                         const std::string& modified_ice_pwd) {

[pthatcher1, 2016/02/17 06:46:58]

Why not just call it SetIceUfragPwd and remove the comment?

[pthatcher1, 2016/02/17 06:46:58]

I think |ufrag| and |pwd| are probably sufficient (don't need modified_ice_
prefix).

[Taylor Brandstetter, 2016/02/17 21:43:50]

Done.

[Taylor Brandstetter, 2016/02/17 21:43:50]

Done.

+    cricket::SessionDescription* desc = current_desc->description();
+    const cricket::ContentInfo* cinfo =
+        cricket::GetFirstMediaContent(desc->contents(), media_type);
+    // Replace ufrag and pwd with |modified_ice_ufrag| and
+    // |modified_ice_pwd| strings.
+    TransportInfo* transport_info = desc->GetTransportInfoByName(cinfo->name);
+    cricket::TransportDescription* transport_desc =
+        &transport_info->description;
+    transport_desc->ice_ufrag = modified_ice_ufrag;
+    transport_desc->ice_pwd = modified_ice_pwd;
+  }
+
   // Creates a remote offer and and applies it as a remote description,
   // creates a local answer and applies is as a local description.
   // Call SendAudioVideoStreamX() before this function
@@ -3642,9 +3688,10 @@ TEST_F(WebRtcSessionTest, TestCreateAnswerWithNewUfragAndPassword) {
   SetLocalDescriptionWithoutError(answer.release());
 
   // Receive an offer with new ufrag and password.
-  options.audio_transport_options.ice_restart = true;
-  options.video_transport_options.ice_restart = true;
-  options.data_transport_options.ice_restart = true;
+  for (const cricket::ContentInfo& content :
+       session_->local_description()->description()->contents()) {
+    options.transport_options[content.name].ice_restart = true;
+  }
   rtc::scoped_ptr<JsepSessionDescription> updated_offer1(
       CreateRemoteOffer(options, session_->remote_description()));
   SetRemoteDescriptionWithoutError(updated_offer1.release());
@@ -3656,7 +3703,16 @@ TEST_F(WebRtcSessionTest, TestCreateAnswerWithNewUfragAndPassword) {
                              session_->local_description()->description(),
                              false);
 
-  SetLocalDescriptionWithoutError(updated_answer1.release());
+  // Even a second answer (created before the description is set) should have
+  // a new ufrag/password.
+  rtc::scoped_ptr<SessionDescriptionInterface> updated_answer2(
+      CreateAnswer(NULL));
+
+  CompareIceUfragAndPassword(updated_answer2->description(),
+                             session_->local_description()->description(),
+                             false);
+
+  SetLocalDescriptionWithoutError(updated_answer2.release());
 }
 
 // This test verifies that an answer contains old ufrag and password if an offer
@@ -3675,9 +3731,6 @@ TEST_F(WebRtcSessionTest, TestCreateAnswerWithOldUfragAndPassword) {
   SetLocalDescriptionWithoutError(answer.release());
 
   // Receive an offer without changed ufrag or password.
-  options.audio_transport_options.ice_restart = false;
-  options.video_transport_options.ice_restart = false;
-  options.data_transport_options.ice_restart = false;
   rtc::scoped_ptr<JsepSessionDescription> updated_offer2(
       CreateRemoteOffer(options, session_->remote_description()));
   SetRemoteDescriptionWithoutError(updated_offer2.release());
@@ -3692,6 +3745,49 @@ TEST_F(WebRtcSessionTest, TestCreateAnswerWithOldUfragAndPassword) {
   SetLocalDescriptionWithoutError(updated_answer2.release());
 }
 
+// This test verifies that if an offer does an ICE restart on some, but not all
+// media sections, the answer will change the ufrag/password in the correct
+// media sections.
+TEST_F(WebRtcSessionTest, TestCreateAnswerWithNewAndOldUfragAndPassword) {
+  Init();
+  cricket::MediaSessionOptions options;
+  options.recv_video = true;
+  options.recv_audio = true;
+  options.bundle_enabled = false;
+  rtc::scoped_ptr<JsepSessionDescription> offer(CreateRemoteOffer(options));
+
+  ModifyIceUfragPwd(offer.get(), cricket::MEDIA_TYPE_AUDIO, "aaaa",
+                    "aaaaaaaaaaaaaaaaaaaaaa");

[pthatcher1, 2016/02/17 06:46:58]

This might be more readable as "ufrag_a1" and "pwd_a1" rather than "aaaa"
"aaaaaaa".  Same for video (_v1 and _v2).

[Taylor Brandstetter, 2016/02/17 21:43:50]

I think "a", "b" and "c" makes it more clear since you don't have to hunt for
the "v1" or "v2" number. And there are other tests in this file that use similar
nonsense values.

+  ModifyIceUfragPwd(offer.get(), cricket::MEDIA_TYPE_VIDEO, "bbbb",
+                    "bbbbbbbbbbbbbbbbbbbbbb");
+  SetRemoteDescriptionWithoutError(offer.release());
+
+  SendAudioVideoStream1();
+  rtc::scoped_ptr<SessionDescriptionInterface> answer(CreateAnswer(nullptr));
+  SetLocalDescriptionWithoutError(answer.release());
+
+  // Receive an offer with new ufrag and password, but only for the video media
+  // section.
+  rtc::scoped_ptr<JsepSessionDescription> updated_offer(
+      CreateRemoteOffer(options, session_->remote_description()));
+  ModifyIceUfragPwd(updated_offer.get(), cricket::MEDIA_TYPE_VIDEO, "cccc",
+                    "cccccccccccccccccccccc");
+  SetRemoteDescriptionWithoutError(updated_offer.release());
+
+  rtc::scoped_ptr<SessionDescriptionInterface> updated_answer(
+      CreateAnswer(nullptr));
+
+  CompareIceUfragAndPassword(updated_answer->description(),
+                             session_->local_description()->description(),
+                             cricket::MEDIA_TYPE_AUDIO, true);
+
+  CompareIceUfragAndPassword(updated_answer->description(),
+                             session_->local_description()->description(),
+                             cricket::MEDIA_TYPE_VIDEO, false);
+
+  SetLocalDescriptionWithoutError(updated_answer.release());

[pthatcher1, 2016/02/17 06:46:58]

Should we also test if you change only the ufrag or only the pwd, but not both?

[Taylor Brandstetter, 2016/02/17 21:43:50]

Maybe? What SHOULD we do when that happens? Is it even allowed? It seems like it
shouldn't be.

[pthatcher1, 2016/02/17 21:53:38]

I think we should treat changing either as restarting ICE.  What do you think? 
The other options are: ignore it and reject it, both of which seem worse.

[Taylor Brandstetter, 2016/02/18 00:33:21]

I was more asking what the spec says. The spec definitely says (paraphrased)
"you MUST change both to do an ICE restart", but it also says "If the offer
contained a change in the a=ice-ufrag or a=ice-pwd attributes compared to the
previous SDP from the peer, it indicates that ICE is restarting for this media
stream." Notice the "or". So I guess we should treat changing either as an ICE
restart, but always change both when we make our own descriptions.

I added a unit test and revised a comment.

+}
+
 TEST_F(WebRtcSessionTest, TestSessionContentError) {
   Init();
   SendAudioVideoStream1();