Prevent data race in MessageQueue.

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}

[joachim, 2016-02-06 01:17:07]

jbauch@webrtc.org changed reviewers:
+ ivoc@webrtc.org, pthatcher@webrtc.org

[joachim, 2016-02-06 01:17:08]

Ptal, this should fix one of the data races found by TSan.

[ivoc, 2016-02-10 10:38:10]

Thanks for helping to fix this! I'm not very familiar with this part of the
code, but LGTM.

[pthatcher1, 2016-02-12 00:16:33]

pthatcher@webrtc.org changed reviewers:
+ deadbeef@webrtc.org

[pthatcher1, 2016-02-12 00:16:34]

https://codereview.webrtc.org/1675923002/diff/1/webrtc/base/messagequeue.cc
File webrtc/base/messagequeue.cc (right):

https://codereview.webrtc.org/1675923002/diff/1/webrtc/base/messagequeue.cc#n...
webrtc/base/messagequeue.cc:175: ExclusiveScope es(&ss_lock_);
Can you leave a comment explaining why this place needs an exclusive lock,
whereas none of the others do?

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

https://codereview.webrtc.org/1675923002/diff/1/webrtc/base/thread.cc#newcode358
webrtc/base/thread.cc:358: ss_->WakeUp();
Instead of making ss_lock_ protected, could we just call
socketserver()->WakeUp() here?

[joachim, 2016-02-12 15:09:52]

Updated based on your feedback.

https://codereview.chromium.org/1675923002/diff/1/webrtc/base/messagequeue.cc
File webrtc/base/messagequeue.cc (right):

https://codereview.chromium.org/1675923002/diff/1/webrtc/base/messagequeue.cc...
webrtc/base/messagequeue.cc:175: ExclusiveScope es(&ss_lock_);
On 2016/02/12 00:16:33, pthatcher1 wrote:
> Can you leave a comment explaining why this place needs an exclusive lock,
> whereas none of the others do?

Done.

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

https://codereview.chromium.org/1675923002/diff/1/webrtc/base/thread.cc#newco...
webrtc/base/thread.cc:358: ss_->WakeUp();
On 2016/02/12 00:16:33, pthatcher1 wrote:
> Instead of making ss_lock_ protected, could we just call
> socketserver()->WakeUp() here?

That would still be racy imho because the variable would not be locked between
the return of the socketserver() and the invokation of "WakeUp". Instead I
introduced a method "WakeUpSocketServer()" in MessageQueue that takes care of
proper locking.

With that, the socket server related properties can be made private in
MessageQueue.

[joachim, 2016-02-12 15:11:08]

On 2016/02/12 15:09:52, joachim wrote:
> Updated based on your feedback.
> 
> https://codereview.chromium.org/1675923002/diff/1/webrtc/base/messagequeue.cc
> File webrtc/base/messagequeue.cc (right):
> 
>
https://codereview.chromium.org/1675923002/diff/1/webrtc/base/messagequeue.cc...
> webrtc/base/messagequeue.cc:175: ExclusiveScope es(&ss_lock_);
> On 2016/02/12 00:16:33, pthatcher1 wrote:
> > Can you leave a comment explaining why this place needs an exclusive lock,
> > whereas none of the others do?
> 
> Done.
> 
> https://codereview.chromium.org/1675923002/diff/1/webrtc/base/thread.cc
> File webrtc/base/thread.cc (right):
> 
>
https://codereview.chromium.org/1675923002/diff/1/webrtc/base/thread.cc#newco...
> webrtc/base/thread.cc:358: ss_->WakeUp();
> On 2016/02/12 00:16:33, pthatcher1 wrote:
> > Instead of making ss_lock_ protected, could we just call
> > socketserver()->WakeUp() here?
> 
> That would still be racy imho because the variable would not be locked between
> the return of the socketserver() and the invokation of "WakeUp". Instead I
> introduced a method "WakeUpSocketServer()" in MessageQueue that takes care of
> proper locking.
> 
> With that, the socket server related properties can be made private in
> MessageQueue.

Sorry, sent from "codereview.chromium.org", correct urls should be on
https://codereview.webrtc.org/1675923002/

[Taylor Brandstetter, 2016-02-17 01:23:24]

lgtm

[pthatcher1, 2016-02-19 05:02:28]

lgtm

[joachim, 2016-02-19 07:13:07]

The CQ bit was checked by jbauch@webrtc.org

[joachim, 2016-02-19 07:13:08]

The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1675923002/#ps20001 (title:
"Feedback from Peter")

[commit-bot: I haz the power, 2016-02-19 07:13:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1675923002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1675923002/20001

[commit-bot: I haz the power, 2016-02-19 07:14:40]

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

[commit-bot: I haz the power, 2016-02-19 07:14:41]

Try jobs failed on following builders:
  ios64_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/5247)

[joachim, 2016-02-19 09:27:49]

The CQ bit was checked by jbauch@webrtc.org

[joachim, 2016-02-19 09:27:49]

The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org,
deadbeef@webrtc.org, pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1675923002/#ps40001 (title:
"Fixed initialization order.")

[commit-bot: I haz the power, 2016-02-19 09:27:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1675923002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1675923002/40001

[commit-bot: I haz the power, 2016-02-19 11:33:04]

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

[commit-bot: I haz the power, 2016-02-19 11:33:05]

Try jobs failed on following builders:
  linux_baremetal on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...)

[joachim, 2016-02-19 14:29:36]

The CQ bit was checked by jbauch@webrtc.org

[commit-bot: I haz the power, 2016-02-19 14:29:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1675923002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1675923002/40001

[commit-bot: I haz the power, 2016-02-19 15:03:34]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-19 15:03:45]

Description was changed from

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

to

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

[commit-bot: I haz the power, 2016-02-19 15:03:46]

Patchset 3 (id:??) landed as
https://crrev.com/df88460372e7ce78c871a87774d7e6d82aac6ee3
Cr-Commit-Position: refs/heads/master@{#11683}

[joachim, 2016-02-19 15:15:55]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.webrtc.org/1714463003/ by jbauch@webrtc.org.

The reason for reverting is: Broke chromium.webrtc.fyi bots:
https://build.chromium.org/p/chromium.webrtc.fyi/builders/Mac%20Builder/build...
https://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"..