1188665 - nsIDOMWindowUtils.disableDialogs() should disable onbeforeunload dialogs

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Drew Willcoxon :adw]

In a new profile:

1. Mute your sound to avoid annoyance in the next step
2. Visit http://lotto.pch.com/
3. Open a new tab

The lotto.pch.com tile doesn't have a thumbnail.  If you open the browser console you'll see (eventually):

TypeError: window.gBrowser is undefined RemotePrompt.jsm:39:9

The problem happens when backgroundPageThumbsContent.js tries to load about:blank after completing the capture.  The page throws up an are-you-sure-you-want-leave prompt, which ends up here: http://mxr.mozilla.org/mozilla-central/source/browser/modules/RemotePrompt.jsm#39

Because of the error RemotePrompt never responds with a Prompt:Close message so the navigation isn't allowed to continue (apparently), which blocks the captured thumbnail from being written to disk -- and keeps that turdly page loaded in the thumbnailer browser for 60 seconds, until the browser is destroyed.

We did some work to block popups in the browser, so I'm not sure yet why this is happening: bug 875157, bug 917610

I'll look into it more.

Comment 1

[Jefferson]

Seems the thumbnailer should disable/strip/ignore window.onbeforeunload handlers.

Comment 2

[Drew Willcoxon :adw]

Looks like the problem is around here: http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#1197  It just doesn't check whether dialogs are disabled.  It even disables dialogs beforehand, so that the page can't call alert() etc. from onbeforeunload, but lotto.pch.com returns a string from onbeforeunload, and the code here consequently puts up a prompt.  Wonder why it doesn't respect disableDialogs()...  Seems like maybe a deliberate decision.

Comment 3

[Drew Willcoxon :adw]

Just chatted with Tim, who did bug 856977, and he said that it wasn't a deliberate decision to make disableDialogs() exclude onbeforeunload dialogs triggered by pages returning strings, and that it sounds OK to make it not exclude them.  I'll work on a patch for that and ask some people for feedback/review.

Failing that we should just change RemotePrompt I guess.

Comment 4

[Drew Willcoxon :adw]

Oh and looking at the old code before 856977, it doesn't look like the old preventFurtherDialogs prevented these dialogs either, so this must just be a case that we never noticed or thought about.

Comment 5

[Drew Willcoxon :adw]

Attachment: make disableDialogs() disable onbeforeunload dialogs

Boris, it would be nice if nsIDOMWindowUtils.disableDialogs() disabled onbeforeunload dialogs.  Seems like a bug that it does not.  Please see earlier comments for context.  Is this OK?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8640782 [details] [diff] [review]
make disableDialogs() disable onbeforeunload dialogs

So with this patch if dialogs are disabled on one subframe, that will prevent the dialog for other subframes of the same page... but in an order-dependent way.  That seems weird.  I would prefer you used a function-local boolean here that gets checked around the "put up the dialog" block instead of writing to the global *aShouldPrompt state.

Comment 7

[Drew Willcoxon :adw]

Attachment: patch v2

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8641150 [details] [diff] [review]
patch v2

r=me

Comment 9

[Drew Willcoxon :adw]

Thanks!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=18be2e721c8c

Comment 10

[Drew Willcoxon :adw]

Well, this breaks browser_test_new_window_from_content.js and maybe others.

1. Set browser.link.open_newwindow=1
2. Set browser.link.open_newwindow.restriction=0
3. Open test_new_window_from_content_child.html or any page with <a onclick="return openWindow();"> and a beforeunload listener
4. Click the link

Before this patch: you get an onbeforeunload dialog
After this patch: you don't, and the page navigates away

The problem is that utils->AreDialogsEnabled(&dialogsAreEnabled) fails because the caller is not chrome.  Therefore dialogsAreEnabled remains false and the dialog isn't shown.

Trying to figure out what that means.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Ugh.  Calling that method from nsDocumentViewer is just buggy; I'm not sure why that hasn't bitten us before.

We should either remove that security check from AreDialogsEnabled, or ask the window directly, not the windowutils.  :(

Comment 12

[Drew Willcoxon :adw]

Attachment: patch v3

This caller is AreDialogsEnabled's only caller in m-c, but AreDialogsEnabled is the implementation of an idl method, and the implementations of the two related methods, DisableDialogs and EnableDialogs, both have the same assertion.  In fact many of the nsIDOMWindowUtils method implementations have that assertion (if not all, there are a lot, I didn't look through all of them), so it seems prudent to keep it.

So this calls the window directly for all three dialog-related calls.  Thanks for your help!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1786299ff724

Comment 13

[Drew Willcoxon :adw]

(In reply to Boris Zbarsky [:bz] from comment #11)
> I'm not sure why that hasn't bitten us before.

It's pretty subtle.  Looks like this was never working exactly as intended when callers are not chrome.  The chrome check fails, so dialogsWereEnabled remains false, so dialogs are never reenabled when they should be after dispatching beforeunload.

So in order to see any "problem," you'd have to be on a page that puts up an beforeunload dialog, you cancel the navigation, and then you'd have to notice that the page doesn't put up any dialogs at all anymore, if it even did in the first place.  Except for more beforeunload dialogs when you try to leave.

Comment 14

[Drew Willcoxon :adw]

Try run with a fresher tree: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58daa2355f52

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8641302 [details] [diff] [review]
patch v3

r=me, but those windowutils methods are still footguns.  Please file a followup to either remove them or if used from script to binaryname them to something that is clearly for script only?

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4f7a655c74a

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a4f7a655c74a