From 2efb8a5ec61b1b87475d046c03d20244f53b14b6 Mon Sep 17 00:00:00 2001 From: Tomas Gunnarsson Date: Thu, 1 Apr 2021 16:26:57 +0200 Subject: [PATCH] Invalidate weak pointers in SdpOfferAnswerHandler::Close(). This stops pending internal callbacks from performing unnecessary operations when closed. Also update tests pc tests to call Close(). This will allow PeerConnection to be able to expect the normal path to be that IsClosed() be true in the dtor once all 'normal' paths do that Bug: webrtc:12633 Change-Id: I3882bedf200feda0d04594adeb0fdac85bfef652 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/213426 Commit-Queue: Tommi Reviewed-by: Niels Moller Cr-Commit-Position: refs/heads/master@{#33617} --- pc/peer_connection.cc | 4 ++ pc/peer_connection.h | 3 +- pc/peer_connection_interface_unittest.cc | 11 +++- pc/peer_connection_jsep_unittest.cc | 1 + pc/peer_connection_wrapper.cc | 5 +- pc/sdp_offer_answer.cc | 68 +++++++++++++----------- pc/sdp_offer_answer.h | 7 +++ pc/test/integration_test_helpers.h | 8 ++- 8 files changed, 70 insertions(+), 37 deletions(-) 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(