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.

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

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.

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.

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.

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 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 on attachment 8641150 [details] [diff] [review] patch v2 r=me

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

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.

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. :(

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

(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.

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

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?