Don't destroy a request while it is being processed

If ResourceDispatcherHost receives a CancelRequest, it
should call OnResponseComplete() only if the cancellation
is actually processed.  Otherwise, it might destroy a
request that is still active.

In addition, make sure DownloadRequestLimiter::Callback is
only invoked from the IO thread and prevent
DownloadThrottlingResourceHandler from processing
callbacks after the request has been closed.

BUG=76202
TEST=none

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

[asanka, 2011-03-19 12:52:43]

See http://crbug.com/76202 for what we are trying to avoid. The core part of the
fix is in resource_dispatcher_host.{c,h} which prevents a download request from
being improperly destroyed while still waiting for a callback.

The changes in download_throttling_resource_handler.{c,h} are for dealing with
the request being cancelled before DownloadRequestLimiter makes the callback.

download_request_limiter* changes are to prevent the callback from running
outside the IO thread.

[asanka, 2011-03-21 16:57:33]

[+skylined]

[Randy Smith (Not in Mondays), 2011-03-21 17:31:01]

LGTM, but to me the key section of this CL is about the protocol for
interactions with the ResourceDispatcherHost, and I don't feel competent to do
reviews in that area.  Hopefully one of your other reviewers will be able to
cover that.

http://codereview.chromium.org/6713008/diff/4001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/download_throttling_resource_handler.cc
(right):

http://codereview.chromium.org/6713008/diff/4001/chrome/browser/renderer_host...
chrome/browser/renderer_host/download_throttling_resource_handler.cc:172: if
(!request_closed_) {
nit: I'd suggest restructuring this routing by putting the Release() at the
beginning and then inverting the request_closed_ test and doing a return at that
point, just to reduce indenting and make the unindented code by the common path.

[lzheng, 2011-03-21 21:02:53]

I can not access: http://crbug.com/76202. Is it possible the link is wrong?

I am trying to figure out how this could be triggered.

Lei

On 2011/03/21 17:31:01, rdsmith wrote:
> LGTM, but to me the key section of this CL is about the protocol for
> interactions with the ResourceDispatcherHost, and I don't feel competent to do
> reviews in that area.  Hopefully one of your other reviewers will be able to
> cover that.
> 
>
http://codereview.chromium.org/6713008/diff/4001/chrome/browser/renderer_host...
> File chrome/browser/renderer_host/download_throttling_resource_handler.cc
> (right):
> 
>
http://codereview.chromium.org/6713008/diff/4001/chrome/browser/renderer_host...
> chrome/browser/renderer_host/download_throttling_resource_handler.cc:172: if
> (!request_closed_) {
> nit: I'd suggest restructuring this routing by putting the Release() at the
> beginning and then inverting the request_closed_ test and doing a return at
that
> point, just to reduce indenting and make the unindented code by the common
path.

[rvargas (doing something else), 2011-03-21 21:09:21]

lzheng: try now.

On Mon, Mar 21, 2011 at 2:02 PM, <lzheng@chromium.org> wrote:

> I can not access: http://crbug.com/76202. Is it possible the link is
> wrong?
>
> I am trying to figure out how this could be triggered.
>
> Lei
>
>
> On 2011/03/21 17:31:01, rdsmith wrote:
>
>> LGTM, but to me the key section of this CL is about the protocol for
>> interactions with the ResourceDispatcherHost, and I don't feel competent
>> to do
>> reviews in that area.  Hopefully one of your other reviewers will be able
>> to
>> cover that.
>>
>
>
>
>
http://codereview.chromium.org/6713008/diff/4001/chrome/browser/renderer_host...
>
>> File chrome/browser/renderer_host/download_throttling_resource_handler.cc
>> (right):
>>
>
>
>
>
http://codereview.chromium.org/6713008/diff/4001/chrome/browser/renderer_host...
>
>> chrome/browser/renderer_host/download_throttling_resource_handler.cc:172:
>> if
>> (!request_closed_) {
>> nit: I'd suggest restructuring this routing by putting the Release() at
>> the
>> beginning and then inverting the request_closed_ test and doing a return
>> at
>>
> that
>
>> point, just to reduce indenting and make the unindented code by the common
>>
> path.
>
>
>
> http://codereview.chromium.org/6713008/
>

[rvargas (doing something else), 2011-03-21 22:38:10]

Please add a test for this. It doesn't have to be part of this CL though.

LGTM.

http://codereview.chromium.org/6713008/diff/4001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/download_throttling_resource_handler.cc
(right):

http://codereview.chromium.org/6713008/diff/4001/chrome/browser/renderer_host...
chrome/browser/renderer_host/download_throttling_resource_handler.cc:172: if
(!request_closed_) {
On 2011/03/21 17:31:01, rdsmith wrote:
> nit: I'd suggest restructuring this routing by putting the Release() at the
> beginning and then inverting the request_closed_ test and doing a return at
that
> point, just to reduce indenting and make the unindented code by the common
path.

The problem would be if we hold the last reference.

http://codereview.chromium.org/6713008/diff/4001/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/6713008/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.cc:1199:
request->Cancel();
Did you considered moving the pending check to this code instead? I'm actually
fine either way, just wondering.

[lzheng, 2011-03-21 22:54:02]

LGTM.

Echo the need for a test. Otherwise, we won't know when this is broken the next
time.

Lei

On 2011/03/21 22:38:10, rvargas wrote:
> Please add a test for this. It doesn't have to be part of this CL though.
> 
> LGTM.
> 
>
http://codereview.chromium.org/6713008/diff/4001/chrome/browser/renderer_host...
> File chrome/browser/renderer_host/download_throttling_resource_handler.cc
> (right):
> 
>
http://codereview.chromium.org/6713008/diff/4001/chrome/browser/renderer_host...
> chrome/browser/renderer_host/download_throttling_resource_handler.cc:172: if
> (!request_closed_) {
> On 2011/03/21 17:31:01, rdsmith wrote:
> > nit: I'd suggest restructuring this routing by putting the Release() at the
> > beginning and then inverting the request_closed_ test and doing a return at
> that
> > point, just to reduce indenting and make the unindented code by the common
> path.
> 
> The problem would be if we hold the last reference.
> 
>
http://codereview.chromium.org/6713008/diff/4001/content/browser/renderer_hos...
> File content/browser/renderer_host/resource_dispatcher_host.cc (right):
> 
>
http://codereview.chromium.org/6713008/diff/4001/content/browser/renderer_hos...
> content/browser/renderer_host/resource_dispatcher_host.cc:1199:
> request->Cancel();
> Did you considered moving the pending check to this code instead? I'm actually
> fine either way, just wondering.

[asanka, 2011-03-22 14:52:42]

Thanks for the reviews!  I'll add a test in a separate CL.

http://codereview.chromium.org/6713008/diff/4001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/download_throttling_resource_handler.cc
(right):

http://codereview.chromium.org/6713008/diff/4001/chrome/browser/renderer_host...
chrome/browser/renderer_host/download_throttling_resource_handler.cc:172: if
(!request_closed_) {
On 2011/03/21 17:31:01, rdsmith wrote:
> nit: I'd suggest restructuring this routing by putting the Release() at the
> beginning and then inverting the request_closed_ test and doing a return at
that
> point, just to reduce indenting and make the unindented code by the common
path.

It's not very pretty, but as rvargas points out, once we call Release() we have
no guarantee that we can safely access any members.

http://codereview.chromium.org/6713008/diff/4001/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/6713008/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.cc:1199:
request->Cancel();
On 2011/03/21 22:38:10, rvargas wrote:
> Did you considered moving the pending check to this code instead? I'm actually
> fine either way, just wondering.

My concern was that request->is_pending() being false is not a good signal in
all cases where we end up here.

[Randy Smith (Not in Mondays), 2011-03-22 14:55:45]

http://codereview.chromium.org/6713008/diff/4001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/download_throttling_resource_handler.cc
(right):

http://codereview.chromium.org/6713008/diff/4001/chrome/browser/renderer_host...
chrome/browser/renderer_host/download_throttling_resource_handler.cc:172: if
(!request_closed_) {
On 2011/03/22 14:52:42, asanka wrote:
> On 2011/03/21 17:31:01, rdsmith wrote:
> > nit: I'd suggest restructuring this routing by putting the Release() at the
> > beginning and then inverting the request_closed_ test and doing a return at
> that
> > point, just to reduce indenting and make the unindented code by the common
> path.
> 
> It's not very pretty, but as rvargas points out, once we call Release() we
have
> no guarantee that we can safely access any members.

Fair enough; destruction could happen right then, couldn't it?  Never mind.