Delete method MessageQueue::set_socketserver

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}
Committed: https://chromium.googlesource.com/external/webrtc/+/7eaa4ea75f8d3440b735c0b8311b4c9c7997763d

[nisse-webrtc, 2017-04-26 13:46:44]

Description was changed from

==========
Convert physicalsocketserver_unittest.cc to not use SocketServerScoped.

BUG=
==========

to

==========
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
==========

[nisse-webrtc, 2017-04-26 14:25:20]

nisse@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, tommi@webrtc.org

[nisse-webrtc, 2017-04-26 14:25:22]

PTAL. If this is landed, sharedexclusivelock becomes unused.

I imagine peerconnection_client needs some manual tests before landing.

BTW, W32Thread and W32SocketServer appear to be used only in
peerconnection_client, we use something different on windows for main webrtc
sockets?

[tommi, 2017-04-27 13:57:08]

lgtm. Can we take a look at the example separately? I'm sure we can figure
something out there to avoid the dependency.

https://codereview.webrtc.org/2828223002/diff/260001/webrtc/base/thread.h
File webrtc/base/thread.h (right):

https://codereview.webrtc.org/2828223002/diff/260001/webrtc/base/thread.h#new...
webrtc/base/thread.h:319: class AutoSocketServerThread : public Thread {
if this is only for tests, can we indicate that in the name?  E.g.
AutoSocketServerThreadForTesting

https://codereview.webrtc.org/2828223002/diff/260001/webrtc/base/thread.h#new...
webrtc/base/thread.h:325: private:
super private? :)

[nisse-webrtc, 2017-04-27 14:08:56]

https://codereview.webrtc.org/2828223002/diff/260001/webrtc/base/thread.h
File webrtc/base/thread.h (right):

https://codereview.webrtc.org/2828223002/diff/260001/webrtc/base/thread.h#new...
webrtc/base/thread.h:319: class AutoSocketServerThread : public Thread {
On 2017/04/27 13:57:07, tommi (wëbrtc) wrote:
> if this is only for tests, can we indicate that in the name?  E.g.
> AutoSocketServerThreadForTesting

It's *almost* test-only ;-/

It's used by peerconnection_client on linux. While the corresponding windows
code uses rtc::ThreadManager::Instance()->SetCurrentThread() directly. Not sure
what's most appropriate for an example.

I'm also not very fond of the name. I tried for consistenct with the related
AutoThread. But ScopedSomething would be more consistent with other webrtc
classes.

https://codereview.webrtc.org/2828223002/diff/260001/webrtc/base/thread.h#new...
webrtc/base/thread.h:325: private:
On 2017/04/27 13:57:07, tommi (wëbrtc) wrote:
> super private? :)

Top-secret! And deleted.

[nisse-webrtc, 2017-04-27 14:10:36]

On 2017/04/27 13:57:08, tommi (wëbrtc) wrote:
> Can we take a look at the example separately? I'm sure we can figure
> something out there to avoid the dependency.

Can you be more explicit? I'm not sure what you're suggesting we do.

[tommi, 2017-04-27 14:22:57]

On 2017/04/27 14:10:36, nisse-webrtc wrote:
> On 2017/04/27 13:57:08, tommi (wëbrtc) wrote:
> > Can we take a look at the example separately? I'm sure we can figure
> > something out there to avoid the dependency.
> 
> Can you be more explicit? I'm not sure what you're suggesting we do.

I was referring to W32Thread and W32SocketServer.  I probably added that
dependency to peerconnection_client back in 2011 and I'm sure that there are
ways to not use those classes there if they're in the way.

[Taylor Brandstetter, 2017-04-28 02:59:34]

lgtm with some minor comments

https://codereview.webrtc.org/2828223002/diff/280001/webrtc/base/messagequeue.cc
File webrtc/base/messagequeue.cc (right):

https://codereview.webrtc.org/2828223002/diff/280001/webrtc/base/messagequeue...
webrtc/base/messagequeue.cc:149: if (!queue->IsProcessingMessages() ||
queue->empty()) {
Is the "|| queue->empty()" part still needed?

https://codereview.webrtc.org/2828223002/diff/280001/webrtc/base/physicalsock...
File webrtc/base/physicalsocketserver_unittest.cc (right):

https://codereview.webrtc.org/2828223002/diff/280001/webrtc/base/physicalsock...
webrtc/base/physicalsocketserver_unittest.cc:121:
rtc::ThreadManager::Instance()->SetCurrentThread(&thread_);
Doesn't AutoSocketServerThread already do this?

https://codereview.webrtc.org/2828223002/diff/280001/webrtc/base/thread.cc
File webrtc/base/thread.cc (right):

https://codereview.webrtc.org/2828223002/diff/280001/webrtc/base/thread.cc#ne...
webrtc/base/thread.cc:533: old_thread_ =
ThreadManager::Instance()->CurrentThread();
Does it need to do this? Or can it do "DCHECK(!CurrentThread())"? I tried this,
and all the tests I tried running passed.

https://codereview.webrtc.org/2828223002/diff/280001/webrtc/base/thread.h
File webrtc/base/thread.h (right):

https://codereview.webrtc.org/2828223002/diff/280001/webrtc/base/thread.h#new...
webrtc/base/thread.h:315: // construction uninstalls at destruction. If a Thread
object is
missing "and" between "construction and uninstalls"

https://codereview.webrtc.org/2828223002/diff/280001/webrtc/examples/peerconn...
File webrtc/examples/peerconnection/client/linux/main.cc (right):

https://codereview.webrtc.org/2828223002/diff/280001/webrtc/examples/peerconn...
webrtc/examples/peerconnection/client/linux/main.cc:89:
rtc::AutoSocketServerThread thread(&socket_server);
Do you need to call SetMessageQueue here?

https://codereview.webrtc.org/2828223002/diff/280001/webrtc/p2p/base/turnserv...
File webrtc/p2p/base/turnserver_unittest.cc (right):

https://codereview.webrtc.org/2828223002/diff/280001/webrtc/p2p/base/turnserv...
webrtc/p2p/base/turnserver_unittest.cc:45: // Since this is constructed after
|sct_|, it will pick up |threads_|'s
Replace "sct_" with "thread_"?

[nisse-webrtc, 2017-04-28 09:41:07]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 09:41:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[nisse-webrtc, 2017-04-28 09:46:52]

https://codereview.webrtc.org/2828223002/diff/280001/webrtc/base/messagequeue.cc
File webrtc/base/messagequeue.cc (right):

https://codereview.webrtc.org/2828223002/diff/280001/webrtc/base/messagequeue...
webrtc/base/messagequeue.cc:149: if (!queue->IsProcessingMessages() ||
queue->empty()) {
On 2017/04/28 02:59:34, Taylor Brandstetter wrote:
> Is the "|| queue->empty()" part still needed?

Unfortunately yes. Otherwise, rtc_unittests hangs when it gets to 

[----------] 171 tests from P2PTransportChannelTest
[ RUN      ] P2PTransportChannelTest.TestOPENToOPEN

(because some earlier test triggered autowrap to happen, and that thread object
is now on the message_queues_ lists but not processing any messages).

https://codereview.webrtc.org/2828223002/diff/280001/webrtc/base/physicalsock...
File webrtc/base/physicalsocketserver_unittest.cc (right):

https://codereview.webrtc.org/2828223002/diff/280001/webrtc/base/physicalsock...
webrtc/base/physicalsocketserver_unittest.cc:121:
rtc::ThreadManager::Instance()->SetCurrentThread(&thread_);
On 2017/04/28 02:59:34, Taylor Brandstetter wrote:
> Doesn't AutoSocketServerThread already do this?

Good catch, this is a left-older from an earlier version of the cl.

Deleted.

https://codereview.webrtc.org/2828223002/diff/280001/webrtc/base/thread.cc
File webrtc/base/thread.cc (right):

https://codereview.webrtc.org/2828223002/diff/280001/webrtc/base/thread.cc#ne...
webrtc/base/thread.cc:533: old_thread_ =
ThreadManager::Instance()->CurrentThread();
On 2017/04/28 02:59:34, Taylor Brandstetter wrote:
> Does it need to do this? Or can it do "DCHECK(!CurrentThread())"? I tried
this,
> and all the tests I tried running passed.

PhysicalSocketTest is part of the webrtc_nonparallel_tests binary. If I add a
DCHECK(old_thread == nullptr), running that test binary fails with 

[----------] 38 tests from PhysicalSocketTest
[ RUN      ] PhysicalSocketTest.TestConnectIPv4


#
# Fatal error in ../../webrtc/base/thread.cc, line 534
# last system error: 2
# Check failed: old_thread_ == nullptr
# 
#

https://codereview.webrtc.org/2828223002/diff/280001/webrtc/base/thread.h
File webrtc/base/thread.h (right):

https://codereview.webrtc.org/2828223002/diff/280001/webrtc/base/thread.h#new...
webrtc/base/thread.h:315: // construction uninstalls at destruction. If a Thread
object is
On 2017/04/28 02:59:34, Taylor Brandstetter wrote:
> missing "and" between "construction and uninstalls"

Done.

https://codereview.webrtc.org/2828223002/diff/280001/webrtc/examples/peerconn...
File webrtc/examples/peerconnection/client/linux/main.cc (right):

https://codereview.webrtc.org/2828223002/diff/280001/webrtc/examples/peerconn...
webrtc/examples/peerconnection/client/linux/main.cc:89:
rtc::AutoSocketServerThread thread(&socket_server);
On 2017/04/28 02:59:34, Taylor Brandstetter wrote:
> Do you need to call SetMessageQueue here?

I don't think so, I hope that's taken care of by the MessageQueue constructor
(super class of Thread and AutoSocketServerThread.

https://codereview.webrtc.org/2828223002/diff/280001/webrtc/p2p/base/turnserv...
File webrtc/p2p/base/turnserver_unittest.cc (right):

https://codereview.webrtc.org/2828223002/diff/280001/webrtc/p2p/base/turnserv...
webrtc/p2p/base/turnserver_unittest.cc:45: // Since this is constructed after
|sct_|, it will pick up |threads_|'s
On 2017/04/28 02:59:34, Taylor Brandstetter wrote:
> Replace "sct_" with "thread_"?

Done.

[commit-bot: I haz the power, 2017-04-28 10:25:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-28 10:25:53]

Dry run: Try jobs failed on following builders:
  linux_memcheck on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_memcheck/builds/7551)

[Taylor Brandstetter, 2017-04-28 16:59:45]

https://codereview.webrtc.org/2828223002/diff/280001/webrtc/base/thread.cc
File webrtc/base/thread.cc (right):

https://codereview.webrtc.org/2828223002/diff/280001/webrtc/base/thread.cc#ne...
webrtc/base/thread.cc:533: old_thread_ =
ThreadManager::Instance()->CurrentThread();
On 2017/04/28 09:46:52, nisse-webrtc wrote:
> On 2017/04/28 02:59:34, Taylor Brandstetter wrote:
> > Does it need to do this? Or can it do "DCHECK(!CurrentThread())"? I tried
> this,
> > and all the tests I tried running passed.
> 
> PhysicalSocketTest is part of the webrtc_nonparallel_tests binary. If I add a
> DCHECK(old_thread == nullptr), running that test binary fails with 
> 
> [----------] 38 tests from PhysicalSocketTest
> [ RUN      ] PhysicalSocketTest.TestConnectIPv4
> 
> 
> #
> # Fatal error in ../../webrtc/base/thread.cc, line 534
> # last system error: 2
> # Check failed: old_thread_ == nullptr
> # 
> #
> 

I wasn't thinking when I left this comment; I was testing with gtest-parallel
which will create a fresh process for every test. But of course, some tests
still end up auto-wrapping, so if a test run afterwards doesn't do
auto-wrapping, you run into this.

However: we may still want to call "MessageQueueManager::Remove/Add" for the old
thread, so it at least doesn't appear in the MessageQueueManager list while it's
in this limbo state.

https://codereview.webrtc.org/2828223002/diff/280001/webrtc/examples/peerconn...
File webrtc/examples/peerconnection/client/linux/main.cc (right):

https://codereview.webrtc.org/2828223002/diff/280001/webrtc/examples/peerconn...
webrtc/examples/peerconnection/client/linux/main.cc:89:
rtc::AutoSocketServerThread thread(&socket_server);
On 2017/04/28 09:46:52, nisse-webrtc wrote:
> On 2017/04/28 02:59:34, Taylor Brandstetter wrote:
> > Do you need to call SetMessageQueue here?
> 
> I don't think so, I hope that's taken care of by the MessageQueue constructor
> (super class of Thread and AutoSocketServerThread. 

Ah, I didn't notice it was a SocketServer override.

[nisse-webrtc, 2017-05-02 08:35:15]

On 2017/04/28 16:59:45, Taylor Brandstetter wrote:
> However: we may still want to call "MessageQueueManager::Remove/Add" for the
old
> thread, so it at least doesn't appear in the MessageQueueManager list while
it's
> in this limbo state.

I did this in patchset #17, and I could then undo the addition of the empty()
check.

There's also the memory leak, hopefully fixed in patchset #18. Problem was that
cricket::Connection::Destroy
posts a message to a thread that happens to be an AutoSocketServerThread, and
the connection is deleted by
the handler of that message. But when the AutoSocketServerThread is destroyed at
the end of the test, that message gets lost.

Fixed by adding a call to ProcessMessages in the AutoSocketServerThread
destructor, which I'm not sure really is the best way, but I think it solves the
problem.

Taylor, would you like to take a final look on these changes before I land?

[nisse-webrtc, 2017-05-02 08:44:21]

On 2017/05/02 08:35:15, nisse-webrtc wrote:

> There's also the memory leak, hopefully fixed in patchset #18. Problem was
that
> cricket::Connection::Destroy
> posts a message to a thread that happens to be an AutoSocketServerThread, and
> the connection is deleted by
> the handler of that message. But when the AutoSocketServerThread is destroyed
at
> the end of the test, that message gets lost.
> 
> Fixed by adding a call to ProcessMessages in the AutoSocketServerThread
> destructor, which I'm not sure really is the best way, but I think it solves
the
> problem.

Looks like that there are more leak problems :-(

[nisse-webrtc, 2017-05-02 14:37:27]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 14:37:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[nisse-webrtc, 2017-05-02 14:39:20]

Fixed one more leak. Intend to land when bots are green (but about to leave for
the day now).

[commit-bot: I haz the power, 2017-05-02 16:00:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-02 16:00:10]

Dry run: Try jobs failed on following builders:
  linux_memcheck on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_memcheck/builds/7645)

[Taylor Brandstetter, 2017-05-03 22:17:27]

https://codereview.webrtc.org/2828223002/diff/360001/webrtc/base/thread.cc
File webrtc/base/thread.cc (right):

https://codereview.webrtc.org/2828223002/diff/360001/webrtc/base/thread.cc#ne...
webrtc/base/thread.cc:545: // cricket::Connection::Destroy.
I'd consider this a bug with cricket::Connection::Destroy. It could happen if an
application closes a PeerConnection and then quickly destroys the
PeerConnectionFactory (along with the networking thread on which the message was
posted). If you agree, can you leave a TODO to fix the cricket::Connection
memory leak in a more general way?

[nisse-webrtc, 2017-05-05 07:58:36]

https://codereview.webrtc.org/2828223002/diff/360001/webrtc/base/thread.cc
File webrtc/base/thread.cc (right):

https://codereview.webrtc.org/2828223002/diff/360001/webrtc/base/thread.cc#ne...
webrtc/base/thread.cc:545: // cricket::Connection::Destroy.
On 2017/05/03 22:17:27, Taylor Brandstetter wrote:
> I'd consider this a bug with cricket::Connection::Destroy. It could happen if
an
> application closes a PeerConnection and then quickly destroys the
> PeerConnectionFactory (along with the networking thread on which the message
was
> posted). If you agree, can you leave a TODO to fix the cricket::Connection
> memory leak in a more general way?

Done.

https://codereview.webrtc.org/2828223002/diff/380001/webrtc/base/virtualsocke...
File webrtc/base/virtualsocket_unittest.cc (left):

https://codereview.webrtc.org/2828223002/diff/380001/webrtc/base/virtualsocke...
webrtc/base/virtualsocket_unittest.cc:206: void BasicTest(const SocketAddress&
initial_addr) {
I don't see how this test can ever have passed the memcheck, it's full of raw
pointers and allocations without corresponding deallocations.

Local run of 

valgrind ./out/Debug/webrtc_nonparallell_tests --gtest_filter='VirtualSocket*'

reports lots of leaks, also on the master branch. And I find no obvious
supressions either (looking in tools-webrtc/valgrind/memcheck/).

[Taylor Brandstetter, 2017-05-05 08:47:46]

https://codereview.webrtc.org/2828223002/diff/380001/webrtc/base/virtualsocke...
File webrtc/base/virtualsocket_unittest.cc (left):

https://codereview.webrtc.org/2828223002/diff/380001/webrtc/base/virtualsocke...
webrtc/base/virtualsocket_unittest.cc:206: void BasicTest(const SocketAddress&
initial_addr) {
On 2017/05/05 07:58:36, nisse-webrtc wrote:
> I don't see how this test can ever have passed the memcheck, it's full of raw
> pointers and allocations without corresponding deallocations.
> 
> Local run of 
> 
> valgrind ./out/Debug/webrtc_nonparallell_tests --gtest_filter='VirtualSocket*'
> 
> reports lots of leaks, also on the master branch. And I find no obvious
> supressions either (looking in tools-webrtc/valgrind/memcheck/).

My guess is that Thread indirectly references the sockets that are leaked, and
previously the global auto-wrap Thread would never be deleted, so the sockets
weren't detected as leaked.

But these are definitely leaks.

[nisse-webrtc, 2017-05-05 08:57:41]

On 2017/05/05 08:47:46, Taylor Brandstetter wrote:
> My guess is that Thread indirectly references the sockets that are leaked, and
> previously the global auto-wrap Thread would never be deleted, so the sockets
> weren't detected as leaked.

But I got leak reports from a local valgrind run also on the master branch.
 
> But these are definitely leaks.

I've made cl 
https://codereview.webrtc.org/2859373003/
to try to fix them.

[nisse-webrtc, 2017-05-08 11:21:37]

On 2017/05/05 08:57:41, nisse-webrtc wrote:
> On 2017/05/05 08:47:46, Taylor Brandstetter wrote:
> > My guess is that Thread indirectly references the sockets that are leaked,
and
> > previously the global auto-wrap Thread would never be deleted, so the
sockets
> > weren't detected as leaked.
> 
> But I got leak reports from a local valgrind run also on the master branch.
>  
> > But these are definitely leaks.
> 
> I've made cl 
> https://codereview.webrtc.org/2859373003/
> to try to fix them.

After landing that cl, memcheck bot seems happy with this cl too. I'm attempting
to land now.

[nisse-webrtc, 2017-05-08 11:22:06]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 11:22:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-05-08 11:44:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-08 11:45:00]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2017-05-08 12:22:54]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-05-08 12:22:55]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org,
deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2828223002/#ps400001 (title:
"Rebase.")

[commit-bot: I haz the power, 2017-05-08 12:23:04]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-05-08 12:25:42]

CQ is committing da patch.

Bot data: {"patchset_id": 400001, "attempt_start_ts": 1494246174919390,
"parent_rev": "d2690ddfff9202ee3caad5ba3a6ac125160cfcd5", "commit_rev":
"7eaa4ea75f8d3440b735c0b8311b4c9c7997763d"}

[commit-bot: I haz the power, 2017-05-08 12:25:47]

Description was changed from

==========
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
==========

to

==========
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}
Committed:
https://chromium.googlesource.com/external/webrtc/+/7eaa4ea75f8d3440b735c0b83...
==========

[commit-bot: I haz the power, 2017-05-08 12:25:48]

Committed patchset #22 (id:400001) as
https://chromium.googlesource.com/external/webrtc/+/7eaa4ea75f8d3440b735c0b83...