zqz/platform-external-webrtc
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[Connection] Replace pending_delete_ with !port_.

Browse Source 
`pending_delete_` was being used to protect from referencing a
potentially bad `port_` pointer. We now use a WeakPtr for the port
reference, which we clear inside of the Destroy() method. This means
we don't need both flags.

Bug: webrtc:13892
Change-Id: I9427829444486e97d30752893ba2a30b153a70e5
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/258980
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36554}
This commit is contained in:
Tommi
2022-04-14 20:18:03 +02:00
committed by WebRTC LUCI CQ
parent 8a1a0af36f
commit d229326578
2 changed files with 13 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files

26
p2p/base/connection.cc
View File

@ -315,6 +315,7 @@ Connection::Connection(rtc::WeakPtr<Port> port,
field_trials_(&kDefaultFieldTrials), field_trials_(&kDefaultFieldTrials),
rtt_estimate_(DEFAULT_RTT_ESTIMATE_HALF_TIME_MS) { rtt_estimate_(DEFAULT_RTT_ESTIMATE_HALF_TIME_MS) {
RTC_DCHECK_RUN_ON(network_thread_); RTC_DCHECK_RUN_ON(network_thread_);
RTC_DCHECK(port_);
RTC_LOG(LS_INFO) << ToString() << ": Connection created"; RTC_LOG(LS_INFO) << ToString() << ": Connection created";
} }
@ -831,11 +832,9 @@ void Connection::Prune() {
void Connection::Destroy() { void Connection::Destroy() {
RTC_DCHECK_RUN_ON(network_thread_); RTC_DCHECK_RUN_ON(network_thread_);
if (pending_delete_) if (!port_)
return; return;
pending_delete_ = true;
RTC_DLOG(LS_VERBOSE) << ToString() << ": Connection destroyed"; RTC_DLOG(LS_VERBOSE) << ToString() << ": Connection destroyed";
// Fire the 'destroyed' event before deleting the object. This is done // Fire the 'destroyed' event before deleting the object. This is done
@ -847,6 +846,10 @@ void Connection::Destroy() {
LogCandidatePairConfig(webrtc::IceCandidatePairConfigType::kDestroyed); LogCandidatePairConfig(webrtc::IceCandidatePairConfigType::kDestroyed);
// Reset the `port_` after logging and firing the destroyed signal since
// information required for logging needs access to `port_`.
port_.reset();
// Unwind the stack before deleting the object in case upstream callers // Unwind the stack before deleting the object in case upstream callers
// need to refer to the Connection's state as part of teardown. // need to refer to the Connection's state as part of teardown.
// NOTE: We move ownership of 'this' into the capture section of the lambda // NOTE: We move ownership of 'this' into the capture section of the lambda
@ -868,16 +871,13 @@ void Connection::FailAndPrune() {
// TODO(bugs.webrtc.org/13865): There's a circular dependency between Port // TODO(bugs.webrtc.org/13865): There's a circular dependency between Port
// and Connection. In some cases (Port dtor), a Connection object is deleted // and Connection. In some cases (Port dtor), a Connection object is deleted
// without using the `Destroy` method (pending_delete_ won't be raised and // without using the `Destroy` method (port_ won't be nulled and some
// some functionality won't run as expected), while in other cases // functionality won't run as expected), while in other cases
// (AddOrReplaceConnection), the Connection object is deleted asynchronously // (AddOrReplaceConnection), the Connection object is deleted asynchronously
// via the `Destroy` method and in that case `pending_delete_` will be raised. // via the `Destroy` method and in that case `port_` will be nulled.
// However, in that case, there's a chance that the Port object gets // However, in that case, there's a chance that the Port object gets
// deleted before the Connection object ends up being deleted. So, the // deleted before the Connection object ends up being deleted.
// Connection object holds on to a potentially bogus `port_` pointer. if (!port_)
// Here, we avoid such a potential, but the cleanup operations in Port
// need to be made consistent and safe.
if (pending_delete_)
return; return;
set_state(IceCandidatePairState::FAILED); set_state(IceCandidatePairState::FAILED);
@ -1226,7 +1226,7 @@ std::string Connection::ToString() const {
rtc::StringBuilder ss; rtc::StringBuilder ss;
ss << "Conn[" << ToDebugId(); ss << "Conn[" << ToDebugId();
if (pending_delete_) { if (!port_) {
// No content name for pending delete, so temporarily substitute the name // No content name for pending delete, so temporarily substitute the name
// with a hash (rhyming with trash) and don't include any information about // with a hash (rhyming with trash) and don't include any information about
// the network or candidates, state that belongs to a potentially deleted // the network or candidates, state that belongs to a potentially deleted
@ -1249,7 +1249,7 @@ std::string Connection::ToString() const {
<< "|" << SELECTED_STATE_ABBREV[selected_] << "|" << remote_nomination_ << "|" << SELECTED_STATE_ABBREV[selected_] << "|" << remote_nomination_
<< "|" << nomination_ << "|"; << "|" << nomination_ << "|";
if (!pending_delete_) if (port_)
ss << priority() << "|"; ss << priority() << "|";
if (rtt_ < DEFAULT_RTT) { if (rtt_ < DEFAULT_RTT) {

3
p2p/base/connection.h
View File

@ -367,9 +367,6 @@ class Connection : public CandidatePairInterface, public sigslot::has_slots<> {
rtc::RateTracker recv_rate_tracker_; rtc::RateTracker recv_rate_tracker_;
rtc::RateTracker send_rate_tracker_; rtc::RateTracker send_rate_tracker_;
int64_t last_send_data_ = 0; int64_t last_send_data_ = 0;
// Set to true when deletion has been scheduled and must not be done again.
// See `Destroy()` for more details.
bool pending_delete_ RTC_GUARDED_BY(network_thread_) = false;
private: private:
// Update the local candidate based on the mapped address attribute. // Update the local candidate based on the mapped address attribute.