856977 - Alert is still possible in onbeforeunload

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: testcase

alert() in onbeforeunload is still possible, at least on Mac.

It was supposedly disallowed by the for bug 391834.  There's even a test, which somehow isn't turning the tree orange:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_unloaddialogs.js

(Split from bug 854902.)

Comment 1

[Biju]

NOTFIXED in winvista, win7, andriod(recent nightly) and ubundu(old nightly)

Comment 2

[Tom S [:evilpie]]

I just ran into this and it's quite curious. A quick debugging sessions suggests that mDialogsPermanentlyDisabled is not set correctly.

Comment 3

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

I'll take a look at this

Comment 4

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Time for a brain dump.

nsGlobalWindow seems to have 4 distinct mechanisms that keep track of whether dialogs should be allowed:

1. mDialogAbuseCount/mLastDialogQuitTime. We keep track of how quickly dialogs are being shown and how many times a dialog was shown too quickly (not enough time elapsed between the last dialog closing and the new dialog being created). When mDialogAbuseCount is above a threshold, we should not be showing more dialogs. mDialogAbuseCount is reset any time a dialog

2. mStopAbuseDialogs. This flag is set when the user explicitly chooses the "prevent this page from creating additional dialogs" checkbox from a dialog. If the comments are to be believed, it is possible for dialogs to be shown if mDialogAbuseCount gets reset, even if mStopAbuseDialogs is set!

3. mDialogsPermanentlyDisabled. This flag gets set in special cases like the window being closed, to indicate that no dialogs should be created for the rest of the lifetime of the window.

4. gPopupControlState. This is a static enum declared in nsGlobalWindow.cpp. It has 4 possible values, defined in nsPIDOMWindow.h: openAllowed, openControlled, openAbused, openOverridden. It's not super clear what subtleties exist, but roughly the first two mean 'dialogs allowed' and the second two mean 'dialogs disallowed'.
  The state is controlled through the functions ::PushPopupState, ::PopPopupState (both defined in nsGlobalWindow.cpp), nsGlobalWindow::PushPopupState, and nsGlobalWindow::PopPopupState, and through the use of the nsAutoPopupStatePusher class.
  This seems to be the mechanism that most code uses to affect dialog prevention.
  The functions for maintaining this state are named as if there is a stack backing gPopupControlState (Push*/Pop*), but there is no such stack. When consumers push a value, they receive the old value and are responsible for passing that value to the pop function later.

This bug appears to be one manifestation of a larger issue that can be summarized as "nsGlobalWindow sometimes gets confused about whether dialogs should be allowed". The other manifestations I'm aware of are bug 910501 and bug 910439.

So far, it seems like an ideal fix would be to simplify/consolidate our mechanisms for tracking whether dialogs are currently allowed.

Comment 5

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #4)
[...]
> 1. mDialogAbuseCount/mLastDialogQuitTime. We keep track of how quickly
> dialogs are being shown and how many times a dialog was shown too quickly
> (not enough time elapsed between the last dialog closing and the new dialog
> being created). When mDialogAbuseCount is above a threshold, we should not
> be showing more dialogs. mDialogAbuseCount is reset any time a dialog

Forgot to finish this sentence. mDialogAbuseCount is reset any time a dialog is shown that allowed a reasonable amount of time to elapse after the previous dialog was closed.

Comment 6

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Item 4 from my analysis in comment 4 actually applies only to popups (new windows/tabs), and not to dialogs (alert/prompt/confirm), so it can be ignored.

In nsDocumentViewer::PermitUnload, we disallow popups while a beforeunload dialog is active, but we don't do anything about dialogs [1]. It seems like we could fix this bug by correctly setting 'mStopAbuseDialogs' or 'mDialogsPermanentlyDisabled' and resetting it if the user chooses to remain on the page. There is no mechanism for temporarily setting those values, so we'd have to add one. This seems like a pretty easy fix at this point.

[1] https://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp?rev=b9e4777416d8#1092

Comment 7

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v1

This patch seems to fix the issue. As an added bonus, this patch also fixes bug 910501.

Still to do:
 - Finish writing comments
 - Update/fix test(s)

Comment 8

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v2

Writing tests is the worst.

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=2b393ae0aac2

If that turns green I'll r? a bunch of people :)

Comment 9

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v3

Between the time that I tested locally and the time that I pushed to try, a random '2' found its way into a source file. Nice.

On the plus side, the try run died almost immediately so it didn't waste a bunch of resources.

New try run:
https://tbpl.mozilla.org/?tree=Try&rev=5d2927f15797

Comment 10

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Browser patch v1

This patch contains just the browser/ pieces

Comment 11

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: DOM patch v1

This patch contains just the dom/ pieces

Comment 12

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Layout patch v1

This patch contains just the layout/ pieces

Comment 13

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Toolkit patch v1

This patch contains just the toolkit changes

Comment 14

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

I tried to minimize the number of reviewers by choosing reviewers who overlapped multiple modules that this patch changes.

Overview of what the patch does:
  - Replaces nsGlobalWindow::PreventFurtherDialogs and nsDOMWindowUtils::PreventFurtherDialogs with 3 functions that allow callers to enable, disable, and check the current dialog (enabled/disabled) status
  - Calls those functions from nsDocumentViewer::PermitUnload so that dialogs are disabled while the beforeunload event is being handled
  - Updates browser_unloaddialogs test to actually expect dialogs to be disabled during beforeunload handlers
  - Updates other callers of preventFurtherDialogs to call disableDialogs instead

Comment 15

[Boris Zbarsky [:bzbarsky]]

Tim, I think the intent of the existing code is that the "prevent this page from creating additional dialogs" thing will stop a dialog-storm, but allow the page to show a dialog 2 hours (or 15 minutes, or whatever) later if needed...

It sounds like the patch is changing this basic intent, right?

Comment 16

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Dialog storm

(In reply to Boris Zbarsky [:bz] from comment #15)
> Tim, I think the intent of the existing code is that the "prevent this page
> from creating additional dialogs" thing will stop a dialog-storm, but allow
> the page to show a dialog 2 hours (or 15 minutes, or whatever) later if
> needed...
> 
> It sounds like the patch is changing this basic intent, right?

It appears (see attached test) that "prevent this page from creating additional dialogs" limits the rate at which a page can display dialogs to 1 dialog every 3 seconds (maybe configurable via a pref?)

I had assumed that the intent of "prevent this page from creating additional dialogs" was to prevent the page from creating any dialogs, which is why I filed bug 910501 and wrote this patch this way.

So the short answer is "yes, this patch changes the basic intent of the 'prevent this page from creating additional dialogs' checkbox." If I had realized what the original intent was, I would have written this patch in a way that preserved it.

However, I find the original behavior confusing (hence me filing bug 910501 :) and I'd like to advocate for the new behavior. Would that discussion be better to have on a mailing list? And, in the meantime, should I rewrite this patch to preserve the rate-limiting behavior of the checkbox?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Yeah, I think the behavior change could use some input from UX and front-end engineers.

Let's see if we can make that conversation quick, then decide what to do here?  Sadly, none of our existing tests cover this behavior.

> 3 seconds (maybe configurable via a pref?)

Yes, "dom.successive_dialog_time_limit".

Comment 18

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Posted a message to firefox-dev referencing bug 910501:
  https://mail.mozilla.org/pipermail/firefox-dev/2013-September/000955.html

When that discussion completes, we can continue the efforts in this bug.

Comment 19

[Brian R. Bondy [:bbondy]]

Comment on attachment 805452 [details] [diff] [review]
Toolkit patch v1

Please re-request once a decision is made, that way I won't forget to review :)

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 805448 [details] [diff] [review]
DOM patch v1

Unsetting review request for now.

Comment 21

[Mark Hammond [:markh] [:mhammond]]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #4)

Swapping the quoting order:

> So far, it seems like an ideal fix would be to simplify/consolidate our
> mechanisms for tracking whether dialogs are currently allowed.

I'm inclined to think this might be worthwhile, although I believe there are some subtleties.  I just made a followup post in dev-platform about this.

However, for this specific bug:

> 3. mDialogsPermanentlyDisabled. This flag gets set in special cases like the
> window being closed, to indicate that no dialogs should be created for the
> rest of the lifetime of the window.

It looks like the problem here is that this flag isn't being set early enough in the "window being closed" case (or that it is being reset).  Do you know why that is?

I guess I'm asking if it makes sense to fix this specific bug by looking only at the issues with the mDialogsPermanentlyDisabled flag, and addressing the larger issues (ie, the "prevent more dialogs" question and some refactoring) in a different bug where we have the luxury of more time to consider our options?

Comment 22

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

> > 3. mDialogsPermanentlyDisabled. This flag gets set in special cases like the
> > window being closed, to indicate that no dialogs should be created for the
> > rest of the lifetime of the window.
> 
> It looks like the problem here is that this flag isn't being set early
> enough in the "window being closed" case (or that it is being reset).  Do
> you know why that is?
>
> I guess I'm asking if it makes sense to fix this specific bug by looking
> only at the issues with the mDialogsPermanentlyDisabled flag, and addressing
> the larger issues (ie, the "prevent more dialogs" question and some
> refactoring) in a different bug where we have the luxury of more time to
> consider our options?

When we're firing the beforeunload event and letting content handle it, we don't yet know whether the window is actually going to unload (close, navigate away, reload, etc). nsDocumentViewer::PermitUnload - the function that sends the beforeunload event and checks the return value of its handler - needs to be able to disable dialogs before sending the beforeunload event and then re-enable them if the beforeunload handler returned a string. 

The patch(es) from this bug create nsIDOMWindowUtils::EnableDialogs and nsIDOMWindowUtils::DisableDialogs for those purposes. Those functions (or equivalents) will need to exist for this bug to be fixed. They could be implemented in terms of mDialogsPermanentlyDisabled or some new member variable. They specifically can't be implemented in terms of mStopAbuseDialogs because of the confusing behavior described by bug 910501. The approach we've taken so far in this bug is to consolidate the various members into mAreDialogsEnabled. This consolidation can't take place if we decide that we want to keep the rate-limiting behavior of the checkbox.

Because of the following:
 1. The checkbox already only affects alert/prompt/confirm, and will continue to only affect alert/prompt/confirm
 2. The discussion about the behavior of the checkbox indicates that bug 910501 is a valid bug

I feel pretty comfortable landing this behavior change and minor refactoring now, and opening bugs to discuss/fix issues with window.print() and showModalDialog() (and perhaps others) later.

Does that sound reasonable?

Comment 23

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Per discussion in #developers, I'll update the patch in this bug to not affect the behavior of the checkbox. Expect that updated patch when I get back from PTO next week.

Comment 24

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: DOM patch v2

Comment 25

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Layout patch v2

Comment 26

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Toolkit patch v2

Comment 27

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Browser patch v2

Comment 28

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 810790 [details] [diff] [review]
DOM patch v2

Updated this patch to maintain the rate-limiting behavior of the "Prevent this page from creating additional dialogs" checkbox

Comment 29

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 810792 [details] [diff] [review]
Layout patch v2

Carrying forward r+

Comment 30

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 810793 [details] [diff] [review]
Toolkit patch v2

Re-requesting review

Comment 31

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 810794 [details] [diff] [review]
Browser patch v2

Re-requesting review

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 810790 [details] [diff] [review]
DOM patch v2

LimitDialogs() could use some documentation in the header.

r=me with that

Comment 33

[Brian R. Bondy [:bbondy]]

Comment on attachment 810794 [details] [diff] [review]
Browser patch v2

Review of attachment 810794 [details] [diff] [review]:
-----------------------------------------------------------------

There's something wrong with this patch.  In particular the patch for file browser_unloaddialogs.js is a diff of a diff.

Comment 34

[Brian R. Bondy [:bbondy]]

Comment on attachment 810793 [details] [diff] [review]
Toolkit patch v2

Review of attachment 810793 [details] [diff] [review]:
-----------------------------------------------------------------

I thought we weren't changing the existing handling based on comment 23? Maybe I don't fully understand the patch though.

Comment 35

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v4

Eep, sorry about those obvious mistakes. This rollup patch should have those fixed, and have the additional documentation requested by bz. I'll double-check that everything looks OK with this patch, then request review.

Comment 36

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v5

Turns out you have to update the uuid of an interface if you change the interface.

Comment 37

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 811633 [details] [diff] [review]
Patch v5

Everything seems OK in local testing. Also running this through try:
  https://tbpl.mozilla.org/?tree=Try&rev=032fb48f1663

bz - sorry to ask for re-review, but limitDialogs is now exposed through nsIDOMWindowUtils, so it seemed like enough had changed to warrant another once-over.

Comment 38

[Brian R. Bondy [:bbondy]]

Comment on attachment 811633 [details] [diff] [review]
Patch v5

Review of attachment 811633 [details] [diff] [review]:
-----------------------------------------------------------------

for the /browser and /toolkit parts

Comment 39

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 811633 [details] [diff] [review]
Patch v5

In backgroundPageThumbsContent.js, I'd think you really want to prevent dialogs, not limit them.  Limiting makes no sense there.

r=me on DOM bits.

Comment 40

[Mark Hammond [:markh] [:mhammond]]

(In reply to Boris Zbarsky [:bz] from comment #39)
> In backgroundPageThumbsContent.js, I'd think you really want to prevent
> dialogs, not limit them.  Limiting makes no sense there.

Indeed - they must be fully disabled for thumbnails.

Comment 41

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v6

Addressing comments, carrying forward r+

Comment 42

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v7

Changing the limitDialogs call in tabbrowser.xml to a disableDialogs call (see bug 910501 comment 8) and carrying forward r+

Comment 43

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v8

nsIDOMWindowUtils.limitDialogs function is unnecessary so I'm removing it.

Comment 44

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Ran this through try one last time:
  https://tbpl.mozilla.org/?tree=Try&rev=1ebd9b65e7b2

Landed on m-i:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/4f19ea7c5390

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4f19ea7c5390