Send notification about outgoing p2p packets from browser to renderer.

Fix IpcPacketSocketFactory to return EWOULDBLOCK when network interface is congested.

PeerConnection implementation in libjingle will need to handle the case when 
a network interface is congested, so it needs to be notified when send buffers
are full. This CL adds send result notification message that is sent to the
renderer process after each message is sent. Sockets created using 
IpcPacketSocketFactory will be returning EWOULDBLOCK when Send() buffers are 
full instead of dropping the packet silently.

BUG=226158
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192721

[Sergey Ulanov, 2013-04-03 22:41:28]

ronghuawu@ - please review this change.
cevans@ - please approve changes in p2p_messages.h

[Ronghua Wu (Left Chromium), 2013-04-04 04:39:10]

+juberti

[Ronghua Wu (Left Chromium), 2013-04-04 05:23:59]

This change is more than the notification, you also add write_queue_ to host_tcp
etc. Maybe update the cl description.

https://codereview.chromium.org/13584008/diff/1001/content/browser/renderer_h...
File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):

https://codereview.chromium.org/13584008/diff/1001/content/browser/renderer_h...
content/browser/renderer_host/p2p/socket_host_tcp.cc:241: if (result < 0) {
you can remove this part now.

https://codereview.chromium.org/13584008/diff/1001/content/browser/renderer_h...
File content/browser/renderer_host/p2p/socket_host_udp.cc (right):

https://codereview.chromium.org/13584008/diff/1001/content/browser/renderer_h...
content/browser/renderer_host/p2p/socket_host_udp.cc:141: 
remove extra line

https://codereview.chromium.org/13584008/diff/1001/content/browser/renderer_h...
content/browser/renderer_host/p2p/socket_host_udp.cc:205: void
P2PSocketHostUdp::DidCompleteSend(int result) {
HandleSendResult?

https://codereview.chromium.org/13584008/diff/1001/content/browser/renderer_h...
content/browser/renderer_host/p2p/socket_host_udp.cc:210: " transient error.
Dropping the packet.";
indent

https://codereview.chromium.org/13584008/diff/1001/content/renderer/p2p/ipc_s...
File content/renderer/p2p/ipc_socket_factory.cc (right):

https://codereview.chromium.org/13584008/diff/1001/content/renderer/p2p/ipc_s...
content/renderer/p2p/ipc_socket_factory.cc:91: // Number which have been sent to
the renderer, but for which we haven't
s/renderer/browser?

https://codereview.chromium.org/13584008/diff/1001/content/renderer/p2p/ipc_s...
content/renderer/p2p/ipc_socket_factory.cc:308: void
IpcPacketSocket::OnSendComplete() {
DCHECK_EQ(MessageLoop::current(), message_loop_);

[Sergey Ulanov, 2013-04-04 18:40:34]

https://codereview.chromium.org/13584008/diff/1001/content/browser/renderer_h...
File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):

https://codereview.chromium.org/13584008/diff/1001/content/browser/renderer_h...
content/browser/renderer_host/p2p/socket_host_tcp.cc:241: if (result < 0) {
On 2013/04/04 05:23:59, Ronghua Wu wrote:
> you can remove this part now.

Done.

https://codereview.chromium.org/13584008/diff/1001/content/browser/renderer_h...
File content/browser/renderer_host/p2p/socket_host_udp.cc (right):

https://codereview.chromium.org/13584008/diff/1001/content/browser/renderer_h...
content/browser/renderer_host/p2p/socket_host_udp.cc:141: 
On 2013/04/04 05:23:59, Ronghua Wu wrote:
> remove extra line

Done.

https://codereview.chromium.org/13584008/diff/1001/content/browser/renderer_h...
content/browser/renderer_host/p2p/socket_host_udp.cc:205: void
P2PSocketHostUdp::DidCompleteSend(int result) {
On 2013/04/04 05:23:59, Ronghua Wu wrote:
> HandleSendResult?

Done. Also renamed DidReadComplete().

https://codereview.chromium.org/13584008/diff/1001/content/browser/renderer_h...
content/browser/renderer_host/p2p/socket_host_udp.cc:210: " transient error.
Dropping the packet.";
On 2013/04/04 05:23:59, Ronghua Wu wrote:
> indent
I believe it's indented correctly (there is no << operator in the beginning of
the line).

https://codereview.chromium.org/13584008/diff/1001/content/renderer/p2p/ipc_s...
File content/renderer/p2p/ipc_socket_factory.cc (right):

https://codereview.chromium.org/13584008/diff/1001/content/renderer/p2p/ipc_s...
content/renderer/p2p/ipc_socket_factory.cc:91: // Number which have been sent to
the renderer, but for which we haven't
On 2013/04/04 05:23:59, Ronghua Wu wrote:
> s/renderer/browser?

Done.

https://codereview.chromium.org/13584008/diff/1001/content/renderer/p2p/ipc_s...
content/renderer/p2p/ipc_socket_factory.cc:308: void
IpcPacketSocket::OnSendComplete() {
On 2013/04/04 05:23:59, Ronghua Wu wrote:
> DCHECK_EQ(MessageLoop::current(), message_loop_);

Done.

[juberti, 2013-04-04 22:33:45]

I still don't understand why we want to keep a write queue when the OS has its
own write buffer. Since we are marking packets with send timestamps we want to
ensure that packets get dumped, not buffered, if they can't be written out in a
timely fashion.

[pwestin, 2013-04-04 22:37:42]

Since we don't have a synchronous call path from webrtc to the chrome udp
socket, we must queue those packets that are in flight (until we can stop
the flow) not just drop them. Since any packet loss on the send side that
we can avoid save a lot of issues down the line. This queue should only
hold a handful of packets.


On Thu, Apr 4, 2013 at 3:33 PM, <juberti@google.com> wrote:

> I still don't understand why we want to keep a write queue when the OS has
> its
> own write buffer. Since we are marking packets with send timestamps we
> want to
> ensure that packets get dumped, not buffered, if they can't be written out
> in a
> timely fashion.
>
>
https://codereview.chromium.**org/13584008/<https://codereview.chromium.org/1...
>

[juberti, 2013-04-04 22:40:29]

Still don't understand why OS socket buffer isn't sufficient.

[pwestin, 2013-04-04 22:44:06]

it's currently only 9k for UDP in chrome, maybe we can change that but the
core problem remain; when the OS socket buffer is full we need to stop
sending packet to it until it is ready, and the packet(s) that caused the
buffer to flow over need to be handled correctly, aka stored


On Thu, Apr 4, 2013 at 3:40 PM, <juberti@google.com> wrote:

> Still don't understand why OS socket buffer isn't sufficient.
>
>
>
https://codereview.chromium.**org/13584008/<https://codereview.chromium.org/1...
>

[juberti, 2013-04-04 22:49:47]

I think 9k is plenty big, possibly too big. If the buffer fills, any queued
packets are going to be stale - why not dump them and let NACK sort it out when
connectivity is restored?

[pwestin, 2013-04-04 22:56:19]

we can try both approaches once this lands.


On Thu, Apr 4, 2013 at 3:49 PM, <juberti@google.com> wrote:

> I think 9k is plenty big, possibly too big. If the buffer fills, any queued
> packets are going to be stale - why not dump them and let NACK sort it out
> when
> connectivity is restored?
>
>
https://codereview.chromium.**org/13584008/<https://codereview.chromium.org/1...
>

[Sergey Ulanov, 2013-04-04 23:03:38]

On 2013/04/04 22:33:45, juberti wrote:
> I still don't understand why we want to keep a write queue when the OS has its
> own write buffer.

Even when there is a space in the OS buffer there is no guarantee that sendto()
can complete synchronously. That particularly affects overlapped socket API on
windows. That's why the queue was added in the first place, see
crbug.com/100865. There are probably good reasons why write may not always
complete synchronously even when there is space in the buffer (e.g. the buffer
may be locked on a different CPU while OS tries to send a new packet from the
buffer). As I mentioned before, we don't need the queue to be longer than 1
packet, but that would mean that context would need to be switched twice per
packet, which is not very efficient, so it makes sense to keep the queue a bit
longer than 1 packet. Also note that I removed the code that sets send buffer
size, so we will no longer be buffering data in the OS buffer.

> Since we are marking packets with send timestamps we want to
> ensure that packets get dumped, not buffered, if they can't be written out in
a
> timely fashion.

Because the queue is small (only 8 packets), it should delay them too much
(normally less than 10ms with 10Mb network).

[juberti, 2013-04-04 23:08:32]

On 2013/04/04 23:03:38, sergeyu wrote:
> On 2013/04/04 22:33:45, juberti wrote:
> > I still don't understand why we want to keep a write queue when the OS has
its
> > own write buffer.
> 
> Even when there is a space in the OS buffer there is no guarantee that
sendto()
> can complete synchronously. That particularly affects overlapped socket API on
> windows. That's why the queue was added in the first place, see
> crbug.com/100865. There are probably good reasons why write may not always
> complete synchronously even when there is space in the buffer (e.g. the buffer
> may be locked on a different CPU while OS tries to send a new packet from the
> buffer). As I mentioned before, we don't need the queue to be longer than 1
> packet, but that would mean that context would need to be switched twice per
> packet, which is not very efficient, so it makes sense to keep the queue a bit
> longer than 1 packet. Also note that I removed the code that sets send buffer
> size, so we will no longer be buffering data in the OS buffer.
> 
> > Since we are marking packets with send timestamps we want to
> > ensure that packets get dumped, not buffered, if they can't be written out
in
> a
> > timely fashion.
> 
> Because the queue is small (only 8 packets), it should delay them too much
> (normally less than 10ms with 10Mb network).

So is the OS buffer now zero? (just trying to understand where we are ending up
here)

[Sergey Ulanov, 2013-04-04 23:15:29]

On 2013/04/04 23:08:32, juberti wrote:
> On 2013/04/04 23:03:38, sergeyu wrote:
> > On 2013/04/04 22:33:45, juberti wrote:
> > > I still don't understand why we want to keep a write queue when the OS has
> its
> > > own write buffer.
> > 
> > Even when there is a space in the OS buffer there is no guarantee that
> sendto()
> > can complete synchronously. That particularly affects overlapped socket API
on
> > windows. That's why the queue was added in the first place, see
> > crbug.com/100865. There are probably good reasons why write may not always
> > complete synchronously even when there is space in the buffer (e.g. the
buffer
> > may be locked on a different CPU while OS tries to send a new packet from
the
> > buffer). As I mentioned before, we don't need the queue to be longer than 1
> > packet, but that would mean that context would need to be switched twice per
> > packet, which is not very efficient, so it makes sense to keep the queue a
bit
> > longer than 1 packet. Also note that I removed the code that sets send
buffer
> > size, so we will no longer be buffering data in the OS buffer.
> > 
> > > Since we are marking packets with send timestamps we want to
> > > ensure that packets get dumped, not buffered, if they can't be written out
> in
> > a
> > > timely fashion.
> > 
> > Because the queue is small (only 8 packets), it should delay them too much
> > (normally less than 10ms with 10Mb network).
> 
> So is the OS buffer now zero? (just trying to understand where we are ending
up
> here)

With this change send buffer size is kept at OS default value (e.g. 8k on
windows). Send buffer size can't be set it to 0.

[juberti, 2013-04-04 23:41:04]

On 2013/04/04 23:15:29, sergeyu wrote:
> On 2013/04/04 23:08:32, juberti wrote:
> > On 2013/04/04 23:03:38, sergeyu wrote:
> > > On 2013/04/04 22:33:45, juberti wrote:
> > > > I still don't understand why we want to keep a write queue when the OS
has
> > its
> > > > own write buffer.
> > > 
> > > Even when there is a space in the OS buffer there is no guarantee that
> > sendto()
> > > can complete synchronously. That particularly affects overlapped socket
API
> on
> > > windows. That's why the queue was added in the first place, see
> > > crbug.com/100865. There are probably good reasons why write may not always
> > > complete synchronously even when there is space in the buffer (e.g. the
> buffer
> > > may be locked on a different CPU while OS tries to send a new packet from
> the
> > > buffer). As I mentioned before, we don't need the queue to be longer than
1
> > > packet, but that would mean that context would need to be switched twice
per
> > > packet, which is not very efficient, so it makes sense to keep the queue a
> bit
> > > longer than 1 packet. Also note that I removed the code that sets send
> buffer
> > > size, so we will no longer be buffering data in the OS buffer.
> > > 
> > > > Since we are marking packets with send timestamps we want to
> > > > ensure that packets get dumped, not buffered, if they can't be written
out
> > in
> > > a
> > > > timely fashion.
> > > 
> > > Because the queue is small (only 8 packets), it should delay them too much
> > > (normally less than 10ms with 10Mb network).
> > 
> > So is the OS buffer now zero? (just trying to understand where we are ending
> up
> > here)
> 
> With this change send buffer size is kept at OS default value (e.g. 8k on
> windows). Send buffer size can't be set it to 0.

I think the send buffer can be set to zero, which makes send effectively a
blocking call, which might make sense here. But we can tune that later, overall
this approach LG.

Would it make sense to dump the queued packets when the socket decides to block
for a while? Otherwise we are going to send stale data.

[Ronghua Wu (Left Chromium), 2013-04-04 23:53:02]

LGTM if Justin is fine with the approach.

https://codereview.chromium.org/13584008/diff/17002/content/renderer/p2p/ipc_...
File content/renderer/p2p/ipc_socket_factory.cc (right):

https://codereview.chromium.org/13584008/diff/17002/content/renderer/p2p/ipc_...
content/renderer/p2p/ipc_socket_factory.cc:200: return EWOULDBLOCK;
Justin, you mentioned in my cl that you want something like
SignalReadyToSend(bool) for both down and resume. Or are you ok with EWOULDBLOCK
+ SignalWritable?

[juberti, 2013-04-05 00:01:45]

Since the only way we know if we're unable to send is if we try to send and our
queue is full, I'm OK with EWOUDBLOCK on send and SignalReadyToSend when
connectivity is restored.

[Ronghua Wu (Left Chromium), 2013-04-05 17:56:04]

https://codereview.chromium.org/13584008/diff/17002/content/renderer/p2p/ipc_...
File content/renderer/p2p/ipc_socket_factory.cc (right):

https://codereview.chromium.org/13584008/diff/17002/content/renderer/p2p/ipc_...
content/renderer/p2p/ipc_socket_factory.cc:200: return EWOULDBLOCK;
Should we set error_ to EWOULDBLOCK? IOW, should the upper layer get EWOULDBLOCK
from return value of SendTo or from the GetError()?

[Sergey Ulanov, 2013-04-05 21:04:03]

https://codereview.chromium.org/13584008/diff/17002/content/renderer/p2p/ipc_...
File content/renderer/p2p/ipc_socket_factory.cc (right):

https://codereview.chromium.org/13584008/diff/17002/content/renderer/p2p/ipc_...
content/renderer/p2p/ipc_socket_factory.cc:200: return EWOULDBLOCK;
On 2013/04/05 17:56:04, Ronghua Wu wrote:
> Should we set error_ to EWOULDBLOCK? IOW, should the upper layer get
EWOULDBLOCK
> from return value of SendTo or from the GetError()?

That's a good point. I think we should return -1 here and EWOULDBLOCK in
GetError() for consistency with AsyncSocket and also because EWOULDBLOCK is
positive.

[Ronghua Wu (Left Chromium), 2013-04-05 21:51:57]

https://codereview.chromium.org/13584008/diff/32001/content/renderer/p2p/ipc_...
File content/renderer/p2p/ipc_socket_factory.cc (right):

https://codereview.chromium.org/13584008/diff/32001/content/renderer/p2p/ipc_...
content/renderer/p2p/ipc_socket_factory.cc:209: NOTREACHED();
set error_ here and all the "return -1".

[juberti, 2013-04-05 21:56:20]

On 2013/04/05 21:04:03, sergeyu wrote:
>
https://codereview.chromium.org/13584008/diff/17002/content/renderer/p2p/ipc_...
> File content/renderer/p2p/ipc_socket_factory.cc (right):
> 
>
https://codereview.chromium.org/13584008/diff/17002/content/renderer/p2p/ipc_...
> content/renderer/p2p/ipc_socket_factory.cc:200: return EWOULDBLOCK;
> On 2013/04/05 17:56:04, Ronghua Wu wrote:
> > Should we set error_ to EWOULDBLOCK? IOW, should the upper layer get
> EWOULDBLOCK
> > from return value of SendTo or from the GetError()?
> 
> That's a good point. I think we should return -1 here and EWOULDBLOCK in
> GetError() for consistency with AsyncSocket and also because EWOULDBLOCK is
> positive.

+1

[Sergey Ulanov, 2013-04-05 22:28:14]

https://codereview.chromium.org/13584008/diff/32001/content/renderer/p2p/ipc_...
File content/renderer/p2p/ipc_socket_factory.cc (right):

https://codereview.chromium.org/13584008/diff/32001/content/renderer/p2p/ipc_...
content/renderer/p2p/ipc_socket_factory.cc:209: NOTREACHED();
On 2013/04/05 21:51:57, Ronghua Wu wrote:
> set error_ here and all the "return -1".

Actually SocketAddressToIPEndPoint() never fails in the current implementation.
I'll just fix it to return void in a separate CL.

[jschuh, 2013-04-05 22:53:27]

ipc security lgtm. innocuous looking browser -> renderer.

[commit-bot: I haz the power, 2013-04-05 22:55:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/13584008/32001

[commit-bot: I haz the power, 2013-04-06 06:59:44]

Change committed as 192721