Ignore JavaScript messages (alert/confirm/prompt) during unload handlers.

Ignore JavaScript messages (alert/confirm/prompt) during unload handlers.

When in an unload handler (onunload() or onbeforeunload()), calls to alert()
will result in no dialog being shown. Calls to confirm() or prompt() will also
show no dialog, and will receive false/empty-string as the return value.

If onbeforeunload() returns a non-NULL value, the "Stay or Leave?" dialog is
still shown as usual.

A page that has an onbeforeunload() that shows an alert/confirm/prompt as well
as returns a non-NULL value behaves differently depending on how the unload was
initiated:
+ If it was a within-renderer navigation (such as clicking a link), the
  renderer continues unloading as usual. I.e., the alert/confirm/prompt is
  not displayed, but the "Stay or Leave?" dialog is, and the user can choose
  whether to continue the navigation.
+ If it was browser-initiated (such as typing into the omnibox), the page is
  killed (due to the "is_waiting && are_javascript_messages_suppressed_" check
  in render_view_host.cc). I.e., the alert/confirm/prompt is not displayed as
  before, but the "Stay or Leave?" dialog appears briefly and disappears as the
  page is killed, and the navigation happens anyway.

This CL doesn't address the showModalDialog() JavaScript call, because:
1. Currently such dialogs are not modal at all (http://crbug.com/16045), so
   while the page is paused (waiting for the dialog to be closed), the user can
   ignore the dialog and interact with the tab anyway.
2. It wreaks havoc inside unload handlers already (see http://crbug.com/44357).
3. It seems non-trivial to handle: The RenderViewHost of the main frame doesn't
   get any message. The dialog window is created in webkit, and runModal() is
   called on the RenderView of the dialog (and not the main frame).

BUG=68780
TEST=Open a page that has an alert() message in its onunload() handler. Navigate away. No dialog should appear.

[sreeram, 2011-03-07 23:09:45]

Please review.

[Charlie Reis, 2011-03-08 02:31:56]

You'll need a test for this, likely something in browser_tests.

http://codereview.chromium.org/6627063/diff/1/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6627063/diff/1/chrome/renderer/render_view.cc#...
chrome/renderer/render_view.cc:2407: frame->pageDismissalEventBeingDispatched(),
It seems strange to me to thread a new parameter all the way over to the browser
process when we already know that we shouldn't show the message.  Couldn't we
intercept this here or even sooner and avoid calling RunJavaScriptMessage?

I think that would fix the broken behavior you describe for browser-initiated
navigations, too, since the browser wouldn't get confused about its current
state.

[sreeram, 2011-03-12 07:09:56]

On 2011/03/08 02:31:56, creis wrote:
> You'll need a test for this, likely something in browser_tests.

Done. I've verified that the test fails without the bugfix and passes with the
fix.

> http://codereview.chromium.org/6627063/diff/1/chrome/renderer/render_view.cc
> File chrome/renderer/render_view.cc (right):
> 
>
http://codereview.chromium.org/6627063/diff/1/chrome/renderer/render_view.cc#...
> chrome/renderer/render_view.cc:2407:
frame->pageDismissalEventBeingDispatched(),
> It seems strange to me to thread a new parameter all the way over to the
browser
> process when we already know that we shouldn't show the message.  Couldn't we
> intercept this here or even sooner and avoid calling RunJavaScriptMessage?
> 
> I think that would fix the broken behavior you describe for browser-initiated
> navigations, too, since the browser wouldn't get confused about its current
> state.

I think it's better to do it in the browser, for these reasons:

1. Most of the current logic to suppress alerts happens in TabContents. The
additional check introduced in this CL logically belongs there. It would be
weird to have some of the suppression logic in TabContents, and some in
RenderView.

2. In general, the suppression logic belongs in the browser, I think. For one,
it could conceivably be controlled by a browser option. Second, I think the job
of deciding whether to show a dialog belongs in the browser. There's no inherent
reason why a RenderView should be making these kinds of browser/window
decisions.

3. Some RenderViewHost::Delegates may NOT want to suppress dialogs. I don't
fully understand the extension model, but extension_host.cc appears to do so. It
would be awkward to try and add special handling for such browser delegates in
the renderer.

4. The beforeunload discrepancy mentioned in the CL description already exists.
I.e., under some conditions, even without this CL, alerts in beforeunload will
kill the page, and in some cases, will not. This CL just adds another instance
of the same discrepancy.

A couple of other notes:

+ I believe Darin wants this to not be in Chromium land at all (i.e., neither in
the browser nor the renderer), but instead to be in webkit/webcore. See the
recent comments at https://bugs.webkit.org/show_bug.cgi?id=55844. So, I might
withdraw this CL, pending his decision.

+ I added some comments about showModalDialog() to the CL description.

[Charlie Reis, 2011-03-14 23:56:10]

On 2011/03/12 07:09:56, sreeram wrote:
> On 2011/03/08 02:31:56, creis wrote:
> > You'll need a test for this, likely something in browser_tests.
> 
> Done. I've verified that the test fails without the bugfix and passes with the
> fix.

Thanks!

> I think it's better to do it in the browser, for these reasons:
> 
> 1. Most of the current logic to suppress alerts happens in TabContents. The
> additional check introduced in this CL logically belongs there. It would be
> weird to have some of the suppression logic in TabContents, and some in
> RenderView.
> 
> 2. In general, the suppression logic belongs in the browser, I think. For one,
> it could conceivably be controlled by a browser option. Second, I think the
job
> of deciding whether to show a dialog belongs in the browser. There's no
inherent
> reason why a RenderView should be making these kinds of browser/window
> decisions.
> 
> 3. Some RenderViewHost::Delegates may NOT want to suppress dialogs. I don't
> fully understand the extension model, but extension_host.cc appears to do so.
It
> would be awkward to try and add special handling for such browser delegates in
> the renderer.
> 
> 4. The beforeunload discrepancy mentioned in the CL description already
exists.
> I.e., under some conditions, even without this CL, alerts in beforeunload will
> kill the page, and in some cases, will not. This CL just adds another instance
> of the same discrepancy.
> 
> A couple of other notes:
> 
> + I believe Darin wants this to not be in Chromium land at all (i.e., neither
in
> the browser nor the renderer), but instead to be in webkit/webcore. See the
> recent comments at https://bugs.webkit.org/show_bug.cgi?id=55844. So, I might
> withdraw this CL, pending his decision.
> 

I think I agree with Darin here, and it looks like you guys are arriving at a
good solution.  It's better to let WebKit prevent modal dialogs during unload
than route the messages all the way to TabContents.  (Putting it behind an
#ifdef seems reasonable while we discuss the platform change with standards
folks.)

Charlie
 
> + I added some comments about showModalDialog() to the CL description.