Fix handling of JS dialog suppressino during tab closing

Fix handling of JS dialog suppressino during tab closing

Send the dialog response reply to the renderer before killing the tab, since RenderView can't close while it's waiting for a response.

BUG=37324
TEST=Close a tab which plays audio, has multiple alerts in onbeforeunload, and shares a renderer process with at least one other tab. Choose to prevent the page from creating more dialogs. The audio should stop when the tab closes.

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

[Charlie Reis, 2010-10-07 00:26:50]

Thanks for finding a quick fix.

It would be nice to have a test for this, especially since I haven't found a way
to repro it yet (the page in the bug report closes without any alerts).  It
doesn't have to have sound if we can verify that the renderer process went away.

http://codereview.chromium.org/3621009/diff/1/2
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/3621009/diff/1/2#newcode604
chrome/browser/renderer_host/render_view_host.cc:604: // Kill the tab
immediately, since a page that spamming alerts in
Please start the comment with something like "If we are waiting for an unload or
beforeunload ack and the user has suppressed messages, then kill..."  Otherwise
it sounds like we intend to kill it regardless.

nit: that's

[stuartmorgan, 2010-10-07 15:30:27]

On 2010/10/07 00:26:50, creis wrote:
> It would be nice to have a test for this, especially since I haven't found a
way
> to repro it yet (the page in the bug report closes without any alerts).

Yes, the bug report example has changed. I was synthesizing an example from the
dup, using the dev tools. I can whip up a manual test case.

> It doesn't have to have sound if we can verify that the renderer process went
away.

So that's the problem; the renderer process doesn't go away. The bug only
happens when the renderer process is still serving another tab. I tried to
figure out how to write an automated test for this, but couldn't think of a way
given that.

[stuartmorgan, 2010-10-07 16:10:40]

http://codereview.chromium.org/3621009/diff/1/2
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/3621009/diff/1/2#newcode604
chrome/browser/renderer_host/render_view_host.cc:604: // Kill the tab
immediately, since a page that spamming alerts in
On 2010/10/07 00:26:50, creis wrote:
> Please start the comment with something like "If we are waiting for an unload
or
> beforeunload ack and the user has suppressed messages, then kill..." 
Otherwise
> it sounds like we intend to kill it regardless.
> 
> nit: that's

Done.

[Charlie Reis, 2010-10-07 17:04:39]

Fair enough.  I'll think about how we can test it, but the fix sounds about
right.  LGTM.