856858 - Alert service notifications open in the wrong place and don't auto-dismiss

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Dave Townsend [:mossop]]

In current nightly open the error console and paste this into the evaluate box:

Components.classes["@mozilla.org/alerts-service;1"].getService(Components.interfaces.nsIAlertsService).showAlertNotification("", "title", "text")

Hit evaluate and you'll see a notification appear in the lower right as expected. Wait for it to auto-dismiss or close it and then click evaluate again. Now the alter will appear in the upper left and never go away until you close it. You also get this in the error console:

Timestamp: 4/1/2013 4:04:50 PM
Error: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindow.outerHeight]
Source File: chrome://global/content/alerts/alert.js
Line: 107

Sometimes you will also see:

Timestamp: 4/1/2013 4:04:20 PM
Error: NS_ERROR_XPC_BAD_CONVERT_NATIVE: Component returned failure code: 0x8057000a (NS_ERROR_XPC_BAD_CONVERT_NATIVE) [nsIAlertsService.showAlertNotification]
Source File: javascript:%20Components.classes["@mozilla.org/alerts-service;1"].getService(Components.interfaces.nsIAlertsService).showAlertNotification("",%20"title",%20"text")
Line: 1

Comment 1

[William Chen [:wchen]]

Attachment: Always add observer to observe alert windows closing.

The problem here is that an observer isn't always added to watch for alert windows being closed so the list of active notifications isn't consistent and new alerts will try to position themselves relative to closed windows and get exceptions trying to access properties of the closed windows.

Comment 2

[Dave Townsend [:mossop]]

Comment on attachment 734783 [details] [diff] [review]
Always add observer to observe alert windows closing.

Can you add an automated test for this?

Comment 3

[Dave Townsend [:mossop]]

Comment on attachment 734783 [details] [diff] [review]
Always add observer to observe alert windows closing.

Waiting on a test here

Comment 4

[William Chen [:wchen]]

Attachment: Always add observer to observe alert windows closing. v2

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 747801 [details] [diff] [review]
Always add observer to observe alert windows closing. v2

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

This bug happens when you show an alert after dismissing a previous alert. The test doesn't seem to simulate that case. The bug is also about where the alert appears and whether it automatically dismisses itself. The test doesn't seem to verify either of those things. It should probably also wait until any alerts have been dismissed before continuing on to the next test.

Comment 6

[William Chen [:wchen]]

(In reply to Dave Townsend (:Mossop) from comment #5)
> Comment on attachment 747801 [details] [diff] [review]
> Always add observer to observe alert windows closing. v2
> 
> Review of attachment 747801 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This bug happens when you show an alert after dismissing a previous alert.
> The test doesn't seem to simulate that case. The bug is also about where the
> alert appears and whether it automatically dismisses itself. The test
> doesn't seem to verify either of those things. It should probably also wait
> until any alerts have been dismissed before continuing on to the next test.

Waiting until or checking that the alert is dismissed is tricky because normally an observer would be used but in this case having an observer changes the behavior such that the bug does not occur. The test case I provided will create multiple alerts with the same name and this causes the previous alert with the same name to be to dismissed.

Also, checking things like position of the alert would make this test fragile, instead I think it's better to check for other symptoms of this bug like the exception being thrown. In addition, without observers it's difficult to tell when the alert is visible in order to check for position.

Comment 7

[Dave Townsend [:mossop]]

(In reply to William Chen [:wchen] from comment #6)
> (In reply to Dave Townsend (:Mossop) from comment #5)
> > Comment on attachment 747801 [details] [diff] [review]
> > Always add observer to observe alert windows closing. v2
> > 
> > Review of attachment 747801 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This bug happens when you show an alert after dismissing a previous alert.
> > The test doesn't seem to simulate that case. The bug is also about where the
> > alert appears and whether it automatically dismisses itself. The test
> > doesn't seem to verify either of those things. It should probably also wait
> > until any alerts have been dismissed before continuing on to the next test.
> 
> Waiting until or checking that the alert is dismissed is tricky because
> normally an observer would be used but in this case having an observer
> changes the behavior such that the bug does not occur. The test case I
> provided will create multiple alerts with the same name and this causes the
> previous alert with the same name to be to dismissed.
> 
> Also, checking things like position of the alert would make this test
> fragile, instead I think it's better to check for other symptoms of this bug
> like the exception being thrown. In addition, without observers it's
> difficult to tell when the alert is visible in order to check for position.

According to my notes the exception isn't always thrown. As for position being fragile presumably we can at least assume that a second alert should appear at the same place as the first right?

Why does using observers to monitor the window opening/closing impact this case?

Comment 8

[William Chen [:wchen]]

(In reply to Dave Townsend (:Mossop) from comment #7)
> According to my notes the exception isn't always thrown. As for position
> being fragile presumably we can at least assume that a second alert should
> appear at the same place as the first right?
> 
> Why does using observers to monitor the window opening/closing impact this
> case?
This bug happens when an alert is created without an observer. Listening to events on the alert window is not a problem, however, we need an observer on the alert or some other way to figure out when the alert window has been created in order to attach event listeners and/or check for the position of the window.

Comment 9

[Dave Townsend [:mossop]]

(In reply to William Chen [:wchen] from comment #8)
> (In reply to Dave Townsend (:Mossop) from comment #7)
> > According to my notes the exception isn't always thrown. As for position
> > being fragile presumably we can at least assume that a second alert should
> > appear at the same place as the first right?
> > 
> > Why does using observers to monitor the window opening/closing impact this
> > case?
> This bug happens when an alert is created without an observer. Listening to
> events on the alert window is not a problem, however, we need an observer on
> the alert or some other way to figure out when the alert window has been
> created in order to attach event listeners and/or check for the position of
> the window.

Oh, I'm talking about something like the domwindowopened observer service notification.

Comment 10

[William Chen [:wchen]]

Attachment: Always add observer to observe alert windows closing. v3

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 751760 [details] [diff] [review]
Always add observer to observe alert windows closing. v3

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

Awesome, thank you

::: toolkit/components/alerts/test/test_multiple_alerts.html
@@ +34,5 @@
> +  // when a XUL alert window does get created.
> +  var xulNotSupported = false;
> +  var timer = setTimeout(function() {
> +    ok(true, "Platform does not use XUL alerts.");
> +    xulNotSupported = true;

You never unregister the window observer in this case so the test will leak. Just do so here and you shouldn't need the xulNotSupported variable.

Comment 12

[William Chen [:wchen]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b070df3287e

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6b070df3287e

Comment 14

[William Chen [:wchen]]

Comment on attachment 751760 [details] [diff] [review]
Always add observer to observe alert windows closing. v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 782211
User impact if declined: Alert windows may open in the wrong place and not auto-dismiss.
Testing completed (on m-c, etc.): The patch landed on nightly with tests included.
Risk to taking this patch (and alternatives if risky): No known risks.
String or IDL/UUID changes made by this patch: None

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3674c40dc33c
https://hg.mozilla.org/releases/mozilla-beta/rev/5dc353dd2cec

Comment 16

[Simona Badau, Desktop QA]

Verified as fixed on Firefox 22 beta 4: - the alert service notification opens in the right place, auto-dismisses and there are no errors present in the Error Console.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0
Build ID: 20130605070403