Add protection for RTCPSender::max_packet_size_.

Add protection for RTCPSender::max_packet_size_.

This cl protects the access to the max_packet_size_, without fixing
the underlying race; the value is simply copied to a local variable,
whose value might be stale when used.

BUG=webrtc:7189
Review-Url: https://codereview.webrtc.org/2704263003
Cr-Commit-Position: refs/heads/master@{#16754}
Committed: https://chromium.googlesource.com/external/webrtc/+/6f142eb36ed2bcc8c3b7557daf6839e976ccd4b8

[nisse-webrtc, 2017-02-21 09:07:25]

nisse@webrtc.org changed reviewers:
+ danilchap@webrtc.org, tommi@webrtc.org

[nisse-webrtc, 2017-02-21 09:07:26]

https://codereview.webrtc.org/2704263003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right):

https://codereview.webrtc.org/2704263003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:830: size_t bytes_sent =
container.SendPackets(max_packet_size);
Or can we move this inside the above scope, which is protected by the critical
section?

https://codereview.webrtc.org/2704263003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:1057: return
packet.BuildExternalBuffer(buffer, max_packet_size, &sender) &&
Same here, can we move this inside the above, protected, scope?

[tommi, 2017-02-21 09:14:33]

please also see the comment I added to the bug.  I'd rather reduce locking if we
can and add thread checks to make usage clear.

[danilchap, 2017-02-21 09:16:31]

lgtm

nice to see you fix it without adding locks on send path.

https://codereview.webrtc.org/2704263003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right):

https://codereview.webrtc.org/2704263003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:830: size_t bytes_sent =
container.SendPackets(max_packet_size);
On 2017/02/21 09:07:26, nisse-webrtc wrote:
> Or can we move this inside the above scope, which is protected by the critical
> section?

Better not: SendPackets involves calling transport 
that is better to do outside lock.

https://codereview.webrtc.org/2704263003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:1057: return
packet.BuildExternalBuffer(buffer, max_packet_size, &sender) &&
On 2017/02/21 09:07:26, nisse-webrtc wrote:
> Same here, can we move this inside the above, protected, scope?

same here: calling BuildExternalBuffer would call OnPacketReady that calls both
transport and event_log. those operations can be unknownly long, so better to do
it outside lock.

[nisse-webrtc, 2017-02-21 12:54:29]

On 2017/02/21 09:14:33, tommi (webrtc) wrote:
> please also see the comment I added to the bug.  I'd rather reduce locking if
we
> can and add thread checks to make usage clear.

If I understood danil correctly, the |max_packet_size_| member is accessed from
multiple threads, so it's not easy to make this single-threaded.

[tommi, 2017-02-21 13:12:19]

Since I seem to have added myself as a reviewer - lgtm since this fixes the
issue and doesn't add any more complexity (nice).

On 2017/02/21 12:54:29, nisse-webrtc wrote:
> On 2017/02/21 09:14:33, tommi (webrtc) wrote:
> > please also see the comment I added to the bug.  I'd rather reduce locking
if
> we
> > can and add thread checks to make usage clear.
> 
> If I understood danil correctly, the |max_packet_size_| member is accessed
from
> multiple threads, so it's not easy to make this single-threaded.

It's not clear if that's how things are, how they should be or if they are but
that's a bug.
I would start to hammer down what threads we're on in each method, with thread
checks, so we can start to make informed decisions about when to use locks and
when not to (and catch when the caller is breaking the rules).

[nisse-webrtc, 2017-02-21 13:43:01]

The CQ bit was checked by nisse@webrtc.org

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

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-21 15:32:47]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1487684581413640, "parent_rev":
"5ef2bc191428928ee27f853c722f2ebd7d0a8a0c", "commit_rev":
"6f142eb36ed2bcc8c3b7557daf6839e976ccd4b8"}

[commit-bot: I haz the power, 2017-02-21 15:32:50]

Description was changed from

==========
Add protection for RTCPSender::max_packet_size_.

This cl protects the access to the max_packet_size_, without fixing
the underlying race; the value is simply copied to a local variable,
whose value might be stale when used.

BUG=webrtc:7189
==========

to

==========
Add protection for RTCPSender::max_packet_size_.

This cl protects the access to the max_packet_size_, without fixing
the underlying race; the value is simply copied to a local variable,
whose value might be stale when used.

BUG=webrtc:7189

Review-Url: https://codereview.webrtc.org/2704263003
Cr-Commit-Position: refs/heads/master@{#16754}
Committed:
https://chromium.googlesource.com/external/webrtc/+/6f142eb36ed2bcc8c3b7557da...
==========

[commit-bot: I haz the power, 2017-02-21 15:32:51]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/external/webrtc/+/6f142eb36ed2bcc8c3b7557da...