Add IsClosed checks to various PeerConnection methods

This brings the implementations in line with the WebRTC
specification.

Bug: chromium:829238
Change-Id: I7ef64e7b6ccf0e9f60f017443565494239ff19cc
Reviewed-on: https://webrtc-review.googlesource.com/71961
Commit-Queue: Steve Anton <steveanton@webrtc.org>
Reviewed-by: Seth Hampson <shampson@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#23013}

pc/peerconnection.cc

@@ -2813,6 +2813,11 @@ bool PeerConnection::SetConfiguration(const RTCConfiguration& configuration,
                                       RTCError* error) {
   TRACE_EVENT0("webrtc", "PeerConnection::SetConfiguration");
 
+  if (IsClosed()) {
+    RTC_LOG(LS_ERROR) << "SetConfiguration: PeerConnection is closed.";
+    return SafeSetError(RTCErrorType::INVALID_STATE, error);
+  }
+
   // According to JSEP, after setLocalDescription, changing the candidate pool
   // size is not allowed, and changing the set of ICE servers will not result
   // in new candidates being gathered.
@@ -2904,17 +2909,18 @@ bool PeerConnection::AddIceCandidate(
     const IceCandidateInterface* ice_candidate) {
   TRACE_EVENT0("webrtc", "PeerConnection::AddIceCandidate");
   if (IsClosed()) {
+    RTC_LOG(LS_ERROR) << "AddIceCandidate: PeerConnection is closed.";
     return false;
   }
 
   if (!remote_description()) {
-    RTC_LOG(LS_ERROR) << "ProcessIceMessage: ICE candidates can't be added "
+    RTC_LOG(LS_ERROR) << "AddIceCandidate: ICE candidates can't be added "
                          "without any remote session description.";
     return false;
   }
 
   if (!ice_candidate) {
-    RTC_LOG(LS_ERROR) << "ProcessIceMessage: Candidate is NULL.";
+    RTC_LOG(LS_ERROR) << "AddIceCandidate: Candidate is null.";
     return false;
   }
 
@@ -2926,14 +2932,14 @@ bool PeerConnection::AddIceCandidate(
 
   // Add this candidate to the remote session description.
   if (!mutable_remote_description()->AddCandidate(ice_candidate)) {
-    RTC_LOG(LS_ERROR) << "ProcessIceMessage: Candidate cannot be used.";
+    RTC_LOG(LS_ERROR) << "AddIceCandidate: Candidate cannot be used.";
     return false;
   }
 
   if (ready) {
     return UseCandidate(ice_candidate);
   } else {
-    RTC_LOG(LS_INFO) << "ProcessIceMessage: Not ready to use candidate.";
+    RTC_LOG(LS_INFO) << "AddIceCandidate: Not ready to use candidate.";
     return true;
   }
 }
@@ -2941,14 +2947,19 @@ bool PeerConnection::AddIceCandidate(
 bool PeerConnection::RemoveIceCandidates(
     const std::vector<cricket::Candidate>& candidates) {
   TRACE_EVENT0("webrtc", "PeerConnection::RemoveIceCandidates");
+  if (IsClosed()) {
+    RTC_LOG(LS_ERROR) << "RemoveIceCandidates: PeerConnection is closed.";
+    return false;
+  }
+
   if (!remote_description()) {
-    RTC_LOG(LS_ERROR) << "RemoveRemoteIceCandidates: ICE candidates can't be "
-                         "removed without any remote session description.";
+    RTC_LOG(LS_ERROR) << "RemoveIceCandidates: ICE candidates can't be removed "
+                         "without any remote session description.";
     return false;
   }
 
   if (candidates.empty()) {
-    RTC_LOG(LS_ERROR) << "RemoveRemoteIceCandidates: candidates are empty.";
+    RTC_LOG(LS_ERROR) << "RemoveIceCandidates: candidates are empty.";
     return false;
   }
 
@@ -2956,8 +2967,7 @@ bool PeerConnection::RemoveIceCandidates(
       mutable_remote_description()->RemoveCandidates(candidates);
   if (number_removed != candidates.size()) {
     RTC_LOG(LS_ERROR)
-        << "RemoveRemoteIceCandidates: Failed to remove candidates. "
-           "Requested "
+        << "RemoveIceCandidates: Failed to remove candidates. Requested "
         << candidates.size() << " but only " << number_removed
         << " are removed.";
   }
@@ -2965,8 +2975,9 @@ bool PeerConnection::RemoveIceCandidates(
   // Remove the candidates from the transport controller.
   RTCError error = transport_controller_->RemoveRemoteCandidates(candidates);
   if (!error.ok()) {
-    RTC_LOG(LS_ERROR) << "Error when removing remote candidates: "
-                      << error.message();
+    RTC_LOG(LS_ERROR)
+        << "RemoveIceCandidates: Error when removing remote candidates: "
+        << error.message();
   }
   return true;
 }
@@ -2993,8 +3004,8 @@ void PeerConnection::RegisterUMAObserver(UMAObserver* observer) {
 void PeerConnection::SetMetricObserver_n(UMAObserver* observer) {
   RTC_DCHECK(network_thread()->IsCurrent());
   uma_observer_ = observer;
-  if (transport_controller()) {
-    transport_controller()->SetMetricsObserver(uma_observer_);
+  if (transport_controller_) {
+    transport_controller_->SetMetricsObserver(uma_observer_);
   }
 
   for (auto transceiver : transceivers_) {

pc/peerconnection.h

@@ -711,10 +711,6 @@ class PeerConnection : public PeerConnectionInternal,
       const rtc::scoped_refptr<rtc::RTCCertificate>& certificate);
   void OnDtlsSrtpSetupFailure(cricket::BaseChannel*, bool rtcp);
 
-  JsepTransportController* transport_controller() const {
-    return transport_controller_.get();
-  }
-
   // Non-const versions of local_description()/remote_description(), for use
   // internally.
   SessionDescriptionInterface* mutable_local_description() {

pc/peerconnection_ice_unittest.cc

@@ -398,6 +398,24 @@ TEST_P(PeerConnectionIceTest, CannotAddCandidateWhenRemoteDescriptionNotSet) {
   EXPECT_FALSE(caller->pc()->AddIceCandidate(&jsep_candidate));
 }
 
+TEST_P(PeerConnectionIceTest, CannotAddCandidateWhenPeerConnectionClosed) {
+  const SocketAddress kCalleeAddress("1.1.1.1", 1111);
+
+  auto caller = CreatePeerConnectionWithAudioVideo();
+  auto callee = CreatePeerConnectionWithAudioVideo();
+
+  ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get()));
+
+  cricket::Candidate candidate = CreateLocalUdpCandidate(kCalleeAddress);
+  auto* audio_content = cricket::GetFirstAudioContent(
+      caller->pc()->local_description()->description());
+  JsepIceCandidate jsep_candidate(audio_content->name, 0, candidate);
+
+  caller->pc()->Close();
+
+  EXPECT_FALSE(caller->pc()->AddIceCandidate(&jsep_candidate));
+}
+
 TEST_P(PeerConnectionIceTest, DuplicateIceCandidateIgnoredWhenAdded) {
   const SocketAddress kCalleeAddress("1.1.1.1", 1111);
 
@@ -414,6 +432,27 @@ TEST_P(PeerConnectionIceTest, DuplicateIceCandidateIgnoredWhenAdded) {
   EXPECT_EQ(1u, caller->GetIceCandidatesFromRemoteDescription().size());
 }
 
+TEST_P(PeerConnectionIceTest,
+       CannotRemoveIceCandidatesWhenPeerConnectionClosed) {
+  const SocketAddress kCalleeAddress("1.1.1.1", 1111);
+
+  auto caller = CreatePeerConnectionWithAudioVideo();
+  auto callee = CreatePeerConnectionWithAudioVideo();
+
+  ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get()));
+
+  cricket::Candidate candidate = CreateLocalUdpCandidate(kCalleeAddress);
+  auto* audio_content = cricket::GetFirstAudioContent(
+      caller->pc()->local_description()->description());
+  JsepIceCandidate ice_candidate(audio_content->name, 0, candidate);
+
+  ASSERT_TRUE(caller->pc()->AddIceCandidate(&ice_candidate));
+
+  caller->pc()->Close();
+
+  EXPECT_FALSE(caller->pc()->RemoveIceCandidates({candidate}));
+}
+
 TEST_P(PeerConnectionIceTest,
        AddRemoveCandidateWithEmptyTransportDoesNotCrash) {
   const SocketAddress kCalleeAddress("1.1.1.1", 1111);

pc/peerconnectioninterface_unittest.cc

@@ -1440,6 +1440,15 @@ TEST_P(PeerConnectionInterfaceTest, GetConfigurationAfterSetConfiguration) {
   EXPECT_EQ(PeerConnectionInterface::kRelay, returned_config.type);
 }
 
+TEST_P(PeerConnectionInterfaceTest, SetConfigurationFailsAfterClose) {
+  CreatePeerConnection();
+
+  pc_->Close();
+
+  EXPECT_FALSE(
+      pc_->SetConfiguration(PeerConnectionInterface::RTCConfiguration()));
+}
+
 TEST_F(PeerConnectionInterfaceTestPlanB, AddStreams) {
   CreatePeerConnectionWithoutDtls();
   AddVideoStream(kStreamId1);