Fix a browser crash when the async resource handler keeps an...

Fix a browser crash when the async resource handler keeps an
IOBuffer after a failed attempt to send the data to a (now gone)
renderer. The shared memory section is unmapped so the buffer
must not be kept around and possibly re-used.

I'm also fixing a leak reported by purify.

BUG=7487
BUG=7374
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=9540

[rvargas (doing something else), 2009-02-10 22:06:10]

Now that we are at it, I'm also fixing a leak.

[wtc, 2009-02-10 22:37:51]

LGTM.  Please review my suggested changes below.

http://codereview.chromium.org/21222/diff/5/6
File chrome/browser/renderer_host/async_resource_handler.cc (right):

http://codereview.chromium.org/21222/diff/5/6#newcode106
Line 106: read_buffer_ = NULL;
I suggest that we repeat the "We just unmapped the memory."
comment here.

Is it safe to set read_buffer_ to NULL before the
GiveToProcess call?  That would avoid the duplication of
read_buffer_ = NULL inside and after the if statement.

Should we add a sentence to the block comment above
GiveToProcess in shared_memory.h, to stress the fact
that the shared memory segment is closed whether
GiveToProcess returns true or false?

http://codereview.chromium.org/21222/diff/5/6#newcode135
Line 135: spare_read_buffer_->Release();
Perhaps we should just define spare_read_buffer_ as
a scoped_refptr<SharedIOBuffer>?

[rvargas (doing something else), 2009-02-10 23:06:36]

On 2009/02/10 22:37:51, wtc wrote:
> LGTM.  Please review my suggested changes below.
> 
> http://codereview.chromium.org/21222/diff/5/6
> File chrome/browser/renderer_host/async_resource_handler.cc (right):
> 
> http://codereview.chromium.org/21222/diff/5/6#newcode106
> Line 106: read_buffer_ = NULL;
> I suggest that we repeat the "We just unmapped the memory."
> comment here.

done

> 
> Is it safe to set read_buffer_ to NULL before the
> GiveToProcess call?  That would avoid the duplication of
> read_buffer_ = NULL inside and after the if statement.

nope. We need a valid buffer to pass to the renderer.

> 
> Should we add a sentence to the block comment above
> GiveToProcess in shared_memory.h, to stress the fact
> that the shared memory segment is closed whether
> GiveToProcess returns true or false?

done

> 
> http://codereview.chromium.org/21222/diff/5/6#newcode135
> Line 135: spare_read_buffer_->Release();
> Perhaps we should just define spare_read_buffer_ as
> a scoped_refptr<SharedIOBuffer>?

The problem with it is that the variable is class static so it should not be a
complex object (with constructors) per the style guide.

[wtc, 2009-02-10 23:14:46]

LGTM.