Make sure read/write buffer alive in TCPSocketLibevent.

Make sure read/write buffer alive in TCPSocketLibevent.

SocketLibevent release ref-count of read/write buffer before calling callback.
Need to hold ref-count of buffer in TCPSocketLibevent to make sure the buffer
alive in callback.

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

[byungchul, 2014-07-11 16:54:57]

This is a fix for regression error. Please review.

[Ryan Sleevi, 2014-07-11 18:13:35]

https://codereview.chromium.org/382143007/diff/1/net/socket/tcp_socket_libeve...
File net/socket/tcp_socket_libevent.cc (right):

https://codereview.chromium.org/382143007/diff/1/net/socket/tcp_socket_libeve...
net/socket/tcp_socket_libevent.cc:530: buf->data());
Right, this is the unsafe access point.

https://codereview.chromium.org/382143007/diff/1/net/socket/tcp_socket_libeve...
File net/socket/tcp_socket_libevent.h (right):

https://codereview.chromium.org/382143007/diff/1/net/socket/tcp_socket_libeve...
net/socket/tcp_socket_libevent.h:160: void
WriteCompleted(scoped_refptr<IOBuffer> buf,
scoped-refptrs should always be const-ref

[byungchul, 2014-07-11 18:58:19]

https://codereview.chromium.org/382143007/diff/1/net/socket/tcp_socket_libeve...
File net/socket/tcp_socket_libevent.cc (right):

https://codereview.chromium.org/382143007/diff/1/net/socket/tcp_socket_libeve...
net/socket/tcp_socket_libevent.cc:530: buf->data());
On 2014/07/11 18:13:35, Ryan Sleevi wrote:
> Right, this is the unsafe access point.

Yes.

https://codereview.chromium.org/382143007/diff/1/net/socket/tcp_socket_libeve...
File net/socket/tcp_socket_libevent.h (right):

https://codereview.chromium.org/382143007/diff/1/net/socket/tcp_socket_libeve...
net/socket/tcp_socket_libevent.h:160: void
WriteCompleted(scoped_refptr<IOBuffer> buf,
On 2014/07/11 18:13:35, Ryan Sleevi wrote:
> scoped-refptrs should always be const-ref

Done.

[mmenke, 2014-07-11 19:01:41]

LGTM

[mmenke, 2014-07-11 19:01:43]

LGTM

[Ryan Sleevi, 2014-07-11 19:04:43]

lgtm

https://codereview.chromium.org/382143007/diff/20001/net/socket/tcp_socket_li...
File net/socket/tcp_socket_libevent.cc (right):

https://codereview.chromium.org/382143007/diff/20001/net/socket/tcp_socket_li...
net/socket/tcp_socket_libevent.cc:214: base::Unretained(this),
make_scoped_refptr(buf), callback));
This is where I would mention why you're making things a scoped-refptr

// Grab a reference to |buf| so that ReadCompleted() can still use
// it when Read() completes, as otherwise, this transfers ownership
// of buf to socket.

https://codereview.chromium.org/382143007/diff/20001/net/socket/tcp_socket_li...
File net/socket/tcp_socket_libevent.h (right):

https://codereview.chromium.org/382143007/diff/20001/net/socket/tcp_socket_li...
net/socket/tcp_socket_libevent.h:159: // alive when this callback is called.
I don't think either of these comments are needed/beneficial, at least within
the header.

[byungchul, 2014-07-11 21:32:50]

The CQ bit was checked by byungchul@chromium.org

[byungchul, 2014-07-11 21:33:27]

The CQ bit was unchecked by byungchul@chromium.org

[byungchul, 2014-07-11 21:35:52]

https://codereview.chromium.org/382143007/diff/20001/net/socket/tcp_socket_li...
File net/socket/tcp_socket_libevent.cc (right):

https://codereview.chromium.org/382143007/diff/20001/net/socket/tcp_socket_li...
net/socket/tcp_socket_libevent.cc:214: base::Unretained(this),
make_scoped_refptr(buf), callback));
On 2014/07/11 19:04:42, Ryan Sleevi wrote:
> This is where I would mention why you're making things a scoped-refptr
> 
> // Grab a reference to |buf| so that ReadCompleted() can still use
> // it when Read() completes, as otherwise, this transfers ownership
> // of buf to socket.

Done.

https://codereview.chromium.org/382143007/diff/20001/net/socket/tcp_socket_li...
File net/socket/tcp_socket_libevent.h (right):

https://codereview.chromium.org/382143007/diff/20001/net/socket/tcp_socket_li...
net/socket/tcp_socket_libevent.h:159: // alive when this callback is called.
On 2014/07/11 19:04:43, Ryan Sleevi wrote:
> I don't think either of these comments are needed/beneficial, at least within
> the header.

Moved to .cc

[byungchul, 2014-07-11 21:35:55]

The CQ bit was checked by byungchul@chromium.org

[commit-bot: I haz the power, 2014-07-11 21:37:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/byungchul@chromium.org/382143007/60001

[commit-bot: I haz the power, 2014-07-12 00:33:18]

Change committed as 282756