DCHECK -> CHECK for UDPSocket Read/Write calls when previouse in progress.

DCHECK -> CHECK for UDPSocket Read/Write calls when previous in progress.
Crashes in net::AssertEventNotSignaled() are hard reproduce, but looks like this is the primary reason.
With crash on CHECK(..._callback_.is_null()) we could identify incorrect code.

BUG=415753
Committed: https://crrev.com/17ee0af873389eba94ebe7629a44045ecf0b681a
Cr-Commit-Position: refs/heads/master@{#296234}

[Vitaly Buka (NO REVIEWS), 2014-09-19 06:46:34]

vitalybuka@chromium.org changed reviewers:
+ sergeyu@chromium.org, wtc@chromium.org

[Sergey Ulanov, 2014-09-19 20:09:09]

For this particular crash the bug is obviously in MDnsConnection::SocketHandler.
MDnsConnection::SocketHandler::Send() just calls Socket::Send() but there is
nothing in there to make sure that the previous Send() has completed.
net::Socket does not support multiple concurrent Send() requests.

[Vitaly Buka (NO REVIEWS), 2014-09-19 20:11:26]

That's true. CHnage already on review https://codereview.chromium.org/581813004/

Would not be just more obvious to crash on this CHECK and not on event?

On 2014/09/19 20:09:09, Sergey Ulanov wrote:
> For this particular crash the bug is obviously in
MDnsConnection::SocketHandler.
> MDnsConnection::SocketHandler::Send() just calls Socket::Send() but there is
> nothing in there to make sure that the previous Send() has completed.
> net::Socket does not support multiple concurrent Send() requests.

[Vitaly Buka (NO REVIEWS), 2014-09-19 20:16:25]

I am asking because I already noticed similar crashes, but blamed issue like
https://code.google.com/p/chromium/issues/detail?id=121085, until recently I
realized that the event is reset in posted task.

On 2014/09/19 20:11:26, Vitaly Buka wrote:
> That's true. CHnage already on review
https://codereview.chromium.org/581813004/
> 
> Would not be just more obvious to crash on this CHECK and not on event?
> 
> On 2014/09/19 20:09:09, Sergey Ulanov wrote:
> > For this particular crash the bug is obviously in
> MDnsConnection::SocketHandler.
> > MDnsConnection::SocketHandler::Send() just calls Socket::Send() but there is
> > nothing in there to make sure that the previous Send() has completed.
> > net::Socket does not support multiple concurrent Send() requests.

[Sergey Ulanov, 2014-09-20 00:36:26]

On 2014/09/19 20:16:25, Vitaly Buka wrote:
> I am asking because I already noticed similar crashes, but blamed issue like
> https://code.google.com/p/chromium/issues/detail?id=121085, until recently I
> realized that the event is reset in posted task.

I see. Makes sense. LGTM

[Vitaly Buka (NO REVIEWS), 2014-09-22 19:19:14]

vitalybuka@chromium.org changed reviewers:
+ rch@chromium.org

[Ryan Hamilton, 2014-09-22 20:52:46]

Is this CL necessary if https://codereview.chromium.org/581813004/ lands?

[Vitaly Buka (NO REVIEWS), 2014-09-22 23:00:40]

This CL would just help to have more meaningful crashes for bugs like this.
mDns is the top source of such crashes today but is not the only one.

On 2014/09/22 20:52:46, Ryan Hamilton wrote:
> Is this CL necessary if https://codereview.chromium.org/581813004/ lands?

[wtc, 2014-09-23 14:05:19]

Patch set 1 LGTM. This change seems reasonable.

Note that I am no longer a net owner, so you should
wait for Ryan's approval.

The CL's description has a spelling error: indentify

[Ryan Hamilton, 2014-09-23 14:11:32]

lgtm

[Vitaly Buka (NO REVIEWS), 2014-09-23 17:36:58]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-09-23 17:59:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/580163003/1

[commit-bot: I haz the power, 2014-09-23 20:59:21]

Committed patchset #1 (id:1) as d6dc083f1e0fa63176541673020a887cd528eaba

[commit-bot: I haz the power, 2014-09-23 21:00:17]

Patchset 1 (id:??) landed as
https://crrev.com/17ee0af873389eba94ebe7629a44045ecf0b681a
Cr-Commit-Position: refs/heads/master@{#296234}

[wtc, 2014-09-24 15:10:07]

https://codereview.chromium.org/580163003/diff/1/net/udp/udp_socket_libevent.cc
File net/udp/udp_socket_libevent.cc (right):

https://codereview.chromium.org/580163003/diff/1/net/udp/udp_socket_libevent....
net/udp/udp_socket_libevent.cc:173: CHECK(read_callback_.is_null());

Ryan: I think it would be better to add a new net error
code, say ERR_BUSY or ERR_IN_USE, and make these functions
return that error code rather than crashing with a CHECK
failure:

  if (!read_callback_.is_null())
    return ERR_BUSY;

[Ryan Hamilton, 2014-09-24 15:40:19]

Sure, that'd probably be better... Though that wouldn't help track down the
places where people are reading while reading. But not crashing is always
nice :>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Vitaly Buka (NO REVIEWS), 2014-09-24 16:59:13]

On 2014/09/24 15:40:19, Ryan Hamilton wrote:
> Sure, that'd probably be better... Though that wouldn't help track down the
> places where people are reading while reading. But not crashing is always
> nice :>
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

This case was hard to reproduce for mDns.
Without crashing in prod we'd probably didn't notice that.

[Vitaly Buka (NO REVIEWS), 2014-09-24 22:20:20]

On 2014/09/24 15:10:07, wtc wrote:
>
https://codereview.chromium.org/580163003/diff/1/net/udp/udp_socket_libevent.cc
> File net/udp/udp_socket_libevent.cc (right):
> 
>
https://codereview.chromium.org/580163003/diff/1/net/udp/udp_socket_libevent....
> net/udp/udp_socket_libevent.cc:173: CHECK(read_callback_.is_null());
> 
> Ryan: I think it would be better to add a new net error
> code, say ERR_BUSY or ERR_IN_USE, and make these functions
> return that error code rather than crashing with a CHECK
> failure:
> 
>   if (!read_callback_.is_null())
>     return ERR_BUSY;

maybe it's better to move QUEUE logic inside sockets
looks like all clients should implements something like this?

[Ryan Hamilton, 2014-09-25 03:05:13]

On 2014/09/24 22:20:20, Vitaly Buka wrote:
> On 2014/09/24 15:10:07, wtc wrote:
> >
>
https://codereview.chromium.org/580163003/diff/1/net/udp/udp_socket_libevent.cc
> > File net/udp/udp_socket_libevent.cc (right):
> > 
> >
>
https://codereview.chromium.org/580163003/diff/1/net/udp/udp_socket_libevent....
> > net/udp/udp_socket_libevent.cc:173: CHECK(read_callback_.is_null());
> > 
> > Ryan: I think it would be better to add a new net error
> > code, say ERR_BUSY or ERR_IN_USE, and make these functions
> > return that error code rather than crashing with a CHECK
> > failure:
> > 
> >   if (!read_callback_.is_null())
> >     return ERR_BUSY;
> 
> maybe it's better to move QUEUE logic inside sockets
> looks like all clients should implements something like this?

I don't think that is a good idea because it leads to potentially infinite
buffering within the socket, which consumes memory, and leads to non-performant
code.