Change URLRequest to use a ref-counted buffer for actual IO....

Change URLRequest to use a ref-counted buffer for actual IO.The ref-counting will prevent the deletion / reuse of memorywhile the buffer is actually being used by pending IO.This seems a very intrusive change, but at least we will be ableto make sure that it works without having to chase every singledestruction of an URLRequest to make sure that any pending IOwas cancelled, and also allows us to avoid blocking onthe object destruction.BUG=5325
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=8603

[rvargas (doing something else), 2009-01-21 03:54:04]

Matt, please take a look at the Gears related files.

[darin (slow to review), 2009-01-21 17:27:12]

good stuff!

http://codereview.chromium.org/18390/diff/258/295
File chrome/browser/chrome_plugin_host.cc (right):

http://codereview.chromium.org/18390/diff/258/295#newcode168
Line 168: CHECK(!my_buffer_.get());
why CHECK and not DCHECK?

http://codereview.chromium.org/18390/diff/258/295#newcode174
Line 174: memcpy(dest, my_buffer_->buffer(), *bytes_read);
perhaps we should explore a change to the chrome plugin api to avoid this copy?

http://codereview.chromium.org/18390/diff/258/324
File chrome/browser/dom_ui/chrome_url_data_manager.cc (right):

http://codereview.chromium.org/18390/diff/258/324#newcode281
Line 281: if (pending_buf_.get()) {
nit: .get() isn't needed here for scoped_refptr types.  it is OK to call it, but
we often do not.  up to you if you want to change this and other similar places.

http://codereview.chromium.org/18390/diff/258/296
File chrome/browser/net/url_fetcher.cc (right):

http://codereview.chromium.org/18390/diff/258/296#newcode16
Line 16: const int kBufferSize = 4096;
^ static

http://codereview.chromium.org/18390/diff/258/297
File chrome/browser/net/url_fetcher.h (right):

http://codereview.chromium.org/18390/diff/258/297#newcode188
Line 188: scoped_refptr<net::IOBuffer> buffer_;  // Read buffer
nit: it looks like some effort was made by the original author to line up these
comments, but this change is breaking that.  one fix would be to name the
variable buf_ ;-)

http://codereview.chromium.org/18390/diff/258/306
File chrome/browser/renderer_host/async_resource_handler.cc (right):

http://codereview.chromium.org/18390/diff/258/306#newcode14
Line 14: public:
nit: indent by one space

http://codereview.chromium.org/18390/diff/258/306#newcode19
Line 19: return reinterpret_cast<char*>(shared_memory_.memory());
instead of a virtual buffer() method, initialize buffer_ to
shared_memory_.memory() in the constructor and in your destructor set buffer_ to
null.

http://codereview.chromium.org/18390/diff/258/306#newcode24
Line 24: private:
nit: indent by one space

http://codereview.chromium.org/18390/diff/258/306#newcode74
Line 74: kReadBufSize))
nit: indentation is wrong

http://codereview.chromium.org/18390/diff/258/306#newcode119
Line 119: read_buffer_.swap(&spare_read_buffer_);
nit: indentation

http://codereview.chromium.org/18390/diff/258/311
File chrome/browser/renderer_host/buffered_resource_handler.cc (right):

http://codereview.chromium.org/18390/diff/258/311#newcode169
Line 169: CHECK(bytes_read + bytes_read_ < read_buffer_size_);
why CHECK and not DCHECK?

http://codereview.chromium.org/18390/diff/258/294
File net/base/completion_callback.h (right):

http://codereview.chromium.org/18390/diff/258/294#newcode45
Line 45: // Increases the ref counters of the given buffer and the callback.
hmm... i'm not sure why these are needed.  why does the callback need to own the
IOBuffer?

http://codereview.chromium.org/18390/diff/258/283
File net/base/io_buffer.h (right):

http://codereview.chromium.org/18390/diff/258/283#newcode12
Line 12: class IOBuffer : public base::RefCounted<IOBuffer> {
I suspect that we'll probably want this to be RefCountedThreadSafe.

http://codereview.chromium.org/18390/diff/258/283#newcode20
Line 20: delete[] buffer_;
nit: no need to null check prior to calling delete[]... however, perhaps you
should just make buffer_ be a scoped_array<char> instead.

http://codereview.chromium.org/18390/diff/258/283#newcode23
Line 23: virtual char* buffer() { return buffer_; }
why virtual?  all subclasses have a buffer_ member, so it seems that this method
should always just return the buffer_ member.  making it non-virtual means that
subclasses have to initialize buffer_ from within their constructor, but that
seems OK right?

if this is going to be virtual, then it is named improperly.  unix_hacker naming
should only be for cheap, inline functions.  a virtual function by definition
doesn't have that guarantee since the subclass could make it expensive.

naming suggestion: perhaps this accessor should be called "data", so that you
don't have code like "buffer.buffer()" as it would be tempting to name an object
of type IOBuffer buffer.  plus, it is common for STL structures to reveal their
data via a "data" accessor.  same goes for the buffer_ member... maybe just name
it data_.

http://codereview.chromium.org/18390/diff/258/283#newcode24
Line 24: 
it might have also made sense for IOBuffer to know its size.  is there a reason
why you didn't do that?  i wonder...  it seems like it might simplify some
things.  i guess if we gave it a size member, folks might get confused between
the buffer's capacity and its actual number of relevant bytes.  hmm...

http://codereview.chromium.org/18390/diff/258/283#newcode25
Line 25: protected:
nit: indent by one space

http://codereview.chromium.org/18390/diff/258/290
File net/http/http_cache.cc (right):

http://codereview.chromium.org/18390/diff/258/290#newcode444
Line 444: cache_read_callback_->UseBuffer(buf);  // Balanced in
OnCacheReadCompleted
why do we need the extra reference to the IOBuffer here?  it seems like we
should be passing IOBuffer down to the disk cache layer, and the disk cache
layer should be the one to hold a reference to the IOBuffer until the completion
callback fires.

http://codereview.chromium.org/18390/diff/258/287
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/18390/diff/258/287#newcode146
Line 146: read_buf_ = buf->buffer();
don't we also need to take an owning reference to IOBuffer here?  i think that
read_buf_ should change to be a scoped_refptr<IOBuffer>, and we should just
assign buf to it.  that way, we know that IOBuffer will remain valid until we
clear read_buf_.

http://codereview.chromium.org/18390/diff/258/293
File net/http/http_transaction.h (right):

http://codereview.chromium.org/18390/diff/258/293#newcode70
Line 70: virtual int Read(IOBuffer* buf, int buf_len,
please add a note here about the fact that the HttpTransaction should take a
reference to buf if it is not going to complete the Read operation
synchronously.

http://codereview.chromium.org/18390/diff/258/274
File net/url_request/mime_sniffer_proxy.cc (right):

http://codereview.chromium.org/18390/diff/258/274#newcode9
Line 9: const int kBufferSize = 1024;
^ static

http://codereview.chromium.org/18390/diff/258/268
File net/url_request/url_request.h (right):

http://codereview.chromium.org/18390/diff/258/268#newcode373
Line 373: // and an asynchronous Read is initiated.  The caller guarantees the
this sentence is no longer accurate.  the caller does not guarantee the lifetime
of the buffer.  instead, the Read call is responsible for taking a reference to
the given buf.

http://codereview.chromium.org/18390/diff/258/268#newcode380
Line 380: // initiated, the caller guarantees availability of this buffer until
some repetition here... probably worth cleaning up so we only mention the
ownership of buf once.

http://codereview.chromium.org/18390/diff/258/278
File net/url_request/url_request_unittest.h (right):

http://codereview.chromium.org/18390/diff/258/278#newcode34
Line 34: const int kBufferSize = 4096;
this header file is included by a lot of unit tests.  i'd worry about
kBufferSize colliding with other constants that unit tests might wish to define.
 can it be changed to just be a member of the TestDelegate class?

[eroman, 2009-01-21 19:45:35]

Have you considered adding a size() method to IOBuffer?
And then removing the |buf_len| parameter from all the consumers of IOBuffer.

[Matt Perry, 2009-01-21 21:42:00]

http://codereview.chromium.org/18390/diff/258/295
File chrome/browser/chrome_plugin_host.cc (right):

http://codereview.chromium.org/18390/diff/258/295#newcode174
Line 174: memcpy(dest, my_buffer_->buffer(), *bytes_read);
On 2009/01/21 17:27:13, darin wrote:
> perhaps we should explore a change to the chrome plugin api to avoid this
copy?

I don't think this is worth doing, since we'd just be pushing the copy further
into gears.  Eventually a copy is made of the data.

[rvargas (doing something else), 2009-01-21 22:19:28]

Thanks for all the comments.

I made some comments here. I'll be making the code changes so please let me know
if what I'm saying doesn't make sense :)

http://codereview.chromium.org/18390/diff/258/295
File chrome/browser/chrome_plugin_host.cc (right):

http://codereview.chromium.org/18390/diff/258/295#newcode168
Line 168: CHECK(!my_buffer_.get());
On 2009/01/21 17:27:13, darin wrote:
> why CHECK and not DCHECK?

There are a lot of CHECKS instead of DCHECKS in this file, when the code is
executed as a consequence of a plug-in action. This function is called from
CPR_Read so I'm just trying to be consistent... of course, I can change it :)

http://codereview.chromium.org/18390/diff/258/295#newcode174
Line 174: memcpy(dest, my_buffer_->buffer(), *bytes_read);
On 2009/01/21 17:27:13, darin wrote:
> perhaps we should explore a change to the chrome plugin api to avoid this
copy?

Yes, I talked about that with Matt. This seemed to be simpler. (I don't think we
want c++ interfaces between dlls)

http://codereview.chromium.org/18390/diff/258/324
File chrome/browser/dom_ui/chrome_url_data_manager.cc (right):

http://codereview.chromium.org/18390/diff/258/324#newcode281
Line 281: if (pending_buf_.get()) {
On 2009/01/21 17:27:13, darin wrote:
> nit: .get() isn't needed here for scoped_refptr types.  it is OK to call it,
but
> we often do not.  up to you if you want to change this and other similar
places.

Yes, I wanted to be explicit about it (although I was not very consistent)

http://codereview.chromium.org/18390/diff/258/306
File chrome/browser/renderer_host/async_resource_handler.cc (right):

http://codereview.chromium.org/18390/diff/258/306#newcode19
Line 19: return reinterpret_cast<char*>(shared_memory_.memory());
On 2009/01/21 17:27:13, darin wrote:
> instead of a virtual buffer() method, initialize buffer_ to
> shared_memory_.memory() in the constructor and in your destructor set buffer_
to
> null.

That was the original idea but in this case the memory was not mapped on the
constructor. I'll fix it.

http://codereview.chromium.org/18390/diff/258/311
File chrome/browser/renderer_host/buffered_resource_handler.cc (right):

http://codereview.chromium.org/18390/diff/258/311#newcode169
Line 169: CHECK(bytes_read + bytes_read_ < read_buffer_size_);
On 2009/01/21 17:27:13, darin wrote:
> why CHECK and not DCHECK?

There is already a DCHECK on OnWillRead that will cover the normal case of
triggering this one (developing another handler that allocates a small
buffer)... If that was not hit, this one will prevent an exploitable buffer
overflow on the browser.

http://codereview.chromium.org/18390/diff/258/294
File net/base/completion_callback.h (right):

http://codereview.chromium.org/18390/diff/258/294#newcode45
Line 45: // Increases the ref counters of the given buffer and the callback.
On 2009/01/21 17:27:13, darin wrote:
> hmm... i'm not sure why these are needed.  why does the callback need to own
the
> IOBuffer?

It doesn't really has to own it, but using a cancelable callback implies that
the owner wants to go away before the callback is fired so it is natural to put
all the cleanup together. This is really the bug, that this class allows
canceling something without taking care of all the things that must be alive
while the work is in progress.

http://codereview.chromium.org/18390/diff/258/283
File net/base/io_buffer.h (right):

http://codereview.chromium.org/18390/diff/258/283#newcode12
Line 12: class IOBuffer : public base::RefCounted<IOBuffer> {
On 2009/01/21 17:27:13, darin wrote:
> I suspect that we'll probably want this to be RefCountedThreadSafe.

I don't think we have to do that. The buffer itself is not thread safe so it's
probably better if we are explicit about not providing any extra thread-safety
with this class. I think all users are already aware of using this from only one
thread.

http://codereview.chromium.org/18390/diff/258/283#newcode24
Line 24: 
On 2009/01/21 17:27:13, darin wrote:
> it might have also made sense for IOBuffer to know its size.  is there a
reason
> why you didn't do that?  i wonder...  it seems like it might simplify some
> things.  i guess if we gave it a size member, folks might get confused between
> the buffer's capacity and its actual number of relevant bytes.  hmm...

Yes, I considered that but I didn't want to prevent users from using a given
IOBuffer telling somebody else that the size was x instead of the actual size
used during the allocation. At the end, I don't think anybody is using something
like that, and adding a size here will remove static ints for the callers (but
to be fair, they are all inlined by the compiler today instead of using memory).

The other thing was that it seemed useful to have a constructor that just
received a buffer (so maybe without explicit knowlegde of the size), but at the
end that is something that I think nobody ended up needing so I think I'll
remove it and add a size member. (the intended user was the shared memory
object).

http://codereview.chromium.org/18390/diff/258/290
File net/http/http_cache.cc (right):

http://codereview.chromium.org/18390/diff/258/290#newcode444
Line 444: cache_read_callback_->UseBuffer(buf);  // Balanced in
OnCacheReadCompleted
On 2009/01/21 17:27:13, darin wrote:
> why do we need the extra reference to the IOBuffer here?  it seems like we
> should be passing IOBuffer down to the disk cache layer, and the disk cache
> layer should be the one to hold a reference to the IOBuffer until the
completion
> callback fires.

That is also possible. After making everybody use IOBuffer, it seemed simpler to
fix the bug here instead of keep going down with it. See the comment on
CancelableCompletionCallback. We can always do that later, when removing the
blocking calls that we have to cancel pending io. This CL is too long already :)

[darin (slow to review), 2009-01-21 22:47:10]

http://codereview.chromium.org/18390/diff/258/295
File chrome/browser/chrome_plugin_host.cc (right):

http://codereview.chromium.org/18390/diff/258/295#newcode168
Line 168: CHECK(!my_buffer_.get());
OK, yeah... I do see a lot of CHECKs too.  I don't know why we are so
CHECK-happy in this file.  Probably trying to track down crashes.  It is fine to
use CHECK.  I just wanted to make sure we were CHECKing for a good reason.

http://codereview.chromium.org/18390/diff/258/295#newcode174
Line 174: memcpy(dest, my_buffer_->buffer(), *bytes_read);
It wouldn't have to be a C++ interface.  You could have a separate function that
the plugin implements that you would call to have the plugin free the buffer. 
Anyways, I agree that it is fine to leave this copy here.

http://codereview.chromium.org/18390/diff/258/306
File chrome/browser/renderer_host/async_resource_handler.cc (right):

http://codereview.chromium.org/18390/diff/258/306#newcode19
Line 19: return reinterpret_cast<char*>(shared_memory_.memory());
OK

http://codereview.chromium.org/18390/diff/258/311
File chrome/browser/renderer_host/buffered_resource_handler.cc (right):

http://codereview.chromium.org/18390/diff/258/311#newcode169
Line 169: CHECK(bytes_read + bytes_read_ < read_buffer_size_);
OK

http://codereview.chromium.org/18390/diff/258/294
File net/base/completion_callback.h (right):

http://codereview.chromium.org/18390/diff/258/294#newcode45
Line 45: // Increases the ref counters of the given buffer and the callback.
yeah, but it seems like the IOBuffer is now capable of being released
independently of everything else.  it seems strange to lump IOBuffer lifetime
with the callback lifetime.  there is no reason to release the IOBuffer
precisely at the same time as the callback is canceled.  it could safely happen
later.

http://codereview.chromium.org/18390/diff/258/283
File net/base/io_buffer.h (right):

http://codereview.chromium.org/18390/diff/258/283#newcode12
Line 12: class IOBuffer : public base::RefCounted<IOBuffer> {
the buffer is not thread-safe, but there are places in the code where the
IOBuffer gets passed between threads.  the URLFetcher has an IOBuffer member,
IOBuffers are passed to the DownloadFileManager, which as I recall lives on
another thread.  you basically do have cases where AddRef is happening on a
separate thread from Release, so I'm pretty sure you need to use thread-safe
reference counting.

http://codereview.chromium.org/18390/diff/258/283#newcode24
Line 24: 
i'm ok not having a size member, but if you do add it, maybe capacity would be a
better name.  that way it is a bit more clear that it is the capacity of the
buffer, and not necessarily the total number of useful bytes in the buffer.

in the shared memory case, i can see the single parameter constructor being
useful.  that assumes that you are able to do that and eliminate the virtual
buffer() call.

http://codereview.chromium.org/18390/diff/258/290
File net/http/http_cache.cc (right):

http://codereview.chromium.org/18390/diff/258/290#newcode444
Line 444: cache_read_callback_->UseBuffer(buf);  // Balanced in
OnCacheReadCompleted
OK... temporarily addrefing on behalf of the disk cache is fine, but I think it
would be very good to push the IOBuffer down into the disk cache.  anything that
asynchronously fills a buffer should make use of this IOBuffer class.

i would just do the addref of the IOBuffer manually here to avoid the need for
the UseBuffer / ReleaseBuffer methods.  it is also strange that those methods do
more than their names suggest (which is part of why i'm not too crazy about
them!)

[Matt Perry, 2009-01-21 23:02:11]

http://codereview.chromium.org/18390/diff/258/295
File chrome/browser/chrome_plugin_host.cc (right):

http://codereview.chromium.org/18390/diff/258/295#newcode168
Line 168: CHECK(!my_buffer_.get());
On 2009/01/21 22:47:10, darin wrote:
> OK, yeah... I do see a lot of CHECKs too.  I don't know why we are so
> CHECK-happy in this file.  Probably trying to track down crashes.  It is fine
to
> use CHECK.  I just wanted to make sure we were CHECKing for a good reason.

The rationale for using CHECKs was that this is a public plugin interface, and
we decided it was better to crash than warn if a plugin developer was misusing
the API.

[darin (slow to review), 2009-01-22 16:00:06]

> What do you think about adding capacity(), size() and set_size()
> members to IOBuffer and changing URLRequest from:
> bool Read(net::IOBuffer* buf, int max_bytes, int *bytes_read);
> to:
> bool Read(net::IOBuffer* buf);

I think I prefer your original solution.  IOBuffer is just a
reference counted "char*" that does not know its capacity or
size.

I think reporting the size via IOBuffer would be inconsistent
with the way we use CompletionCallback.  So, size/set_size are
out.

As for capacity, I'm on the fence with that one.  But, I think
having the callsite be explicit about the number of bytes the
user wishes to have read is good.

[rvargas (doing something else), 2009-01-23 01:05:50]

All previous comments should be addressed with this new version.

I changed IOBuffer to be RefCountedThreadSafe although to be  honest I don't
think that is really required. DownloadFileManager and SaveFileManager only pass
the buffer to another thread after the operation completes (from
OnReadCompleted) so nobody else should be holding a reference, and
URLFetcher::Core performs everything related to the request on the IO thread. If
there are simultaneous AddRef/Release that is a bug by itself. However, I could
see somebody in the future passing an IOBuffer to a worker thread and releasing
it from there.

Please take another look :)   (rev 4 was just a sync with tot)

[rvargas (doing something else), 2009-01-23 01:08:02]

On 2009/01/23 01:05:50, rvargas wrote:
> Please take another look :)   (rev 4 was just a sync with tot)

err... rev 5 was the sync one.

[Matt Perry, 2009-01-23 01:10:16]

lgtm (I only looked at the CPAPI-related changes)

[darin (slow to review), 2009-01-23 17:01:25]

http://codereview.chromium.org/18390/diff/195/576
File net/base/completion_callback.h (right):

http://codereview.chromium.org/18390/diff/195/576#newcode64
Line 64: CompletionCallbackImpl<T>::RunWithParams(params);
don't we need to release the buffer in this case too?

http://codereview.chromium.org/18390/diff/195/572
File net/http/http_cache.cc (right):

http://codereview.chromium.org/18390/diff/195/572#newcode445
Line 445: cache_read_callback_->UseBuffer(buf);
i still don't understand the UseBuffer / ReleaseBuffer stuff.  why should the
callback object know about buffers or manage the lifetime of them?

i think there should be a TODO about pushing the IOBuffer usage down into the
disk cache.

http://codereview.chromium.org/18390/diff/195/550
File net/url_request/url_request.h (right):

http://codereview.chromium.org/18390/diff/195/550#newcode377
Line 377: // The buf parameter is a buffer to receive the data.  The caller must
take a
i don't understand this comment.  this makes it sound like the caller is
supposed to take an additional reference to IOBuffer, but for what reason?  i
think the right wording is to say that the URLRequest will take a reference to
the IOBuffer if Read does not complete synchronously.  and then i would say that
the URLRequest will release that reference upon completion of the Read.

[rvargas (doing something else), 2009-01-23 19:08:19]

http://codereview.chromium.org/18390/diff/195/576
File net/base/completion_callback.h (right):

http://codereview.chromium.org/18390/diff/195/576#newcode64
Line 64: CompletionCallbackImpl<T>::RunWithParams(params);
On 2009/01/23 17:01:25, darin wrote:
> don't we need to release the buffer in this case too?

ReleaseBuffer() is called from the callback (in addition to Release(), so it is
symmetrical)

http://codereview.chromium.org/18390/diff/195/572
File net/http/http_cache.cc (right):

http://codereview.chromium.org/18390/diff/195/572#newcode445
Line 445: cache_read_callback_->UseBuffer(buf);
On 2009/01/23 17:01:25, darin wrote:
> i still don't understand the UseBuffer / ReleaseBuffer stuff.  why should the
> callback object know about buffers or manage the lifetime of them?
> 
> i think there should be a TODO about pushing the IOBuffer usage down into the
> disk cache.

I have the TODO on completion_callback.h to remove the interface.

http://codereview.chromium.org/18390/diff/195/550
File net/url_request/url_request.h (right):

http://codereview.chromium.org/18390/diff/195/550#newcode377
Line 377: // The buf parameter is a buffer to receive the data.  The caller must
take a
On 2009/01/23 17:01:25, darin wrote:
> i don't understand this comment.  this makes it sound like the caller is
> supposed to take an additional reference to IOBuffer, but for what reason?  i
> think the right wording is to say that the URLRequest will take a reference to
> the IOBuffer if Read does not complete synchronously.  and then i would say
that
> the URLRequest will release that reference upon completion of the Read.

I was trying to state the obvious: that we are not supposed to receive a buffer
with the refcount on 0. I hope it looks better now.

[darin (slow to review), 2009-01-23 20:48:47]

LGTM

http://codereview.chromium.org/18390/diff/195/572
File net/http/http_cache.cc (right):

http://codereview.chromium.org/18390/diff/195/572#newcode445
Line 445: cache_read_callback_->UseBuffer(buf);
OK