Add NetLog support to UDP sockets.

Add NetLog support to UDP sockets.

BUG=99508
TEST=UDPSocketTest.Connect


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=106109

[mmenke, 2011-10-11 14:13:29]

eroman:  Please review everything.

agayev:  Please review the UDP component of this CL.

[cbentzel, 2011-10-11 14:23:33]

Not sure if agayev is going to have time to review this. sergeyu may be a
good choice.
On Oct 11, 2011 10:13 AM, <mmenke@chromium.org> wrote:

> Reviewers: eroman, agayev,
>
> Message:
> eroman:  Please review everything.
>
> agayev:  Please review the UDP component of this CL.
>
> Description:
> Add NetLog support to UDP sockets.
>
> BUG=99508
> TEST=UDPSocketTest.Connect
>
>
> Please review this at
http://codereview.chromium.**org/8200011/<http://codereview.chromium.org/8200...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src/<http://svn.chromium.org/chrome/trunk/src/>
>
> Affected files:
>  M     chrome/browser/net/passive_**log_collector.h
>  M     chrome/browser/net/passive_**log_collector.cc
>  M     chrome/browser/resources/net_**internals/events_view.css
>  M     chrome/browser/resources/net_**internals/source_entry.js
>  M     chrome/browser/resources/net_**internals/source_tracker.js
>  M     net/base/net_log.h
>  M     net/base/net_log.cc
>  M     net/base/net_log_event_type_**list.h
>  M     net/base/net_log_source_type_**list.h
>  M     net/net.gyp
>  M     net/socket/tcp_client_socket_**win.cc
>  A     net/udp/udp_data_transfer_**param.h
>  A     net/udp/udp_data_transfer_**param.cc
>  M     net/udp/udp_socket_libevent.h
>  M     net/udp/udp_socket_libevent.cc
>  M     net/udp/udp_socket_unittest.cc
>  M     net/udp/udp_socket_win.h
>  M     net/udp/udp_socket_win.cc
>
>
>

[mmenke, 2011-10-11 14:31:16]

Oops...  I mean sergeyu.  Those g's and y's confuse me, for some reason.

sergeyu:  Please review the UDP stuff.

agayev:  Sorry about that.

On 2011/10/11 14:23:33, cbentzel wrote:
> Not sure if agayev is going to have time to review this. sergeyu may be a
> good choice.

[eroman, 2011-10-11 20:50:06]

lgtm

http://codereview.chromium.org/8200011/diff/18003/net/udp/udp_data_transfer_p...
File net/udp/udp_data_transfer_param.h (right):

http://codereview.chromium.org/8200011/diff/18003/net/udp/udp_data_transfer_p...
net/udp/udp_data_transfer_param.h:1: // Copyright (c) 2010 The Chromium Authors.
All rights reserved.
nit: 2011

http://codereview.chromium.org/8200011/diff/18003/net/udp/udp_socket_libevent.cc
File net/udp/udp_socket_libevent.cc (right):

http://codereview.chromium.org/8200011/diff/18003/net/udp/udp_socket_libevent...
net/udp/udp_socket_libevent.cc:239: int rv = InternalConnect(address);
So this can only complete synchronously?

[mmenke, 2011-10-11 21:14:13]

My response isn't displayed as a reply, so resending.  My great wisdom must be
recorded for eternal posterity (Such as it is).

"Total comment" count wasn't updated either, curiously, though the comments are
visible.

On 2011/10/11 20:50:06, eroman wrote:
> lgtm
> 
>
http://codereview.chromium.org/8200011/diff/18003/net/udp/udp_data_transfer_p...
> File net/udp/udp_data_transfer_param.h (right):
> 
>
http://codereview.chromium.org/8200011/diff/18003/net/udp/udp_data_transfer_p...
> net/udp/udp_data_transfer_param.h:1: // Copyright (c) 2010 The Chromium
Authors.
> All rights reserved.
> nit: 2011
> 
>
http://codereview.chromium.org/8200011/diff/18003/net/udp/udp_socket_libevent.cc
> File net/udp/udp_socket_libevent.cc (right):
> 
>
http://codereview.chromium.org/8200011/diff/18003/net/udp/udp_socket_libevent...
> net/udp/udp_socket_libevent.cc:239: int rv = InternalConnect(address);
> So this can only complete synchronously?

[mmenke, 2011-10-11 21:16:54]

Oops... Copied wrong email.

http://codereview.chromium.org/8200011/diff/18003/net/udp/udp_data_transfer_p...
File net/udp/udp_data_transfer_param.h (right):

http://codereview.chromium.org/8200011/diff/18003/net/udp/udp_data_transfer_p...
net/udp/udp_data_transfer_param.h:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
On 2011/10/11 20:50:07, eroman wrote:
nit: 2011

Done.  You think someone could stop that from changing every year or so.
 Maybe I'll file a bug: "Year changes too frequently".


http://codereview.chromium.org/8200011/diff/18003/net/udp/udp_socket_libevent.cc
File net/udp/udp_socket_libevent.cc (right):

http://codereview.chromium.org/8200011/diff/18003/net/udp/udp_socket_libevent...
net/udp/udp_socket_libevent.cc:239: int rv = InternalConnect(address);
On 2011/10/11 20:50:07, eroman wrote:
So this can only complete synchronously?

Apparently not.  I wondered that, too, but there's no callback for it in
the Linux or Windows code, and the unit tests assume synchronous Connect
success.

This behavior does make sense, since it's not actually connecting
anything, but I can't find it documented anywhere as guaranteed
behavior.

http://codereview.chromium.org/8200011/

[Sergey Ulanov, 2011-10-13 04:01:54]

Looks good overall, but I'm concerned about creation of
UDPDataTransferNetLogParam for every packet sent/received, and that every
UDPDataTransferNetLogParam is rather heavy. I think it can be optimized.

http://codereview.chromium.org/8200011/diff/18006/net/udp/udp_data_transfer_p...
File net/udp/udp_data_transfer_param.cc (right):

http://codereview.chromium.org/8200011/diff/18006/net/udp/udp_data_transfer_p...
net/udp/udp_data_transfer_param.cc:18: hex_encoded_bytes_(log_bytes ?
base::HexEncode(bytes, byte_count) : "") {
I think we can call HexEncode in ToValue() if necessary.

http://codereview.chromium.org/8200011/diff/18006/net/udp/udp_data_transfer_p...
File net/udp/udp_data_transfer_param.h (right):

http://codereview.chromium.org/8200011/diff/18006/net/udp/udp_data_transfer_p...
net/udp/udp_data_transfer_param.h:34: const std::string hex_encoded_bytes_;
Do we need to store this here. This object is created every time we send/receive
UDP packet, so it's better to make it as light as possible.

http://codereview.chromium.org/8200011/diff/18006/net/udp/udp_data_transfer_p...
net/udp/udp_data_transfer_param.h:35: scoped_ptr<IPEndPoint> address_;
Does this need to be a pointer? Can we store it by value?
I think ideally this should be a ref-counted object to avoid copying the address
for every event.

http://codereview.chromium.org/8200011/diff/18006/net/udp/udp_socket_libevent.cc
File net/udp/udp_socket_libevent.cc (right):

http://codereview.chromium.org/8200011/diff/18006/net/udp/udp_socket_libevent...
net/udp/udp_socket_libevent.cc:322: net_log_.AddEvent(
I think we should log this event only when log level is above LOG_BASIC, i.e.
when IsLoggingAllEvents() returns true.

http://codereview.chromium.org/8200011/diff/18006/net/udp/udp_socket_libevent...
net/udp/udp_socket_libevent.cc:368: net_log_.AddEvent(
same here.

[mmenke, 2011-10-13 14:37:23]

Thanks for the feedback.

http://codereview.chromium.org/8200011/diff/18006/net/udp/udp_data_transfer_p...
File net/udp/udp_data_transfer_param.cc (right):

http://codereview.chromium.org/8200011/diff/18006/net/udp/udp_data_transfer_p...
net/udp/udp_data_transfer_param.cc:18: hex_encoded_bytes_(log_bytes ?
base::HexEncode(bytes, byte_count) : "") {
On 2011/10/13 04:01:54, sergeyu wrote:
> I think we can call HexEncode in ToValue() if necessary.

We can't.  ToValue may be called on other threads, or on the IOThread
asynchronously.  I believe in this particular case, it'll actually be called on
the UI thread, but I'm not positive.

http://codereview.chromium.org/8200011/diff/18006/net/udp/udp_data_transfer_p...
File net/udp/udp_data_transfer_param.h (right):

http://codereview.chromium.org/8200011/diff/18006/net/udp/udp_data_transfer_p...
net/udp/udp_data_transfer_param.h:34: const std::string hex_encoded_bytes_;
On 2011/10/13 04:01:54, sergeyu wrote:
> Do we need to store this here. This object is created every time we
send/receive
> UDP packet, so it's better to make it as light as possible.

These may be accessed on other threads, after the destruction of all underlying
objects.  Since we only use this field when "log all bytes" is enabled, which is
pretty much never, unless we're manually investigating a particular failure,
it's not as bad as it sounds.

http://codereview.chromium.org/8200011/diff/18006/net/udp/udp_data_transfer_p...
net/udp/udp_data_transfer_param.h:35: scoped_ptr<IPEndPoint> address_;
On 2011/10/13 04:01:54, sergeyu wrote:
> Does this need to be a pointer? Can we store it by value?
> I think ideally this should be a ref-counted object to avoid copying the
address
> for every event.

We'll access it on another thread, so we can't do it safely, since IPEndPoints
are mutable.

http://codereview.chromium.org/8200011/diff/18006/net/udp/udp_socket_libevent.cc
File net/udp/udp_socket_libevent.cc (right):

http://codereview.chromium.org/8200011/diff/18006/net/udp/udp_socket_libevent...
net/udp/udp_socket_libevent.cc:322: net_log_.AddEvent(
On 2011/10/13 04:01:54, sergeyu wrote:
> I think we should log this event only when log level is above LOG_BASIC, i.e.
> when IsLoggingAllEvents() returns true.

Sounds reasonable to me.  This was actually modelled off of what we do for
TCP/IP sockets, which had the eventual intention of keeping track of aggregate
transferred data, but looks like we're never going to do that, anyways.  Think I
may do the same thing in the other places, too.

Done (And on Windows, too, of course).

http://codereview.chromium.org/8200011/diff/18006/net/udp/udp_socket_libevent...
net/udp/udp_socket_libevent.cc:368: net_log_.AddEvent(
On 2011/10/13 04:01:54, sergeyu wrote:
> same here.

Done.

[mmenke, 2011-10-14 18:43:31]

sergeyu:  Ping.

[Sergey Ulanov, 2011-10-14 19:44:50]

On 2011/10/14 18:43:31, Matt Menke wrote:
> sergeyu:  Ping.
Looking now.

[Sergey Ulanov, 2011-10-14 20:15:47]

LGTM
Feel free to ignore my nits.

http://codereview.chromium.org/8200011/diff/37002/net/udp/udp_socket_libevent.cc
File net/udp/udp_socket_libevent.cc (right):

http://codereview.chromium.org/8200011/diff/37002/net/udp/udp_socket_libevent...
net/udp/udp_socket_libevent.cc:156: if (nread < 0) {
nit: remove braces here - they are not used in this file for single-line
statements.

http://codereview.chromium.org/8200011/diff/37002/net/udp/udp_socket_libevent...
net/udp/udp_socket_libevent.cc:209:
net_log_.AddEventWithNetErrorCode(NetLog::TYPE_UDP_SEND_ERROR,
optional nit: This code is duplicated in DidCompleteWrite(). Maybe rename
ProcessSuccessfulWrite() to LogWrite() and just call it here for any result.
same for ProcessSuccessulRead()

http://codereview.chromium.org/8200011/diff/37002/net/udp/udp_socket_win.cc
File net/udp/udp_socket_win.cc (right):

http://codereview.chromium.org/8200011/diff/37002/net/udp/udp_socket_win.cc#n...
net/udp/udp_socket_win.cc:157:
net_log_.AddEventWithNetErrorCode(NetLog::TYPE_UDP_RECEIVE_ERROR, nread);
nit: InternalRecvFrom() logs success events, so maybe it should log errors too?

http://codereview.chromium.org/8200011/diff/37002/net/udp/udp_socket_win.cc#n...
net/udp/udp_socket_win.cc:194:
net_log_.AddEventWithNetErrorCode(NetLog::TYPE_UDP_SEND_ERROR, nwrite);
move this to InternalSendTo()

http://codereview.chromium.org/8200011/diff/37002/net/udp/udp_socket_win.h
File net/udp/udp_socket_win.h (right):

http://codereview.chromium.org/8200011/diff/37002/net/udp/udp_socket_win.h#ne...
net/udp/udp_socket_win.h:193: scoped_ptr<IPEndPoint> send_to_address_;
Does this need to be a pointer? Can we store it by value?

[mmenke, 2011-10-17 17:35:19]

http://codereview.chromium.org/8200011/diff/37002/net/udp/udp_socket_libevent.cc
File net/udp/udp_socket_libevent.cc (right):

http://codereview.chromium.org/8200011/diff/37002/net/udp/udp_socket_libevent...
net/udp/udp_socket_libevent.cc:156: if (nread < 0) {
On 2011/10/14 20:15:47, sergeyu wrote:
> nit: remove braces here - they are not used in this file for single-line
> statements.

Done.

http://codereview.chromium.org/8200011/diff/37002/net/udp/udp_socket_libevent...
net/udp/udp_socket_libevent.cc:209:
net_log_.AddEventWithNetErrorCode(NetLog::TYPE_UDP_SEND_ERROR,
On 2011/10/14 20:15:47, sergeyu wrote:
> optional nit: This code is duplicated in DidCompleteWrite(). Maybe rename
> ProcessSuccessfulWrite() to LogWrite() and just call it here for any result.
> same for ProcessSuccessulRead()

Done.  I really like this idea.  Results in a lot less logging code all over the
place.

http://codereview.chromium.org/8200011/diff/37002/net/udp/udp_socket_win.cc
File net/udp/udp_socket_win.cc (right):

http://codereview.chromium.org/8200011/diff/37002/net/udp/udp_socket_win.cc#n...
net/udp/udp_socket_win.cc:157:
net_log_.AddEventWithNetErrorCode(NetLog::TYPE_UDP_RECEIVE_ERROR, nread);
On 2011/10/14 20:15:47, sergeyu wrote:
> nit: InternalRecvFrom() logs success events, so maybe it should log errors
too?

Done.

http://codereview.chromium.org/8200011/diff/37002/net/udp/udp_socket_win.cc#n...
net/udp/udp_socket_win.cc:194:
net_log_.AddEventWithNetErrorCode(NetLog::TYPE_UDP_SEND_ERROR, nwrite);
On 2011/10/14 20:15:47, sergeyu wrote:
> move this to InternalSendTo()

Done.

http://codereview.chromium.org/8200011/diff/37002/net/udp/udp_socket_win.h
File net/udp/udp_socket_win.h (right):

http://codereview.chromium.org/8200011/diff/37002/net/udp/udp_socket_win.h#ne...
net/udp/udp_socket_win.h:193: scoped_ptr<IPEndPoint> send_to_address_;
On 2011/10/14 20:15:47, sergeyu wrote:
> Does this need to be a pointer? Can we store it by value?

It can be null, and there's no IPEndPoint::IsValid() function.  Maybe there
should be, but I think that change, while minor, is beyond the scope of this CL,
since it would involve the same change to all the scoped_ptr<IPEndPoint>s in
this code.

[Sergey Ulanov, 2011-10-17 17:48:13]

http://codereview.chromium.org/8200011/diff/53013/net/udp/udp_socket_libevent.cc
File net/udp/udp_socket_libevent.cc (right):

http://codereview.chromium.org/8200011/diff/53013/net/udp/udp_socket_libevent...
net/udp/udp_socket_libevent.cc:197: return LogWrite(nwrite, buf->data(),
address);
Can we move this to InternalSendTo() so that it is consistent with
InternalRecvFrom()?

http://codereview.chromium.org/8200011/diff/53013/net/udp/udp_socket_libevent...
net/udp/udp_socket_libevent.cc:309: if (!address->FromSockAddr(addr, addr_len))
I think that conversion of the address should stay in InternalRecvFrom()

http://codereview.chromium.org/8200011/diff/53013/net/udp/udp_socket_win.cc
File net/udp/udp_socket_win.cc (right):

http://codereview.chromium.org/8200011/diff/53013/net/udp/udp_socket_win.cc#n...
net/udp/udp_socket_win.cc:297: if (!address->FromSockAddr(addr, recv_addr_len_))
This should be moved to DidCompleteRead()

[mmenke, 2011-10-17 19:41:38]

http://codereview.chromium.org/8200011/diff/53013/net/udp/udp_socket_libevent.cc
File net/udp/udp_socket_libevent.cc (right):

http://codereview.chromium.org/8200011/diff/53013/net/udp/udp_socket_libevent...
net/udp/udp_socket_libevent.cc:197: return LogWrite(nwrite, buf->data(),
address);
On 2011/10/17 17:48:13, sergeyu wrote:
> Can we move this to InternalSendTo() so that it is consistent with
> InternalRecvFrom()?

Done.

http://codereview.chromium.org/8200011/diff/53013/net/udp/udp_socket_libevent...
net/udp/udp_socket_libevent.cc:309: if (!address->FromSockAddr(addr, addr_len))
On 2011/10/17 17:48:13, sergeyu wrote:
> I think that conversion of the address should stay in InternalRecvFrom()

Done.  If address is NULL and we're logging, I still want to log it (As I see
this is a potential source of errors).  To keep things simple, I now just always
call IPEndPoint::FromSockAddr instead of using an existing IPEndPoint if we have
it.

http://codereview.chromium.org/8200011/diff/53013/net/udp/udp_socket_win.cc
File net/udp/udp_socket_win.cc (right):

http://codereview.chromium.org/8200011/diff/53013/net/udp/udp_socket_win.cc#n...
net/udp/udp_socket_win.cc:297: if (!address->FromSockAddr(addr, recv_addr_len_))
On 2011/10/17 17:48:13, sergeyu wrote:
> This should be moved to DidCompleteRead()

Done. Also had to add it to InternalRecvFrom, as it can theoretically complete
synchronously (Which was the real reason for the function in the first place,
amusingly).

[Sergey Ulanov, 2011-10-18 01:29:25]

couple more nits

http://codereview.chromium.org/8200011/diff/44036/net/udp/udp_socket_libevent.cc
File net/udp/udp_socket_libevent.cc (right):

http://codereview.chromium.org/8200011/diff/44036/net/udp/udp_socket_libevent...
net/udp/udp_socket_libevent.cc:162: return LogRead(MapSystemError(errno), NULL,
0, NULL);
LogRead() always logs |result|. Maybe change this code to 
 int result = MapSystemError(errno);
 LogRead(result, NULL, 0, NULL);
 return result;
Makes the code more readable. Same with LogWrite and in UDPSocketWin

http://codereview.chromium.org/8200011/diff/44036/net/udp/udp_socket_libevent...
net/udp/udp_socket_libevent.cc:397: if (result != ERR_IO_PENDING) {
nit: remove braces

[mmenke, 2011-10-18 15:16:57]

http://codereview.chromium.org/8200011/diff/44036/net/udp/udp_socket_libevent.cc
File net/udp/udp_socket_libevent.cc (right):

http://codereview.chromium.org/8200011/diff/44036/net/udp/udp_socket_libevent...
net/udp/udp_socket_libevent.cc:162: return LogRead(MapSystemError(errno), NULL,
0, NULL);
On 2011/10/18 01:29:25, sergeyu wrote:
> LogRead() always logs |result|. Maybe change this code to 
>  int result = MapSystemError(errno);
>  LogRead(result, NULL, 0, NULL);
>  return result;
> Makes the code more readable. Same with LogWrite and in UDPSocketWin

Done.

http://codereview.chromium.org/8200011/diff/44036/net/udp/udp_socket_libevent...
net/udp/udp_socket_libevent.cc:397: if (result != ERR_IO_PENDING) {
On 2011/10/18 01:29:25, sergeyu wrote:
> nit: remove braces

Done.

[Sergey Ulanov, 2011-10-18 16:48:09]

LGTM. Thanks!

[mmenke, 2011-10-18 16:49:18]

On 2011/10/18 16:48:09, sergeyu wrote:
> LGTM. Thanks!

Thanks for all the feedback.

[commit-bot: I haz the power, 2011-10-18 16:51:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/8200011/61006

[commit-bot: I haz the power, 2011-10-18 16:51:43]

Can't process patch for file
chrome/browser/resources/net_internals/source_entry.js.
File's status is None, patchset upload is incomplete.

[commit-bot: I haz the power, 2011-10-18 18:20:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/8200011/67001

[commit-bot: I haz the power, 2011-10-18 20:08:11]

Change committed as 106109