Add CHECK() in UdpSocketLibevent to debug crbug.com/452121

Return result from Close() for UDP sockets.

Returning result from Close() allows to add CHECK() in 
remoting::UdpPacketSocket to debug linked bug.

BUG=452121

[Sergey Ulanov, 2015-02-11 01:28:02]

Patchset #1 (id:1) has been deleted

[Sergey Ulanov, 2015-02-11 01:28:12]

sergeyu@chromium.org changed reviewers:
+ mef@chromium.org

[mef, 2015-02-11 19:25:35]

Matt, what is the policy / guideline on adding CHECK()?

https://codereview.chromium.org/914853002/diff/20001/net/udp/udp_socket_libev...
File net/udp/udp_socket_libevent.cc (right):

https://codereview.chromium.org/914853002/diff/20001/net/udp/udp_socket_libev...
net/udp/udp_socket_libevent.cc:133: // Crash if errno=EBADF, to debug
crbug.com/452121 .
Could you add TODO(sergeyu) to remove it once investigation is complete?

[mmenke, 2015-02-11 19:29:21]

On 2015/02/11 19:25:35, mef wrote:
> Matt, what is the policy / guideline on adding CHECK()?
> 
>
https://codereview.chromium.org/914853002/diff/20001/net/udp/udp_socket_libev...
> File net/udp/udp_socket_libevent.cc (right):
> 
>
https://codereview.chromium.org/914853002/diff/20001/net/udp/udp_socket_libev...
> net/udp/udp_socket_libevent.cc:133: // Crash if errno=EBADF, to debug
> crbug.com/452121 .
> Could you add TODO(sergeyu) to remove it once investigation is complete?

Not sure we have anything official on it, but it's generally best not to use it
unless your'e investigating a particular bugs, or regressions are almost
inevitable, and would be too painful to catch otherwise (Like
AssertNoURLRequests in url_request)

[Sergey Ulanov, 2015-02-11 23:24:51]

On 2015/02/11 19:29:21, mmenke wrote:
> On 2015/02/11 19:25:35, mef wrote:
> > Matt, what is the policy / guideline on adding CHECK()?
> > 
> >
>
https://codereview.chromium.org/914853002/diff/20001/net/udp/udp_socket_libev...
> > File net/udp/udp_socket_libevent.cc (right):
> > 
> >
>
https://codereview.chromium.org/914853002/diff/20001/net/udp/udp_socket_libev...
> > net/udp/udp_socket_libevent.cc:133: // Crash if errno=EBADF, to debug
> > crbug.com/452121 .
> > Could you add TODO(sergeyu) to remove it once investigation is complete?
> 
> Not sure we have anything official on it, but it's generally best not to use
it
> unless your'e investigating a particular bugs, or regressions are almost
> inevitable, and would be too painful to catch otherwise (Like
> AssertNoURLRequests in url_request)

Two other approaches that avoid adding this CHECK:
 1. Return a result from Close(). Then I can add CHECK() in the code that I
suspect triggers this problem.
 2. Add something like ReleaseConnectedSocket(), similar to net::SocketLibevent.
The I can call close() explicitly and verify the result.

WDYT?

[mef, 2015-02-12 16:51:48]

On 2015/02/11 23:24:51, Sergey Ulanov wrote:
> On 2015/02/11 19:29:21, mmenke wrote:
> > On 2015/02/11 19:25:35, mef wrote:
> > > Matt, what is the policy / guideline on adding CHECK()?
> > > 
> > >
> >
>
https://codereview.chromium.org/914853002/diff/20001/net/udp/udp_socket_libev...
> > > File net/udp/udp_socket_libevent.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/914853002/diff/20001/net/udp/udp_socket_libev...
> > > net/udp/udp_socket_libevent.cc:133: // Crash if errno=EBADF, to debug
> > > crbug.com/452121 .
> > > Could you add TODO(sergeyu) to remove it once investigation is complete?
> > 
> > Not sure we have anything official on it, but it's generally best not to use
> it
> > unless your'e investigating a particular bugs, or regressions are almost
> > inevitable, and would be too painful to catch otherwise (Like
> > AssertNoURLRequests in url_request)
> 
> Two other approaches that avoid adding this CHECK:
>  1. Return a result from Close(). Then I can add CHECK() in the code that I
> suspect triggers this problem.
I like this. It seems that other methods of UDPSocketLibevent return int, so it
will be consistent.
It will also allow us to limit CHECK to suspect scenario as opposed to all
possibly innocuous failures.

>  2. Add something like ReleaseConnectedSocket(), similar to
net::SocketLibevent.
> The I can call close() explicitly and verify the result.
> 
> WDYT?

[Sergey Ulanov, 2015-02-12 19:05:01]

Done. PTAL. Unfortunately returning result from Close() makes this CL bigger as
I had to update every class that overrides Close().

[mef, 2015-02-16 17:46:02]

lgtm

[mef, 2015-02-17 09:41:49]

Hi Ricardo,

this looks good to me, but I would appreciate if you could take a look and
comment if this breaks some assumptions.

thanks,
-m

[rvargas (doing something else), 2015-02-17 19:12:53]

On 2015/02/17 09:41:49, mef wrote:
> Hi Ricardo,
> 
> this looks good to me, but I would appreciate if you could take a look and
> comment if this breaks some assumptions.
> 
> thanks,
> -m

It's better to CHECK in close. That is the standard behavior on all platforms
for base stuff (see scoped_file.cc and
http://www.chromium.org/developers/crash-reports/crash-with-invalid-handle). I
think net should align better with that instead of diverging.

[Sergey Ulanov, 2015-02-17 23:53:02]

On 2015/02/17 19:12:53, rvargas wrote:
> On 2015/02/17 09:41:49, mef wrote:
> > Hi Ricardo,
> > 
> > this looks good to me, but I would appreciate if you could take a look and
> > comment if this breaks some assumptions.
> > 
> > thanks,
> > -m
> 
> It's better to CHECK in close. That is the standard behavior on all platforms
> for base stuff (see scoped_file.cc and
> http://www.chromium.org/developers/crash-reports/crash-with-invalid-handle). I
> think net should align better with that instead of diverging.

Done in crrev.com/316668. Thanks. Closing this CL.

[mef, 2015-02-18 10:51:32]

On 2015/02/17 23:53:02, Sergey Ulanov wrote:
> On 2015/02/17 19:12:53, rvargas wrote:
> > On 2015/02/17 09:41:49, mef wrote:
> > > Hi Ricardo,
> > > 
> > > this looks good to me, but I would appreciate if you could take a look and
> > > comment if this breaks some assumptions.
> > > 
> > > thanks,
> > > -m
> > 
> > It's better to CHECK in close. That is the standard behavior on all
platforms
> > for base stuff (see scoped_file.cc and
> > http://www.chromium.org/developers/crash-reports/crash-with-invalid-handle).
I
> > think net should align better with that instead of diverging.
> 
> Done in crrev.com/316668. Thanks. Closing this CL.

Ricardo, thanks! Sergey, sorry for pushing you to a long and incorrect behavior.