Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(160)

Issue 6627063: Ignore JavaScript messages (alert/confirm/prompt) during unload handlers. (Closed)

Created:
9 years, 9 months ago by sreeram
Modified:
9 years, 7 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, tburkard+watch_chromium.org, Erik does not do reviews, PaweĊ‚ Hajdan Jr., jam, Aaron Boodman, pam+watch_chromium.org, brettw-cc_chromium.org, darin-cc_chromium.org, tburkard, cbentzel
Visibility:
Public.

Description

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.

Patch Set 1 #

Total comments: 1

Patch Set 2 : Added a browsertest #

Unified diffs Side-by-side diffs Delta from patch set Stats (+46 lines, -5 lines) Patch
M chrome/browser/browser_browsertest.cc View 1 1 chunk +19 lines, -0 lines 0 comments Download
M chrome/browser/extensions/extension_host.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/extensions/extension_host.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/prerender/prerender_contents.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/prerender/prerender_contents.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/tab_contents/background_contents.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/tab_contents/background_contents.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/tab_contents/web_contents_unittest.cc View 1 chunk +3 lines, -2 lines 0 comments Download
M chrome/common/render_messages_internal.h View 1 1 chunk +2 lines, -1 line 0 comments Download
M chrome/renderer/render_view.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M chrome/renderer/render_view.cc View 1 5 chunks +6 lines, -1 line 0 comments Download
M content/browser/renderer_host/render_view_host.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/renderer_host/render_view_host.cc View 1 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/renderer_host/render_view_host_delegate.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/tab_contents/background_contents.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/tab_contents/tab_contents.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/tab_contents/tab_contents.cc View 1 2 chunks +3 lines, -0 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
sreeram
Please review.
9 years, 9 months ago (2011-03-07 23:09:45 UTC) #1
Charlie Reis
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#newcode2407 ...
9 years, 9 months ago (2011-03-08 02:31:56 UTC) #2
sreeram
On 2011/03/08 02:31:56, creis wrote: > You'll need a test for this, likely something in ...
9 years, 9 months ago (2011-03-12 07:09:56 UTC) #3
Charlie Reis
9 years, 9 months ago (2011-03-14 23:56:10 UTC) #4
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.

Powered by Google App Engine
This is Rietveld 408576698