Don't leave renderer process frozen when canceling JavaScript dialogs.

Don't leave renderer process frozen when canceling JavaScript dialogs.

BUG=297289
TEST=See bug for repro steps.
R=avi@chromium.org, sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=225253

[Charlie Reis, 2013-09-24 22:09:15]

John, can I have you look at this one as well?  I'm not 100% sure it's correct
to run the callback in Invalidate(), but otherwise we seem to end up in a bad
state.  This appears to do the right thing for both the repro steps in
http://crbug.com/297289 and the interstitial page case in
http://crbug.com/295695.

[jam, 2013-09-25 00:25:56]

Avi is more familiar with this code than me, redirecting

[Avi (use Gerrit), 2013-09-25 04:28:40]

I like this, though not quite enough.

https://codereview.chromium.org/24436003/diff/1/chrome/browser/ui/app_modal_d...
File chrome/browser/ui/app_modal_dialogs/javascript_app_modal_dialog.cc (right):

https://codereview.chromium.org/24436003/diff/1/chrome/browser/ui/app_modal_d...
chrome/browser/ui/app_modal_dialogs/javascript_app_modal_dialog.cc:106: void
JavaScriptAppModalDialog::Invalidate() {
It seems like at the end of JavaScriptAppModalDialog::NotifyDelegate, below, it
calls this function, so would the callback then happen twice?

What I would do is at both invocations of callback_ (here on line 111 and at
line 164) do:

if (!callback_.is_null()) {
  callback_.Run(/*...*/);
  callback_.Reset();
}

[Charlie Reis, 2013-09-25 16:48:34]

https://codereview.chromium.org/24436003/diff/1/chrome/browser/ui/app_modal_d...
File chrome/browser/ui/app_modal_dialogs/javascript_app_modal_dialog.cc (right):

https://codereview.chromium.org/24436003/diff/1/chrome/browser/ui/app_modal_d...
chrome/browser/ui/app_modal_dialogs/javascript_app_modal_dialog.cc:106: void
JavaScriptAppModalDialog::Invalidate() {
On 2013/09/25 04:28:40, Avi wrote:
> It seems like at the end of JavaScriptAppModalDialog::NotifyDelegate, below,
it
> calls this function, so would the callback then happen twice?

Not quite.  NotifyDelegate calls AppModalDialog::Invalidate (which only sets
valid_ to false), not JavaScriptAppModalDialog::Invalidate.  However, I like
your suggestion-- it's safer in general.


> What I would do is at both invocations of callback_ (here on line 111 and at
> line 164) do:
> 
> if (!callback_.is_null()) {
>   callback_.Run(/*...*/);
>   callback_.Reset();
> }

Done.

[Avi (use Gerrit), 2013-09-25 16:56:40]

I'm pretty paranoid :)

LGTM

[Charlie Reis, 2013-09-25 17:02:04]

Thanks!  sky@, can you look for owner's approval?

[sky, 2013-09-25 20:27:07]

LGTM

[commit-bot: I haz the power, 2013-09-25 20:32:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/24436003/11001

[Charlie Reis, 2013-09-25 21:32:27]

Committed patchset #2 manually as r225253 (presubmit successful).

[commit-bot: I haz the power, 2013-09-25 21:48:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/24436003/11001