Make the callback from JavaScript dialogs even when closing a WebContents.

Make the callback from JavaScript dialogs even when closing a WebContents.

Otherwise, other WebContentses sharing that render process will hang.

The reason the code didn't do this before was because I had been afraid that doing the callback would crash. Therefore, this CL adds a test JavaScriptDialogTest.ClosingPageWithSubframeAlertingDoesntCrash to make sure the crash doesn't happen. (It doesn't; my fears were unfounded.) The other new test, JavaScriptDialogTest.ClosingPageSharingRendererDoesntHang, tests for the bug, and passes with this fix.

BUG=682060
TEST=as noted

Review-Url: https://codereview.chromium.org/2641173004
Cr-Commit-Position: refs/heads/master@{#444874}
Committed: https://chromium.googlesource.com/chromium/src/+/2de41893d8301d98cfc64a3691672d2197e9a4f3

[Avi (use Gerrit), 2017-01-19 21:02:13]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 21:03:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Avi (use Gerrit), 2017-01-19 21:17:25]

Description was changed from

==========
Make the callback from JavaScript dialogs even when closing a WebContents.

Otherwise, other WebContentses sharing that render process will hang.

I was afraid of a crash doing the callback, so I added the test
JavaScriptDialogTest.ClosingPageWithSubframeAlertingDoesntCrash to make sure it
doesn't happen. (It doesn't.) The other new test,
JavaScriptDialogTest.ClosingPageSharingRendererDoesntHang, tests for the bug,
and passes with this fix.

BUG=682060
TEST=as noted
==========

to

==========
Make the callback from JavaScript dialogs even when closing a WebContents.

Otherwise, other WebContentses sharing that render process will hang.

The reason the code didn't do this before was because I had been afraid of a
crash doing the callback. Therefore, this CL adds a test
JavaScriptDialogTest.ClosingPageWithSubframeAlertingDoesntCrash to make sure the
crash doesn't happen. (It doesn't; my fears were unfounded) The other new test,
JavaScriptDialogTest.ClosingPageSharingRendererDoesntHang, tests for the bug,
and passes with this fix.

BUG=682060
TEST=as noted
==========

[Avi (use Gerrit), 2017-01-19 21:18:09]

Description was changed from

==========
Make the callback from JavaScript dialogs even when closing a WebContents.

Otherwise, other WebContentses sharing that render process will hang.

The reason the code didn't do this before was because I had been afraid of a
crash doing the callback. Therefore, this CL adds a test
JavaScriptDialogTest.ClosingPageWithSubframeAlertingDoesntCrash to make sure the
crash doesn't happen. (It doesn't; my fears were unfounded) The other new test,
JavaScriptDialogTest.ClosingPageSharingRendererDoesntHang, tests for the bug,
and passes with this fix.

BUG=682060
TEST=as noted
==========

to

==========
Make the callback from JavaScript dialogs even when closing a WebContents.

Otherwise, other WebContentses sharing that render process will hang.

The reason the code didn't do this before was because I had been afraid that
doing the callback would crash. Therefore, this CL adds a test
JavaScriptDialogTest.ClosingPageWithSubframeAlertingDoesntCrash to make sure the
crash doesn't happen. (It doesn't; my fears were unfounded) The other new test,
JavaScriptDialogTest.ClosingPageSharingRendererDoesntHang, tests for the bug,
and passes with this fix.

BUG=682060
TEST=as noted
==========

[Avi (use Gerrit), 2017-01-19 21:19:19]

Description was changed from

==========
Make the callback from JavaScript dialogs even when closing a WebContents.

Otherwise, other WebContentses sharing that render process will hang.

The reason the code didn't do this before was because I had been afraid that
doing the callback would crash. Therefore, this CL adds a test
JavaScriptDialogTest.ClosingPageWithSubframeAlertingDoesntCrash to make sure the
crash doesn't happen. (It doesn't; my fears were unfounded) The other new test,
JavaScriptDialogTest.ClosingPageSharingRendererDoesntHang, tests for the bug,
and passes with this fix.

BUG=682060
TEST=as noted
==========

to

==========
Make the callback from JavaScript dialogs even when closing a WebContents.

Otherwise, other WebContentses sharing that render process will hang.

The reason the code didn't do this before was because I had been afraid that
doing the callback would crash. Therefore, this CL adds a test
JavaScriptDialogTest.ClosingPageWithSubframeAlertingDoesntCrash to make sure the
crash doesn't happen. (It doesn't; my fears were unfounded.) The other new test,
JavaScriptDialogTest.ClosingPageSharingRendererDoesntHang, tests for the bug,
and passes with this fix.

BUG=682060
TEST=as noted
==========

[Avi (use Gerrit), 2017-01-19 21:43:01]

avi@chromium.org changed reviewers:
+ creis@chromium.org

[Charlie Reis, 2017-01-19 21:51:00]

LGTM

https://codereview.chromium.org/2641173004/diff/1/chrome/browser/ui/javascrip...
File chrome/browser/ui/javascript_dialogs/javascript_dialog_browsertest.cc
(right):

https://codereview.chromium.org/2641173004/diff/1/chrome/browser/ui/javascrip...
chrome/browser/ui/javascript_dialogs/javascript_dialog_browsertest.cc:5:
#include "base/logging.h"
Is this leftover from some earlier logging?

https://codereview.chromium.org/2641173004/diff/1/chrome/browser/ui/javascrip...
chrome/browser/ui/javascript_dialogs/javascript_dialog_browsertest.cc:100:
"document.body.appendChild(iframe);";
Another test that might be useful (in a followup CL, no need to block this one):
Show a dialog from an OOPIF and detach the OOPIF from the parent frame while the
dialog is showing.

(If there's a way to do it in a same-process subframe, that's useful too, but
presumably the process is blocked waiting for the dialog to close and can't
detach the subframe.  I suppose there's always nested message loops, though.) 
:)

[commit-bot: I haz the power, 2017-01-19 21:51:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 21:51:16]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2017-01-19 22:03:38]

https://codereview.chromium.org/2641173004/diff/1/chrome/browser/ui/javascrip...
File chrome/browser/ui/javascript_dialogs/javascript_dialog_browsertest.cc
(right):

https://codereview.chromium.org/2641173004/diff/1/chrome/browser/ui/javascrip...
chrome/browser/ui/javascript_dialogs/javascript_dialog_browsertest.cc:5:
#include "base/logging.h"
On 2017/01/19 21:51:00, Charlie Reis wrote:
> Is this leftover from some earlier logging?

Done.

https://codereview.chromium.org/2641173004/diff/1/chrome/browser/ui/javascrip...
chrome/browser/ui/javascript_dialogs/javascript_dialog_browsertest.cc:100:
"document.body.appendChild(iframe);";
On 2017/01/19 21:51:00, Charlie Reis wrote:
> Another test that might be useful (in a followup CL, no need to block this
one):
> Show a dialog from an OOPIF and detach the OOPIF from the parent frame while
the
> dialog is showing.
> 
> (If there's a way to do it in a same-process subframe, that's useful too, but
> presumably the process is blocked waiting for the dialog to close and can't
> detach the subframe.  I suppose there's always nested message loops, though.) 
> :)

That is sooo evil; I'm totally writing that one.

When you say "detach" you mean delete the subframe?

[Avi (use Gerrit), 2017-01-19 22:04:20]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2017-01-19 22:04:20]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2641173004/#ps20001
(title: "fix")

[commit-bot: I haz the power, 2017-01-19 22:05:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Reis, 2017-01-19 22:08:32]

https://codereview.chromium.org/2641173004/diff/1/chrome/browser/ui/javascrip...
File chrome/browser/ui/javascript_dialogs/javascript_dialog_browsertest.cc
(right):

https://codereview.chromium.org/2641173004/diff/1/chrome/browser/ui/javascrip...
chrome/browser/ui/javascript_dialogs/javascript_dialog_browsertest.cc:100:
"document.body.appendChild(iframe);";
On 2017/01/19 22:03:38, Avi (offsites OOO 17Jan-2Feb) wrote:
> On 2017/01/19 21:51:00, Charlie Reis wrote:
> > Another test that might be useful (in a followup CL, no need to block this
> one):
> > Show a dialog from an OOPIF and detach the OOPIF from the parent frame while
> the
> > dialog is showing.
> > 
> > (If there's a way to do it in a same-process subframe, that's useful too,
but
> > presumably the process is blocked waiting for the dialog to close and can't
> > detach the subframe.  I suppose there's always nested message loops,
though.) 
> > :)
> 
> That is sooo evil; I'm totally writing that one.
> 
> When you say "detach" you mean delete the subframe?

Yep, exactly.

[commit-bot: I haz the power, 2017-01-19 22:57:13]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484863460261250,
"parent_rev": "3cc294f047397929621de8cb19c2e911c8f488a1", "commit_rev":
"2de41893d8301d98cfc64a3691672d2197e9a4f3"}

[commit-bot: I haz the power, 2017-01-19 22:57:47]

Description was changed from

==========
Make the callback from JavaScript dialogs even when closing a WebContents.

Otherwise, other WebContentses sharing that render process will hang.

The reason the code didn't do this before was because I had been afraid that
doing the callback would crash. Therefore, this CL adds a test
JavaScriptDialogTest.ClosingPageWithSubframeAlertingDoesntCrash to make sure the
crash doesn't happen. (It doesn't; my fears were unfounded.) The other new test,
JavaScriptDialogTest.ClosingPageSharingRendererDoesntHang, tests for the bug,
and passes with this fix.

BUG=682060
TEST=as noted
==========

to

==========
Make the callback from JavaScript dialogs even when closing a WebContents.

Otherwise, other WebContentses sharing that render process will hang.

The reason the code didn't do this before was because I had been afraid that
doing the callback would crash. Therefore, this CL adds a test
JavaScriptDialogTest.ClosingPageWithSubframeAlertingDoesntCrash to make sure the
crash doesn't happen. (It doesn't; my fears were unfounded.) The other new test,
JavaScriptDialogTest.ClosingPageSharingRendererDoesntHang, tests for the bug,
and passes with this fix.

BUG=682060
TEST=as noted

Review-Url: https://codereview.chromium.org/2641173004
Cr-Commit-Position: refs/heads/master@{#444874}
Committed:
https://chromium.googlesource.com/chromium/src/+/2de41893d8301d98cfc64a369167...
==========

[commit-bot: I haz the power, 2017-01-19 22:57:48]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/2de41893d8301d98cfc64a369167...