Track pending ICE restarts independently for different media sections.

webrtc/api/webrtcsession_unittest.cc

 /*
  *  Copyright 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 741 matching lines @@
       }
       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);
   }
 
+  // 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();
     EXPECT_TRUE(current_desc->ToString(sdp));
 
     const cricket::ContentInfos& contents = desc->contents();
     cricket::ContentInfos::const_iterator it = contents.begin();
     // Replace ufrag and pwd lines with empty strings.
     for (; it != contents.end(); ++it) {
       const cricket::TransportDescription* transport_desc =
@@ skipping 33 matching lines @@
       std::string mod_pwd = "a=ice-pwd:" + modified_ice_pwd + "\r\n";
       rtc::replace_substrs(ufrag_line.c_str(), ufrag_line.length(),
                                  mod_ufrag.c_str(), mod_ufrag.length(),
                                  sdp);
       rtc::replace_substrs(pwd_line.c_str(), pwd_line.length(),
                                  mod_pwd.c_str(), mod_pwd.length(),
                                  sdp);
     }
   }
 
+  // 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
   // to decide which local and remote streams to create.
   void CreateAndSetRemoteOfferAndLocalAnswer() {
     SessionDescriptionInterface* offer = CreateRemoteOffer();
     SetRemoteDescriptionWithoutError(offer);
     SessionDescriptionInterface* answer = CreateAnswer(NULL);
     SetLocalDescriptionWithoutError(answer);
   }
@@ skipping 2810 matching lines @@
   rtc::scoped_ptr<JsepSessionDescription> offer(
       CreateRemoteOffer(options));
   SetRemoteDescriptionWithoutError(offer.release());
 
   SendAudioVideoStream1();
   rtc::scoped_ptr<SessionDescriptionInterface> answer(
       CreateAnswer(NULL));
   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());
 
   rtc::scoped_ptr<SessionDescriptionInterface> updated_answer1(
       CreateAnswer(NULL));
 
   CompareIceUfragAndPassword(updated_answer1->description(),
                              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
 // with old ufrag and password is received.
 TEST_F(WebRtcSessionTest, TestCreateAnswerWithOldUfragAndPassword) {
   Init();
   cricket::MediaSessionOptions options;
   options.recv_video = true;
   rtc::scoped_ptr<JsepSessionDescription> offer(
       CreateRemoteOffer(options));
   SetRemoteDescriptionWithoutError(offer.release());
 
   SendAudioVideoStream1();
   rtc::scoped_ptr<SessionDescriptionInterface> answer(
       CreateAnswer(NULL));
   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());
 
   rtc::scoped_ptr<SessionDescriptionInterface> updated_answer2(
       CreateAnswer(NULL));
 
   CompareIceUfragAndPassword(updated_answer2->description(),
                              session_->local_description()->description(),
                              true);
 
   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();
   SessionDescriptionInterface* offer = CreateOffer();
   const std::string session_id_orig = offer->session_id();
   const std::string session_version_orig = offer->session_version();
   SetLocalDescriptionWithoutError(offer);
 
   video_channel_ = media_engine_->GetVideoChannel(0);
   video_channel_->set_fail_set_send_codecs(true);
@@ skipping 571 matching lines @@
 }
 
 // TODO(bemasc): Add a TestIceStatesBundle with BUNDLE enabled.  That test
 // currently fails because upon disconnection and reconnection OnIceComplete is
 // called more than once without returning to IceGatheringGathering.
 
 INSTANTIATE_TEST_CASE_P(WebRtcSessionTests,
                         WebRtcSessionTest,
                         testing::Values(ALREADY_GENERATED,
                                         DTLS_IDENTITY_STORE));