Fix TSAN race in webrtc::voe::Channel

Fix TSAN race in webrtc::voe::Channel.

|transport_overhead_per_packet_| and |rtp_overhead_per_packet_| could
be read from and written to on different threads concurrently. This CL
introduces a lock to GUARD these variables.

NOTRY because master.tryserver.webrtc.linux_ubsan_vptr is broken, all
other tests pass.

BUG=webrtc:7231
NOTRY=True

Review-Url: https://codereview.webrtc.org/2710363003
Cr-Commit-Position: refs/heads/master@{#16900}
Committed: https://chromium.googlesource.com/external/webrtc/+/3fd31fe5023b299a531ee1ac74234aeba75026dd

[hbos, 2017-02-24 14:48:43]

Description was changed from

==========
Fix TSAN race in webrtc::voe::Channel

BUG=
==========

to

==========
Fix TSAN race in webrtc::voe::Channel

BUG=webrtc:7231
==========

[hbos, 2017-02-24 14:50:09]

Description was changed from

==========
Fix TSAN race in webrtc::voe::Channel

BUG=webrtc:7231
==========

to

==========
Fix TSAN race in webrtc::voe::Channel.

|transport_overhead_per_packet_| and |rtp_overhead_per_packet_| could
be read from and written to on different threads concurrently. This CL
introduces a lock to GUARD these variables.

BUG=webrtc:7231
==========

[hbos, 2017-02-24 14:52:05]

hbos@webrtc.org changed reviewers:
+ nisse@webrtc.org, solenberg@webrtc.org

[hbos, 2017-02-24 14:52:06]

Please take a look, solenberg and nisse.

[hbos, 2017-02-24 14:56:48]

On 2017/02/24 14:52:06, hbos wrote:
> Please take a look, solenberg and nisse.

The problem is that Channel::SetTransportOverhead and Channel::OnOverheadChanged
can be invoked from different threads, see bug for details.

[hbos, 2017-02-24 15:59:18]

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

[commit-bot: I haz the power, 2017-02-24 15:59:21]

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-02-24 16:39:20]

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

[commit-bot: I haz the power, 2017-02-24 16:39:21]

Dry run: This issue passed the CQ dry run.

[hbos, 2017-02-27 18:26:40]

Ping solenberg

[the sun, 2017-02-28 11:13:10]

lgtm

https://codereview.webrtc.org/2710363003/diff/20001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2710363003/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:2760: void Channel::OnOverheadChanged(size_t
overhead_bytes_per_packet) {
Please add:
// TODO(solenberg): Make AudioSendStream an OverheadObserver instead.

[hbos, 2017-02-28 11:44:55]

https://codereview.webrtc.org/2710363003/diff/20001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2710363003/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:2760: void Channel::OnOverheadChanged(size_t
overhead_bytes_per_packet) {
On 2017/02/28 11:13:10, the sun wrote:
> Please add:
> // TODO(solenberg): Make AudioSendStream an OverheadObserver instead.

Done.

[hbos, 2017-02-28 11:44:59]

The CQ bit was checked by hbos@webrtc.org

[hbos, 2017-02-28 11:44:59]

The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2710363003/#ps40001 (title:
"Merge and added TODO")

[commit-bot: I haz the power, 2017-02-28 11:45:12]

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-02-28 11:49:37]

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

[commit-bot: I haz the power, 2017-02-28 11:49:38]

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

[hta-webrtc, 2017-02-28 12:21:19]

hta@webrtc.org changed reviewers:
+ hta@webrtc.org

[hta-webrtc, 2017-02-28 12:21:20]

lgtm

https://codereview.webrtc.org/2710363003/diff/40001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.h (right):

https://codereview.webrtc.org/2710363003/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.h:419: void UpdateOverheadForEncoder(size_t
overhead_per_packet);
Should there be a lock notation here saying "must be called with
overhead_per_packet_lock held"?

It's private, so I guess it's OK to leave it out.

[hbos, 2017-02-28 12:32:53]

https://codereview.webrtc.org/2710363003/diff/40001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.h (right):

https://codereview.webrtc.org/2710363003/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.h:419: void UpdateOverheadForEncoder(size_t
overhead_per_packet);
On 2017/02/28 12:21:20, hta-webrtc wrote:
> Should there be a lock notation here saying "must be called with
> overhead_per_packet_lock held"?
> 
> It's private, so I guess it's OK to leave it out.

Good idea! There's EXCLUSIVE_LOCKS_REQUIRED. Now using that, which also meant I
could remove the function parameter and calculate it inside of the function
instead knowing the lock is already held.

[hbos, 2017-02-28 12:33:03]

The CQ bit was checked by hbos@webrtc.org

[hbos, 2017-02-28 12:33:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, hta@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2710363003/#ps60001 (title:
"EXCLUSIVE_LOCKS_REQUIRED used (and moved TODO to right function, whoops)")

[commit-bot: I haz the power, 2017-02-28 12:33:15]

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-02-28 13:34:41]

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

[commit-bot: I haz the power, 2017-02-28 13:34:42]

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

[hbos, 2017-02-28 13:39:10]

Description was changed from

==========
Fix TSAN race in webrtc::voe::Channel.

|transport_overhead_per_packet_| and |rtp_overhead_per_packet_| could
be read from and written to on different threads concurrently. This CL
introduces a lock to GUARD these variables.

BUG=webrtc:7231
==========

to

==========
Fix TSAN race in webrtc::voe::Channel.

|transport_overhead_per_packet_| and |rtp_overhead_per_packet_| could
be read from and written to on different threads concurrently. This CL
introduces a lock to GUARD these variables.

BUG=webrtc:7231
CQ_EXCLUDE_TRYBOTS=master.tryserver.webrtc.linux_ubsan_vptr
==========

[hbos, 2017-02-28 13:39:17]

The CQ bit was checked by hbos@webrtc.org

[commit-bot: I haz the power, 2017-02-28 13:39:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[hbos, 2017-02-28 13:40:09]

The CQ bit was unchecked by hbos@webrtc.org

[hbos, 2017-02-28 13:40:54]

Description was changed from

==========
Fix TSAN race in webrtc::voe::Channel.

|transport_overhead_per_packet_| and |rtp_overhead_per_packet_| could
be read from and written to on different threads concurrently. This CL
introduces a lock to GUARD these variables.

BUG=webrtc:7231
CQ_EXCLUDE_TRYBOTS=master.tryserver.webrtc.linux_ubsan_vptr
==========

to

==========
Fix TSAN race in webrtc::voe::Channel.

|transport_overhead_per_packet_| and |rtp_overhead_per_packet_| could
be read from and written to on different threads concurrently. This CL
introduces a lock to GUARD these variables.

NOTRY because master.tryserver.webrtc.linux_ubsan_vptr is broken, all
other tests pass.

BUG=webrtc:7231
NOTRY=True
==========

[hbos, 2017-02-28 13:41:16]

The CQ bit was checked by hbos@webrtc.org

[commit-bot: I haz the power, 2017-02-28 13:41:22]

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-02-28 13:43:17]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488289276410310,
"parent_rev": "4974df41838367e4d50c42bef3be121c7ac6e331", "commit_rev":
"3fd31fe5023b299a531ee1ac74234aeba75026dd"}

[commit-bot: I haz the power, 2017-02-28 13:43:21]

Description was changed from

==========
Fix TSAN race in webrtc::voe::Channel.

|transport_overhead_per_packet_| and |rtp_overhead_per_packet_| could
be read from and written to on different threads concurrently. This CL
introduces a lock to GUARD these variables.

NOTRY because master.tryserver.webrtc.linux_ubsan_vptr is broken, all
other tests pass.

BUG=webrtc:7231
NOTRY=True
==========

to

==========
Fix TSAN race in webrtc::voe::Channel.

|transport_overhead_per_packet_| and |rtp_overhead_per_packet_| could
be read from and written to on different threads concurrently. This CL
introduces a lock to GUARD these variables.

NOTRY because master.tryserver.webrtc.linux_ubsan_vptr is broken, all
other tests pass.

BUG=webrtc:7231
NOTRY=True

Review-Url: https://codereview.webrtc.org/2710363003
Cr-Commit-Position: refs/heads/master@{#16900}
Committed:
https://chromium.googlesource.com/external/webrtc/+/3fd31fe5023b299a531ee1ac7...
==========

[commit-bot: I haz the power, 2017-02-28 13:43:22]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/3fd31fe5023b299a531ee1ac7...