Use the dummy address 0.0.0.0:9 in the c= and the m= lines if the

default connection address is a hostname candidate.

Using a FQDN in the c= line has caused an inter-op issue with Firefox
when hostname candidates are the only candidates gathered when forming
the media sections. To address this issue, we use 0.0.0.0:9 when a
hostname candidate would be used to populate the c= and the m= lines.

The SDP grammar related to ICE candidates has been moved out of RFC8445,
and is currently defined in draft-ietf-mmusic-ice-sip-sdp. A FQDN
address must not be used in the connection address attribute per the
latest draft, if the ICE agent generates local candidates. Also, the
wildcard addresses  (0.0.0.0 or ::) with port 9 are given the exception
as the connection address that will not result in an ICE mismatch. We
thus adopt the aforementioned solution after combining these
considerations.

Bug: chromium:927309, chromium:982108
Change-Id: I3df2db0f154276da39f99650289cf81baa677e74
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/145280
Commit-Queue: Qingsi Wang <qingsi@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28547}

pc/jsep_session_description.cc

@@ -90,7 +90,23 @@ void UpdateConnectionAddress(
   }
   rtc::SocketAddress connection_addr(ip, port);
   if (rtc::IPIsUnspec(connection_addr.ipaddr()) && !hostname.empty()) {
-    connection_addr = rtc::SocketAddress(hostname, port);
+    // When a hostname candidate becomes the (default) connection address,
+    // we use the dummy address 0.0.0.0 and port 9 in the c= and the m= lines.
+    //
+    // We have observed in deployment that with a FQDN in a c= line, SDP parsing
+    // could fail in other JSEP implementations. We note that the wildcard
+    // addresses (0.0.0.0 or ::) with port 9 are given the exception as the
+    // connection address that will not result in an ICE mismatch
+    // (draft-ietf-mmusic-ice-sip-sdp). Also, 0.0.0.0 or :: can be used as the
+    // connection address in the initial offer or answer with trickle ICE
+    // if the offerer or answerer does not want to include the host IP address
+    // (draft-ietf-mmusic-trickle-ice-sip), and in particular 0.0.0.0 has been
+    // widely deployed for this use without outstanding compatibility issues.
+    // Combining the above considerations, we use 0.0.0.0 with port 9 to
+    // populate the c= and the m= lines. See |BuildMediaDescription| in
+    // webrtc_sdp.cc for the SDP generation with
+    // |media_desc->connection_address()|.
+    connection_addr = rtc::SocketAddress(kDummyAddress, kDummyPort);
   }
   media_desc->set_connection_address(connection_addr);
 }

pc/jsep_session_description_unittest.cc

@@ -223,11 +223,14 @@ TEST_F(JsepSessionDescriptionTest, AddHostnameCandidate) {
   c.set_protocol(cricket::UDP_PROTOCOL_NAME);
   c.set_address(rtc::SocketAddress("example.local", 1234));
   c.set_type(cricket::LOCAL_PORT_TYPE);
-  JsepIceCandidate hostname_candidate("audio", 0, c);
+  const size_t audio_index = 0;
+  JsepIceCandidate hostname_candidate("audio", audio_index, c);
   EXPECT_TRUE(jsep_desc_->AddCandidate(&hostname_candidate));
+
   ASSERT_NE(nullptr, jsep_desc_->description());
-  const auto& content = jsep_desc_->description()->contents()[0];
-  EXPECT_EQ("example.local:1234",
+  ASSERT_EQ(2u, jsep_desc_->description()->contents().size());
+  const auto& content = jsep_desc_->description()->contents()[audio_index];
+  EXPECT_EQ("0.0.0.0:9",
             content.media_description()->connection_address().ToString());
 }
 
@@ -242,6 +245,39 @@ TEST_F(JsepSessionDescriptionTest, SerializeDeserialize) {
   EXPECT_EQ(sdp, parsed_sdp);
 }
 
+// Test that we can serialize a JsepSessionDescription when a hostname candidate
+// is the default destination and deserialize it again. The connection address
+// in the deserialized description should be the dummy address 0.0.0.0:9.
+TEST_F(JsepSessionDescriptionTest, SerializeDeserializeWithHostnameCandidate) {
+  cricket::Candidate c;
+  c.set_component(cricket::ICE_CANDIDATE_COMPONENT_RTP);
+  c.set_protocol(cricket::UDP_PROTOCOL_NAME);
+  c.set_address(rtc::SocketAddress("example.local", 1234));
+  c.set_type(cricket::LOCAL_PORT_TYPE);
+  const size_t audio_index = 0;
+  const size_t video_index = 1;
+  JsepIceCandidate hostname_candidate_audio("audio", audio_index, c);
+  JsepIceCandidate hostname_candidate_video("video", video_index, c);
+  EXPECT_TRUE(jsep_desc_->AddCandidate(&hostname_candidate_audio));
+  EXPECT_TRUE(jsep_desc_->AddCandidate(&hostname_candidate_video));
+
+  std::string sdp = Serialize(jsep_desc_.get());
+
+  auto parsed_jsep_desc = DeSerialize(sdp);
+  EXPECT_EQ(2u, parsed_jsep_desc->number_of_mediasections());
+
+  ASSERT_NE(nullptr, parsed_jsep_desc->description());
+  ASSERT_EQ(2u, parsed_jsep_desc->description()->contents().size());
+  const auto& audio_content =
+      parsed_jsep_desc->description()->contents()[audio_index];
+  const auto& video_content =
+      parsed_jsep_desc->description()->contents()[video_index];
+  EXPECT_EQ("0.0.0.0:9",
+            audio_content.media_description()->connection_address().ToString());
+  EXPECT_EQ("0.0.0.0:9",
+            video_content.media_description()->connection_address().ToString());
+}
+
 // Tests that we can serialize and deserialize a JsepSesssionDescription
 // with candidates.
 TEST_F(JsepSessionDescriptionTest, SerializeDeserializeWithCandidates) {

pc/webrtc_sdp.cc

@@ -1468,10 +1468,6 @@ void BuildMediaDescription(const ContentInfo* content_info,
   } else if (media_desc->connection_address().family() == AF_INET6) {
     os << " " << kConnectionIpv6Addrtype << " "
        << media_desc->connection_address().ipaddr().ToString();
-  } else if (!media_desc->connection_address().hostname().empty()) {
-    // For hostname candidates, we use c=IN IP4 <hostname>.
-    os << " " << kConnectionIpv4Addrtype << " "
-       << media_desc->connection_address().hostname();
   } else {
     os << " " << kConnectionIpv4Addrtype << " " << kDummyAddress;
   }

pc/webrtc_sdp_unittest.cc

@@ -4259,31 +4259,6 @@ TEST_F(WebRtcSdpTest, SerializeAndDeserializeWithConnectionAddress) {
             video_desc->connection_address().ToString());
 }
 
-// Test that a media description that contains a hostname connection address can
-// be correctly serialized.
-TEST_F(WebRtcSdpTest, SerializeAndDeserializeWithHostnameConnectionAddress) {
-  JsepSessionDescription expected_jsep(kDummyType);
-  cricket::Candidate c;
-  const rtc::SocketAddress hostname_addr("example.local", 1234);
-  audio_desc_->set_connection_address(hostname_addr);
-  video_desc_->set_connection_address(hostname_addr);
-  ASSERT_TRUE(
-      expected_jsep.Initialize(desc_.Clone(), kSessionId, kSessionVersion));
-  // Serialization.
-  std::string message = webrtc::SdpSerialize(expected_jsep);
-  // Deserialization.
-  JsepSessionDescription jdesc(kDummyType);
-  ASSERT_TRUE(SdpDeserialize(message, &jdesc));
-  auto audio_desc = jdesc.description()
-                        ->GetContentByName(kAudioContentName)
-                        ->media_description();
-  auto video_desc = jdesc.description()
-                        ->GetContentByName(kVideoContentName)
-                        ->media_description();
-  EXPECT_EQ(hostname_addr, audio_desc->connection_address());
-  EXPECT_EQ(hostname_addr, video_desc->connection_address());
-}
-
 // RFC4566 says "If a session has no meaningful name, the value "s= " SHOULD be
 // used (i.e., a single space as the session name)." So we should accept that.
 TEST_F(WebRtcSdpTest, DeserializeEmptySessionName) {