Protect MessageQueue stop field with a critical section to avoid data races.

Protect MessageQueue stop field with a critical section to avoid data races.

Committed: https://crrev.com/1d35d2971b1e89b3ecadb7fb1ff064f9af850ad4
Cr-Commit-Position: refs/heads/master@{#13430}

[andresp, 2016-06-01 09:22:08]

andresp@webrtc.org changed reviewers:
+ tommi@webrtc.org

[andresp, 2016-06-01 10:52:11]

On 2016/06/01 09:22:08, andresp wrote:
> mailto:andresp@webrtc.org changed reviewers:
> + mailto:tommi@webrtc.org

The win_drmemory_light fail was probably unrelated. It was the "WinPing" test.

[tommi, 2016-06-01 12:41:56]

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

[tommi, 2016-06-01 12:41:57]

swapping myself out for pthatcher (but you have my lgtm rubber stamp if you need
it).

Is there a bug report?

[andresp, 2016-06-01 14:06:29]

On 2016/06/01 12:41:57, tommi-webrtc wrote:
> swapping myself out for pthatcher (but you have my lgtm rubber stamp if you
need
> it).
> 
> Is there a bug report?

Not that I know of. But I suspect I will be cleaning a few of the data races as
when I run tsan in a project that uses webrtc I see these races and they clutter
other races derived from a potentially incorrect use of webrtc.

[tommi, 2016-06-01 14:25:31]

On 2016/06/01 14:06:29, andresp wrote:
> On 2016/06/01 12:41:57, tommi-webrtc wrote:
> > swapping myself out for pthatcher (but you have my lgtm rubber stamp if you
> need
> > it).
> > 
> > Is there a bug report?
> 
> Not that I know of. But I suspect I will be cleaning a few of the data races
as
> when I run tsan in a project that uses webrtc I see these races and they
clutter
> other races derived from a potentially incorrect use of webrtc.

It's great to fix these things.  What I was thinking about is that when we do
e.g. builds of Chrome or other products that use WebRTC, we look over the list
of issues (bugs) that have been fixed in the period since the last release, and
include in the release notes. I was wondering if this would be worth tracking
that way.

[pthatcher1, 2016-06-01 15:29:13]

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

[pthatcher1, 2016-06-01 15:29:14]

Taylor, can you also check this?  Anything in MessageQueue could use multiple
eyes.

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

https://codereview.webrtc.org/2023193002/diff/1/webrtc/base/messagequeue.cc#n...
webrtc/base/messagequeue.cc:215: stop_ = true;
Might as well rename "stop_" to "quitting_".

https://codereview.webrtc.org/2023193002/diff/1/webrtc/base/messagequeue.cc#n...
webrtc/base/messagequeue.cc:221: CritScope cs(&stop_crit_);
Should we use an std::atomic here once they are allowed?  Perhaps with a comment
similar to the one in thread_unittest.cc above AtomicBool.

[tommi (sloooow) - chröme, 2016-06-01 15:53:28]

tommi@chromium.org changed reviewers:
+ tommi@chromium.org

[tommi (sloooow) - chröme, 2016-06-01 15:53:29]

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

https://codereview.webrtc.org/2023193002/diff/1/webrtc/base/messagequeue.cc#n...
webrtc/base/messagequeue.cc:221: CritScope cs(&stop_crit_);
On 2016/06/01 15:29:14, pthatcher1 wrote:
> Should we use an std::atomic here once they are allowed?  Perhaps with a
comment
> similar to the one in thread_unittest.cc above AtomicBool.

We do have alternatives to std::atomic that we can use instead.  See
webrtc/base/atomicops.h.

[Taylor Brandstetter, 2016-06-01 17:00:38]

I don't see that this race causes any issues. Is this just fixing a TSan
warning? If that's the case, is there a way to fix this without adding a
critical section acquisition in the thread's event loop?

[tommi, 2016-06-01 17:03:14]

On 2016/06/01 17:00:38, Taylor Brandstetter wrote:
> I don't see that this race causes any issues. Is this just fixing a TSan
> warning? If that's the case, is there a way to fix this without adding a
> critical section acquisition in the thread's event loop?

Using atomics is one way

[pbos-webrtc, 2016-06-17 20:08:59]

On 2016/06/01 17:03:14, tommi-webrtc wrote:
> On 2016/06/01 17:00:38, Taylor Brandstetter wrote:
> > I don't see that this race causes any issues. Is this just fixing a TSan
> > warning? If that's the case, is there a way to fix this without adding a
> > critical section acquisition in the thread's event loop?
> 
> Using atomics is one way

Other project attempted to suppress ::Clear as well, is this CL addressing that
as well? (If so remove tsan suppression).

If this is just to protect a bool then rtc::AtomicOps::ReleaseStore/AcquireLoad
on a volatile int that's 1 or 0.

[pbos-webrtc, 2016-06-17 20:10:00]

On 2016/06/17 20:08:59, pbos-webrtc wrote:
> On 2016/06/01 17:03:14, tommi-webrtc wrote:
> > On 2016/06/01 17:00:38, Taylor Brandstetter wrote:
> > > I don't see that this race causes any issues. Is this just fixing a TSan
> > > warning? If that's the case, is there a way to fix this without adding a
> > > critical section acquisition in the thread's event loop?
> > 
> > Using atomics is one way
> 
> Other project attempted to suppress ::Clear as well, is this CL addressing
that
> as well? (If so remove tsan suppression).
> 
> If this is just to protect a bool then
rtc::AtomicOps::ReleaseStore/AcquireLoad
> on a volatile int that's 1 or 0.

https://chromium.googlesource.com/external/webrtc/+/f03a8d4c4dabcc18b36bce6b0...
does this for shutdown synchronization.

[Taylor Brandstetter, 2016-06-21 23:17:43]

On 2016/06/17 20:10:00, pbos-webrtc wrote:
> On 2016/06/17 20:08:59, pbos-webrtc wrote:
> > On 2016/06/01 17:03:14, tommi-webrtc wrote:
> > > On 2016/06/01 17:00:38, Taylor Brandstetter wrote:
> > > > I don't see that this race causes any issues. Is this just fixing a TSan
> > > > warning? If that's the case, is there a way to fix this without adding a
> > > > critical section acquisition in the thread's event loop?
> > > 
> > > Using atomics is one way
> > 
> > Other project attempted to suppress ::Clear as well, is this CL addressing
> that
> > as well? (If so remove tsan suppression).
> > 
> > If this is just to protect a bool then
> rtc::AtomicOps::ReleaseStore/AcquireLoad
> > on a volatile int that's 1 or 0.
> 
>
https://chromium.googlesource.com/external/webrtc/+/f03a8d4c4dabcc18b36bce6b0...
> does this for shutdown synchronization.

I think it would be a good idea to do the same thing here. At least until we can
use std::atomic (currently banned for Chromium).

[Taylor Brandstetter, 2016-06-21 23:17:43]

On 2016/06/17 20:10:00, pbos-webrtc wrote:
> On 2016/06/17 20:08:59, pbos-webrtc wrote:
> > On 2016/06/01 17:03:14, tommi-webrtc wrote:
> > > On 2016/06/01 17:00:38, Taylor Brandstetter wrote:
> > > > I don't see that this race causes any issues. Is this just fixing a TSan
> > > > warning? If that's the case, is there a way to fix this without adding a
> > > > critical section acquisition in the thread's event loop?
> > > 
> > > Using atomics is one way
> > 
> > Other project attempted to suppress ::Clear as well, is this CL addressing
> that
> > as well? (If so remove tsan suppression).
> > 
> > If this is just to protect a bool then
> rtc::AtomicOps::ReleaseStore/AcquireLoad
> > on a volatile int that's 1 or 0.
> 
>
https://chromium.googlesource.com/external/webrtc/+/f03a8d4c4dabcc18b36bce6b0...
> does this for shutdown synchronization.

I think it would be a good idea to do the same thing here. At least until we can
use std::atomic (currently banned for Chromium).

[andresp, 2016-07-01 13:53:15]

Fixed according to comments. It is now using AtomicOps::Acquire/Release and it
removes more race suppressions.

Btw I still think the previous version with locks was acceptable and it is more
in line with internal google recomendations.

[andresp, 2016-07-07 13:10:56]

On 2016/07/01 13:53:15, andresp wrote:
> Fixed according to comments. It is now using AtomicOps::Acquire/Release and it
> removes more race suppressions.
> 
> Btw I still think the previous version with locks was acceptable and it is
more
> in line with internal google recomendations.

Actually I had to keep the Clear suppressions in place as there is a
vtable/destructor race which was too much to fix on this go.

[Taylor Brandstetter, 2016-07-07 22:01:47]

lgtm with minor nit

https://codereview.webrtc.org/2023193002/diff/60001/webrtc/base/messagequeue.cc
File webrtc/base/messagequeue.cc (right):

https://codereview.webrtc.org/2023193002/diff/60001/webrtc/base/messagequeue....
webrtc/base/messagequeue.cc:148: stop_(false),
Since it's an int now, should probably initialize with 0 instead of false.

[andresp, 2016-07-08 10:05:37]

https://codereview.webrtc.org/2023193002/diff/60001/webrtc/base/messagequeue.cc
File webrtc/base/messagequeue.cc (right):

https://codereview.webrtc.org/2023193002/diff/60001/webrtc/base/messagequeue....
webrtc/base/messagequeue.cc:148: stop_(false),
On 2016/07/07 22:01:47, Taylor Brandstetter wrote:
> Since it's an int now, should probably initialize with 0 instead of false.

Done.

[andresp, 2016-07-08 10:05:43]

The CQ bit was checked by andresp@webrtc.org

[andresp, 2016-07-08 10:05:43]

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/2023193002/#ps80001 (title:
"initialize with 0")

[commit-bot: I haz the power, 2016-07-08 10:05:54]

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, 2016-07-08 12:06:28]

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

[commit-bot: I haz the power, 2016-07-08 12:06:29]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[andresp, 2016-07-08 12:43:29]

The CQ bit was checked by andresp@webrtc.org

[commit-bot: I haz the power, 2016-07-08 12:43:40]

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, 2016-07-08 14:44:09]

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

[commit-bot: I haz the power, 2016-07-08 14:44:10]

Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[andresp, 2016-07-11 07:59:32]

The CQ bit was checked by andresp@webrtc.org

[commit-bot: I haz the power, 2016-07-11 07:59:39]

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, 2016-07-11 09:23:49]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-11 09:23:56]

Description was changed from

==========
Protect MessageQueue stop field with a critical section to avoid data races.
==========

to

==========
Protect MessageQueue stop field with a critical section to avoid data races.

Committed: https://crrev.com/1d35d2971b1e89b3ecadb7fb1ff064f9af850ad4
Cr-Commit-Position: refs/heads/master@{#13430}
==========

[commit-bot: I haz the power, 2016-07-11 09:23:57]

Patchset 5 (id:??) landed as
https://crrev.com/1d35d2971b1e89b3ecadb7fb1ff064f9af850ad4
Cr-Commit-Position: refs/heads/master@{#13430}

[henrika_webrtc, 2016-07-11 09:48:26]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.webrtc.org/2135173002/ by henrika@webrtc.org.

The reason for reverting is: Only reasonable CL in blameslist for broken Chrome
FYI bots on all platforms. See 

https://build.chromium.org/p/chromium.webrtc.fyi/waterfall?builder=Mac%20Builder.