609521 - browser_bug538331.js leaves alert windows open on Windows

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[(no longer active)]

While testing the patch in bug 607639, I realized that this test <http://mxr.mozilla.org/mozilla-central/source/browser/components/test/browser/browser_bug538331.js> leaves alert windows open when run on Windows.  This is because Windows doesn't provide a platform specific notification service, therefore we fall back to this code <http://mxr.mozilla.org/mozilla-central/source/toolkit/components/alerts/src/nsAlertsService.cpp#117> which shows a regular alert dialog.  This is not handled in the test.

Comment 1

[Robert Strong (they/them - no direct email)]

That is actually Firefox specific code so moving over to Firefox -> General.

These tests were written on Windows and there was no alert dialog ever displayed. There was an alert notification though which automatically disappeared.

Gavin, do you know what might have changed that caused this?

Comment 2

[Robert Strong (they/them - no direct email)]

Ehsan, perhaps this isn't an alert dialog and is instead an alert notification?

Comment 3

[Robert Strong (they/them - no direct email)]

Attachment: possible patch

We don't plan on using alert service notifications and I suspect this is what you are actually seeing hang around. Could you try out the patch? Thanks

Comment 4

[(no longer active)]

Attachment: Real test fix

The window that gets opened is alert.xul.

This patch is a better approach I think, because it handles alert windows opened without removing any test functionality, and it also works if in the future we port this test to platforms which don't have the native alerts service (in addition to Windows, that is.)

Comment 5

[(no longer active)]

I don't see the alert window either.  I suspect that it's just registered with the window watcher service, and for some reason fails to be displayed on screen somewhere during its creation process.

Which makes me think that this _could_ cause some weird intermittent oranges in the browser-chrome suite as well.

Comment 6

[Robert Strong (they/them - no direct email)]

Comment on attachment 488120 [details] [diff] [review]
Real test fix

So my suspicion was correct that it isn't the alert dialog as stated in comment #0 and is the alert service notification... whew!

I'm not a peer of this code so Gavin will need to review it.

btw: the check in this test is extremely minimal because there is no way to write a decent test for the platform specific implementations. All the check does is verify that a notification box is not shown when the custom xml attribute has showAlert without a showNotification so it isn't all that valuable,.

Comment 7

[Robert Strong (they/them - no direct email)]

(In reply to comment #5)
> I don't see the alert window either.  I suspect that it's just registered with
> the window watcher service, and for some reason fails to be displayed on screen
> somewhere during its creation process.
> 
> Which makes me think that this _could_ cause some weird intermittent oranges in
> the browser-chrome suite as well.
Very doubtful... it is the alert service notification slider (a.k.a. toast) that displays slides in, stays for a very short while, and slides out.

Comment 8

[(no longer active)]

The thing that is opened is a XUL window registered with the window watcher service with an "alert:alert" windowtype.  AFAIK the platform specific notifications are neither XUL windows nor have windowtypes.  I'm not sure why you insist that the thing that gets open is not an alert window.

Comment 9

[Robert Strong (they/them - no direct email)]

I never said it wasn't an alert type window... I did say it wasn't an alert dialog or as you stated a "regular alert dialog". If in JavaScript you call
alert("alert dialog");

then in JavaScript call
Components.classes["@mozilla.org/alerts-service;1"].getService(Components.interfaces.nsIAlertsService).showAlertNotification(null, "Alert Service", "Notification", false, "", null);

you will see the difference between a standard alert dialog and the alert service notification.

Comment 10

[Robert Strong (they/them - no direct email)]

Attachment: screenshot of an alert notification and an alert dialog

The main difference between the two as far as the tests are concerned and why I was very concerned when you reported this is that if there were a dialog displayed during the test then the test would hang until the dialog was dismissed and I know there wasn't any code to dismiss a dialog whereas the alert service notification slides into view, stays a short time, slides out of view, and destroys itself.

Comment 11

[(no longer active)]

Attachment: Real test fix

Of course, the observers should be removed when we're done...

Comment 12

[(no longer active)]

(In reply to comment #9)
> I never said it wasn't an alert type window... I did say it wasn't an alert
> dialog or as you stated a "regular alert dialog". If in JavaScript you call
> alert("alert dialog");
> 
> then in JavaScript call
> Components.classes["@mozilla.org/alerts-service;1"].getService(Components.interfaces.nsIAlertsService).showAlertNotification(null,
> "Alert Service", "Notification", false, "", null);
> 
> you will see the difference between a standard alert dialog and the alert
> service notification.

Yes, I guess I shouldn't have used the word "dialog".  My main point in filing this bug is that this test has the potential of leaving a XUL window open after it's finished (and it's the only test in the tree which does that, BTW), which _can_ lead to problems in other tests.  If not now, maybe in the future.  And I only found out about this using my patch in bug 607639...

Comment 13

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

Comment on attachment 488707 [details] [diff] [review]
Real test fix

>diff --git a/browser/components/test/browser/browser_bug538331.js b/browser/components/test/browser/browser_bug538331.js

Why do you have firstAttempt? I guess it's because setup()/finish() are called immediately in sequence, before load handler can run? I don't really like relying on a timeout for that, I think it would be better to track that state explicitly: set a flag as soon as you get domwindowopened, and clear it in domwindowclosed. If it's set when finish() is called, just return early. If it's set when the load handler runs, have it call finish().

>+var gAlertCatcher = {

>+  setup: function() {
>+    Services.obs.addObserver(this, "domwindowopened", false);
>+    Services.obs.addObserver(this, "domwindowclosed", false);

nit: Services.ww.registerNotification(this); can replace both of these (same with unregisterNotification).

Comment 14

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

Attachment: tweaked patch

I played around with this a bit and this is what I came up with. I tested that it also works if the test shows multiple notifications (by having the showAlert test run multiple times).

Comment 15

[(no longer active)]

Comment on attachment 488905 [details] [diff] [review]
tweaked patch

This looks good to me.  I'll just point out that this is not really the same as my patch, as my approach was to wait for the window to be actually closed before ending the test, but I guess that doesn't matter a lot.  Will you take care of landing this, or do you want me to?

Comment 16

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

Window closing is synchronous, AFAIK, so I think this patch waits until they are closed just as much as the other one did.

Comment 17

[(no longer active)]

Temporarily assigning this to myself to land the patch tomorrow.

Comment 18

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/1eccbdcbcd57