Add a race-checking mechanism.

Add a race-checking mechanism.

Permits CHECKing/DCHECKing that methods are being accessed in a
thread-safe manner, even if they are not used by one single thread
(thread pools such as VideoToolbox OK).

BUG=
R=danilchap@webrtc.org, tommi@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/02bafc6379969075127760e060fc262b0d76b3ca

[pbos-webrtc, 2016-06-27 14:07:59]

PTAL, the thread check in IncomingVideoStream was recently removed because the
decoders didn't call it on the same thread always. I think something concurrency
checking is still in order.

https://codereview.webrtc.org/2097403002/diff/1/webrtc/common_video/incoming_...
File webrtc/common_video/incoming_video_stream.cc (right):

https://codereview.webrtc.org/2097403002/diff/1/webrtc/common_video/incoming_...
webrtc/common_video/incoming_video_stream.cc:52:
RTC_CHECK_RUNS_SINGLE_THREADED(&decoder_race_checker_);
This is a CHECK (not a DCHECK) because we have many decoder implementations and
a lot of them may have significantly lower debug coverage.

[pbos-webrtc, 2016-06-27 14:18:42]

race_checker.cc

[pbos-webrtc, 2016-06-27 14:44:16]

remove synchronization, fix Release()

[tommi, 2016-06-27 14:46:35]

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

[commit-bot: I haz the power, 2016-06-27 14:46: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, 2016-06-27 14:48:15]

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

[commit-bot: I haz the power, 2016-06-27 14:48:16]

Dry run: Try jobs failed on following builders:
  ios64_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_dbg/builds/779)
  ios64_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/8526)
  ios_api_framework on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_api_framework/build...)
  ios_arm64_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/10845)
  ios_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/16079)
  ios_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/14718)
  mac_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/4232)

[pbos-webrtc, 2016-06-27 14:53:32]

bah

[pbos-webrtc, 2016-06-27 15:04:00]

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

[commit-bot: I haz the power, 2016-06-27 15:04:06]

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, 2016-06-27 15:06:35]

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

[commit-bot: I haz the power, 2016-06-27 15:06:36]

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

[danilchap, 2016-06-27 17:58:58]

https://codereview.webrtc.org/2097403002/diff/40001/webrtc/base/race_checker.h
File webrtc/base/race_checker.h (right):

https://codereview.webrtc.org/2097403002/diff/40001/webrtc/base/race_checker....
webrtc/base/race_checker.h:55: explicit RaceCheckerScopeDoNothing(RaceChecker*
race_checker)
const RaceChecker*

https://codereview.webrtc.org/2097403002/diff/60001/webrtc/base/race_checker.cc
File webrtc/base/race_checker.cc (right):

https://codereview.webrtc.org/2097403002/diff/60001/webrtc/base/race_checker....
webrtc/base/race_checker.cc:24: const_cast<const
PlatformThreadRef&>(accessing_thread_), current_thread);
may be
const PlatformThreadRef accessing_thread = accessing_thread_;
return IsThreadRefEqual(accesing_thread, current_thread);
would look nicer than const_cast?

https://codereview.webrtc.org/2097403002/diff/60001/webrtc/base/race_checker.h
File webrtc/base/race_checker.h (right):

https://codereview.webrtc.org/2097403002/diff/60001/webrtc/base/race_checker....
webrtc/base/race_checker.h:64: #define RTC_CHECK_RUNS_SINGLE_THREADED(x)        
 \
race checkers doesn't verify it is single thread, it verify it was properly
synchronized (may be by running on same thread, but any other synchronization
method is fine too unlike for thread_checker)

may be rename to RTC_(D)CHECK_RUNS_SYNCHRONIZED
or use more generic name RTC_(D)CHECK_WITH(&race_checker_object);

[pbos-webrtc, 2016-06-27 18:05:53]

fixes

[pbos-webrtc, 2016-06-27 18:06:39]

I think the other names sound more complicated, but I don't want this to be
misleading either. Seeing with Tommi for >2 inputs.

https://codereview.webrtc.org/2097403002/diff/40001/webrtc/base/race_checker.h
File webrtc/base/race_checker.h (right):

https://codereview.webrtc.org/2097403002/diff/40001/webrtc/base/race_checker....
webrtc/base/race_checker.h:55: explicit RaceCheckerScopeDoNothing(RaceChecker*
race_checker)
On 2016/06/27 17:58:57, danilchap wrote:
> const RaceChecker*

Done.

https://codereview.webrtc.org/2097403002/diff/60001/webrtc/base/race_checker.cc
File webrtc/base/race_checker.cc (right):

https://codereview.webrtc.org/2097403002/diff/60001/webrtc/base/race_checker....
webrtc/base/race_checker.cc:24: const_cast<const
PlatformThreadRef&>(accessing_thread_), current_thread);
On 2016/06/27 17:58:57, danilchap wrote:
> may be
> const PlatformThreadRef accessing_thread = accessing_thread_;
> return IsThreadRefEqual(accesing_thread, current_thread);
> would look nicer than const_cast?

Done.

https://codereview.webrtc.org/2097403002/diff/60001/webrtc/base/race_checker.h
File webrtc/base/race_checker.h (right):

https://codereview.webrtc.org/2097403002/diff/60001/webrtc/base/race_checker....
webrtc/base/race_checker.h:64: #define RTC_CHECK_RUNS_SINGLE_THREADED(x)        
 \
On 2016/06/27 17:58:57, danilchap wrote:
> race checkers doesn't verify it is single thread, it verify it was properly
> synchronized (may be by running on same thread, but any other synchronization
> method is fine too unlike for thread_checker)
> 
> may be rename to RTC_(D)CHECK_RUNS_SYNCHRONIZED
> or use more generic name RTC_(D)CHECK_WITH(&race_checker_object);

For me single threaded means that it's accessed as if only by a single thread.
Tommi, do you have a preferred name for this?

[pbos-webrtc, 2016-06-28 11:03:34]

RUNS_SYNCHRONIZED

[pbos-webrtc, 2016-06-28 11:04:25]

https://codereview.webrtc.org/2097403002/diff/60001/webrtc/base/race_checker.h
File webrtc/base/race_checker.h (right):

https://codereview.webrtc.org/2097403002/diff/60001/webrtc/base/race_checker....
webrtc/base/race_checker.h:64: #define RTC_CHECK_RUNS_SINGLE_THREADED(x)        
 \
On 2016/06/27 18:06:39, pbos-webrtc wrote:
> On 2016/06/27 17:58:57, danilchap wrote:
> > race checkers doesn't verify it is single thread, it verify it was properly
> > synchronized (may be by running on same thread, but any other
synchronization
> > method is fine too unlike for thread_checker)
> > 
> > may be rename to RTC_(D)CHECK_RUNS_SYNCHRONIZED
> > or use more generic name RTC_(D)CHECK_WITH(&race_checker_object);
> 
> For me single threaded means that it's accessed as if only by a single thread.
> Tommi, do you have a preferred name for this?

I replaced this with RUNS_SYNCHRONIZED, I don't remember if that was the
phrasing you used this morning tommi@?

[danilchap, 2016-06-28 13:48:12]

looks good to me % nit

https://codereview.webrtc.org/2097403002/diff/100001/webrtc/base/race_checker.cc
File webrtc/base/race_checker.cc (right):

https://codereview.webrtc.org/2097403002/diff/100001/webrtc/base/race_checker...
webrtc/base/race_checker.cc:14: RaceChecker::RaceChecker() {}
empty line after the constructor would look nicer.

[danilchap, 2016-06-28 13:48:48]

On 2016/06/28 13:48:12, danilchap wrote:
> looks good to me % nit
> 
>
https://codereview.webrtc.org/2097403002/diff/100001/webrtc/base/race_checker.cc
> File webrtc/base/race_checker.cc (right):
> 
>
https://codereview.webrtc.org/2097403002/diff/100001/webrtc/base/race_checker...
> webrtc/base/race_checker.cc:14: RaceChecker::RaceChecker() {}
> empty line after the constructor would look nicer.

i.e. lgtm

[tommi, 2016-06-28 14:21:13]

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

[commit-bot: I haz the power, 2016-06-28 14:21:23]

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, 2016-06-28 14:28:55]

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

[commit-bot: I haz the power, 2016-06-28 14:28:56]

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

[pbos-webrtc, 2016-06-28 15:05:36]

space between ctor

[pbos-webrtc, 2016-06-28 15:05:43]

https://codereview.webrtc.org/2097403002/diff/100001/webrtc/base/race_checker.cc
File webrtc/base/race_checker.cc (right):

https://codereview.webrtc.org/2097403002/diff/100001/webrtc/base/race_checker...
webrtc/base/race_checker.cc:14: RaceChecker::RaceChecker() {}
On 2016/06/28 13:48:12, danilchap wrote:
> empty line after the constructor would look nicer.

Done.

[tommi, 2016-06-30 20:14:09]

https://codereview.webrtc.org/2097403002/diff/120001/webrtc/base/race_checker.h
File webrtc/base/race_checker.h (right):

https://codereview.webrtc.org/2097403002/diff/120001/webrtc/base/race_checker...
webrtc/base/race_checker.h:65: #define RTC_CHECK_RUNS_SYNCHRONIZED(x)           
 \
Just checking - did you want to change this to RTC_CHECK_RUNS_SERIALIZED?

[pbos-webrtc, 2016-07-01 09:38:26]

RUNS_SERIALIZED

[pbos-webrtc, 2016-07-01 09:38:33]

Yep, thanks, done.

[tommi, 2016-07-01 09:42:20]

lgtm

[pbos-webrtc, 2016-07-01 10:45:34]

Description was changed from

==========
Add a race-checking mechanism.

Permits CHECKing/DCHECKing that methods are being accessed in a
thread-safe manner, even if they are not used by one single thread
(thread pools such as VideoToolbox OK).

BUG=
R=danilchap@webrtc.org, tommi@webrtc.org
==========

to

==========
Add a race-checking mechanism.

Permits CHECKing/DCHECKing that methods are being accessed in a
thread-safe manner, even if they are not used by one single thread
(thread pools such as VideoToolbox OK).

BUG=
R=danilchap@webrtc.org, tommi@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/02bafc6379969075127760e06...
==========

[commit-bot: I haz the power, 2016-07-01 10:45:35]

Description was changed from

==========
Add a race-checking mechanism.

Permits CHECKing/DCHECKing that methods are being accessed in a
thread-safe manner, even if they are not used by one single thread
(thread pools such as VideoToolbox OK).

BUG=
R=danilchap@webrtc.org, tommi@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/02bafc6379969075127760e06...
==========

to

==========
Add a race-checking mechanism.

Permits CHECKing/DCHECKing that methods are being accessed in a
thread-safe manner, even if they are not used by one single thread
(thread pools such as VideoToolbox OK).

BUG=
R=danilchap@webrtc.org, tommi@webrtc.org

Committed: https://crrev.com/02bafc6379969075127760e060fc262b0d76b3ca
Cr-Commit-Position: refs/heads/master@{#13358}
==========

[pbos-webrtc, 2016-07-01 10:45:38]

Committed patchset #8 (id:140001) manually as
02bafc6379969075127760e060fc262b0d76b3ca (presubmit successful).