Fix for 1498134, possible crash when cancelling a url request. The...

Fix for 1498134, possible crash when cancelling a url request. The
crash happened because OnReadCompleted would invoke CompleteRead which
might invoke CancelRequest and delete the request. If this happened
when control returned back to OnReadCompleted the request was deleted,
yet ResourceDispatcherHost assumed it wasn't.

BUG=1498134
TEST=none


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

[darin (slow to review), 2008-11-24 18:54:09]

OK, seems like a safe change to me.

[eroman, 2008-11-24 19:16:19]

LGTM

http://codereview.chromium.org/12602/diff/1/2
File chrome/browser/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/12602/diff/1/2#newcode1732
Line 1732: CancelRequest(render_process_host_id, request_id, from_renderer,
true);
I would recommend getting rid of the 3 parameter CancelRequest, and updating its
callers to pass the final argument.

http://codereview.chromium.org/12602/diff/1/2#newcode2044
Line 2044: void ResourceDispatcherHost::CancelRequest(int
render_process_host_id,
Can you move this beside the original CancelRequest() so the diff shows up with
fewer deltas? (it is hard to tell what actually changed, which is the &&
allow_delete).

http://codereview.chromium.org/12602/diff/1/2#newcode2254
Line 2254: // deleted. We do this as callers of us assume request is valid after
we
"assume request is valid" --> "assume |request| is valid"

[sky, 2008-11-24 19:36:39]

New snapshot uploaded.

http://codereview.chromium.org/12602/diff/1/2
File chrome/browser/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/12602/diff/1/2#newcode1732
Line 1732: CancelRequest(render_process_host_id, request_id, from_renderer,
true);
On 2008/11/24 19:16:19, eroman wrote:
> I would recommend getting rid of the 3 parameter CancelRequest, and updating
its
> callers to pass the final argument.

I decided not to go this route as it seems like external callers of the public
method shouldn't care about whether it is deleted or not.

http://codereview.chromium.org/12602/diff/1/2#newcode2044
Line 2044: void ResourceDispatcherHost::CancelRequest(int
render_process_host_id,
On 2008/11/24 19:16:19, eroman wrote:
> Can you move this beside the original CancelRequest() so the diff shows up
with
> fewer deltas? (it is hard to tell what actually changed, which is the &&
> allow_delete).

Done.

http://codereview.chromium.org/12602/diff/1/2#newcode2254
Line 2254: // deleted. We do this as callers of us assume request is valid after
we
On 2008/11/24 19:16:19, eroman wrote:
> "assume request is valid" --> "assume |request| is valid"

Done.

[wtc, 2008-11-24 19:43:48]

I have some incoherent comments below (I wrote them as I
tried to understand the code).  In summary, I wonder if
we should fix this bug by doing
MessageLoop::current()->PostTask(&ResourceDispatcherHost::RemovePendingRequest)
instead of a direct call to RemovePendingRequest.

http://codereview.chromium.org/12602/diff/1/2
File chrome/browser/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/12602/diff/1/2#newcode2072
Line 2072: RemovePendingRequest(info->render_process_host_id, info->request_id);
I wonder if we should take a different approach: since
i->second->Cancel() causes RemovePendingRequest to be
called later in a Task, we can simulate that effect by
doing a
MessageLoop::current()->PostTask(&ResourceDispatcherHost::RemovePendingRequest)
here
instead.

See how URLRequestJob::NotifyDone does a PostTask of
URLRequestJob::CompleteNotifyDone.

http://codereview.chromium.org/12602/diff/1/2#newcode2256
Line 2256: CancelRequest(info->render_process_host_id, info->request_id, false,
false);
I searched for all calls to "CancelRequest" in our source
tree.  It seems that
ResourceDispatcherHost::OnReceivedRedirect and
ResourceDispatcherHost::OnResponseStarted may also need the
same change (passing false as the last argument).

I suspect that ResourceDispatcherHost::OnReceivedRedirect
is OK because it already calls RemovePendingRequest.

http://codereview.chromium.org/12602/diff/1/3
File chrome/browser/resource_dispatcher_host.h (right):

http://codereview.chromium.org/12602/diff/1/3#newcode402
Line 402: void CancelRequest(int render_process_host_id,
You are overloading the CancelRequest method.  Doesn't our
style guide discourage function overloading, especially to
simulate default function parameters?
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl...

[wtc, 2008-11-24 19:58:49]

Let me elaborate on my previous comment.

I was unhappy that I couldn't decide whether the other two
calls of CancelRequest
(ResourceDispatcherHost::OnReceivedRedirect
and ResourceDispatcherHost::OnResponseStarted) need the
same change.  So I considered alternative solutions.

I first looked into if we can just call i->second->Cancel(),
and change URLRequest::CancelWithError to call job_->Kill()
even if is_pending_ is false.  The problem with this
approach is that I am worried some delegate of URLRequest
may not want to be notified done twice.  I couldn't
answer this question myself.  If you know it's fine for
a URLRequest delegate to be notified done twice, then this
may be the best solution.

Then I came up with the idea of calling
MessageLoop::current()->PostTask(&ResourceDispatcherHost::RemovePendingRequest).
What I like about this solution is that I can almost
explain it completely.