From 86746c475f146a9f830ceccc3b0f63be12ed8b4a Mon Sep 17 00:00:00 2001 From: deadbeef Date: Sat, 29 Apr 2017 12:00:21 -0700 Subject: [PATCH] Revert of Fixing crash that can occur if signal is modified while firing. (patchset #3 id:40001 of https://codereview.webrtc.org/2846593005/ ) Reason for revert: Revealed a race condition (now reported by TSan), and breaks the scenario where a signal is deleted while firing. Original issue's description: > Fixing crash that can occur if signal is modified while firing. > > The crash occurs if a slot causes the very next slot in iteration order > to be disconnected. > > BUG=webrtc:7527 > > Review-Url: https://codereview.webrtc.org/2846593005 > Cr-Commit-Position: refs/heads/master@{#17943} > Committed: https://chromium.googlesource.com/external/webrtc/+/961c2adf1ea25bcc40ad64cae1ac2ea258e26de2 TBR=pthatcher@webrtc.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=webrtc:7527 Review-Url: https://codereview.webrtc.org/2847243006 Cr-Commit-Position: refs/heads/master@{#17947} --- webrtc/base/sigslot.h | 59 +++++++++------------------------ webrtc/base/sigslot_unittest.cc | 47 -------------------------- 2 files changed, 16 insertions(+), 90 deletions(-) diff --git a/webrtc/base/sigslot.h b/webrtc/base/sigslot.h index 652d63c0d2..45e0da216b 100644 --- a/webrtc/base/sigslot.h +++ b/webrtc/base/sigslot.h @@ -394,8 +394,7 @@ namespace sigslot { protected: typedef std::list< _opaque_connection > connections_list; - _signal_base() : _signal_base_interface(&_signal_base::do_slot_disconnect, &_signal_base::do_slot_duplicate), - m_current_iterator(m_connected_slots.end()) + _signal_base() : _signal_base_interface(&_signal_base::do_slot_disconnect, &_signal_base::do_slot_duplicate) { } @@ -408,8 +407,7 @@ namespace sigslot { _signal_base& operator= (_signal_base const& that); public: - _signal_base(const _signal_base& o) : _signal_base_interface(&_signal_base::do_slot_disconnect, &_signal_base::do_slot_duplicate), - m_current_iterator(m_connected_slots.end()) { + _signal_base(const _signal_base& o) : _signal_base_interface(&_signal_base::do_slot_disconnect, &_signal_base::do_slot_duplicate) { lock_block lock(this); for (const auto& connection : o.m_connected_slots) { @@ -462,14 +460,7 @@ namespace sigslot { { if(it->getdest() == pclass) { - // If we're currently using this iterator, - // don't erase it and invalidate it yet; set a - // flag to do so afterwards. - if (m_current_iterator == it) { - m_erase_current_iterator = true; - } else { - m_connected_slots.erase(it); - } + m_connected_slots.erase(it); pclass->signal_disconnect(static_cast< _signal_base_interface* >(this)); return; } @@ -493,15 +484,8 @@ namespace sigslot { if(it->getdest() == pslot) { - // If we're currently using this iterator, - // don't erase it and invalidate it yet; set a - // flag to do so afterwards. - if (self->m_current_iterator == it) { - self->m_erase_current_iterator = true; - } else { - self->m_connected_slots.erase(it); - } - } + self->m_connected_slots.erase(it); + } it = itNext; } @@ -527,12 +511,7 @@ namespace sigslot { protected: connections_list m_connected_slots; - - // Used to handle a slot being disconnected while a signal is - // firing (iterating m_connected_slots). - connections_list::iterator m_current_iterator; - bool m_erase_current_iterator = false; - }; + }; template class has_slots : public has_slots_interface, public mt_policy @@ -626,22 +605,16 @@ namespace sigslot { void emit(Args... args) { lock_block lock(this); - this->m_current_iterator = - this->m_connected_slots.begin(); - while (this->m_current_iterator != - this->m_connected_slots.end()) { - _opaque_connection const& conn = - *this->m_current_iterator; - conn.emit(args...); - if (this->m_erase_current_iterator) { - this->m_current_iterator = - this->m_connected_slots.erase( - this->m_current_iterator); - this->m_erase_current_iterator = false; - } else { - ++(this->m_current_iterator); - } - } + typename connections_list::const_iterator it = this->m_connected_slots.begin(); + typename connections_list::const_iterator itEnd = this->m_connected_slots.end(); + + while(it != itEnd) + { + _opaque_connection const& conn = *it; + ++it; + + conn.emit(args...); + } } void operator()(Args... args) diff --git a/webrtc/base/sigslot_unittest.cc b/webrtc/base/sigslot_unittest.cc index 94d0d4687f..e860fe3373 100644 --- a/webrtc/base/sigslot_unittest.cc +++ b/webrtc/base/sigslot_unittest.cc @@ -272,50 +272,3 @@ TEST(SigslotTest, CopyConnectedSlot) { signal(); EXPECT_EQ(1, copied_receiver.signal_count()); } - -// Just used for the test below. -class Disconnector : public sigslot::has_slots<> { - public: - Disconnector(SigslotReceiver<>* receiver1, SigslotReceiver<>* receiver2) - : receiver1_(receiver1), receiver2_(receiver2) {} - - void Connect(sigslot::signal<>* signal) { - signal_ = signal; - signal->connect(this, - &Disconnector::Disconnect); - } - - private: - void Disconnect() { - receiver1_->Disconnect(); - receiver2_->Disconnect(); - signal_->disconnect(this); - } - - sigslot::signal<>* signal_; - SigslotReceiver<>* receiver1_; - SigslotReceiver<>* receiver2_; -}; - -// Test that things work as expected if a signal is disconnected from a slot while it's -TEST(SigslotTest, DisconnectFromSignalWhileFiring) { - sigslot::signal<> signal; - SigslotReceiver<> receiver1; - SigslotReceiver<> receiver2; - SigslotReceiver<> receiver3; - Disconnector disconnector(&receiver1, &receiver2); - - // From this ordering, receiver1 should receive the signal, then the - // disconnector will be invoked, causing receiver2 to be disconnected before - // it receives the signal. And receiver2 should also receive the signal, - // since it was never disconnected. - receiver1.Connect(&signal); - disconnector.Connect(&signal); - receiver2.Connect(&signal); - receiver3.Connect(&signal); - signal(); - - EXPECT_EQ(1, receiver1.signal_count()); - EXPECT_EQ(0, receiver2.signal_count()); - EXPECT_EQ(1, receiver3.signal_count()); -}