Prevent modal dialogs when preparing to swap out.

Prevent modal dialogs when preparing to swap out.

This ensures that no PageGroupLoadDeferrers will be on the stack when
processing ViewMsg_SwapOut, which would lead to an ASSERT.

BUG=312490
TEST=Start a cross-process navigation and show a loop of alerts before commit.
R=cdn@chromium.org, darin@chromium.org

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

[Charlie Reis, 2013-10-29 02:59:55]

Here's a followup CL that I could land after
https://codereview.chromium.org/46403006/ bakes on Canary, assuming that this is
a little riskier to merge to beta.

https://codereview.chromium.org/50223002/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/50223002/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:3007: // TODO(creis): runModalDialog looks
deprecated.  Is this code dead?
Darin, can we still get here?  I didn't see any callers of it.

[Charlie Reis, 2013-10-30 16:09:52]

Ok, now that https://codereview.chromium.org/46403006/ has landed, this CL
should prevent issues in the renderer process with a loop of modal dialogs.

Darin, can you take a look?

https://codereview.chromium.org/50223002/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/50223002/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:3007: // TODO(creis): runModalDialog looks
deprecated.  Is this code dead?
On 2013/10/29 02:59:56, creis wrote:
> Darin, can we still get here?  I didn't see any callers of it.

Ah, I had the name wrong.  showModalDialog still exists, unfortunately.  Guess
we still need this code.

[darin (slow to review), 2013-11-01 21:01:41]

https://codereview.chromium.org/50223002/diff/90001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/50223002/diff/90001/chrome/browser/ui/browser...
chrome/browser/ui/browser_browsertest.cc:481:
ASCIIToUTF16("setInterval(\"alert('Dialog showing!');\", 0);"));
This feels like such an imprecise test. I wonder if there is a way to get a more
exact signal for running the dialog.

We already suppress modal alerts generated from unload handlers and after unload
handlers (I presume).

What is the exact sequence of events that needs to happen renderer-side to repro
this bug?

https://codereview.chromium.org/50223002/diff/90001/content/browser/frame_hos...
File content/browser/frame_host/render_view_host_manager.cc (right):

https://codereview.chromium.org/50223002/diff/90001/content/browser/frame_hos...
content/browser/frame_host/render_view_host_manager.cc:447: // no longer on the
stack when we send the SwapOut message.
But isn't there a race condition here? What if a PageGroupLoadDeferrer is active
before the call to SuppressDialogsUntilSwapOut?

[Charlie Reis, 2013-11-04 19:32:49]

Thanks-- PTAL.

https://codereview.chromium.org/50223002/diff/90001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/50223002/diff/90001/chrome/browser/ui/browser...
chrome/browser/ui/browser_browsertest.cc:481:
ASCIIToUTF16("setInterval(\"alert('Dialog showing!');\", 0);"));
On 2013/11/01 21:01:42, darin wrote:
> This feels like such an imprecise test. I wonder if there is a way to get a
more
> exact signal for running the dialog.
> 
> We already suppress modal alerts generated from unload handlers and after
unload
> handlers (I presume).
> 
> What is the exact sequence of events that needs to happen renderer-side to
repro
> this bug?

Good point.  I've simplified this to just be two back-to-back dialogs.

The bug is that canceling the first dialog gives the renderer process a chance
to create a new dialog before the SwapOut message arrives.

(I was originally testing with a simple while loop just to generalize a bit, but
then the renderer goes unresponsive (like while(true){}) so I switched to
setInterval.)

https://codereview.chromium.org/50223002/diff/90001/content/browser/frame_hos...
File content/browser/frame_host/render_view_host_manager.cc (right):

https://codereview.chromium.org/50223002/diff/90001/content/browser/frame_hos...
content/browser/frame_host/render_view_host_manager.cc:447: // no longer on the
stack when we send the SwapOut message.
On 2013/11/01 21:01:42, darin wrote:
> But isn't there a race condition here? What if a PageGroupLoadDeferrer is
active
> before the call to SuppressDialogsUntilSwapOut?

This should work in that case, as long as there's only one PageGroupLoadDeferrer
on the stack.  Here's what should happen:
1) Renderer creates a PageGroupLoadDeferrer for some kind of modal dialog.
2) Browser sends a SuppressDialogsUntilSwapOut message.  This means no further
dialogs can be created, though one is still showing.
3) Browser calls CancelModalDialogsForRenderManager, closing any current
dialogs.  No further dialogs can be created, and none are showing.
4) Browser sends a SwapOut message, with no PageGroupLoadDeferrer on the stack.

Sound right?

[darin (slow to review), 2013-11-05 20:12:08]

OK, makes sense. LGTM.

[Charlie Reis, 2013-11-05 20:26:06]

@cdn: Can you review view_messages.h for IPC owners?

[Cris Neckar, 2013-11-05 20:30:43]

On 2013/11/05 20:26:06, creis wrote:
> @cdn: Can you review view_messages.h for IPC owners?

IPC changes lgtm

[commit-bot: I haz the power, 2013-11-05 21:10:25]

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

[commit-bot: I haz the power, 2013-11-05 23:12:32]

Retried try job too often on linux_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-06 00:10:55]

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

[commit-bot: I haz the power, 2013-11-06 04:02:24]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[Charlie Reis, 2013-11-06 21:22:21]

Committed patchset #6 manually as r233371 (presubmit successful).