Extend the use of IOBuffers to the code underneath...

Extend the use of IOBuffers to the code underneath
HttpNetworkTransaction (to the Socket class).

This is the first step to remove the blocking call on the
destructor of the network transaction, from IO thread.

BUG=9258
R=wtc


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

[rvargas (doing something else), 2009-04-22 22:20:26]

Avi, could you take a look at the changes on ssl_client_socket_mac (I'm doing
small changes to the logic of SSLReadCallback)?

[wtc, 2009-04-23 01:29:47]

I reviewed everything except ssl_client_socket_mac.cc.
I'm running out of steam.  I'll review it tomorrow morning.
Sorry.

I see the pattern of new IOBuffer(n) followed by a memcpy
in many places.  How about adding a new IOBuffer constructor
that takes care of the memcpy?

  IOBuffer::IOBuffer(const char* buffer, int buffer_size) {
    DCHECK(buffer_size > 0);
    data_ = new char[buffer_size];
    memcpy(data_, buffer, buffer_size);
  }

Let's meet with Darin tomorrow to brainstorm alternative
solutions that avoid memory allocations and copies.  For
now we have at least a working solution.

http://codereview.chromium.org/87073/diff/2001/2014
File net/base/socket.h (right):

http://codereview.chromium.org/87073/diff/2001/2014#newcode31
Line 31: virtual int Write(IOBuffer* buf, int buf_len,
It's unfortunate that we can't pass a const pointer to the
data.  We should add a comment to specify that the Write
method should not modify the data in |buf|.

Does it make sense to define a ConstIOBuffer class that
contains a |const char* data_| member?  Just thinking out
loud...

Why doesn't the IOBuffer class have a buffer_size_ member?

http://codereview.chromium.org/87073/diff/2001/2015
File net/base/ssl_client_socket_mac.h (right):

http://codereview.chromium.org/87073/diff/2001/2015#newcode97
Line 97: scoped_refptr<IOBuffer> read_buffer_;
Nit: add a comment because read_buffer_ and recv_buffer_
sound so similar.

http://codereview.chromium.org/87073/diff/2001/2009
File net/base/ssl_client_socket_win.cc (right):

http://codereview.chromium.org/87073/diff/2001/2009#newcode515
Line 515: memcpy(buf, transport_buf_->data(), std::min(result, buf_len));
Don't need to compute |buf_len|.  Just pass |result| as the
third argument to memcpy.

http://codereview.chromium.org/87073/diff/2001/2009#newcode748
Line 748: // We come directly from DoPayloadRead(). We have to transfer the data
from
Nit: remove "directly", which to me implies that we skip
the transport_->Read call in DoPayloadRead.

It took me a while to figure out why transport_buf_ can be
NULL, because we can also come from DoPayloadReadComplete.
Would be nice to explain that in the comment.

http://codereview.chromium.org/87073/diff/2001/2009#newcode752
Line 752: memcpy(buf, transport_buf_->data(), std::min(result, buf_len));
Don't need to compute |buf_len|.  Just pass |result| as the
third argument to memcpy.

http://codereview.chromium.org/87073/diff/2001/2011
File net/base/tcp_client_socket_libevent.h (right):

http://codereview.chromium.org/87073/diff/2001/2011#newcode74
Line 74: scoped_refptr<IOBuffer> read_buf_;
For TCPClientSocketLibevent, read_buf_ and write_buf_
can actually remain as plain pointers because libevent
is not doing async IO and therefore doesn't have a
reference to the buffer.

But I guess this would be too confusing and we would
only save one reference count...

http://codereview.chromium.org/87073/diff/2001/2005
File net/base/tcp_client_socket_unittest.cc (right):

http://codereview.chromium.org/87073/diff/2001/2005#newcode106
Line 106: scoped_refptr<net::IOBuffer> request_buffer =
It is a shame that we won't be able to send a static buffer
in a straightforward way.  Now we need to allocate a buffer
and do a memcpy.

I wonder if we should have a kind of IOBuffer that contains a pointer to
static/stack data and doesn't delete the buffer
in the destructor...

http://codereview.chromium.org/87073/diff/2001/2005#newcode235
Line 235: rv = sock_->Read(buf, 4096, &callback);
Nit: we should define 4096 as a constant.  It was OK in the
previous tests, but in this test and the next test, 4096
is used more times.

http://codereview.chromium.org/87073/diff/2001/2002
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/87073/diff/2001/2002#newcode787
Line 787: memcpy(buf, read_buffer_->data(), std::min(result, buf_len));
You don't need to compute |buf_len|; you can just pass
|result| as the third argument to memcpy.

[wtc, 2009-04-23 02:37:44]

Ricardo,

An idea just occurred to me while I was talking a walk
after work.

Assuming that TCPClientSocketLibevent doesn't have any
problem cancelling an in-progress IO operation, we can
localize the use of IOBuffer inside TCPClientSocketWin.
We don't need to change the API of Socket::Read() and
Socket::Write().

We create an IOBuffer in TCPClientSocketWin::Read() and
TCPClientSocketWin::Write(), and pass the IOBuffer instead
of the user buffer to WSARecv and WSASend.

What do you think of this idea?

[rvargas (doing something else), 2009-04-23 03:27:53]

On 2009/04/23 02:37:44, wtc wrote:
> Ricardo,
> 
> An idea just occurred to me while I was talking a walk
> after work.
> 
> Assuming that TCPClientSocketLibevent doesn't have any
> problem cancelling an in-progress IO operation, we can
> localize the use of IOBuffer inside TCPClientSocketWin.
> We don't need to change the API of Socket::Read() and
> Socket::Write().
> 
> We create an IOBuffer in TCPClientSocketWin::Read() and
> TCPClientSocketWin::Write(), and pass the IOBuffer instead
> of the user buffer to WSARecv and WSASend.
> 
> What do you think of this idea?

We could do that. It requires having a detachable object inside
TCPClienSocketWin that is actually listening for the notification from the
object watcher and owns the buffer passed to WSAxxx. When we want to close the
socket, we detach this object so that when the notification arrives all
resources are released.

It is a little weird having IOBuffers above and below the socket interface but
not on the socket interface, but it is doable.

From the point of view of memcpys, we will be adding one extra one on the
ReadBody path (all the others would move from the network transaction to the
socket layer).

[darin (slow to review), 2009-04-23 04:37:54]

Why don't we just switch TCPClientSocketWin over to use IO completion ports like
we did for FileStream?  Then we could use CancelIo +
MessagePumpForIO::WaitForIOCompletion just as we do in FileStream.

Or, is there a concern that this might exhibit a similar problem?

[Avi (use Gerrit), 2009-04-23 14:46:06]

The Mac side looks plausible, though it's been long enough that I'd need to take
the time to page in memories of how this all played together.

This passes all tests?

[rvargas (doing something else), 2009-04-23 17:23:22]

On 2009/04/23 04:37:54, darin wrote:
> Why don't we just switch TCPClientSocketWin over to use IO completion ports
like
> we did for FileStream?  Then we could use CancelIo +
> MessagePumpForIO::WaitForIOCompletion just as we do in FileStream.
> 
> Or, is there a concern that this might exhibit a similar problem?

WaitForIOCompletion() keeps retrieving notifications from the OS but stores them
to be dispatched later, to avoid re-entrancy issues. So we could have the same
problem there... so we should probably modify file_stream at some point to avoid
that problem (it seems like we just wait for a few milliseconds for the vast
majority of users... but I suspect that is the case also for this particular
wait. See AsyncIo Histograms on the dashboard).

[rvargas (doing something else), 2009-04-23 17:24:07]

On 2009/04/23 14:46:06, Avi wrote:
> The Mac side looks plausible, though it's been long enough that I'd need to
take
> the time to page in memories of how this all played together.
> 
> This passes all tests?
yes, it does.

[wtc, 2009-04-23 20:49:58]

Sorry about the delay.  I spent some time understanding the
code in this file so that I can understand the nontrivial
changes.

Avi, please note my first comment below and let me know
what you think.

http://codereview.chromium.org/87073/diff/2001/2018
File net/base/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/87073/diff/2001/2018#newcode692
Line 692: *data_length = std::min(total_read, *data_length);
So this change is to fix the bug that total_read may exceed
*data_length because we're passing
us->recv_buffer_tail_slop_, which may be greater than
*data_length - total_read, to the us->transport_->Read()
call above.

We can also fix this bug in another way, which requires
fewer changes.

We can pass
    *data_length - total_read
instead of
    us->recv_buffer_tail_slop_
to the us->transport_->Read() call above.  This will prevent
total_read from exceeding *data_length.

The reason I think this fix may be better is that I don't
know why we want to read more bytes from us->transport_ than
what we're asked to read.

http://codereview.chromium.org/87073/diff/2001/2018#newcode696
Line 696: // We have I/O in flight which is going to land in our buffer. We
can't
If you decide to allow total_read to exceed *data_length,
then we need to update this comment to reflect the new
test, which now also includes total_read != *data_length.

[rvargas (doing something else), 2009-04-24 00:04:27]

Thanks for all the comments.

I changed HttpNetworkTransaction significantly, to remove the memory copies
related to headers. I think that the change for uploads can wait until we
actually fix the real issue.

http://codereview.chromium.org/87073/diff/2001/2014
File net/base/socket.h (right):

http://codereview.chromium.org/87073/diff/2001/2014#newcode31
Line 31: virtual int Write(IOBuffer* buf, int buf_len,
On 2009/04/23 01:29:47, wtc wrote:
> It's unfortunate that we can't pass a const pointer to the
> data.  We should add a comment to specify that the Write
> method should not modify the data in |buf|.
> 
> Does it make sense to define a ConstIOBuffer class that
> contains a |const char* data_| member?  Just thinking out
> loud...

I agree it is nicer to have Write() interfaces that receive a const object. On
the other hand, 4 out of 5 of our implementations of this method remove the
const qualifier right away, so removing it from the interface doesn't bother me
that much :). I added the comment.

> Why doesn't the IOBuffer class have a buffer_size_ member?

That option was considered when the IOBuffer review was going on, but the desire
to keep an explicit size argument on the interfaces with IOBuffers was stronger.

http://codereview.chromium.org/87073/diff/2001/2018
File net/base/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/87073/diff/2001/2018#newcode696
Line 696: // We have I/O in flight which is going to land in our buffer. We
can't
On 2009/04/23 20:49:58, wtc wrote:
> If you decide to allow total_read to exceed *data_length,
> then we need to update this comment to reflect the new
> test, which now also includes total_read != *data_length.

I updated the comment, but I'll wait for Avi to say which way we want to go
here. They're both fine to me.

http://codereview.chromium.org/87073/diff/2001/2011
File net/base/tcp_client_socket_libevent.h (right):

http://codereview.chromium.org/87073/diff/2001/2011#newcode74
Line 74: scoped_refptr<IOBuffer> read_buf_;
On 2009/04/23 01:29:47, wtc wrote:
> For TCPClientSocketLibevent, read_buf_ and write_buf_
> can actually remain as plain pointers because libevent
> is not doing async IO and therefore doesn't have a
> reference to the buffer.
> 
> But I guess this would be too confusing and we would
> only save one reference count...

That's correct, but I agree that is simpler if we just add an extra reference
instead of keeping a pointer without accounting for it. By the way, if we don't
do this, SSLClientSocketNSS::BufferSend() will have to manually keep the buffer
alive when we return ERR_IO_PENDING from this code.

I added a comment in socket.h to make it part of the contract (as it is for
HttpTransaction).

http://codereview.chromium.org/87073/diff/2001/2005
File net/base/tcp_client_socket_unittest.cc (right):

http://codereview.chromium.org/87073/diff/2001/2005#newcode106
Line 106: scoped_refptr<net::IOBuffer> request_buffer =
On 2009/04/23 01:29:47, wtc wrote:
> It is a shame that we won't be able to send a static buffer
> in a straightforward way.  Now we need to allocate a buffer
> and do a memcpy.
> 
> I wonder if we should have a kind of IOBuffer that contains a pointer to
> static/stack data and doesn't delete the buffer
> in the destructor...

We can automate the process of doing the memcpy (as you suggested somewhere
else). I would prefer keep doing the extra allocation just to make sure that the
unit tests exercise the code that delete the buffer. (WrappedIOBuffer provides
the functionality you mention).

[Avi (use Gerrit), 2009-04-24 14:55:07]

This looks like it's _correct_, but it doesn't look very pleasant.

First, why are we passing around lengths in the Socket interface when IOBuffers
have built-in lengths? Don't say that you can't ask an IOBuffer what its length
is. I just looked, and the IOBuffer API is that limited. Fix it.

Second, IOBuffer is IMHO too dumb a wrapper. Why don't we add the concept of
resizing it? What if it had:

size_t getSize();
void resizeAtEnd(size_t);
void resizeAtFront(size_t);

Let us further suggest an extension to the Socket interface that would allow
giving a non-empty IOBuffer to Read, and Read would just append the incoming
data to the buffer.

Then the entire nightmare about head and tail slop just vanishes. SSLMac would
keep a rcv_buffer (an IOBuffer) and would pass it to the provider. We wouldn't
need tail slop because the provider would be capable of extending it for new
data (resizeAtEnd). We wouldn't need head slop because we could call
resizeAtFront and the provider would be able to get at the new front.

This change feels to me like a layer on top of a bad solution. The existing
design was only written this way because of the API that it had to live with.
The existing design is not sacred. If we change the API, let's do it in a way
that allows us to move to a better internal design.

http://codereview.chromium.org/87073/diff/2001/2018
File net/base/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/87073/diff/2001/2018#newcode692
Line 692: *data_length = std::min(total_read, *data_length);
On 2009/04/23 20:49:58, wtc wrote:
> The reason I think this fix may be better is that I don't
> know why we want to read more bytes from us->transport_ than
> what we're asked to read.

I like this answer better. Guess my code wasn't so perfect the first time 'round
:)

http://codereview.chromium.org/87073/diff/2001/2018#newcode696
Line 696: // We have I/O in flight which is going to land in our buffer. We
can't
wtc's right. We should cap the amount we read from the underlying provider at
what our caller asks.

[rvargas (doing something else), 2009-04-24 18:16:44]

On 2009/04/24 14:55:07, Avi wrote:
> This looks like it's _correct_, but it doesn't look very pleasant.
> 
> First, why are we passing around lengths in the Socket interface when
IOBuffers
> have built-in lengths? Don't say that you can't ask an IOBuffer what its
length
> is. I just looked, and the IOBuffer API is that limited. Fix it.
> 
> Second, IOBuffer is IMHO too dumb a wrapper. Why don't we add the concept of
> resizing it? What if it had:
> 
> size_t getSize();
> void resizeAtEnd(size_t);
> void resizeAtFront(size_t);
> 
> Let us further suggest an extension to the Socket interface that would allow
> giving a non-empty IOBuffer to Read, and Read would just append the incoming
> data to the buffer.
> 
> Then the entire nightmare about head and tail slop just vanishes. SSLMac would
> keep a rcv_buffer (an IOBuffer) and would pass it to the provider. We wouldn't
> need tail slop because the provider would be capable of extending it for new
> data (resizeAtEnd). We wouldn't need head slop because we could call
> resizeAtFront and the provider would be able to get at the new front.
> 
> This change feels to me like a layer on top of a bad solution. The existing

Having specializations of IOBuffer that implement methods needed by the code
that wants to manage the buffer in a particular way is totally fine. ResizeAtXX
can be implemented that way, by an IOBuffer defined by SSLMac. This CL is not
intended to refactor all the code that touches, and there is already a TODO to
refactor this code, so it is just natural to leave this specialization to that
task.

Having extra members on the base class so that consumers of the IOBuffer are
aware (and handle) details about where to write inside the buffer moves the
complexity out of this file  and into all implementations of Read & co. (ok, I'm
exaggerating a little) More important, it is not extensible: we have n layers of
code passing the buffer around, and one of those layers could also want to reuse
the same buffer to perform multiple operations. That's why I think that the base
class should not be aware of that reuse. The model (so far) is to have a simple
buffer to be used on *this* particular IO operation, while enabling transfer of
ownership of that buffer. Reuse of the buffer should be handled by the
originator of the operation. (See HttpNetworkTransaction on this CL).


> design was only written this way because of the API that it had to live with.
> The existing design is not sacred. If we change the API, let's do it in a way
> that allows us to move to a better internal design.
> 
> http://codereview.chromium.org/87073/diff/2001/2018
> File net/base/ssl_client_socket_mac.cc (right):
> 
> http://codereview.chromium.org/87073/diff/2001/2018#newcode692
> Line 692: *data_length = std::min(total_read, *data_length);
> On 2009/04/23 20:49:58, wtc wrote:
> > The reason I think this fix may be better is that I don't
> > know why we want to read more bytes from us->transport_ than
> > what we're asked to read.
> 
> I like this answer better. Guess my code wasn't so perfect the first time
'round
> :)
> 

I changed the code to fix the bug  this way.

> http://codereview.chromium.org/87073/diff/2001/2018#newcode696
> Line 696: // We have I/O in flight which is going to land in our buffer. We
> can't
> wtc's right. We should cap the amount we read from the underlying provider at
> what our caller asks.

[Avi (use Gerrit), 2009-04-24 18:27:53]

On 2009/04/24 18:16:44, rvargas wrote:
> Having specializations of IOBuffer that implement methods needed by the code
> that wants to manage the buffer in a particular way is totally fine.
ResizeAtXX
> can be implemented that way, by an IOBuffer defined by SSLMac.

Right, but that behavior needs to be extended down to TCPSocket, and since the
Socket interface exposed is general, it needs to spread out some more...

> Having extra members on the base class so that consumers of the IOBuffer are
> aware (and handle) details about where to write inside the buffer

Neither aware nor handle. Something like

IOBuffer() // constructs with zero contents
void Append(void*, len)
void* Data()
size_t Size()

leaves all knowledge of the buffer to the buffer.

> we have n layers of
> code passing the buffer around, and one of those layers could also want to
reuse
> the same buffer to perform multiple operations.

That's not clear to me. There's a clear contract between the layers as to who
owns the buffer.

Perhaps I'm not totally clear as to what role the buffer plays, but while a
simple sharing wrapper around an array works, and the code LGTM for commit, I
feel like we're not taking full advantage of what we could do.

[rvargas (doing something else), 2009-04-24 19:38:35]

On 2009/04/24 18:27:53, Avi wrote:
> On 2009/04/24 18:16:44, rvargas wrote:
> > Having specializations of IOBuffer that implement methods needed by the code
> > that wants to manage the buffer in a particular way is totally fine.
> ResizeAtXX
> > can be implemented that way, by an IOBuffer defined by SSLMac.
> 
> Right, but that behavior needs to be extended down to TCPSocket, and since the
> Socket interface exposed is general, it needs to spread out some more...
> 
> > Having extra members on the base class so that consumers of the IOBuffer are
> > aware (and handle) details about where to write inside the buffer
> 
> Neither aware nor handle. Something like
> 
> IOBuffer() // constructs with zero contents
> void Append(void*, len)
> void* Data()
> size_t Size()

If I understand correctly, the IO provider (socket) would be calling Append do
add some more data at the end of the buffer?. That implies that the IO actually
happened somewhere else, and that any IOBuffer can be extended by the user (and
that is not the case).

But in general you are right, we could add a lot of extra features to the
IOBuffer and simplify all the interfaces.

> 
> leaves all knowledge of the buffer to the buffer.
> 
> > we have n layers of
> > code passing the buffer around, and one of those layers could also want to
> reuse
> > the same buffer to perform multiple operations.
> 
> That's not clear to me. There's a clear contract between the layers as to who
> owns the buffer.
> 
> Perhaps I'm not totally clear as to what role the buffer plays, but while a
> simple sharing wrapper around an array works, and the code LGTM for commit, I
> feel like we're not taking full advantage of what we could do.

[wtc, 2009-04-24 22:28:44]

LGTM.

http://codereview.chromium.org/87073/diff/3025/2072
File net/base/socket.h (right):

http://codereview.chromium.org/87073/diff/3025/2072#newcode23
Line 23: // immediately, the socket must acquire a reference to the provided
buffer.
Nit: "must acquire" => "acquires".

Please also document when the socket releases that reference.  The reference is
released right before the completion callback
runs.  But when a pending IO is cancelled, it is harder to
describe when the reference is released...

http://codereview.chromium.org/87073/diff/3025/2072#newcode34
Line 34: // buffer that is written.
Nit: "written" => "written to the socket" to avoid the
confusion that the buffer is overwritten.

http://codereview.chromium.org/87073/diff/3025/2073
File net/base/ssl_client_socket_mac.h (right):

http://codereview.chromium.org/87073/diff/3025/2073#newcode102
Line 102: scoped_refptr<IOBuffer> read_buffer_;
Nit: perhaps we can rename this member read_io_buffer_ to
suggest that it is an IOBuffer?

http://codereview.chromium.org/87073/diff/3025/2061
File net/http/http_network_transaction.h (right):

http://codereview.chromium.org/87073/diff/3025/2061#newcode63
Line 63: ~RequestHeaders() { data_ = NULL; }
The destructor doesn't really need to reset data_ to NULL,
right?

http://codereview.chromium.org/87073/diff/3025/2061#newcode65
Line 65: void set_data(size_t offset) {
Should we rename this method set_offset or increment_data?

Since IOBuffer already has the data() method, the name
"set_data" strongly suggests that data() should return
what set_data() sets, but that's not the case.

http://codereview.chromium.org/87073/diff/3025/2061#newcode68
Line 68: std::string& headers() { return headers_; }
This accessor is unusual -- it is effectively both a getter
and setter.  I wonder if we should have a getter and setter,
or just have a public member |headers|.

http://codereview.chromium.org/87073/diff/3025/2061#newcode83
Line 83: char* headers() { return headers_.get(); }
Should this return const char*?

[rvargas (doing something else), 2009-04-28 00:03:01]

Thanks, the new version has arrived :)

http://codereview.chromium.org/87073/diff/3025/2072
File net/base/socket.h (right):

http://codereview.chromium.org/87073/diff/3025/2072#newcode23
Line 23: // immediately, the socket must acquire a reference to the provided
buffer.
On 2009/04/24 22:28:44, wtc wrote:
> Nit: "must acquire" => "acquires".
> 
> Please also document when the socket releases that reference.  The reference
is
> released right before the completion callback
> runs.  But when a pending IO is cancelled, it is harder to
> describe when the reference is released...

there is no cancellation, so the reference lives until the callback is called in
any case.

http://codereview.chromium.org/87073/diff/3025/2061
File net/http/http_network_transaction.h (right):

http://codereview.chromium.org/87073/diff/3025/2061#newcode63
Line 63: ~RequestHeaders() { data_ = NULL; }
On 2009/04/24 22:28:44, wtc wrote:
> The destructor doesn't really need to reset data_ to NULL,
> right?

It does, otherwise the destructor of the base class will attempt to delete a
random value.

http://codereview.chromium.org/87073/diff/3025/2061#newcode65
Line 65: void set_data(size_t offset) {
On 2009/04/24 22:28:44, wtc wrote:
> Should we rename this method set_offset or increment_data?
> 
> Since IOBuffer already has the data() method, the name
> "set_data" strongly suggests that data() should return
> what set_data() sets, but that's not the case.

The original intent wast to have set_data(void) to make data_ point to the
current string... but then having an optional offset is more useful. What about
SetDataOffset()?

http://codereview.chromium.org/87073/diff/3025/2061#newcode68
Line 68: std::string& headers() { return headers_; }
On 2009/04/24 22:28:44, wtc wrote:
> This accessor is unusual -- it is effectively both a getter
> and setter.  I wonder if we should have a getter and setter,
> or just have a public member |headers|.

To me, having public getters and setters is pretty much the same as making the
member public and letting the users modify it. I'll go ahead with removing the
accessor and making header_ public; the idea is grant the transaction free
access to it anyway.

http://codereview.chromium.org/87073/diff/3025/2061#newcode83
Line 83: char* headers() { return headers_.get(); }
On 2009/04/24 22:28:44, wtc wrote:
> Should this return const char*?

DidReadResponseHeaders() manipulates the buffer directly. I can add some
const_casts around to fix any issue that arises, but being this a private class
I don't know if that really gives us anything. Let me know if you want me to do
that.

[wtc, 2009-04-28 00:50:35]

Let me answer your questions first.

I'll review the new Patch Set next.

http://codereview.chromium.org/87073/diff/3025/2072
File net/base/socket.h (right):

http://codereview.chromium.org/87073/diff/3025/2072#newcode23
Line 23: // immediately, the socket must acquire a reference to the provided
buffer.
On 2009/04/28 00:03:01, rvargas wrote:
>
> there is no cancellation, so the reference lives until the callback is called
in
> any case.

When we destroy a Socket, any pending callback won't be
called.  That's what I meant by a cancellation.

If we destroy a Socket with a pending IO, the reference
lives until the destructor returns, right?

http://codereview.chromium.org/87073/diff/3025/2061
File net/http/http_network_transaction.h (right):

http://codereview.chromium.org/87073/diff/3025/2061#newcode63
Line 63: ~RequestHeaders() { data_ = NULL; }
On 2009/04/28 00:03:01, rvargas wrote:
>
> It does, otherwise the destructor of the base class will attempt to delete a
> random value.

Thank you for explaining this to me.  I forgot how it works.

http://codereview.chromium.org/87073/diff/3025/2061#newcode65
Line 65: void set_data(size_t offset) {
On 2009/04/28 00:03:01, rvargas wrote:
>What about SetDataOffset()?

SetDataOffset() or set_data_offset() is fine.

http://codereview.chromium.org/87073/diff/3025/2061#newcode83
Line 83: char* headers() { return headers_.get(); }
On 2009/04/28 00:03:01, rvargas wrote:
> 
> DidReadResponseHeaders() manipulates the buffer directly. I can add some
> const_casts around to fix any issue that arises, but being this a private
class
> I don't know if that really gives us anything. Let me know if you want me to
do
> that.

No, this is fine.  I guess we could also just make the
headers_ member of this class a public member...

[wtc, 2009-04-30 01:16:30]

LGTM.

I actually reviewed Patch Set 5 and gave it LGTM,
but it's not shown.  I guess I must have forgotten
to push the "Publish" button.  Sorry about that.

I just reviewed the diffs between Patch Set 5 and
Patch Set 6, and they are exactly as what we
discussed.