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 <tommi@webrtc.org>
Reviewed-by: Niels Moller <nisse@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33617}
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index a571a53..144328c 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -1746,6 +1746,10 @@
// 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 aa46feb..3423424 100644
--- a/pc/peer_connection.h
+++ b/pc/peer_connection.h
@@ -323,7 +323,8 @@
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 e3d0ab6..3f73168 100644
--- a/pc/peer_connection_interface_unittest.cc
+++ b/pc/peer_connection_interface_unittest.cc
@@ -683,7 +683,7 @@
#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 @@
PeerConnectionFactoryForTest::CreatePeerConnectionFactoryForTest();
}
+ void TearDown() override {
+ if (pc_)
+ pc_->Close();
+ }
+
void CreatePeerConnection() {
CreatePeerConnection(PeerConnectionInterface::RTCConfiguration());
}
@@ -734,6 +739,10 @@
}
void CreatePeerConnection(const RTCConfiguration& config) {
+ if (pc_) {
+ pc_->Close();
+ pc_ = nullptr;
+ }
std::unique_ptr<cricket::FakePortAllocator> 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 c3e0936..00e202c 100644
--- a/pc/peer_connection_jsep_unittest.cc
+++ b/pc/peer_connection_jsep_unittest.cc
@@ -2214,6 +2214,7 @@
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 328f579..65384ee 100644
--- a/pc/peer_connection_wrapper.cc
+++ b/pc/peer_connection_wrapper.cc
@@ -48,7 +48,10 @@
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 49b9df9..6b8412c 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 @@
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 @@
[this_weak_ptr = weak_ptr_factory_.GetWeakPtr(),
candidate = std::move(candidate), callback = std::move(callback)](
std::function<void()> 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 a717db8..e168d79 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 @@
// 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 66b7d3f..075a907 100644
--- a/pc/test/integration_test_helpers.h
+++ b/pc/test/integration_test_helpers.h
@@ -1355,10 +1355,12 @@
// 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 @@
}
void ClosePeerConnections() {
- caller()->pc()->Close();
- callee()->pc()->Close();
+ if (caller())
+ caller()->pc()->Close();
+ if (callee())
+ callee()->pc()->Close();
}
void TestNegotiatedCipherSuite(
