From 7eaa4ea75f8d3440b735c0b8311b4c9c7997763d Mon Sep 17 00:00:00 2001 From: nisse Date: Mon, 8 May 2017 05:25:41 -0700 Subject: [PATCH] Delete method MessageQueue::set_socketserver Instead, make the pointer to the associated socket server a construction time const, and delete its lock. Introduces a helper class AutoSocketServerThread for code (mainly tests) which need a socket server associated with the current thread. BUG=webrtc:7501 Review-Url: https://codereview.webrtc.org/2828223002 Cr-Commit-Position: refs/heads/master@{#18047} --- webrtc/base/messagequeue.cc | 15 ----------- webrtc/base/messagequeue.h | 5 +--- webrtc/base/physicalsocketserver_unittest.cc | 7 +++-- webrtc/base/proxy_unittest.cc | 6 ++--- webrtc/base/ssladapter_unittest.cc | 7 ++--- webrtc/base/thread.cc | 22 ++++++++++++++++ webrtc/base/thread.h | 21 +++++++-------- webrtc/base/virtualsocket_unittest.cc | 5 ++-- webrtc/base/win32socketserver.cc | 5 ++-- webrtc/base/win32socketserver.h | 8 ++---- webrtc/base/win32socketserver_unittest.cc | 10 +++---- .../peerconnection/client/linux/main.cc | 21 ++++++++------- webrtc/examples/peerconnection/client/main.cc | 3 ++- webrtc/ortc/ortcfactory_unittest.cc | 4 +-- .../p2p/base/asyncstuntcpsocket_unittest.cc | 5 ++-- .../p2p/base/p2ptransportchannel_unittest.cc | 24 ++++++++--------- webrtc/p2p/base/port.cc | 5 ++++ webrtc/p2p/base/port_unittest.cc | 26 +++++++++---------- webrtc/p2p/base/relayport_unittest.cc | 12 ++++----- webrtc/p2p/base/relayserver_unittest.cc | 4 +-- webrtc/p2p/base/stunport_unittest.cc | 4 +-- webrtc/p2p/base/tcpport_unittest.cc | 12 ++++----- webrtc/p2p/base/turnport_unittest.cc | 20 +++++++------- webrtc/p2p/base/turnserver_unittest.cc | 6 ++--- webrtc/p2p/base/udptransport_unittest.cc | 8 +++--- .../p2p/client/basicportallocator_unittest.cc | 4 +-- webrtc/p2p/stunprober/stunprober_unittest.cc | 8 +++--- webrtc/pc/webrtcsession_unittest.cc | 18 ++++++------- 28 files changed, 142 insertions(+), 153 deletions(-) diff --git a/webrtc/base/messagequeue.cc b/webrtc/base/messagequeue.cc index efe618851a..cafb70bd00 100644 --- a/webrtc/base/messagequeue.cc +++ b/webrtc/base/messagequeue.cc @@ -216,30 +216,16 @@ void MessageQueue::DoDestroy() { MessageQueueManager::Remove(this); Clear(nullptr); - SharedScope ss(&ss_lock_); if (ss_) { ss_->SetMessageQueue(nullptr); } } SocketServer* MessageQueue::socketserver() { - SharedScope ss(&ss_lock_); return ss_; } -void MessageQueue::set_socketserver(SocketServer* ss) { - // Need to lock exclusively here to prevent simultaneous modifications from - // other threads. Can't be a shared lock to prevent races with other reading - // threads. - // Other places that only read "ss_" can use a shared lock as simultaneous - // read access is allowed. - ExclusiveScope es(&ss_lock_); - ss_ = ss ? ss : own_ss_.get(); - ss_->SetMessageQueue(this); -} - void MessageQueue::WakeUpSocketServer() { - SharedScope ss(&ss_lock_); ss_->WakeUp(); } @@ -357,7 +343,6 @@ bool MessageQueue::Get(Message *pmsg, int cmsWait, bool process_io) { { // Wait and multiplex in the meantime - SharedScope ss(&ss_lock_); if (!ss_->Wait(static_cast(cmsNext), process_io)) return false; } diff --git a/webrtc/base/messagequeue.h b/webrtc/base/messagequeue.h index 20e2e13bc5..e39c9f9869 100644 --- a/webrtc/base/messagequeue.h +++ b/webrtc/base/messagequeue.h @@ -26,7 +26,6 @@ #include "webrtc/base/location.h" #include "webrtc/base/messagehandler.h" #include "webrtc/base/scoped_ref_ptr.h" -#include "webrtc/base/sharedexclusivelock.h" #include "webrtc/base/sigslot.h" #include "webrtc/base/socketserver.h" #include "webrtc/base/timeutils.h" @@ -210,7 +209,6 @@ class MessageQueue { virtual ~MessageQueue(); SocketServer* socketserver(); - void set_socketserver(SocketServer* ss); // Note: The behavior of MessageQueue has changed. When a MQ is stopped, // futher Posts and Sends will fail. However, any pending Sends and *ready* @@ -317,10 +315,9 @@ class MessageQueue { volatile int stop_; // The SocketServer might not be owned by MessageQueue. - SocketServer* ss_ GUARDED_BY(ss_lock_); + SocketServer* const ss_; // Used if SocketServer ownership lies with |this|. std::unique_ptr own_ss_; - SharedExclusiveLock ss_lock_; RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(MessageQueue); }; diff --git a/webrtc/base/physicalsocketserver_unittest.cc b/webrtc/base/physicalsocketserver_unittest.cc index de0d485289..e2f05e977f 100644 --- a/webrtc/base/physicalsocketserver_unittest.cc +++ b/webrtc/base/physicalsocketserver_unittest.cc @@ -115,16 +115,15 @@ class PhysicalSocketTest : public SocketTest { protected: PhysicalSocketTest() : server_(new FakePhysicalSocketServer(this)), - scope_(server_.get()), + thread_(server_.get()), fail_accept_(false), - max_send_size_(-1) { - } + max_send_size_(-1) {} void ConnectInternalAcceptError(const IPAddress& loopback); void WritableAfterPartialWrite(const IPAddress& loopback); std::unique_ptr server_; - SocketServerScope scope_; + rtc::AutoSocketServerThread thread_; bool fail_accept_; int max_send_size_; }; diff --git a/webrtc/base/proxy_unittest.cc b/webrtc/base/proxy_unittest.cc index 631d91b605..7e040b7935 100644 --- a/webrtc/base/proxy_unittest.cc +++ b/webrtc/base/proxy_unittest.cc @@ -31,19 +31,19 @@ static const SocketAddress kBogusProxyIntAddr("1.2.3.4", 999); // Sets up a virtual socket server and HTTPS/SOCKS5 proxy servers. class ProxyTest : public testing::Test { public: - ProxyTest() : ss_(new rtc::VirtualSocketServer(nullptr)) { - Thread::Current()->set_socketserver(ss_.get()); + ProxyTest() : ss_(new rtc::VirtualSocketServer(nullptr)), thread_(ss_.get()) { socks_.reset(new rtc::SocksProxyServer( ss_.get(), kSocksProxyIntAddr, ss_.get(), kSocksProxyExtAddr)); https_.reset(new rtc::HttpListenServer()); https_->Listen(kHttpsProxyIntAddr); } - ~ProxyTest() { Thread::Current()->set_socketserver(nullptr); } + ~ProxyTest() {} rtc::SocketServer* ss() { return ss_.get(); } private: std::unique_ptr ss_; + rtc::AutoSocketServerThread thread_; std::unique_ptr socks_; // TODO: Make this a real HTTPS proxy server. std::unique_ptr https_; diff --git a/webrtc/base/ssladapter_unittest.cc b/webrtc/base/ssladapter_unittest.cc index c8afbe6a75..d32f73c364 100644 --- a/webrtc/base/ssladapter_unittest.cc +++ b/webrtc/base/ssladapter_unittest.cc @@ -274,7 +274,8 @@ class SSLAdapterTestBase : public testing::Test, explicit SSLAdapterTestBase(const rtc::SSLMode& ssl_mode, const rtc::KeyParams& key_params) : ssl_mode_(ssl_mode), - ss_scope_(new rtc::VirtualSocketServer(nullptr)), + vss_(new rtc::VirtualSocketServer(nullptr)), + thread_(vss_.get()), server_(new SSLAdapterTestDummyServer(ssl_mode_, key_params)), client_(new SSLAdapterTestDummyClient(ssl_mode_)), handshake_wait_(kTimeout) {} @@ -338,8 +339,8 @@ class SSLAdapterTestBase : public testing::Test, private: const rtc::SSLMode ssl_mode_; - const rtc::SocketServerScope ss_scope_; - + std::unique_ptr vss_; + rtc::AutoSocketServerThread thread_; std::unique_ptr server_; std::unique_ptr client_; diff --git a/webrtc/base/thread.cc b/webrtc/base/thread.cc index 9a71c1aa3f..1be5196c7d 100644 --- a/webrtc/base/thread.cc +++ b/webrtc/base/thread.cc @@ -528,4 +528,26 @@ AutoThread::~AutoThread() { } } +AutoSocketServerThread::AutoSocketServerThread(SocketServer* ss) + : Thread(ss) { + old_thread_ = ThreadManager::Instance()->CurrentThread(); + rtc::ThreadManager::Instance()->SetCurrentThread(this); + if (old_thread_) { + MessageQueueManager::Remove(old_thread_); + } +} + +AutoSocketServerThread::~AutoSocketServerThread() { + RTC_DCHECK(ThreadManager::Instance()->CurrentThread() == this); + // Some tests post destroy messages to this thread. To avoid memory + // leaks, we have to process those messages. In particular + // P2PTransportChannelPingTest, relying on the message posted in + // cricket::Connection::Destroy. + ProcessMessages(0); + rtc::ThreadManager::Instance()->SetCurrentThread(old_thread_); + if (old_thread_) { + MessageQueueManager::Add(old_thread_); + } +} + } // namespace rtc diff --git a/webrtc/base/thread.h b/webrtc/base/thread.h index e9bb3249ea..6e5da61005 100644 --- a/webrtc/base/thread.h +++ b/webrtc/base/thread.h @@ -311,21 +311,20 @@ class AutoThread : public Thread { RTC_DISALLOW_COPY_AND_ASSIGN(AutoThread); }; -// Provides an easy way to install/uninstall a socketserver on a thread. -class SocketServerScope { +// AutoSocketServerThread automatically installs itself at +// construction and uninstalls at destruction. If a Thread object is +// already associated with the current OS thread, it is temporarily +// disassociated and restored by the destructor. + +class AutoSocketServerThread : public Thread { public: - explicit SocketServerScope(SocketServer* ss) { - old_ss_ = Thread::Current()->socketserver(); - Thread::Current()->set_socketserver(ss); - } - ~SocketServerScope() { - Thread::Current()->set_socketserver(old_ss_); - } + explicit AutoSocketServerThread(SocketServer* ss); + ~AutoSocketServerThread() override; private: - SocketServer* old_ss_; + rtc::Thread* old_thread_; - RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(SocketServerScope); + RTC_DISALLOW_COPY_AND_ASSIGN(AutoSocketServerThread); }; } // namespace rtc diff --git a/webrtc/base/virtualsocket_unittest.cc b/webrtc/base/virtualsocket_unittest.cc index 488cada386..54a52d9eae 100644 --- a/webrtc/base/virtualsocket_unittest.cc +++ b/webrtc/base/virtualsocket_unittest.cc @@ -147,6 +147,7 @@ class VirtualSocketServerTest : public testing::Test { public: VirtualSocketServerTest() : ss_(nullptr), + thread_(&ss_), kIPv4AnyAddress(IPAddress(INADDR_ANY), 0), kIPv6AnyAddress(IPAddress(in6addr_any), 0) {} @@ -828,10 +829,8 @@ class VirtualSocketServerTest : public testing::Test { } protected: - virtual void SetUp() { Thread::Current()->set_socketserver(&ss_); } - virtual void TearDown() { Thread::Current()->set_socketserver(nullptr); } - VirtualSocketServer ss_; + AutoSocketServerThread thread_; const SocketAddress kIPv4AnyAddress; const SocketAddress kIPv6AnyAddress; }; diff --git a/webrtc/base/win32socketserver.cc b/webrtc/base/win32socketserver.cc index 3ee88dbaf5..2a63aec0b5 100644 --- a/webrtc/base/win32socketserver.cc +++ b/webrtc/base/win32socketserver.cc @@ -726,9 +726,8 @@ void Win32Socket::OnDnsNotify(HANDLE task, int error) { static UINT s_wm_wakeup_id = 0; const TCHAR Win32SocketServer::kWindowName[] = L"libjingle Message Window"; -Win32SocketServer::Win32SocketServer(MessageQueue* message_queue) - : message_queue_(message_queue), - wnd_(this), +Win32SocketServer::Win32SocketServer() + : wnd_(this), posted_(false), hdlg_(nullptr) { if (s_wm_wakeup_id == 0) diff --git a/webrtc/base/win32socketserver.h b/webrtc/base/win32socketserver.h index a9a53c33c1..cec68ef3fd 100644 --- a/webrtc/base/win32socketserver.h +++ b/webrtc/base/win32socketserver.h @@ -93,7 +93,7 @@ class Win32Socket : public AsyncSocket { class Win32SocketServer : public SocketServer { public: - explicit Win32SocketServer(MessageQueue* message_queue); + Win32SocketServer(); virtual ~Win32SocketServer(); void set_modeless_dialog(HWND hdlg) { @@ -138,12 +138,9 @@ class Win32SocketServer : public SocketServer { class Win32Thread : public Thread { public: - Win32Thread() : ss_(this), id_(0) { - set_socketserver(&ss_); - } + explicit Win32Thread(SocketServer* ss) : Thread(ss), id_(0) {} virtual ~Win32Thread() { Stop(); - set_socketserver(nullptr); } virtual void Run() { id_ = GetCurrentThreadId(); @@ -154,7 +151,6 @@ class Win32Thread : public Thread { PostThreadMessage(id_, WM_QUIT, 0, 0); } private: - Win32SocketServer ss_; DWORD id_; }; diff --git a/webrtc/base/win32socketserver_unittest.cc b/webrtc/base/win32socketserver_unittest.cc index 94d96da428..7211cd7f7f 100644 --- a/webrtc/base/win32socketserver_unittest.cc +++ b/webrtc/base/win32socketserver_unittest.cc @@ -16,7 +16,7 @@ namespace rtc { // Test that Win32SocketServer::Wait works as expected. TEST(Win32SocketServerTest, TestWait) { - Win32SocketServer server(nullptr); + Win32SocketServer server; uint32_t start = Time(); server.Wait(1000, true); EXPECT_GE(TimeSince(start), 1000); @@ -24,8 +24,8 @@ TEST(Win32SocketServerTest, TestWait) { // Test that Win32Socket::Pump does not touch general Windows messages. TEST(Win32SocketServerTest, TestPump) { - Win32SocketServer server(nullptr); - SocketServerScope scope(&server); + Win32SocketServer server; + rtc::AutoSocketServerThread thread(&server); EXPECT_EQ(TRUE, PostMessage(nullptr, WM_USER, 999, 0)); server.Pump(); MSG msg; @@ -37,9 +37,9 @@ TEST(Win32SocketServerTest, TestPump) { // Test that Win32Socket passes all the generic Socket tests. class Win32SocketTest : public SocketTest { protected: - Win32SocketTest() : server_(nullptr), scope_(&server_) {} + Win32SocketTest() : thread_(&server_) {} Win32SocketServer server_; - SocketServerScope scope_; + rtc::AutoSocketServerThread thread_; }; TEST_F(Win32SocketTest, TestConnectIPv4) { diff --git a/webrtc/examples/peerconnection/client/linux/main.cc b/webrtc/examples/peerconnection/client/linux/main.cc index 2982c287f1..7886f766f9 100644 --- a/webrtc/examples/peerconnection/client/linux/main.cc +++ b/webrtc/examples/peerconnection/client/linux/main.cc @@ -20,10 +20,14 @@ class CustomSocketServer : public rtc::PhysicalSocketServer { public: - CustomSocketServer(rtc::Thread* thread, GtkMainWnd* wnd) - : thread_(thread), wnd_(wnd), conductor_(NULL), client_(NULL) {} + explicit CustomSocketServer(GtkMainWnd* wnd) + : wnd_(wnd), conductor_(NULL), client_(NULL) {} virtual ~CustomSocketServer() {} + void SetMessageQueue(rtc::MessageQueue* queue) override { + message_queue_ = queue; + } + void set_client(PeerConnectionClient* client) { client_ = client; } void set_conductor(Conductor* conductor) { conductor_ = conductor; } @@ -39,14 +43,14 @@ class CustomSocketServer : public rtc::PhysicalSocketServer { if (!wnd_->IsWindow() && !conductor_->connection_active() && client_ != NULL && !client_->is_connected()) { - thread_->Quit(); + message_queue_->Quit(); } return rtc::PhysicalSocketServer::Wait(0/*cms == -1 ? 1 : cms*/, process_io); } protected: - rtc::Thread* thread_; + rtc::MessageQueue* message_queue_; GtkMainWnd* wnd_; Conductor* conductor_; PeerConnectionClient* client_; @@ -81,10 +85,8 @@ int main(int argc, char* argv[]) { GtkMainWnd wnd(FLAG_server, FLAG_port, FLAG_autoconnect, FLAG_autocall); wnd.Create(); - rtc::AutoThread auto_thread; - rtc::Thread* thread = rtc::Thread::Current(); - CustomSocketServer socket_server(thread, &wnd); - thread->set_socketserver(&socket_server); + CustomSocketServer socket_server(&wnd); + rtc::AutoSocketServerThread thread(&socket_server); rtc::InitializeSSL(); // Must be constructed after we set the socketserver. @@ -94,12 +96,11 @@ int main(int argc, char* argv[]) { socket_server.set_client(&client); socket_server.set_conductor(conductor); - thread->Run(); + thread.Run(); // gtk_main(); wnd.Destroy(); - thread->set_socketserver(NULL); // TODO(henrike): Run the Gtk main loop to tear down the connection. /* while (gtk_events_pending()) { diff --git a/webrtc/examples/peerconnection/client/main.cc b/webrtc/examples/peerconnection/client/main.cc index 413069d5e9..5ec1a7ae7d 100644 --- a/webrtc/examples/peerconnection/client/main.cc +++ b/webrtc/examples/peerconnection/client/main.cc @@ -21,7 +21,8 @@ int PASCAL wWinMain(HINSTANCE instance, HINSTANCE prev_instance, wchar_t* cmd_line, int cmd_show) { rtc::EnsureWinsockInit(); - rtc::Win32Thread w32_thread; + rtc::Win32SocketServer w32_ss; + rtc::Win32Thread w32_thread(&w32_ss); rtc::ThreadManager::Instance()->SetCurrentThread(&w32_thread); rtc::WindowsCommandLineArguments win_args; diff --git a/webrtc/ortc/ortcfactory_unittest.cc b/webrtc/ortc/ortcfactory_unittest.cc index 200939adaa..84745babf9 100644 --- a/webrtc/ortc/ortcfactory_unittest.cc +++ b/webrtc/ortc/ortcfactory_unittest.cc @@ -28,7 +28,7 @@ class OrtcFactoryTest : public testing::Test { public: OrtcFactoryTest() : virtual_socket_server_(&physical_socket_server_), - socket_server_scope_(&virtual_socket_server_), + thread_(&virtual_socket_server_), fake_packet_transport_("fake transport") { ortc_factory_ = OrtcFactory::Create(nullptr, nullptr, &fake_network_manager_, nullptr, @@ -51,7 +51,7 @@ class OrtcFactoryTest : public testing::Test { rtc::PhysicalSocketServer physical_socket_server_; rtc::VirtualSocketServer virtual_socket_server_; - rtc::SocketServerScope socket_server_scope_; + rtc::AutoSocketServerThread thread_; rtc::FakeNetworkManager fake_network_manager_; rtc::FakePacketTransport fake_packet_transport_; std::unique_ptr ortc_factory_; diff --git a/webrtc/p2p/base/asyncstuntcpsocket_unittest.cc b/webrtc/p2p/base/asyncstuntcpsocket_unittest.cc index 5929d1f84d..41b3be2765 100644 --- a/webrtc/p2p/base/asyncstuntcpsocket_unittest.cc +++ b/webrtc/p2p/base/asyncstuntcpsocket_unittest.cc @@ -70,8 +70,7 @@ class AsyncStunTCPSocketTest : public testing::Test, protected: AsyncStunTCPSocketTest() : vss_(new rtc::VirtualSocketServer(NULL)), - ss_scope_(vss_.get()) { - } + thread_(vss_.get()) {} virtual void SetUp() { CreateSockets(); @@ -125,7 +124,7 @@ class AsyncStunTCPSocketTest : public testing::Test, } std::unique_ptr vss_; - rtc::SocketServerScope ss_scope_; + rtc::AutoSocketServerThread thread_; std::unique_ptr send_socket_; std::unique_ptr recv_socket_; std::unique_ptr listen_socket_; diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index e9d4be60f1..29faac3728 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -182,14 +182,13 @@ class P2PTransportChannelTestBase : public testing::Test, public sigslot::has_slots<> { public: P2PTransportChannelTestBase() - : main_(rtc::Thread::Current()), - pss_(new rtc::PhysicalSocketServer), + : pss_(new rtc::PhysicalSocketServer), vss_(new rtc::VirtualSocketServer(pss_.get())), nss_(new rtc::NATSocketServer(vss_.get())), ss_(new rtc::FirewallSocketServer(nss_.get())), - ss_scope_(ss_.get()), - stun_server_(TestStunServer::Create(main_, kStunAddr)), - turn_server_(main_, kTurnUdpIntAddr, kTurnUdpExtAddr), + main_(ss_.get()), + stun_server_(TestStunServer::Create(&main_, kStunAddr)), + turn_server_(&main_, kTurnUdpIntAddr, kTurnUdpExtAddr), socks_server1_(ss_.get(), kSocksProxyAddrs[0], ss_.get(), @@ -696,8 +695,8 @@ class P2PTransportChannelTestBase : public testing::Test, GetEndpoint(ch)->saved_candidates_.push_back( std::unique_ptr(new CandidatesData(ch, c))); } else { - main_->Post(RTC_FROM_HERE, this, MSG_ADD_CANDIDATES, - new CandidatesData(ch, c)); + main_.Post(RTC_FROM_HERE, this, MSG_ADD_CANDIDATES, + new CandidatesData(ch, c)); } } void OnSelectedCandidatePairChanged( @@ -726,7 +725,7 @@ class P2PTransportChannelTestBase : public testing::Test, const std::vector& candidates) { // Candidate removals are not paused. CandidatesData* candidates_data = new CandidatesData(ch, candidates); - main_->Post(RTC_FROM_HERE, this, MSG_REMOVE_CANDIDATES, candidates_data); + main_.Post(RTC_FROM_HERE, this, MSG_REMOVE_CANDIDATES, candidates_data); } // Tcp candidate verification has to be done when they are generated. @@ -749,7 +748,7 @@ class P2PTransportChannelTestBase : public testing::Test, void ResumeCandidates(int endpoint) { Endpoint* ed = GetEndpoint(endpoint); for (auto& candidate : ed->saved_candidates_) { - main_->Post(RTC_FROM_HERE, this, MSG_ADD_CANDIDATES, candidate.release()); + main_.Post(RTC_FROM_HERE, this, MSG_ADD_CANDIDATES, candidate.release()); } ed->saved_candidates_.clear(); ed->save_candidates_ = false; @@ -875,12 +874,11 @@ class P2PTransportChannelTestBase : public testing::Test, bool nominated() { return nominated_; } private: - rtc::Thread* main_; std::unique_ptr pss_; std::unique_ptr vss_; std::unique_ptr nss_; std::unique_ptr ss_; - rtc::SocketServerScope ss_scope_; + rtc::AutoSocketServerThread main_; std::unique_ptr stun_server_; TestTurnServer turn_server_; rtc::SocksProxyServer socks_server1_; @@ -2934,7 +2932,7 @@ class P2PTransportChannelPingTest : public testing::Test, P2PTransportChannelPingTest() : pss_(new rtc::PhysicalSocketServer), vss_(new rtc::VirtualSocketServer(pss_.get())), - ss_scope_(vss_.get()) {} + thread_(vss_.get()) {} protected: void PrepareChannel(P2PTransportChannel* ch) { @@ -3084,7 +3082,7 @@ class P2PTransportChannelPingTest : public testing::Test, private: std::unique_ptr pss_; std::unique_ptr vss_; - rtc::SocketServerScope ss_scope_; + rtc::AutoSocketServerThread thread_; CandidatePairInterface* last_selected_candidate_pair_ = nullptr; int selected_candidate_pair_switches_ = 0; int last_sent_packet_id_ = -1; diff --git a/webrtc/p2p/base/port.cc b/webrtc/p2p/base/port.cc index 93fda17e7c..bf549248b7 100644 --- a/webrtc/p2p/base/port.cc +++ b/webrtc/p2p/base/port.cc @@ -1148,6 +1148,11 @@ void Connection::Prune() { } void Connection::Destroy() { + // TODO(deadbeef, nisse): This may leak if an application closes a + // PeerConnection and then quickly destroys the PeerConnectionFactory (along + // with the networking thread on which this message is posted). Also affects + // tests, with a workaround in + // AutoSocketServerThread::~AutoSocketServerThread. LOG_J(LS_VERBOSE, this) << "Connection destroyed"; port_->thread()->Post(RTC_FROM_HERE, this, MSG_DELETE); } diff --git a/webrtc/p2p/base/port_unittest.cc b/webrtc/p2p/base/port_unittest.cc index 7cb5275edc..61a573b116 100644 --- a/webrtc/p2p/base/port_unittest.cc +++ b/webrtc/p2p/base/port_unittest.cc @@ -381,19 +381,18 @@ class TestChannel : public sigslot::has_slots<> { class PortTest : public testing::Test, public sigslot::has_slots<> { public: PortTest() - : main_(rtc::Thread::Current()), - pss_(new rtc::PhysicalSocketServer), + : pss_(new rtc::PhysicalSocketServer), ss_(new rtc::VirtualSocketServer(pss_.get())), - ss_scope_(ss_.get()), + main_(ss_.get()), network_("unittest", "unittest", rtc::IPAddress(INADDR_ANY), 32), socket_factory_(rtc::Thread::Current()), nat_factory1_(ss_.get(), kNatAddr1, SocketAddress()), nat_factory2_(ss_.get(), kNatAddr2, SocketAddress()), nat_socket_factory1_(&nat_factory1_), nat_socket_factory2_(&nat_factory2_), - stun_server_(TestStunServer::Create(main_, kStunAddr)), - turn_server_(main_, kTurnUdpIntAddr, kTurnUdpExtAddr), - relay_server_(main_, + stun_server_(TestStunServer::Create(&main_, kStunAddr)), + turn_server_(&main_, kTurnUdpIntAddr, kTurnUdpExtAddr), + relay_server_(&main_, kRelayUdpIntAddr, kRelayUdpExtAddr, kRelayTcpIntAddr, @@ -492,7 +491,7 @@ class PortTest : public testing::Test, public sigslot::has_slots<> { } UDPPort* CreateUdpPort(const SocketAddress& addr, PacketSocketFactory* socket_factory) { - return UDPPort::Create(main_, socket_factory, &network_, addr.ipaddr(), 0, + return UDPPort::Create(&main_, socket_factory, &network_, addr.ipaddr(), 0, 0, username_, password_, std::string(), true); } TCPPort* CreateTcpPort(const SocketAddress& addr) { @@ -500,7 +499,7 @@ class PortTest : public testing::Test, public sigslot::has_slots<> { } TCPPort* CreateTcpPort(const SocketAddress& addr, PacketSocketFactory* socket_factory) { - return TCPPort::Create(main_, socket_factory, &network_, + return TCPPort::Create(&main_, socket_factory, &network_, addr.ipaddr(), 0, 0, username_, password_, true); } @@ -508,7 +507,7 @@ class PortTest : public testing::Test, public sigslot::has_slots<> { rtc::PacketSocketFactory* factory) { ServerAddresses stun_servers; stun_servers.insert(kStunAddr); - return StunPort::Create(main_, factory, &network_, + return StunPort::Create(&main_, factory, &network_, addr.ipaddr(), 0, 0, username_, password_, stun_servers, std::string()); @@ -533,7 +532,7 @@ class PortTest : public testing::Test, public sigslot::has_slots<> { PacketSocketFactory* socket_factory, ProtocolType int_proto, ProtocolType ext_proto, const rtc::SocketAddress& server_addr) { - return TurnPort::Create(main_, socket_factory, &network_, addr.ipaddr(), 0, + return TurnPort::Create(&main_, socket_factory, &network_, addr.ipaddr(), 0, 0, username_, password_, ProtocolAddress(server_addr, int_proto), kRelayCredentials, 0, std::string()); @@ -550,7 +549,7 @@ class PortTest : public testing::Test, public sigslot::has_slots<> { // TODO(pthatcher): Remove GTURN. // Generate a username with length of 16 for Gturn only. std::string username = rtc::CreateRandomString(kGturnUserNameLength); - return RelayPort::Create(main_, &socket_factory_, &network_, addr.ipaddr(), + return RelayPort::Create(&main_, &socket_factory_, &network_, addr.ipaddr(), 0, 0, username, password_); // TODO: Add an external address for ext_proto, so that the // other side can connect to this port using a non-UDP protocol. @@ -767,7 +766,7 @@ class PortTest : public testing::Test, public sigslot::has_slots<> { TestPort* CreateTestPort(const rtc::SocketAddress& addr, const std::string& username, const std::string& password) { - TestPort* port = new TestPort(main_, "test", &socket_factory_, &network_, + TestPort* port = new TestPort(&main_, "test", &socket_factory_, &network_, addr.ipaddr(), 0, 0, username, password); port->SignalRoleConflict.connect(this, &PortTest::OnRoleConflict); return port; @@ -802,10 +801,9 @@ class PortTest : public testing::Test, public sigslot::has_slots<> { rtc::VirtualSocketServer* vss() { return ss_.get(); } private: - rtc::Thread* main_; std::unique_ptr pss_; std::unique_ptr ss_; - rtc::SocketServerScope ss_scope_; + rtc::AutoSocketServerThread main_; rtc::Network network_; rtc::BasicPacketSocketFactory socket_factory_; std::unique_ptr nat_server1_; diff --git a/webrtc/p2p/base/relayport_unittest.cc b/webrtc/p2p/base/relayport_unittest.cc index 8574af3a74..0d699532cf 100644 --- a/webrtc/p2p/base/relayport_unittest.cc +++ b/webrtc/p2p/base/relayport_unittest.cc @@ -45,20 +45,19 @@ class RelayPortTest : public testing::Test, public sigslot::has_slots<> { public: RelayPortTest() - : main_(rtc::Thread::Current()), - physical_socket_server_(new rtc::PhysicalSocketServer), + : physical_socket_server_(new rtc::PhysicalSocketServer), virtual_socket_server_(new rtc::VirtualSocketServer( physical_socket_server_.get())), - ss_scope_(virtual_socket_server_.get()), + main_(virtual_socket_server_.get()), network_("unittest", "unittest", rtc::IPAddress(INADDR_ANY), 32), socket_factory_(rtc::Thread::Current()), username_(rtc::CreateRandomString(16)), password_(rtc::CreateRandomString(16)), - relay_port_(cricket::RelayPort::Create(main_, &socket_factory_, + relay_port_(cricket::RelayPort::Create(&main_, &socket_factory_, &network_, kLocalAddress.ipaddr(), 0, 0, username_, password_)), - relay_server_(new cricket::RelayServer(main_)) { + relay_server_(new cricket::RelayServer(&main_)) { } void OnReadPacket(rtc::AsyncPacketSocket* socket, @@ -247,10 +246,9 @@ class RelayPortTest : public testing::Test, typedef std::map PacketMap; - rtc::Thread* main_; std::unique_ptr physical_socket_server_; std::unique_ptr virtual_socket_server_; - rtc::SocketServerScope ss_scope_; + rtc::AutoSocketServerThread main_; rtc::Network network_; rtc::BasicPacketSocketFactory socket_factory_; std::string username_; diff --git a/webrtc/p2p/base/relayserver_unittest.cc b/webrtc/p2p/base/relayserver_unittest.cc index a53a66668a..057b5eea27 100644 --- a/webrtc/p2p/base/relayserver_unittest.cc +++ b/webrtc/p2p/base/relayserver_unittest.cc @@ -42,7 +42,7 @@ class RelayServerTest : public testing::Test { RelayServerTest() : pss_(new rtc::PhysicalSocketServer), ss_(new rtc::VirtualSocketServer(pss_.get())), - ss_scope_(ss_.get()), + thread_(ss_.get()), username_(rtc::CreateRandomString(12)), password_(rtc::CreateRandomString(12)) {} @@ -168,7 +168,7 @@ class RelayServerTest : public testing::Test { std::unique_ptr pss_; std::unique_ptr ss_; - rtc::SocketServerScope ss_scope_; + rtc::AutoSocketServerThread thread_; std::unique_ptr server_; std::unique_ptr client1_; std::unique_ptr client2_; diff --git a/webrtc/p2p/base/stunport_unittest.cc b/webrtc/p2p/base/stunport_unittest.cc index 52bb3e84a8..9fbcfec5cb 100644 --- a/webrtc/p2p/base/stunport_unittest.cc +++ b/webrtc/p2p/base/stunport_unittest.cc @@ -46,7 +46,7 @@ class StunPortTestBase : public testing::Test, public sigslot::has_slots<> { StunPortTestBase() : pss_(new rtc::PhysicalSocketServer), ss_(new rtc::VirtualSocketServer(pss_.get())), - ss_scope_(ss_.get()), + thread_(ss_.get()), network_("unittest", "unittest", rtc::IPAddress(INADDR_ANY), 32), socket_factory_(rtc::Thread::Current()), stun_server_1_(cricket::TestStunServer::Create(rtc::Thread::Current(), @@ -158,7 +158,7 @@ class StunPortTestBase : public testing::Test, public sigslot::has_slots<> { private: std::unique_ptr pss_; std::unique_ptr ss_; - rtc::SocketServerScope ss_scope_; + rtc::AutoSocketServerThread thread_; rtc::Network network_; rtc::BasicPacketSocketFactory socket_factory_; std::unique_ptr stun_port_; diff --git a/webrtc/p2p/base/tcpport_unittest.cc b/webrtc/p2p/base/tcpport_unittest.cc index 5a232021bb..7b354df673 100644 --- a/webrtc/p2p/base/tcpport_unittest.cc +++ b/webrtc/p2p/base/tcpport_unittest.cc @@ -31,10 +31,9 @@ static const SocketAddress kRemoteAddr("22.22.22.22", 2); class TCPPortTest : public testing::Test, public sigslot::has_slots<> { public: TCPPortTest() - : main_(rtc::Thread::Current()), - pss_(new rtc::PhysicalSocketServer), + : pss_(new rtc::PhysicalSocketServer), ss_(new rtc::VirtualSocketServer(pss_.get())), - ss_scope_(ss_.get()), + main_(ss_.get()), network_("unittest", "unittest", rtc::IPAddress(INADDR_ANY), 32), socket_factory_(rtc::Thread::Current()), username_(rtc::CreateRandomString(ICE_UFRAG_LENGTH)), @@ -59,15 +58,14 @@ class TCPPortTest : public testing::Test, public sigslot::has_slots<> { } TCPPort* CreateTCPPort(const SocketAddress& addr) { - return TCPPort::Create(main_, &socket_factory_, &network_, addr.ipaddr(), 0, - 0, username_, password_, true); + return TCPPort::Create(&main_, &socket_factory_, &network_, addr.ipaddr(), + 0, 0, username_, password_, true); } protected: - rtc::Thread* main_; std::unique_ptr pss_; std::unique_ptr ss_; - rtc::SocketServerScope ss_scope_; + rtc::AutoSocketServerThread main_; rtc::Network network_; rtc::BasicPacketSocketFactory socket_factory_; std::string username_; diff --git a/webrtc/p2p/base/turnport_unittest.cc b/webrtc/p2p/base/turnport_unittest.cc index 7223eabbee..9377fb4ad8 100644 --- a/webrtc/p2p/base/turnport_unittest.cc +++ b/webrtc/p2p/base/turnport_unittest.cc @@ -143,13 +143,12 @@ class TurnPortTest : public testing::Test, public rtc::MessageHandler { public: TurnPortTest() - : main_(rtc::Thread::Current()), - pss_(new rtc::PhysicalSocketServer), + : pss_(new rtc::PhysicalSocketServer), ss_(new TurnPortTestVirtualSocketServer(pss_.get())), - ss_scope_(ss_.get()), + main_(ss_.get()), network_("unittest", "unittest", rtc::IPAddress(INADDR_ANY), 32), socket_factory_(rtc::Thread::Current()), - turn_server_(main_, kTurnUdpIntAddr, kTurnUdpExtAddr), + turn_server_(&main_, kTurnUdpIntAddr, kTurnUdpExtAddr), turn_ready_(false), turn_error_(false), turn_unknown_address_(false), @@ -243,7 +242,7 @@ class TurnPortTest : public testing::Test, const std::string& password, const ProtocolAddress& server_address) { RelayCredentials credentials(username, password); - turn_port_.reset(TurnPort::Create(main_, &socket_factory_, &network_, + turn_port_.reset(TurnPort::Create(&main_, &socket_factory_, &network_, local_address.ipaddr(), 0, 0, kIceUfrag1, kIcePwd1, server_address, credentials, 0, @@ -261,7 +260,7 @@ class TurnPortTest : public testing::Test, const ProtocolAddress& server_address, const std::string& origin) { RelayCredentials credentials(username, password); - turn_port_.reset(TurnPort::Create(main_, &socket_factory_, &network_, + turn_port_.reset(TurnPort::Create(&main_, &socket_factory_, &network_, local_address.ipaddr(), 0, 0, kIceUfrag1, kIcePwd1, server_address, credentials, 0, @@ -286,8 +285,8 @@ class TurnPortTest : public testing::Test, RelayCredentials credentials(username, password); turn_port_.reset(TurnPort::Create( - main_, &socket_factory_, &network_, socket_.get(), kIceUfrag1, kIcePwd1, - server_address, credentials, 0, std::string())); + &main_, &socket_factory_, &network_, socket_.get(), kIceUfrag1, + kIcePwd1, server_address, credentials, 0, std::string())); // This TURN port will be the controlling. turn_port_->SetIceRole(ICEROLE_CONTROLLING); ConnectSignals(); @@ -309,7 +308,7 @@ class TurnPortTest : public testing::Test, void CreateUdpPort() { CreateUdpPort(kLocalAddr2); } void CreateUdpPort(const SocketAddress& address) { - udp_port_.reset(UDPPort::Create(main_, &socket_factory_, &network_, + udp_port_.reset(UDPPort::Create(&main_, &socket_factory_, &network_, address.ipaddr(), 0, 0, kIceUfrag2, kIcePwd2, std::string(), false)); // UDP port will be controlled. @@ -620,10 +619,9 @@ class TurnPortTest : public testing::Test, protected: rtc::ScopedFakeClock fake_clock_; - rtc::Thread* main_; std::unique_ptr pss_; std::unique_ptr ss_; - rtc::SocketServerScope ss_scope_; + rtc::AutoSocketServerThread main_; rtc::Network network_; rtc::BasicPacketSocketFactory socket_factory_; std::unique_ptr socket_; diff --git a/webrtc/p2p/base/turnserver_unittest.cc b/webrtc/p2p/base/turnserver_unittest.cc index a63670b21d..631eb46466 100644 --- a/webrtc/p2p/base/turnserver_unittest.cc +++ b/webrtc/p2p/base/turnserver_unittest.cc @@ -21,7 +21,7 @@ namespace cricket { class TurnServerConnectionTest : public testing::Test { public: - TurnServerConnectionTest() : vss_(&pss_), ss_scope_(&vss_) {} + TurnServerConnectionTest() : vss_(&pss_), thread_(&vss_) {} void ExpectEqual(const TurnServerConnection& a, const TurnServerConnection& b) { @@ -41,8 +41,8 @@ class TurnServerConnectionTest : public testing::Test { protected: rtc::PhysicalSocketServer pss_; rtc::VirtualSocketServer vss_; - rtc::SocketServerScope ss_scope_; - // Since this is constructed after |ss_scope_|, it will pick up |ss_scope_|'s + rtc::AutoSocketServerThread thread_; + // Since this is constructed after |thread_|, it will pick up |threads_|'s // socket server. rtc::BasicPacketSocketFactory socket_factory_; }; diff --git a/webrtc/p2p/base/udptransport_unittest.cc b/webrtc/p2p/base/udptransport_unittest.cc index 339fbbcc40..5c4f265a7f 100644 --- a/webrtc/p2p/base/udptransport_unittest.cc +++ b/webrtc/p2p/base/udptransport_unittest.cc @@ -35,11 +35,10 @@ static const rtc::IPAddress kIPv4LocalHostAddress = class UdpTransportTest : public testing::Test, public sigslot::has_slots<> { public: UdpTransportTest() - : network_thread_(rtc::Thread::Current()), - physical_socket_server_(new rtc::PhysicalSocketServer), + : physical_socket_server_(new rtc::PhysicalSocketServer), virtual_socket_server_( new rtc::VirtualSocketServer(physical_socket_server_.get())), - ss_scope_(virtual_socket_server_.get()), + network_thread_(virtual_socket_server_.get()), ep1_("Name1", std::unique_ptr( socket_factory_.CreateUdpSocket( @@ -121,10 +120,9 @@ class UdpTransportTest : public testing::Test, public sigslot::has_slots<> { uint32_t num_sig_ready_to_send_ = 0; // Increases on SignalReadyToSend. }; - rtc::Thread* network_thread_ = nullptr; std::unique_ptr physical_socket_server_; std::unique_ptr virtual_socket_server_; - rtc::SocketServerScope ss_scope_; + rtc::AutoSocketServerThread network_thread_; // Uses current thread's socket server, which will be set by ss_scope_. rtc::BasicPacketSocketFactory socket_factory_; diff --git a/webrtc/p2p/client/basicportallocator_unittest.cc b/webrtc/p2p/client/basicportallocator_unittest.cc index 9b2146980f..6cc2259f0f 100644 --- a/webrtc/p2p/client/basicportallocator_unittest.cc +++ b/webrtc/p2p/client/basicportallocator_unittest.cc @@ -113,7 +113,7 @@ class BasicPortAllocatorTestBase : public testing::Test, : pss_(new rtc::PhysicalSocketServer), vss_(new rtc::VirtualSocketServer(pss_.get())), fss_(new rtc::FirewallSocketServer(vss_.get())), - ss_scope_(fss_.get()), + thread_(fss_.get()), // Note that the NAT is not used by default. ResetWithStunServerAndNat // must be called. nat_factory_(vss_.get(), kNatUdpAddr, kNatTcpAddr), @@ -461,7 +461,7 @@ class BasicPortAllocatorTestBase : public testing::Test, std::unique_ptr pss_; std::unique_ptr vss_; std::unique_ptr fss_; - rtc::SocketServerScope ss_scope_; + rtc::AutoSocketServerThread thread_; std::unique_ptr nat_server_; rtc::NATSocketFactory nat_factory_; std::unique_ptr nat_socket_factory_; diff --git a/webrtc/p2p/stunprober/stunprober_unittest.cc b/webrtc/p2p/stunprober/stunprober_unittest.cc index e194e4f3ae..5b9158122b 100644 --- a/webrtc/p2p/stunprober/stunprober_unittest.cc +++ b/webrtc/p2p/stunprober/stunprober_unittest.cc @@ -41,10 +41,9 @@ const rtc::SocketAddress kStunMappedAddr("77.77.77.77", 0); class StunProberTest : public testing::Test { public: StunProberTest() - : main_(rtc::Thread::Current()), - pss_(new rtc::PhysicalSocketServer), + : pss_(new rtc::PhysicalSocketServer), ss_(new rtc::VirtualSocketServer(pss_.get())), - ss_scope_(ss_.get()), + main_(ss_.get()), result_(StunProber::SUCCESS), stun_server_1_(cricket::TestStunServer::Create(rtc::Thread::Current(), kStunAddr1)), @@ -120,10 +119,9 @@ class StunProberTest : public testing::Test { stopped_ = true; } - rtc::Thread* main_; std::unique_ptr pss_; std::unique_ptr ss_; - rtc::SocketServerScope ss_scope_; + rtc::AutoSocketServerThread main_; std::unique_ptr prober; int result_ = 0; bool stopped_ = false; diff --git a/webrtc/pc/webrtcsession_unittest.cc b/webrtc/pc/webrtcsession_unittest.cc index 1a5f47da25..031c08e39d 100644 --- a/webrtc/pc/webrtcsession_unittest.cc +++ b/webrtc/pc/webrtcsession_unittest.cc @@ -370,7 +370,11 @@ class WebRtcSessionTest // TODO Investigate why ChannelManager crashes, if it's created // after stun_server. WebRtcSessionTest() - : media_engine_(new cricket::FakeMediaEngine()), + : pss_(new rtc::PhysicalSocketServer), + vss_(new rtc::VirtualSocketServer(pss_.get())), + fss_(new rtc::FirewallSocketServer(vss_.get())), + thread_(fss_.get()), + media_engine_(new cricket::FakeMediaEngine()), data_engine_(new cricket::FakeDataEngine()), channel_manager_(new cricket::ChannelManager( std::unique_ptr(media_engine_), @@ -381,10 +385,6 @@ class WebRtcSessionTest desc_factory_( new cricket::MediaSessionDescriptionFactory(channel_manager_.get(), tdesc_factory_.get())), - pss_(new rtc::PhysicalSocketServer), - vss_(new rtc::VirtualSocketServer(pss_.get())), - fss_(new rtc::FirewallSocketServer(vss_.get())), - ss_scope_(fss_.get()), stun_socket_addr_( rtc::SocketAddress(kStunAddrHost, cricket::STUN_SERVER_PORT)), stun_server_(cricket::TestStunServer::Create(Thread::Current(), @@ -1503,6 +1503,10 @@ class WebRtcSessionTest } webrtc::RtcEventLogNullImpl event_log_; + std::unique_ptr pss_; + std::unique_ptr vss_; + std::unique_ptr fss_; + rtc::AutoSocketServerThread thread_; // |media_engine_| and |data_engine_| are actually owned by // |channel_manager_|. cricket::FakeMediaEngine* media_engine_; @@ -1513,10 +1517,6 @@ class WebRtcSessionTest cricket::FakeCall fake_call_; std::unique_ptr tdesc_factory_; std::unique_ptr desc_factory_; - std::unique_ptr pss_; - std::unique_ptr vss_; - std::unique_ptr fss_; - rtc::SocketServerScope ss_scope_; rtc::SocketAddress stun_socket_addr_; std::unique_ptr stun_server_; cricket::TestTurnServer turn_server_;