diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index a571a53645..144328c511 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -1746,6 +1746,10 @@ void PeerConnection::Close() { // The .h file says that observer can be discarded after close() returns. // Make sure this is true. observer_ = nullptr; + + // Signal shutdown to the sdp handler. This invalidates weak pointers for + // internal pending callbacks. + sdp_handler_->PrepareForShutdown(); } void PeerConnection::SetIceConnectionState(IceConnectionState new_state) { diff --git a/pc/peer_connection.h b/pc/peer_connection.h index aa46feb4aa..34234244e8 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -323,7 +323,8 @@ class PeerConnection : public PeerConnectionInternal, PeerConnectionObserver* Observer() const; bool IsClosed() const { RTC_DCHECK_RUN_ON(signaling_thread()); - return sdp_handler_->signaling_state() == PeerConnectionInterface::kClosed; + return !sdp_handler_ || + sdp_handler_->signaling_state() == PeerConnectionInterface::kClosed; } // Get current SSL role used by SCTP's underlying transport. bool GetSctpSslRole(rtc::SSLRole* role); diff --git a/pc/peer_connection_interface_unittest.cc b/pc/peer_connection_interface_unittest.cc index e3d0ab61a3..3f73168d47 100644 --- a/pc/peer_connection_interface_unittest.cc +++ b/pc/peer_connection_interface_unittest.cc @@ -683,7 +683,7 @@ class PeerConnectionInterfaceBaseTest : public ::testing::Test { #endif } - virtual void SetUp() { + void SetUp() override { // Use fake audio capture module since we're only testing the interface // level, and using a real one could make tests flaky when run in parallel. fake_audio_capture_module_ = FakeAudioCaptureModule::Create(); @@ -701,6 +701,11 @@ class PeerConnectionInterfaceBaseTest : public ::testing::Test { PeerConnectionFactoryForTest::CreatePeerConnectionFactoryForTest(); } + void TearDown() override { + if (pc_) + pc_->Close(); + } + void CreatePeerConnection() { CreatePeerConnection(PeerConnectionInterface::RTCConfiguration()); } @@ -734,6 +739,10 @@ class PeerConnectionInterfaceBaseTest : public ::testing::Test { } void CreatePeerConnection(const RTCConfiguration& config) { + if (pc_) { + pc_->Close(); + pc_ = nullptr; + } std::unique_ptr port_allocator( new cricket::FakePortAllocator(rtc::Thread::Current(), nullptr)); port_allocator_ = port_allocator.get(); diff --git a/pc/peer_connection_jsep_unittest.cc b/pc/peer_connection_jsep_unittest.cc index c3e093617b..00e202c1b5 100644 --- a/pc/peer_connection_jsep_unittest.cc +++ b/pc/peer_connection_jsep_unittest.cc @@ -2214,6 +2214,7 @@ TEST_F(PeerConnectionJsepTest, RollbackRtpDataChannel) { EXPECT_TRUE(pc->CreateOfferAndSetAsLocal()); EXPECT_TRUE(pc->SetRemoteDescription(pc->CreateRollback())); EXPECT_TRUE(pc->SetLocalDescription(std::move(offer))); + pc->pc()->Close(); } } // namespace webrtc diff --git a/pc/peer_connection_wrapper.cc b/pc/peer_connection_wrapper.cc index 328f5795e2..65384ee447 100644 --- a/pc/peer_connection_wrapper.cc +++ b/pc/peer_connection_wrapper.cc @@ -48,7 +48,10 @@ PeerConnectionWrapper::PeerConnectionWrapper( observer_->SetPeerConnectionInterface(pc_.get()); } -PeerConnectionWrapper::~PeerConnectionWrapper() = default; +PeerConnectionWrapper::~PeerConnectionWrapper() { + if (pc_) + pc_->Close(); +} PeerConnectionFactoryInterface* PeerConnectionWrapper::pc_factory() { return pc_factory_.get(); diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 49b9df970c..6b8412caa9 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -28,7 +28,6 @@ #include "api/rtp_parameters.h" #include "api/rtp_receiver_interface.h" #include "api/rtp_sender_interface.h" -#include "api/uma_metrics.h" #include "api/video/builtin_video_bitrate_allocator_factory.h" #include "media/base/codec.h" #include "media/base/media_engine.h" @@ -2280,55 +2279,58 @@ void SdpOfferAnswerHandler::SetAssociatedRemoteStreams( bool SdpOfferAnswerHandler::AddIceCandidate( const IceCandidateInterface* ice_candidate) { + const AddIceCandidateResult result = AddIceCandidateInternal(ice_candidate); + NoteAddIceCandidateResult(result); + // If the return value is kAddIceCandidateFailNotReady, the candidate has been + // added, although not 'ready', but that's a success. + return result == kAddIceCandidateSuccess || + result == kAddIceCandidateFailNotReady; +} + +AddIceCandidateResult SdpOfferAnswerHandler::AddIceCandidateInternal( + const IceCandidateInterface* ice_candidate) { RTC_DCHECK_RUN_ON(signaling_thread()); TRACE_EVENT0("webrtc", "SdpOfferAnswerHandler::AddIceCandidate"); if (pc_->IsClosed()) { RTC_LOG(LS_ERROR) << "AddIceCandidate: PeerConnection is closed."; - NoteAddIceCandidateResult(kAddIceCandidateFailClosed); - return false; + return kAddIceCandidateFailClosed; } if (!remote_description()) { RTC_LOG(LS_ERROR) << "AddIceCandidate: ICE candidates can't be added " "without any remote session description."; - NoteAddIceCandidateResult(kAddIceCandidateFailNoRemoteDescription); - return false; + return kAddIceCandidateFailNoRemoteDescription; } if (!ice_candidate) { RTC_LOG(LS_ERROR) << "AddIceCandidate: Candidate is null."; - NoteAddIceCandidateResult(kAddIceCandidateFailNullCandidate); - return false; + return kAddIceCandidateFailNullCandidate; } bool valid = false; bool ready = ReadyToUseRemoteCandidate(ice_candidate, nullptr, &valid); if (!valid) { - NoteAddIceCandidateResult(kAddIceCandidateFailNotValid); - return false; + return kAddIceCandidateFailNotValid; } // Add this candidate to the remote session description. if (!mutable_remote_description()->AddCandidate(ice_candidate)) { RTC_LOG(LS_ERROR) << "AddIceCandidate: Candidate cannot be used."; - NoteAddIceCandidateResult(kAddIceCandidateFailInAddition); - return false; + return kAddIceCandidateFailInAddition; } - if (ready) { - bool result = UseCandidate(ice_candidate); - if (result) { - pc_->NoteUsageEvent(UsageEvent::ADD_ICE_CANDIDATE_SUCCEEDED); - NoteAddIceCandidateResult(kAddIceCandidateSuccess); - } else { - NoteAddIceCandidateResult(kAddIceCandidateFailNotUsable); - } - return result; - } else { + if (!ready) { RTC_LOG(LS_INFO) << "AddIceCandidate: Not ready to use candidate."; - NoteAddIceCandidateResult(kAddIceCandidateFailNotReady); - return true; + return kAddIceCandidateFailNotReady; } + + if (!UseCandidate(ice_candidate)) { + return kAddIceCandidateFailNotUsable; + } + + pc_->NoteUsageEvent(UsageEvent::ADD_ICE_CANDIDATE_SUCCEEDED); + + return kAddIceCandidateSuccess; } void SdpOfferAnswerHandler::AddIceCandidate( @@ -2342,23 +2344,25 @@ void SdpOfferAnswerHandler::AddIceCandidate( [this_weak_ptr = weak_ptr_factory_.GetWeakPtr(), candidate = std::move(candidate), callback = std::move(callback)]( std::function operations_chain_callback) { - if (!this_weak_ptr) { - operations_chain_callback(); + auto result = + this_weak_ptr + ? this_weak_ptr->AddIceCandidateInternal(candidate.get()) + : kAddIceCandidateFailClosed; + NoteAddIceCandidateResult(result); + operations_chain_callback(); + if (result == kAddIceCandidateFailClosed) { callback(RTCError( RTCErrorType::INVALID_STATE, "AddIceCandidate failed because the session was shut down")); - return; - } - if (!this_weak_ptr->AddIceCandidate(candidate.get())) { - operations_chain_callback(); + } else if (result != kAddIceCandidateSuccess && + result != kAddIceCandidateFailNotReady) { // Fail with an error type and message consistent with Chromium. // TODO(hbos): Fail with error types according to spec. callback(RTCError(RTCErrorType::UNSUPPORTED_OPERATION, "Error processing ICE candidate")); - return; + } else { + callback(RTCError::OK()); } - operations_chain_callback(); - callback(RTCError::OK()); }); } diff --git a/pc/sdp_offer_answer.h b/pc/sdp_offer_answer.h index a717db8120..e168d79859 100644 --- a/pc/sdp_offer_answer.h +++ b/pc/sdp_offer_answer.h @@ -39,6 +39,7 @@ #include "api/set_remote_description_observer_interface.h" #include "api/transport/data_channel_transport_interface.h" #include "api/turn_customizer.h" +#include "api/uma_metrics.h" #include "api/video/video_bitrate_allocator_factory.h" #include "media/base/media_channel.h" #include "media/base/stream_params.h" @@ -638,6 +639,12 @@ class SdpOfferAnswerHandler : public SdpStateProvider, // Updates the error state, signaling if necessary. void SetSessionError(SessionError error, const std::string& error_desc); + // Implements AddIceCandidate without reporting usage, but returns the + // particular success/error value that should be reported (and can be utilized + // for other purposes). + AddIceCandidateResult AddIceCandidateInternal( + const IceCandidateInterface* candidate); + SessionError session_error_ RTC_GUARDED_BY(signaling_thread()) = SessionError::kNone; std::string session_error_desc_ RTC_GUARDED_BY(signaling_thread()); diff --git a/pc/test/integration_test_helpers.h b/pc/test/integration_test_helpers.h index 66b7d3f640..075a907200 100644 --- a/pc/test/integration_test_helpers.h +++ b/pc/test/integration_test_helpers.h @@ -1355,10 +1355,12 @@ class PeerConnectionIntegrationBaseTest : public ::testing::Test { // when Send() is called it will hit a seg fault. if (caller_) { caller_->set_signaling_message_receiver(nullptr); + caller_->pc()->Close(); delete SetCallerPcWrapperAndReturnCurrent(nullptr); } if (callee_) { callee_->set_signaling_message_receiver(nullptr); + callee_->pc()->Close(); delete SetCalleePcWrapperAndReturnCurrent(nullptr); } @@ -1779,8 +1781,10 @@ class PeerConnectionIntegrationBaseTest : public ::testing::Test { } void ClosePeerConnections() { - caller()->pc()->Close(); - callee()->pc()->Close(); + if (caller()) + caller()->pc()->Close(); + if (callee()) + callee()->pc()->Close(); } void TestNegotiatedCipherSuite(