910501 - Selecting "Prevent this page from creating additional dialogs" on an alert/prompt/confirm should prevent all future alert/prompt/confirm on that page

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

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

STR:
  1. Go to http://dolske.net/mozilla/tests/prompt/sizes.html
  2. Use the buttons to quickly create prompts and dismiss them (spacebar is helpful for this)
  3. When the "Prevent this page from creating additional dialogs" checkbox appears, select it
  4. Continue to click on one of the buttons (or use spacebar) and note that eventually dialogs are created

Comment 1

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

The patch I'm working on in bug 856977 also fixes this issue, so I'll just assign myself

Comment 2

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

Attachment: Dialog storm

This testcase demonstrates the issue without the user needing to push any buttons (except the ok/cancel buttons of the dialog, of course)

I've recently learned (see bug 856977 comment 15 and bug 856977 comment 16) that the "prevent this page from creating additional dialogs" doesn't intend to prevent ALL additional dialogs from being created; it just intends to limit the rate at which new dialogs can be created.

Using the attached test case, I found that my version of Aurora limits new dialogs to 1 dialog every 3 seconds after I've selected "prevent this page from creating additional dialogs."

I find this behavior confusing: I expect a checkbox labeled "prevent this page from creating additional dialogs" to prevent the page from creating dialogs, without exception. Also, I don't really want the ability to rate-limit dialogs on a page; I want the ability to disable them completely.

I'm leaving this bug open, pending the outcome of discussion about this behavior.

Comment 3

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

Attachment: Patch v1

This patch should be applied on top of the patch in bug 856977.

This patch removes the rate-limiting behavior for dialogs and prefers a simpler enable/disable mechanism.

I haven't yet built/tested this patch.

Comment 4

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

Attachment: Patch v2

Comment 5

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

Attachment: Patch v3

:gavin for the browser piece.
:bz for the dom pieces.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 812865 [details] [diff] [review]
Patch v3

This seems fine, assuming we've agreed on the behavior.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 812865 [details] [diff] [review]
Patch v3

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>-              windowUtils.limitDialogs();
>+              windowUtils.disableDialogs();

Hmm, I guess this is just fixing a bug introduced by the patch for bug 856977? Ideally that patch would have just done this to begin with, right? Or am I missing something?

Comment 8

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

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #7)
> Comment on attachment 812865 [details] [diff] [review]
> Patch v3
> 
> >diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml
> 
> >-              windowUtils.limitDialogs();
> >+              windowUtils.disableDialogs();
> 
> Hmm, I guess this is just fixing a bug introduced by the patch for bug
> 856977? Ideally that patch would have just done this to begin with, right?
> Or am I missing something?

The patch in bug 856977 renamed "preventFurtherDialogs" to "limitDialogs" in order to make the existing behavior more obvious: The bug existed before the patch for bug 856977, it's just that now the function is named appropriately so it's more obvious that there's a bug.

The patch in bug 856977 hasn't landed yet, so I'm happy to include this change as part of that patch instead of this patch. We've already fixed another similar instance in that patch (bug 856977 comment 39).

Comment 9

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

Attachment: Patch v4

Removing the tabbrowser.xml change (this is moving to bug 856977) and carrying forward r+

Comment 10

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

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #8)
> (In reply to :Gavin Sharp (use [e-mail] for email) from comment
> #7)
> > Comment on attachment 812865 [details] [diff] [review]
> > Patch v3
> > 
> > >diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml
> > 
> > >-              windowUtils.limitDialogs();
> > >+              windowUtils.disableDialogs();
> > 
> > Hmm, I guess this is just fixing a bug introduced by the patch for bug
> > 856977? Ideally that patch would have just done this to begin with, right?
> > Or am I missing something?
> 
> The patch in bug 856977 renamed "preventFurtherDialogs" to "limitDialogs" in
> order to make the existing behavior more obvious: The bug existed before the
> patch for bug 856977, it's just that now the function is named appropriately
> so it's more obvious that there's a bug.

Oops, this is inaccurate. nsIDOMWindowUtils.preventFurtherDialogs calls nsGlobalWindow::PreventFurtherDialogs with an argument of 'true,' which would indeed disable dialogs (instead of rate-limiting them).
 
> The patch in bug 856977 hasn't landed yet, so I'm happy to include this
> change as part of that patch instead of this patch. We've already fixed
> another similar instance in that patch (bug 856977 comment 39).

I've updated the patches in this bug and in bug 856977.

Comment 11

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

Attachment: Patch v5

Rebasing to apply cleanly on top of latest patch in bug 856977

Comment 12

[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/b0d701ca9d9f

Comment 13

[Carsten Book [:Tomcat]]

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