From dd59d7049158a25f97ab1c7d381bfb4f8ed127c7 Mon Sep 17 00:00:00 2001
From: Taylor Brandstetter <deadbeef@webrtc.org>
Date: Tue, 10 Apr 2018 17:25:40 -0700
Subject: [PATCH] Adding test for adding ICE candidate before applying answer.

This was working before, but somewhat by accident (because an error
wasn't being surfaced).

This CL also starts surfacing that error, from
JsepTransportController::AddRemoteCandidates to PeerConnection.

Bug: None
Change-Id: Ib48c9c00ea2a5baa5f7e3210c5dc7a339498b2d0
Reviewed-on: https://webrtc-review.googlesource.com/69015
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Commit-Queue: Taylor Brandstetter <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22830}
---
 pc/jseptransport2.cc              | 12 ++++++++---
 pc/peerconnection.cc              | 35 ++++++++++++++++---------------
 pc/peerconnection_ice_unittest.cc | 19 +++++++++++++++++
 3 files changed, 46 insertions(+), 20 deletions(-)

diff --git a/pc/jseptransport2.cc b/pc/jseptransport2.cc
index a4a03487bd..a86a37a452 100644
--- a/pc/jseptransport2.cc
+++ b/pc/jseptransport2.cc
@@ -253,12 +253,18 @@ webrtc::RTCError JsepTransport2::SetRemoteJsepTransportDescription(
 
 webrtc::RTCError JsepTransport2::AddRemoteCandidates(
     const Candidates& candidates) {
-  if (!local_description_ || !remote_description_) {
+  // Need a remote description for the remote ICE credentials; can't send
+  // connectivity checks without them.
+  //
+  // Assuming the application's signaling mechanism maintains ordering, it
+  // shouldn't be possible to hit this error; session descriptions are
+  // generated before candidates, so they should be received before candidates
+  // as well.
+  if (!remote_description_) {
     return webrtc::RTCError(webrtc::RTCErrorType::INVALID_STATE,
                             mid() +
                                 " is not ready to use the remote candidate "
-                                "because the local or remote description is "
-                                "not set.");
+                                "because the remote description is not set.");
   }
 
   for (const cricket::Candidate& candidate : candidates) {
diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc
index cce90216c1..b3a2c45606 100644
--- a/pc/peerconnection.cc
+++ b/pc/peerconnection.cc
@@ -5340,25 +5340,26 @@ bool PeerConnection::UseCandidate(const IceCandidateInterface* candidate) {
   // Invoking BaseSession method to handle remote candidates.
   RTCError error =
       transport_controller_->AddRemoteCandidates(content.name, candidates);
-  if (error.ok()) {
-    // Candidates successfully submitted for checking.
-    if (ice_connection_state_ == PeerConnectionInterface::kIceConnectionNew ||
-        ice_connection_state_ ==
-            PeerConnectionInterface::kIceConnectionDisconnected) {
-      // If state is New, then the session has just gotten its first remote ICE
-      // candidates, so go to Checking.
-      // If state is Disconnected, the session is re-using old candidates or
-      // receiving additional ones, so go to Checking.
-      // If state is Connected, stay Connected.
-      // TODO(bemasc): If state is Connected, and the new candidates are for a
-      // newly added transport, then the state actually _should_ move to
-      // checking.  Add a way to distinguish that case.
-      SetIceConnectionState(PeerConnectionInterface::kIceConnectionChecking);
-    }
-    // TODO(bemasc): If state is Completed, go back to Connected.
-  } else if (error.message()) {
+  if (!error.ok()) {
     RTC_LOG(LS_WARNING) << error.message();
+    return false;
   }
+
+  // Candidates successfully submitted for checking.
+  if (ice_connection_state_ == PeerConnectionInterface::kIceConnectionNew ||
+      ice_connection_state_ ==
+          PeerConnectionInterface::kIceConnectionDisconnected) {
+    // If state is New, then the session has just gotten its first remote ICE
+    // candidates, so go to Checking.
+    // If state is Disconnected, the session is re-using old candidates or
+    // receiving additional ones, so go to Checking.
+    // If state is Connected, stay Connected.
+    // TODO(bemasc): If state is Connected, and the new candidates are for a
+    // newly added transport, then the state actually _should_ move to
+    // checking.  Add a way to distinguish that case.
+    SetIceConnectionState(PeerConnectionInterface::kIceConnectionChecking);
+  }
+  // TODO(bemasc): If state is Completed, go back to Connected.
   return true;
 }
 
diff --git a/pc/peerconnection_ice_unittest.cc b/pc/peerconnection_ice_unittest.cc
index e8f450fe9f..c19e31d0a1 100644
--- a/pc/peerconnection_ice_unittest.cc
+++ b/pc/peerconnection_ice_unittest.cc
@@ -398,6 +398,25 @@ TEST_P(PeerConnectionIceTest, CannotAddCandidateWhenRemoteDescriptionNotSet) {
   EXPECT_FALSE(caller->pc()->AddIceCandidate(&jsep_candidate));
 }
 
+// Should be able to add a candidate immediately after setting a remote offer,
+// even if an answer hasn't been created yet.
+TEST_P(PeerConnectionIceTest, CanAddCandidateWhenLocalDescriptionNotSet) {
+  const SocketAddress kCallerAddress("1.1.1.1", 0);
+
+  auto caller = CreatePeerConnectionWithAudioVideo();
+  auto callee = CreatePeerConnectionWithAudioVideo();
+  caller->network()->AddInterface(kCallerAddress);
+
+  // Apply offer, but not answer.
+  ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
+
+  // Wait for gathering to finish and try adding a candidate from the caller.
+  EXPECT_TRUE_WAIT(caller->IsIceGatheringDone(), kIceCandidatesTimeout);
+  ASSERT_LT(0u, caller->observer()->GetCandidatesByMline(0).size());
+  EXPECT_TRUE(callee->pc()->AddIceCandidate(
+      caller->observer()->GetCandidatesByMline(0)[0]));
+}
+
 TEST_P(PeerConnectionIceTest, DuplicateIceCandidateIgnoredWhenAdded) {
   const SocketAddress kCalleeAddress("1.1.1.1", 1111);