Fix a browser crash when the URLRequestChromeJob goes away...

Fix a browser crash when the URLRequestChromeJob goes away
from the NotifyReadComplete notification.

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

[darin (slow to review), 2009-02-07 03:19:13]

it's not obvious how this fixes anything.  can you explain why it is needed?

[rvargas (doing something else), 2009-02-07 03:40:50]

On 2009/02/07 03:19:13, darin wrote:
> it's not obvious how this fixes anything.  can you explain why it is needed?

I'm moving the line that touches the object to the line before the one  that may
end up deleting the object. I could also grab an extra reference and release it
after the call, but that seems like not really needed.

[darin (slow to review), 2009-02-07 07:25:00]

I see...

The whole point of URLRequestJob being a reference counted object is so that you
don't have to worry about |this| become invalid during a method call.  The
caller of DataAvailable should really be holding an owning reference before
using the job.

If applied properly, the result is that you have a simpler programming model and
code that is easier to reason about.  Can we do better?  How about...

http://codereview.chromium.org/20162/diff/1/2
File chrome/browser/dom_ui/chrome_url_data_manager.cc (right):

http://codereview.chromium.org/20162/diff/1/2#newcode234
Line 234: URLRequestChromeJob* job = i->second;
how about changing this to:

scoped_refptr<URLRequestChromeJob> job = i->second;

[rvargas (doing something else), 2009-02-09 22:44:25]

On 2009/02/07 07:25:00, darin wrote:
> I see...
> 
> The whole point of URLRequestJob being a reference counted object is so that
you
> don't have to worry about |this| become invalid during a method call.  The
> caller of DataAvailable should really be holding an owning reference before
> using the job.
> 
> If applied properly, the result is that you have a simpler programming model
and
> code that is easier to reason about.  Can we do better?  How about...
> 
> http://codereview.chromium.org/20162/diff/1/2
> File chrome/browser/dom_ui/chrome_url_data_manager.cc (right):
> 
> http://codereview.chromium.org/20162/diff/1/2#newcode234
> Line 234: URLRequestChromeJob* job = i->second;
> how about changing this to:
> 
> scoped_refptr<URLRequestChromeJob> job = i->second;

I was following the code on URLRequestJob::NotifyReadComplete which protects
itself inside the call, but your suggestion is much better, thanks.

Please take another look.

[darin (slow to review), 2009-02-09 22:49:15]

http://codereview.chromium.org/20162/diff/4/5
File chrome/browser/dom_ui/chrome_url_data_manager.cc (right):

http://codereview.chromium.org/20162/diff/4/5#newcode294
Line 294: pending_buf_ = NULL;
so this part of the change is no longer needed right?

[darin (slow to review), 2009-02-09 22:49:59]

> I was following the code on URLRequestJob::NotifyReadComplete which protects
> itself inside the call, but your suggestion is much better, thanks.

Yeah... i really dislike that code for the same reason.  It is part of what
makes URLRequestJob so hard to understand.

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

http://codereview.chromium.org/20162/diff/4/5
File chrome/browser/dom_ui/chrome_url_data_manager.cc (right):

http://codereview.chromium.org/20162/diff/4/5#newcode294
Line 294: pending_buf_ = NULL;
On 2009/02/09 22:49:15, darin wrote:
> so this part of the change is no longer needed right?

Correct, it's not really needed but now I sort of prefer it this way :). From
the point of view of this job, we're done with the buffer after CompleteRead()

[darin (slow to review), 2009-02-10 04:44:59]

OK.. maybe add a comment about why we are taking a reference to the job, and
then LGTM