ASSERT(!crit_.CurrentThreadIsOwner()) was racy due to use of a rtc::IsThreadRefEqual which cannot compare the thread handlers without a lock unless one is already sure it is the thread owning the crit.
Review-Url: https://codereview.webrtc.org/2131503002
Cr-Commit-Position: refs/heads/master@{#13411}
Every message will now be traced with the location from which it was
posted, including function name, file and line number.
This CL also writes a normal LOG message when the dispatch took more
than a certain amount of time (currently 50ms).
This logging should help us identify messages that are taking
longer than expected to be dispatched.
R=pthatcher@webrtc.org, tommi@webrtc.org
Review URL: https://codereview.webrtc.org/2019423006 .
Cr-Commit-Position: refs/heads/master@{#13104}
When the fake clock's time is advanced, it now ensures all pending
queued messages have been dispatched. This allows us to write a
"SIMULATED_WAIT" macro that ticks the simulated clock by milliseconds up
until the target time.
Useful in this case, where we know the STUN timeout should take a total
of 9500ms, but it would be overly complex to write test code that waits
for each individual timeout, ensures a STUN packet has been
retransmited, etc.
(The test described above *should* be written, but it belongs in
p2ptransportchannel_unittest.cc, not webrtcsession_unittest.cc).
Review-Url: https://codereview.webrtc.org/2024813004
Cr-Commit-Position: refs/heads/master@{#13052}
Reason for revert:
There seems to be a TSan warning that wasn't caught by the trybot: https://build.chromium.org/p/client.webrtc/builders/Linux%20Tsan%20v2/builds/6732/steps/peerconnection_unittests/logs/stdio
Apparently a thread is still alive even after destroying WebRTCSession. Need to think of a way to fix this, without adding a critical section around g_clock (since that would hurt performance).
Original issue's description:
> Improving the fake clock and using it to fix a flaky STUN timeout test.
>
> When the fake clock's time is advanced, it now ensures all pending
> queued messages have been dispatched. This allows us to write a
> "SIMULATED_WAIT" macro that ticks the simulated clock by milliseconds up
> until the target time.
>
> Useful in this case, where we know the STUN timeout should take a total
> of 9500ms, but it would be overly complex to write test code that waits
> for each individual timeout, ensures a STUN packet has been
> retransmited, etc.
>
> (The test described above *should* be written, but it belongs in
> p2ptransportchannel_unittest.cc, not webrtcsession_unittest.cc).
>
> Committed: https://crrev.com/ffbe0e17e2c9b7fe101023acf40574dc0c95631a
> Cr-Commit-Position: refs/heads/master@{#13043}
TBR=pthatcher@webrtc.org,tommi@webrtc.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
Review-Url: https://codereview.webrtc.org/2038213002
Cr-Commit-Position: refs/heads/master@{#13045}
When the fake clock's time is advanced, it now ensures all pending
queued messages have been dispatched. This allows us to write a
"SIMULATED_WAIT" macro that ticks the simulated clock by milliseconds up
until the target time.
Useful in this case, where we know the STUN timeout should take a total
of 9500ms, but it would be overly complex to write test code that waits
for each individual timeout, ensures a STUN packet has been
retransmited, etc.
(The test described above *should* be written, but it belongs in
p2ptransportchannel_unittest.cc, not webrtcsession_unittest.cc).
Review-Url: https://codereview.webrtc.org/2024813004
Cr-Commit-Position: refs/heads/master@{#13043}
This will be useful for any tests that test objects with time-dependent
behavior. It will allow such tests to be written in such a way that their
outcome is more repeatable (less flaky), and will also allow such tests
to finish quicker. For example, a test for STUN timeout doesn't need to
wait the full timeout interval in real time; it can simply advance the
simulated clock.
BUG=webrtc:4925
R=pthatcher@webrtc.org
Review URL: https://codereview.webrtc.org/1895933003 .
Cr-Commit-Position: refs/heads/master@{#12950}
Moves logic of default SocketServer from MessageQueue to SocketServer
Review-Url: https://codereview.webrtc.org/1891293002
Cr-Commit-Position: refs/heads/master@{#12541}
The CL prevents a data race in MessageQueue where the variable "ss_" is
modified without a lock while sometimes read inside a lock.
Also thread annotations have been added to the MessageQueue class.
This was already reviewed and landed in https://codereview.webrtc.org/1675923002/
but failed in Chromium GN builds due to sharedexclusivelock.cc not being
compiled in these builds. This changed in https://codereview.webrtc.org/1712773003/
so the reland should work fine now.
BUG=webrtc:5496
Review URL: https://codereview.webrtc.org/1729893002
Cr-Commit-Position: refs/heads/master@{#11758}
Reason for revert:
Broke chromium.webrtc.fyi bots:
https://build.chromium.org/p/chromium.webrtc.fyi/builders/Mac%20Builder/builds/9891https://build.chromium.org/p/chromium.webrtc.fyi/builders/Mac%20GN/builds/11416
Fails with
-----
Undefined symbols for architecture x86_64:
"rtc::SharedExclusiveLock::LockShared()", referenced from:
rtc::MessageQueue::DoDestroy() in librtc_base.a(messagequeue.o)
rtc::MessageQueue::socketserver() in librtc_base.a(messagequeue.o)
rtc::MessageQueue::WakeUpSocketServer() in librtc_base.a(messagequeue.o)
rtc::MessageQueue::Quit() in librtc_base.a(messagequeue.o)
rtc::MessageQueue::Get(rtc::Message*, int, bool) in librtc_base.a(messagequeue.o)
rtc::MessageQueue::Post(rtc::MessageHandler*, unsigned int, rtc::MessageData*, bool) in librtc_base.a(messagequeue.o)
rtc::MessageQueue::DoDelayPost(int, unsigned int, rtc::MessageHandler*, unsigned int, rtc::MessageData*) in librtc_base.a(messagequeue.o)
...
"rtc::SharedExclusiveLock::UnlockShared()", referenced from:
rtc::MessageQueue::DoDestroy() in librtc_base.a(messagequeue.o)
rtc::MessageQueue::socketserver() in librtc_base.a(messagequeue.o)
rtc::MessageQueue::WakeUpSocketServer() in librtc_base.a(messagequeue.o)
rtc::MessageQueue::Quit() in librtc_base.a(messagequeue.o)
rtc::MessageQueue::Get(rtc::Message*, int, bool) in librtc_base.a(messagequeue.o)
rtc::MessageQueue::Post(rtc::MessageHandler*, unsigned int, rtc::MessageData*, bool) in librtc_base.a(messagequeue.o)
rtc::MessageQueue::DoDelayPost(int, unsigned int, rtc::MessageHandler*, unsigned int, rtc::MessageData*) in librtc_base.a(messagequeue.o)
...
"rtc::SharedExclusiveLock::SharedExclusiveLock()", referenced from:
rtc::MessageQueue::MessageQueue(rtc::SocketServer*, bool) in librtc_base.a(messagequeue.o)
ld: symbol(s) not found for architecture x86_64
-----
Looks like these are compiling without "webrtc/base/sharedexclusivelock.cc".
Original issue's description:
> Prevent data race in MessageQueue.
>
> The CL prevents a data race in MessageQueue where the variable "ss_" is
> modified without a lock while sometimes read inside a lock.
>
> Also thread annotations have been added to the MessageQueue class.
>
> BUG=webrtc:5496
>
> Committed: https://crrev.com/df88460372e7ce78c871a87774d7e6d82aac6ee3
> Cr-Commit-Position: refs/heads/master@{#11683}
TBR=ivoc@webrtc.org,pthatcher@webrtc.org,deadbeef@webrtc.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=webrtc:5496
Review URL: https://codereview.webrtc.org/1714463003
Cr-Commit-Position: refs/heads/master@{#11686}
The CL prevents a data race in MessageQueue where the variable "ss_" is
modified without a lock while sometimes read inside a lock.
Also thread annotations have been added to the MessageQueue class.
BUG=webrtc:5496
Review URL: https://codereview.webrtc.org/1675923002
Cr-Commit-Position: refs/heads/master@{#11683}
This CL fixes a race where for Thread objects the parent MessageQueue
constructor registers the object in the MessageQueueManager even though
the Thread is not constructed completely yet. Same happens during
destruction.
BUG=webrtc:1225
Review URL: https://codereview.webrtc.org/1666863002
Cr-Commit-Position: refs/heads/master@{#11497}
This class is being used from both libjingle and webrtc but we recently had a regression when we added dependency on libjingle's Thread class. This cleans that up and moves the implementation of CriticalSection and helper classes into the source file.
I'm also improving debugging facilities and constness.
BUG=
R=magjed@webrtc.org
Review URL: https://webrtc-codereview.appspot.com/51769004
Cr-Commit-Position: refs/heads/master@{#9089}
Mostly this consists of marking functions with override when
applicable, and moving function bodies from .h to .cc files.
Not inlining virtual functions with simple bodies such as
{ return false; }
strikes me as probably losing more in readability than we gain in
binary size and compilation time, but I guess it's just like any other
case where enabling a generally good warning forces us to write
slightly worse code in a couple of places.
BUG=163
R=kjellander@webrtc.org, tommi@webrtc.org
Review URL: https://webrtc-codereview.appspot.com/47429004
Cr-Commit-Position: refs/heads/master@{#8656}
git-svn-id: http://webrtc.googlecode.com/svn/trunk@8656 4adac7df-926f-26a2-2b94-8c16560cd09d
Local testing indicates that the pthread_t member variable might be causing alignment problems on the Chromium bots. After landing this (and once the Chromium tree is open again), I'll try a roll again to see if this has an effect.
R=magjed@webrtc.org
Review URL: https://webrtc-codereview.appspot.com/48449004
Cr-Commit-Position: refs/heads/master@{#8644}
git-svn-id: http://webrtc.googlecode.com/svn/trunk@8644 4adac7df-926f-26a2-2b94-8c16560cd09d
