917610 - backgroundPageThumbsContent.js's use of nsIDOMWindowUtils.preventFurtherDialogs doesn't work as expected

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Drew Willcoxon :adw]

Attachment: patch

Bug 875157 added a call to nsIDOMWindowUtils.preventFurtherDialogs, but unfortunately preventFurtherDialogs only prevents further dialogs on the current inner window, and not all future inner windows of the outer window, so it's not working the way we expected.

nsGlobalWindow::PreventFurtherDialogs sets mDialogsPermanentlyDisabled on the current inner window: http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#2986

nsGlobalWindow::DialogsAreBlocked, called by nsGlobalWindow::Alert, checks mDialogsPermanentlyDisabled on the current inner window: http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#2889

I found this while debugging bug 917609.  In the noAlert test, nsPrompter.js's openModalWindow is reached but it should not be, because nsGlobalWindow::Alert should return NS_ERROR_NOT_AVAILABLE early because DialogsAreBlocked should return true.  That's not happening.  If you comment out all the other subtests, though, it does happen, because preventFurtherDialogs is called on the same inner window used by the subtest.

The alert() ends up failing even without preventFurtherDialogs because winUtils.isParentWindowMainWidgetVisible() in openModalWindow throws an exception, so I wonder whether we even need to call it.

Comment 1

[Drew Willcoxon :adw]

Comment on attachment 806347 [details] [diff] [review]
patch

Er, need to remove the observer or use a weak reference.

Comment 2

[Drew Willcoxon :adw]

Attachment: patch 2

Removes the call altogether like we talked about on IRC.

Comment 3

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

Some of the code in comment 0 will (hopefully) be replaced/simplified in bug 856977 and/or bug 910501. They aren't big changes, and regardless of whether this patch or one of those land first, it should be easy to rebase. Just something to be aware of.

Comment 4

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

Comment on attachment 806358 [details] [diff] [review]
patch 2

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

hrm - sorry to mess you around, but I just did a search for preventFurtherDialogs(), and I found 2 additional uses:

* It is used to prevent window.print() from displaying the dialog.  I tried to write a test for this case but failed - the printing is deferred until after the document is loaded - by which time we've taken the snapshot and unloaded the document.  However, stepping through in the debugger, I see that nsGlobalWindow really does want to print.

* It is used to prevent showModalDialog() from displaying.  However, the normal popup blocker kicks in here as the call wasn't made via a user event (eg, a click).  Given there seems no way to actually cause a user event in this context, it seems it would be impossible.  (I *think* window.open() calls are the same as this - they are blocked from actually opening by the normal popup blocker)

I'm not sure what to do about this - experimentally, printing doesn't work, but theoretically it could.  It kinda sucks to have this code in-place "just in case" when we can't actually demonstrate a problem - but the fact print *tries* to print bothers me.  What do you think?

Comment 5

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

Gavin,
  I'd like your thoughts on this.  The short(ish) version:

We call nsIDOMWindowUtils.preventFurtherDialogs() to prevent alert() etc working.  However, that's not working as expected - nsGlobalWindow isn't actually preventing the alert, but instead the "is window hidden" check in nsPrompter.js is - the same check we rely on to prevent auth dialogs.

Given the above, and as all the tests pass without the call to preventFurtherDialogs(), Drew and I figured that removing the call might be reasonable - and if actual problems arose, we could resolve them and ensure we have test coverage that actually catches the problem, rather than relying on faith that the code still does what we thought it did back when we added it.

However, comment 4 talks about a few other *theoretical* problem that might arise without the preventFurtherDialogs call - and I can't work out how to get test coverage for those theoretical cases.

So my question is: should we keep (and fix) the preventFurtherDialogs call even though we can't demonstrate a need for it in either manual or automated testing?

Comment 6

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

Seems to me like we should keep it. You can test the unrealistic showModalDialog case by simulating a user event in the background tab, I suppose?

Comment 7

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

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #6)
> You can test the unrealistic
> showModalDialog case by simulating a user event in the background tab, I
> suppose?

I don't think that will work - as we kill the document as soon as it completes loading, it's probably possible to simulate the event, but probably not for the dialog to actually open.  A similar problem exists with window.print() - we can arrange for the print request to start and be queued, but it doesn't complete.

IOW, this isn't to argue we shouldn't keep that code, but to suggest it might be impossible to test.

Comment 8

[Drew Willcoxon :adw]

Attachment: nsIObserver patch

I see the same two additional use cases you mention.  Thanks for digging in to see whether they're possible in practice.

I guess I lean toward keeping it in, because preventing further dialogs is exactly what we want to do, forget everything else.  To leave it out would be to rely on two implementation details: one, that the print dialog is deferred until the document loads, and two, that alerts are blocked because isParentWindowMainWidgetVisible throws an exception.  It would also assume that there will be no new types of dialogs blocked by preventFurtherDialogs in the future, which is maybe an OK assumption, but who knows.

Comment 9

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

Comment on attachment 807556 [details] [diff] [review]
nsIObserver patch

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

Looks good to me - as discussed earlier we can't verify this with tests, but Drew tells me he verified this by instrumenting nsGlobalWindow - which is the best thing I can think of too.

Comment 10

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/fx-team/rev/d4c602aa15b7

Comment 11

[Carsten Book [:Tomcat]]

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

Comment 12

[Drew Willcoxon :adw]

Comment on attachment 807556 [details] [diff] [review]
nsIObserver patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 875157
User impact if declined: minimal
Testing completed (on m-c, etc.): tryserver with this patch and two, related others I'd like to uplift: https://tbpl.mozilla.org/?tree=Try&rev=c8eda3703066
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Patch applies cleanly to Aurora.

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 807556 [details] [diff] [review]
nsIObserver patch

Low risk regression fix, OK for uplift.

Comment 14

[Drew Willcoxon :adw]

Comment on attachment 807556 [details] [diff] [review]
nsIObserver patch

Whoops, I meant to cancel these approval requests earlier today.  We're not planning on uplifting this anymore because we're not going to uplift bug 927688 to Aurora/26 after all.

I won't undo the a+, but again, we decided not to uplift this.