From c79268f15a878a20605a83abeade656f40ddaf4e Mon Sep 17 00:00:00 2001 From: Steve Anton Date: Tue, 24 Apr 2018 09:54:10 -0700 Subject: [PATCH] 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 Reviewed-by: Seth Hampson Cr-Commit-Position: refs/heads/master@{#23013} --- pc/peerconnection.cc | 37 +++++++++++++++--------- pc/peerconnection.h | 4 --- pc/peerconnection_ice_unittest.cc | 39 ++++++++++++++++++++++++++ pc/peerconnectioninterface_unittest.cc | 9 ++++++ 4 files changed, 72 insertions(+), 17 deletions(-) diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 3966e2341e..fa2ba7c91e 100644 --- a/pc/peerconnection.cc +++ b/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& 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_) { diff --git a/pc/peerconnection.h b/pc/peerconnection.h index 1b00ef5103..eaebc1d75b 100644 --- a/pc/peerconnection.h +++ b/pc/peerconnection.h @@ -711,10 +711,6 @@ class PeerConnection : public PeerConnectionInternal, const rtc::scoped_refptr& 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() { diff --git a/pc/peerconnection_ice_unittest.cc b/pc/peerconnection_ice_unittest.cc index e8f450fe9f..3911a58646 100644 --- a/pc/peerconnection_ice_unittest.cc +++ b/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); diff --git a/pc/peerconnectioninterface_unittest.cc b/pc/peerconnectioninterface_unittest.cc index acb0dbdd04..0d5f177aaf 100644 --- a/pc/peerconnectioninterface_unittest.cc +++ b/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);