Move autowrap from ThreadManager constructor to Thread::Current.

Move autowrap from ThreadManager constructor to Thread::Current.

This is in preparation for a WrapCurrentThread method,
or AutoThread constructor, taking a socket
server as argument, to eliminate the need for the
MessageQueue::set_socketserver method and its lock.

No intended change in behaviour; the ThreadManager constructor 
records the calling thread id, and autowrap is still restricted to
that thread. Behavior when NO_MAIN_THREAD_WRAPPING is defined is 
also unchanged.

Also makes the ThreadManager a singleton, with private constructor
and destructor. And marks its destructor as RTC_NOTREACHED, since
by the documentation for RTC_DEFINE_STATIC_LOCAL, the intention is to
leak the object and never destruct it.

BUG=webrtc:7501
Review-Url: https://codereview.webrtc.org/2833993003
Cr-Commit-Position: refs/heads/master@{#17879}
Committed: https://chromium.googlesource.com/external/webrtc/+/7866cfe575e85978dba50b1c4a1877a325601fe1

[nisse-webrtc, 2017-04-21 07:56:56]

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

[nisse-webrtc, 2017-04-21 07:56:57]

Hi, please take a look. Taylor suggested I try out this approach, as an
alternative to my (ab)use of SetCurrentThread in
https://codereview.webrtc.org/2828223002/

It would maybe be better to get rid of autowrap altogether. How large changes
would that need? It's already disabled when building webrtc for Chrome, which
has caused me some confusion (see 
https://codereview.chromium.org/2803023002/).

[tommi, 2017-04-21 09:38:53]

On 2017/04/21 07:56:57, nisse-webrtc wrote:
> Hi, please take a look. Taylor suggested I try out this approach, as an
> alternative to my (ab)use of SetCurrentThread in
> https://codereview.webrtc.org/2828223002/
> 
> It would maybe be better to get rid of autowrap altogether. How large changes
> would that need? It's already disabled when building webrtc for Chrome, which
> has caused me some confusion (see 
>
https://codereview.chromium.org/2803023002/?_ga=1.65116971.403986946.1438769549).

Assuming you mean AutoThread, it doesn't look like it's used much, but I don't
know about the side effects that removing it might have. I think you have more
context :)

[tommi, 2017-04-21 09:38:58]

lgtm

https://codereview.webrtc.org/2833993003/diff/1/webrtc/base/thread.cc
File webrtc/base/thread.cc (right):

https://codereview.webrtc.org/2833993003/diff/1/webrtc/base/thread.cc#newcode30
webrtc/base/thread.cc:30: RTC_DEFINE_STATIC_LOCAL(ThreadManager, thread_manager,
());
I hadn't seen this macro before. It doesn't say "LEAKY" in the name, although
the documentation for it says:

  "define a static local variable that gets leaked"

I think it would be an improvement to name it RTC_DEFINE_LEAKY_STATIC_LOCAL.

Then, to make things further more confusing (and arguably misleading), I see it
is used like this in one place:

RTC_DEFINE_STATIC_LOCAL(std::unique_ptr<RandomGenerator>, global_rng,
   (new SecureRandomGenerator()));

:-/

[Taylor Brandstetter, 2017-04-21 09:54:28]

lgtm with some minor comments.

Before this change if "Thread::Current()->Post" (for example) is called on a
non-wrapped, non-main thread, it would crash. With this change, it will
automatically create a new Thread object. So, if we want to keep getting the
"hard failure" behavior, we could add a DCHECK that's triggered when more than 1
thread needed wrapping. Just a thought.

https://codereview.webrtc.org/2833993003/diff/1/webrtc/base/thread.cc
File webrtc/base/thread.cc (right):

https://codereview.webrtc.org/2833993003/diff/1/webrtc/base/thread.cc#newcode36
webrtc/base/thread.cc:36: RTC_NOTREACHED() << "ThreadManager should never be
destructed.";
But it has a public constructor/destructor, so a native application could
construct it explicitly as opposed to using "Instance()". If we expect everyone
to go through "Instance", we should go a step further and make the constructor
private. Otherwise, we should remove the RTC_NOTREACHED(), and unwrap threads on
destruction.

[nisse-webrtc, 2017-04-21 11:05:43]

On 2017/04/21 09:38:53, tommi (wëbrtc) wrote:
> On 2017/04/21 07:56:57, nisse-webrtc wrote:
> > Hi, please take a look. Taylor suggested I try out this approach, as an
> > alternative to my (ab)use of SetCurrentThread in
> > https://codereview.webrtc.org/2828223002/
> > 
> > It would maybe be better to get rid of autowrap altogether. How large
changes
> > would that need? It's already disabled when building webrtc for Chrome,
which
> > has caused me some confusion (see 
> >
>
https://codereview.chromium.org/2803023002/?_ga=1.65116971.403986946.1438769549).
> 
> Assuming you mean AutoThread, it doesn't look like it's used much, but I don't
> know about the side effects that removing it might have. I think you have more
> context :)

I don't know what AutoThread is. By "autowrap", I was referring to the 

#ifndef NO_MAIN_THREAD_WRAPPING
  WrapCurrentThread();
#endif

in ThreadManager's constructor, which automatically creates a Thread object
representing the calling thread.

[nisse-webrtc, 2017-04-21 11:06:07]

https://codereview.webrtc.org/2833993003/diff/1/webrtc/base/thread.cc
File webrtc/base/thread.cc (right):

https://codereview.webrtc.org/2833993003/diff/1/webrtc/base/thread.cc#newcode30
webrtc/base/thread.cc:30: RTC_DEFINE_STATIC_LOCAL(ThreadManager, thread_manager,
());
On 2017/04/21 09:38:58, tommi (wëbrtc) wrote:
> I hadn't seen this macro before. It doesn't say "LEAKY" in the name, although
> the documentation for it says:
> 
>   "define a static local variable that gets leaked"
> 
> I think it would be an improvement to name it RTC_DEFINE_LEAKY_STATIC_LOCAL.
> 
> Then, to make things further more confusing (and arguably misleading), I see
it
> is used like this in one place:
> 
> RTC_DEFINE_STATIC_LOCAL(std::unique_ptr<RandomGenerator>, global_rng,
>    (new SecureRandomGenerator()));
> 
> :-/

I think I'd prefer to delete this macro, I don't think it improves clarity. But
that's for some other cl.

https://codereview.webrtc.org/2833993003/diff/1/webrtc/base/thread.cc#newcode36
webrtc/base/thread.cc:36: RTC_NOTREACHED() << "ThreadManager should never be
destructed.";
On 2017/04/21 09:54:28, Taylor Brandstetter wrote:
> But it has a public constructor/destructor, so a native application could
> construct it explicitly as opposed to using "Instance()". If we expect
everyone
> to go through "Instance", we should go a step further and make the constructor
> private. Otherwise, we should remove the RTC_NOTREACHED(), and unwrap threads
on
> destruction.

I'd prefer to not add the bookkeeping needed to unwrap threads. I could try
making constructor and destructor private. Do we have any other conventions on
how to do singletons (fancy name for global variable) like this?

BTW, according to
http://stackoverflow.com/questions/246564/what-is-the-lifetime-of-a-static-va...,
initialization of static locals like this are thread safe at least from C++11.
Which was news to me.

[Taylor Brandstetter, 2017-04-21 11:32:54]

https://codereview.webrtc.org/2833993003/diff/1/webrtc/base/thread.cc
File webrtc/base/thread.cc (right):

https://codereview.webrtc.org/2833993003/diff/1/webrtc/base/thread.cc#newcode36
webrtc/base/thread.cc:36: RTC_NOTREACHED() << "ThreadManager should never be
destructed.";
On 2017/04/21 11:06:07, nisse-webrtc wrote:
> On 2017/04/21 09:54:28, Taylor Brandstetter wrote:
> > But it has a public constructor/destructor, so a native application could
> > construct it explicitly as opposed to using "Instance()". If we expect
> everyone
> > to go through "Instance", we should go a step further and make the
constructor
> > private. Otherwise, we should remove the RTC_NOTREACHED(), and unwrap
threads
> on
> > destruction.
> 
> I'd prefer to not add the bookkeeping needed to unwrap threads. I could try
> making constructor and destructor private. Do we have any other conventions on
> how to do singletons (fancy name for global variable) like this?
> 
> BTW, according to
>
http://stackoverflow.com/questions/246564/what-is-the-lifetime-of-a-static-va...,
> initialization of static locals like this are thread safe at least from C++11.
> Which was news to me.

Let's try making the constructor/destructor private and see if anyone notices. I
couldn't find any code that uses them.

I'm unaware of any singleton conventions we have, though just keeping
"Instance()" for now seems fine.

And yeah, static locals are pretty convenient. I don't think I've encountered
any problems yet with this approach to a singleton.

[tommi, 2017-04-21 11:36:48]

On 2017/04/21 11:32:54, Taylor Brandstetter wrote:
> https://codereview.webrtc.org/2833993003/diff/1/webrtc/base/thread.cc
> File webrtc/base/thread.cc (right):
> 
>
https://codereview.webrtc.org/2833993003/diff/1/webrtc/base/thread.cc#newcode36
> webrtc/base/thread.cc:36: RTC_NOTREACHED() << "ThreadManager should never be
> destructed.";
> On 2017/04/21 11:06:07, nisse-webrtc wrote:
> > On 2017/04/21 09:54:28, Taylor Brandstetter wrote:
> > > But it has a public constructor/destructor, so a native application could
> > > construct it explicitly as opposed to using "Instance()". If we expect
> > everyone
> > > to go through "Instance", we should go a step further and make the
> constructor
> > > private. Otherwise, we should remove the RTC_NOTREACHED(), and unwrap
> threads
> > on
> > > destruction.
> > 
> > I'd prefer to not add the bookkeeping needed to unwrap threads. I could try
> > making constructor and destructor private. Do we have any other conventions
on
> > how to do singletons (fancy name for global variable) like this?
> > 
> > BTW, according to
> >
>
http://stackoverflow.com/questions/246564/what-is-the-lifetime-of-a-static-va...,
> > initialization of static locals like this are thread safe at least from
C++11.
> > Which was news to me.
> 
> Let's try making the constructor/destructor private and see if anyone notices.
I
> couldn't find any code that uses them.
> 
> I'm unaware of any singleton conventions we have, though just keeping
> "Instance()" for now seems fine.
> 
> And yeah, static locals are pretty convenient. I don't think I've encountered
> any problems yet with this approach to a singleton.

just an fyi on C++ 11, static variables and our tool chains; The standard does
specify that statics are thread safe but for a long while, the Windows tool
chain (possibly elsewhere) didn't support that.  So we didn't use that feature
until all build tools had been updated - and now they have.

[nisse-webrtc, 2017-04-21 12:49:34]

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

[commit-bot: I haz the power, 2017-04-21 12:49:46]

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-04-21 13:39:00]

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

[commit-bot: I haz the power, 2017-04-21 13:39:01]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2017-04-21 14:13:05]

It seems peerconnection_unittest hangs (when running locally, and on the
internal bit). It seems it leaks thread objects. I added debug logging to 

MessageQueueManager::AddInternal
MessageQueueManager::RemoveInternal

If I run 

$ ./out/Debug/peerconnection_unittests

first call is from SctpDataChannelTest.LateCreatedChannelTransitionToOpen,

[000:001] (messagequeue.cc:76): XXX Adding 0x1deacf0 size 0

This is never removed. Then a couple of tests later, in
PeerConnectionIntegrationTest.RtpReceiverObserverOnFirstPacketReceived,

[000:056] (messagequeue.cc:76): XXX Adding 0x1dec560 size 1
[000:056] (messagequeue.cc:76): XXX Adding 0x1decaf0 size 2
[000:056] (messagequeue.cc:76): XXX Adding 0x7fffffffc758 size 3

A total of 187 add and 182 remove for the duratino fo this test.

It finally hangs when it gets to
PeerConnectionIntegrationTest.DataBufferedUntilRtpDataChannelObserverRegistered

At this point, MessageQueueManager::message_queues_ has grown to 209 elements,
and the test hangs in FakeClock::AdvanceTime, which results in a loop repeatedly
calling

select(4, [3], [], NULL, {0, 0})        = 0 (Timeout)

Inside MessageQueueManager::ProcessAllMessageQueuesInternal, the variable
queues_not_done is stuck at 206, so I guess that's the count of leaked thread
objects which aren't responding to any messages.

Further debugging has to wait for next week.

[nisse-webrtc, 2017-04-24 08:36:58]

On 2017/04/21 14:13:05, nisse-webrtc wrote:
> It seems peerconnection_unittest hangs (when running locally, and on the
> internal bit). It seems it leaks thread objects. I added debug logging to 
> 
> MessageQueueManager::AddInternal
> MessageQueueManager::RemoveInternal
> 
> If I run 
> 
> $ ./out/Debug/peerconnection_unittests
> 
> first call is from SctpDataChannelTest.LateCreatedChannelTransitionToOpen,
> 
> [000:001] (messagequeue.cc:76): XXX Adding 0x1deacf0 size 0
> 
> This is never removed. Then a couple of tests later, in
> PeerConnectionIntegrationTest.RtpReceiverObserverOnFirstPacketReceived,
> 
> [000:056] (messagequeue.cc:76): XXX Adding 0x1dec560 size 1
> [000:056] (messagequeue.cc:76): XXX Adding 0x1decaf0 size 2
> [000:056] (messagequeue.cc:76): XXX Adding 0x7fffffffc758 size 3
> 
> A total of 187 add and 182 remove for the duratino fo this test.
> 
> It finally hangs when it gets to
>
PeerConnectionIntegrationTest.DataBufferedUntilRtpDataChannelObserverRegistered
> 
> At this point, MessageQueueManager::message_queues_ has grown to 209 elements,
> and the test hangs in FakeClock::AdvanceTime, which results in a loop
repeatedly
> calling
> 
> select(4, [3], [], NULL, {0, 0})        = 0 (Timeout)
> 
> Inside MessageQueueManager::ProcessAllMessageQueuesInternal, the variable
> queues_not_done is stuck at 206, so I guess that's the count of leaked thread
> objects which aren't responding to any messages.
> 
> Further debugging has to wait for next week.

I think I've identified a problem. Thread::IsCurrent() calls Thread::Current(),
which with this cl, would create a new Thread object wrapping the current
thread.

Particular case: I run 

./out/Debug/peerconnection_unittests
--gtest_filter=PeerConnectionIntegrationTest.RtpReceiverObserverOnFirstPacketReceived

Then the Pacer thread (which is a ProcessThread, not using rtc::Thread) calls
(through a long chain) BaseChannel::IsCurrent(). Which creates a new Thread
object, which (1) is leaked, and (2) is added to
MessageQueueManager::message_queues_, making tests hang.

Complete backtrace for this case below.

I wonder what to do about it, I'll try rewrite IsCurrent to bypass any autowrap.
But I feel this is still a bit brittle, it would be better to delete autowrap
completely, or else, somehow make sure that autowrap will only ever be applied
to the main thread. 

For wrapping current thread with a particular socket server, it seems one could
add a constructor with a socket-server argument to the AutoThread class; then we
wouldn't need any new ScopedWrapCurrentThreadWithSocketServer thing.

#0  rtc::MessageQueueManager::AddInternal (this=0x1ddf5f0, 
    message_queue=0x7fffe00011e0) at ../../webrtc/base/messagequeue.cc:78
#1  0x0000000000ba09dd in rtc::MessageQueueManager::Add (
    message_queue=0x7fffe00011e0) at ../../webrtc/base/messagequeue.cc:72
#2  0x0000000000ba1559 in rtc::MessageQueue::DoInit (this=0x7fffe00011e0)
    at ../../webrtc/base/messagequeue.cc:207
#3  0x0000000000bde122 in rtc::Thread::Thread (this=0x7fffe00011e0, ss=...)
    at ../../webrtc/base/thread.cc:135
#4  0x0000000000bddffd in rtc::Thread::Thread (this=0x7fffe00011e0)
    at ../../webrtc/base/thread.cc:110
#5  0x0000000000bdda73 in rtc::ThreadManager::WrapCurrentThread (this=0x1de52d0)
    at ../../webrtc/base/thread.cc:86
#6  0x0000000000bdda21 in rtc::Thread::Current ()
    at ../../webrtc/base/thread.cc:44
#7  0x000000000053ff19 in rtc::Thread::IsCurrent (this=0x1de7360)
    at ../../webrtc/base/thread.h:128
#8  0x0000000000afa886 in cricket::BaseChannel::SendPacket (this=0x7ffff02df0a0,

    rtcp=false, packet=0x7ffff47b9ec8, options=...)
    at ../../webrtc/pc/channel.cc:706
#9  0x0000000000afa839 in cricket::BaseChannel::SendPacket (this=0x7ffff02df0a0,

    packet=0x7ffff47b9ec8, options=...) at ../../webrtc/pc/channel.cc:559
#10 0x0000000000708a14 in cricket::MediaChannel::DoSendPacket (
    this=0x7ffff02deb30, packet=0x7ffff47b9ec8, rtcp=false, options=...)
    at ../../webrtc/media/base/mediachannel.h:493
#11 0x0000000000708969 in cricket::MediaChannel::SendPacket
(this=0x7ffff02deb30, 
    packet=0x7ffff47b9ec8, options=...)
    at ../../webrtc/media/base/mediachannel.h:453
#12 0x0000000000d60f11 in cricket::WebRtcVoiceMediaChannel::SendRtp (
    this=0x7ffff02deb30, 
    data=0x7fffa4000a80 "\220\357?\254\372\224;\024\037뽏\276", <incomplete
sequence \336>, len=92, options=...)
    at ../../webrtc/media/engine/webrtcvoiceengine.h:223
#13 0x00000000011ad537 in webrtc::voe::Channel::SendRtp (this=0x7ffff034dfa0, 
    data=0x7fffa4000a80 "\220\357?\254\372\224;\024\037뽏\276", <incomplete
sequence \336>, len=92, options=...) at ../../webrtc/voice_engine/channel.cc:488
#14 0x00000000010ea7cb in webrtc::RTPSender::SendPacketToNetwork (
    this=0x7ffff03647f0, packet=..., options=..., pacing_info=...)
    at ../../webrtc/modules/rtp_rtcp/source/rtp_sender.cc:620
#15 0x00000000010e9a5b in webrtc::RTPSender::PrepareAndSendPacket (
    this=0x7ffff03647f0, 
    packet=std::unique_ptr<webrtc::RtpPacketToSend> containing 0x7fffe00008e0, 
    send_over_rtx=false, is_retransmit=false, pacing_info=...)
    at ../../webrtc/modules/rtp_rtcp/source/rtp_sender.cc:746
#16 0x00000000010eb91a in webrtc::RTPSender::TimeToSendPacket (
    this=0x7ffff03647f0, ssrc=535543183, sequence_number=16300, 
    capture_time_ms=5765036925, retransmission=false, pacing_info=...)
    at ../../webrtc/modules/rtp_rtcp/source/rtp_sender.cc:697
#17 0x00000000010e36a0 in webrtc::ModuleRtpRtcpImpl::TimeToSendPacket (
    this=0x7ffff0363cd0, ssrc=535543183, sequence_number=16300, 
    capture_time_ms=5765036925, retransmission=false, pacing_info=...)
    at ../../webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:410
#18 0x000000000119591d in webrtc::PacketRouter::TimeToSendPacket (
    this=0x7ffff02ab238, ssrc=535543183, sequence_number=16300, 
    capture_timestamp=5765036925, retransmission=false, pacing_info=...)
    at ../../webrtc/modules/pacing/packet_router.cc:107
#19 0x000000000118b4e2 in webrtc::PacedSender::SendPacket (this=0x7ffff02d2d40, 
    packet=..., pacing_info=...)
    at ../../webrtc/modules/pacing/paced_sender.cc:499
#20 0x000000000118b1a3 in webrtc::PacedSender::Process (this=0x7ffff02d2d40)
    at ../../webrtc/modules/pacing/paced_sender.cc:449
#21 0x0000000000ffc5b8 in webrtc::ProcessThreadImpl::Process
(this=0x7ffff02d8d00)
    at ../../webrtc/modules/utility/source/process_thread_impl.cc:192
#22 0x0000000000ffb815 in webrtc::ProcessThreadImpl::Run (obj=0x7ffff02d8d00)
    at ../../webrtc/modules/utility/source/process_thread_impl.cc:166
#23 0x0000000000b7faab in rtc::PlatformThread::Run (this=0x7ffff02de920)
    at ../../webrtc/base/platform_thread.cc:244
#24 0x0000000000b7f865 in rtc::PlatformThread::StartThread
(param=0x7ffff02de920)
    at ../../webrtc/base/platform_thread.cc:140
#25 0x00007ffff7bc4184 in start_thread (arg=0x7ffff47bd700)
    at pthread_create.c:312
#26 0x00007ffff6990bed in clone ()
    at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111

[tommi, 2017-04-24 09:55:05]

On 2017/04/24 08:36:58, nisse-webrtc wrote:
> On 2017/04/21 14:13:05, nisse-webrtc wrote:
> > It seems peerconnection_unittest hangs (when running locally, and on the
> > internal bit). It seems it leaks thread objects. I added debug logging to 
> > 
> > MessageQueueManager::AddInternal
> > MessageQueueManager::RemoveInternal
> > 
> > If I run 
> > 
> > $ ./out/Debug/peerconnection_unittests
> > 
> > first call is from SctpDataChannelTest.LateCreatedChannelTransitionToOpen,
> > 
> > [000:001] (messagequeue.cc:76): XXX Adding 0x1deacf0 size 0
> > 
> > This is never removed. Then a couple of tests later, in
> > PeerConnectionIntegrationTest.RtpReceiverObserverOnFirstPacketReceived,
> > 
> > [000:056] (messagequeue.cc:76): XXX Adding 0x1dec560 size 1
> > [000:056] (messagequeue.cc:76): XXX Adding 0x1decaf0 size 2
> > [000:056] (messagequeue.cc:76): XXX Adding 0x7fffffffc758 size 3
> > 
> > A total of 187 add and 182 remove for the duratino fo this test.
> > 
> > It finally hangs when it gets to
> >
>
PeerConnectionIntegrationTest.DataBufferedUntilRtpDataChannelObserverRegistered
> > 
> > At this point, MessageQueueManager::message_queues_ has grown to 209
elements,
> > and the test hangs in FakeClock::AdvanceTime, which results in a loop
> repeatedly
> > calling
> > 
> > select(4, [3], [], NULL, {0, 0})        = 0 (Timeout)
> > 
> > Inside MessageQueueManager::ProcessAllMessageQueuesInternal, the variable
> > queues_not_done is stuck at 206, so I guess that's the count of leaked
thread
> > objects which aren't responding to any messages.
> > 
> > Further debugging has to wait for next week.
> 
> I think I've identified a problem. Thread::IsCurrent() calls
Thread::Current(),
> which with this cl, would create a new Thread object wrapping the current
> thread.
> 
> Particular case: I run 
> 
> ./out/Debug/peerconnection_unittests
>
--gtest_filter=PeerConnectionIntegrationTest.RtpReceiverObserverOnFirstPacketReceived
> 
> Then the Pacer thread (which is a ProcessThread, not using rtc::Thread) calls
> (through a long chain) BaseChannel::IsCurrent(). Which creates a new Thread
> object, which (1) is leaked, and (2) is added to
> MessageQueueManager::message_queues_, making tests hang.
> 
> Complete backtrace for this case below.
> 
> I wonder what to do about it, I'll try rewrite IsCurrent to bypass any
autowrap.
> But I feel this is still a bit brittle, it would be better to delete autowrap
> completely, or else, somehow make sure that autowrap will only ever be applied
> to the main thread. 
> 
> For wrapping current thread with a particular socket server, it seems one
could
> add a constructor with a socket-server argument to the AutoThread class; then
we
> wouldn't need any new ScopedWrapCurrentThreadWithSocketServer thing.
> 
> #0  rtc::MessageQueueManager::AddInternal (this=0x1ddf5f0, 
>     message_queue=0x7fffe00011e0) at ../../webrtc/base/messagequeue.cc:78
> #1  0x0000000000ba09dd in rtc::MessageQueueManager::Add (
>     message_queue=0x7fffe00011e0) at ../../webrtc/base/messagequeue.cc:72
> #2  0x0000000000ba1559 in rtc::MessageQueue::DoInit (this=0x7fffe00011e0)
>     at ../../webrtc/base/messagequeue.cc:207
> #3  0x0000000000bde122 in rtc::Thread::Thread (this=0x7fffe00011e0, ss=...)
>     at ../../webrtc/base/thread.cc:135
> #4  0x0000000000bddffd in rtc::Thread::Thread (this=0x7fffe00011e0)
>     at ../../webrtc/base/thread.cc:110
> #5  0x0000000000bdda73 in rtc::ThreadManager::WrapCurrentThread
(this=0x1de52d0)
>     at ../../webrtc/base/thread.cc:86
> #6  0x0000000000bdda21 in rtc::Thread::Current ()
>     at ../../webrtc/base/thread.cc:44
> #7  0x000000000053ff19 in rtc::Thread::IsCurrent (this=0x1de7360)
>     at ../../webrtc/base/thread.h:128
> #8  0x0000000000afa886 in cricket::BaseChannel::SendPacket
(this=0x7ffff02df0a0,
> 
>     rtcp=false, packet=0x7ffff47b9ec8, options=...)
>     at ../../webrtc/pc/channel.cc:706
> #9  0x0000000000afa839 in cricket::BaseChannel::SendPacket
(this=0x7ffff02df0a0,
> 
>     packet=0x7ffff47b9ec8, options=...) at ../../webrtc/pc/channel.cc:559
> #10 0x0000000000708a14 in cricket::MediaChannel::DoSendPacket (
>     this=0x7ffff02deb30, packet=0x7ffff47b9ec8, rtcp=false, options=...)
>     at ../../webrtc/media/base/mediachannel.h:493
> #11 0x0000000000708969 in cricket::MediaChannel::SendPacket
> (this=0x7ffff02deb30, 
>     packet=0x7ffff47b9ec8, options=...)
>     at ../../webrtc/media/base/mediachannel.h:453
> #12 0x0000000000d60f11 in cricket::WebRtcVoiceMediaChannel::SendRtp (
>     this=0x7ffff02deb30, 
>     data=0x7fffa4000a80 "\220\357?\254\372\224;\024\037뽏\276", <incomplete
> sequence \336>, len=92, options=...)
>     at ../../webrtc/media/engine/webrtcvoiceengine.h:223
> #13 0x00000000011ad537 in webrtc::voe::Channel::SendRtp (this=0x7ffff034dfa0, 
>     data=0x7fffa4000a80 "\220\357?\254\372\224;\024\037뽏\276", <incomplete
> sequence \336>, len=92, options=...) at
../../webrtc/voice_engine/channel.cc:488
> #14 0x00000000010ea7cb in webrtc::RTPSender::SendPacketToNetwork (
>     this=0x7ffff03647f0, packet=..., options=..., pacing_info=...)
>     at ../../webrtc/modules/rtp_rtcp/source/rtp_sender.cc:620
> #15 0x00000000010e9a5b in webrtc::RTPSender::PrepareAndSendPacket (
>     this=0x7ffff03647f0, 
>     packet=std::unique_ptr<webrtc::RtpPacketToSend> containing 0x7fffe00008e0,

>     send_over_rtx=false, is_retransmit=false, pacing_info=...)
>     at ../../webrtc/modules/rtp_rtcp/source/rtp_sender.cc:746
> #16 0x00000000010eb91a in webrtc::RTPSender::TimeToSendPacket (
>     this=0x7ffff03647f0, ssrc=535543183, sequence_number=16300, 
>     capture_time_ms=5765036925, retransmission=false, pacing_info=...)
>     at ../../webrtc/modules/rtp_rtcp/source/rtp_sender.cc:697
> #17 0x00000000010e36a0 in webrtc::ModuleRtpRtcpImpl::TimeToSendPacket (
>     this=0x7ffff0363cd0, ssrc=535543183, sequence_number=16300, 
>     capture_time_ms=5765036925, retransmission=false, pacing_info=...)
>     at ../../webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:410
> #18 0x000000000119591d in webrtc::PacketRouter::TimeToSendPacket (
>     this=0x7ffff02ab238, ssrc=535543183, sequence_number=16300, 
>     capture_timestamp=5765036925, retransmission=false, pacing_info=...)
>     at ../../webrtc/modules/pacing/packet_router.cc:107
> #19 0x000000000118b4e2 in webrtc::PacedSender::SendPacket
(this=0x7ffff02d2d40, 
>     packet=..., pacing_info=...)
>     at ../../webrtc/modules/pacing/paced_sender.cc:499
> #20 0x000000000118b1a3 in webrtc::PacedSender::Process (this=0x7ffff02d2d40)
>     at ../../webrtc/modules/pacing/paced_sender.cc:449
> #21 0x0000000000ffc5b8 in webrtc::ProcessThreadImpl::Process
> (this=0x7ffff02d8d00)
>     at ../../webrtc/modules/utility/source/process_thread_impl.cc:192
> #22 0x0000000000ffb815 in webrtc::ProcessThreadImpl::Run (obj=0x7ffff02d8d00)
>     at ../../webrtc/modules/utility/source/process_thread_impl.cc:166
> #23 0x0000000000b7faab in rtc::PlatformThread::Run (this=0x7ffff02de920)
>     at ../../webrtc/base/platform_thread.cc:244
> #24 0x0000000000b7f865 in rtc::PlatformThread::StartThread
> (param=0x7ffff02de920)
>     at ../../webrtc/base/platform_thread.cc:140
> #25 0x00007ffff7bc4184 in start_thread (arg=0x7ffff47bd700)
>     at pthread_create.c:312
> #26 0x00007ffff6990bed in clone ()
>     at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111

Can we add a DCHECK that we never wrap an rtc::PlatformThread?

[nisse-webrtc, 2017-04-24 10:53:09]

On 2017/04/24 09:55:05, tommi (wëbrtc) wrote:

> Can we add a DCHECK that we never wrap an rtc::PlatformThread?

I don't see any easy way to find out if the current thread is a PlatformThread
or not. I suspect
we don't want to add a new key for pthread_setspecific just for that?

One possible trick might be to record the current thread (as an OS thread id, or
some kind of "dormant" Thread object, in the ThreadManager constructor, and only
do autowrap in Thread::Current if the calling thread matches. Not very pretty,
but that should at least preserve the old behaviour.

[Taylor Brandstetter, 2017-04-24 21:46:08]

How about just doing something to ensure only the "main thread" gets wrapped,
which was the behavior before this CL? Where "main thread" for us means "the
first one that caused the ThreadManager singleton to be constructed".

[nisse-webrtc, 2017-04-25 07:51:17]

On 2017/04/24 21:46:08, Taylor Brandstetter wrote:
> How about just doing something to ensure only the "main thread" gets wrapped,
> which was the behavior before this CL? Where "main thread" for us means "the
> first one that caused the ThreadManager singleton to be constructed".

I've given it a try. I had to add an instance variable to ThreadManager to
record the thread id which created it, and an IsMainThread function.

BTW, I've also tested deleting autowrap. I tried adding AutoThread where needed
to make tests pass, but paused that for now after updating a dozen files which
are part of rtc_unittests. One could add AutoThread the the tests' main
function, but that defeats the purpose, since the goal is to let individual
tests wrap the current thread using a socket server of their own choice.

[Taylor Brandstetter, 2017-04-25 18:44:23]

lgtm. I'd suggest making it clear in the CL description that this CL makes
ThreadManager a singleton. May also want to run with the linux_internal trybot
and a chromium trybot to make sure nothing downstream (that we're aware of)
breaks.

[nisse-webrtc, 2017-04-26 07:16:55]

Description was changed from

==========
Move autowrap from ThreadManager constructor to Thread::Current.

This is in preparation for a WrapCurrentThread method taking a socket
server as argument, eliminating the need for the
MessageQueue::set_socketserver method and its lock.

Before this change, autowrap typically applies only to the first
thread calling Thread::Current, which constructs the thread manager as
a static local variable of ThreadManager::Instance. After this change,
all calls to Thread::Current will check for a missing current thread
object and wrap the calling thread if needed.

Behavior when NO_MAIN_THREAD_WRAPPING is defined is unchanged.

Also marks the ThreadManager destructor as RTC_NOTREACHED, since
by the documentation for RTC_DEFINE_STATIC_LOCAL, the intention is to
leak the object and never destruct it.

BUG=webrtc:7501
==========

to

==========
Move autowrap from ThreadManager constructor to Thread::Current.

This is in preparation for a WrapCurrentThread method,
or AutoThread constructor, taking a socket
server as argument, to eliminate the need for the
MessageQueue::set_socketserver method and its lock.

No intended change in behaviour; the ThreadManager constructor 
records the calling thread id, and autowrap is still restricted to
that thread. Behavior when NO_MAIN_THREAD_WRAPPING is defined is 
also unchanged.

Also makes the ThreadManager a singleton, with private constructor
and destructor. And marks its destructor as RTC_NOTREACHED, since
by the documentation for RTC_DEFINE_STATIC_LOCAL, the intention is to
leak the object and never destruct it.

BUG=webrtc:7501
==========

[nisse-webrtc, 2017-04-26 07:17:02]

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

[commit-bot: I haz the power, 2017-04-26 07:17:13]

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-26 07:18:32]

On 2017/04/25 18:44:23, Taylor Brandstetter wrote:
> lgtm. I'd suggest making it clear in the CL description that this CL makes
> ThreadManager a singleton. May also want to run with the linux_internal trybot
> and a chromium trybot to make sure nothing downstream (that we're aware of)
> breaks.

I've updated the description and started a bot run. Intend to land if all bots
are green.

[commit-bot: I haz the power, 2017-04-26 07:35:02]

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

[commit-bot: I haz the power, 2017-04-26 07:35:03]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2017-04-26 08:30:24]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-04-26 08:30:25]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2833993003/#ps80001 (title:
"Limit autowrap to main thread only. New method ThreadManager::IsMainThread.")

[commit-bot: I haz the power, 2017-04-26 08:30:29]

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-04-26 08:45:32]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493195424947620,
"parent_rev": "20a60853a67193b7fe2ae284ae606958dbf22206", "commit_rev":
"7866cfe575e85978dba50b1c4a1877a325601fe1"}

[commit-bot: I haz the power, 2017-04-26 08:45:35]

Description was changed from

==========
Move autowrap from ThreadManager constructor to Thread::Current.

This is in preparation for a WrapCurrentThread method,
or AutoThread constructor, taking a socket
server as argument, to eliminate the need for the
MessageQueue::set_socketserver method and its lock.

No intended change in behaviour; the ThreadManager constructor 
records the calling thread id, and autowrap is still restricted to
that thread. Behavior when NO_MAIN_THREAD_WRAPPING is defined is 
also unchanged.

Also makes the ThreadManager a singleton, with private constructor
and destructor. And marks its destructor as RTC_NOTREACHED, since
by the documentation for RTC_DEFINE_STATIC_LOCAL, the intention is to
leak the object and never destruct it.

BUG=webrtc:7501
==========

to

==========
Move autowrap from ThreadManager constructor to Thread::Current.

This is in preparation for a WrapCurrentThread method,
or AutoThread constructor, taking a socket
server as argument, to eliminate the need for the
MessageQueue::set_socketserver method and its lock.

No intended change in behaviour; the ThreadManager constructor 
records the calling thread id, and autowrap is still restricted to
that thread. Behavior when NO_MAIN_THREAD_WRAPPING is defined is 
also unchanged.

Also makes the ThreadManager a singleton, with private constructor
and destructor. And marks its destructor as RTC_NOTREACHED, since
by the documentation for RTC_DEFINE_STATIC_LOCAL, the intention is to
leak the object and never destruct it.

BUG=webrtc:7501

Review-Url: https://codereview.webrtc.org/2833993003
Cr-Commit-Position: refs/heads/master@{#17879}
Committed:
https://chromium.googlesource.com/external/webrtc/+/7866cfe575e85978dba50b1c4...
==========

[commit-bot: I haz the power, 2017-04-26 08:45:36]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/7866cfe575e85978dba50b1c4...