Browser should not crash when a page showing a quota infobar is closed/reloaded

Browser should not crash when a page showing a quota infobar is closed/reloaded

BUG=92620
TEST=no crash

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

[Bernhard Bauer, 2011-08-13 00:48:56]

LGTM.

[michaeln, 2011-08-15 17:51:05]

sorry for the late comments, but i've got some questions about this change

http://codereview.chromium.org/7640020/diff/1/chrome/browser/chrome_quota_per...
File chrome/browser/chrome_quota_permission_context.cc (left):

http://codereview.chromium.org/7640020/diff/1/chrome/browser/chrome_quota_per...
chrome/browser/chrome_quota_permission_context.cc:53: DCHECK(!callback_.get());
The fact that we're tripping on this DCHECK indicates something wasn't working
as expected. Is it really ok to just remove the check, or is more needed to
handle this case?

- is it ok to not run the callback, is the caller left hanging?
- is it ok to delete the calback on the UI thread instead of the IO thread?

Would it be better to call...

context_->DispatchCallbackOnIOThread(
   callback_.release(),    
   QuotaPermissionContext::kResponseCancelled);

... at this point?

[kinuko, 2011-08-17 03:58:25]

On 2011/08/15 17:51:05, michaeln wrote:
> sorry for the late comments, but i've got some questions about this change
> 
>
http://codereview.chromium.org/7640020/diff/1/chrome/browser/chrome_quota_per...
> File chrome/browser/chrome_quota_permission_context.cc (left):
> 
>
http://codereview.chromium.org/7640020/diff/1/chrome/browser/chrome_quota_per...
> chrome/browser/chrome_quota_permission_context.cc:53:
DCHECK(!callback_.get());
> The fact that we're tripping on this DCHECK indicates something wasn't working
> as expected. Is it really ok to just remove the check, or is more needed to
> handle this case?
> 
> - is it ok to not run the callback, is the caller left hanging?
> - is it ok to delete the calback on the UI thread instead of the IO thread?
> 
> Would it be better to call...
> 
> context_->DispatchCallbackOnIOThread(
>    callback_.release(),    
>    QuotaPermissionContext::kResponseCancelled);
> 
> ... at this point?

Good catch, seems like we should rather fire the callback on IO thread.  I'll
request review for the fix.