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

RESOLVED FIXED in Firefox 22

Status

()

defect
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: mossop, Assigned: wchen)

Tracking

({regression})

Trunk
mozilla24
x86_64
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox22+ verified, firefox23+ fixed, firefox24 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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
Assignee

Updated

6 years ago
QA Contact: wchen
Assignee

Updated

6 years ago
Assignee: nobody → wchen
QA Contact: wchen
Assignee

Comment 1

6 years ago
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.
Attachment #734783 - Flags: review?(dtownsend+bugmail)
Comment on attachment 734783 [details] [diff] [review]
Always add observer to observe alert windows closing.

Can you add an automated test for this?
Comment on attachment 734783 [details] [diff] [review]
Always add observer to observe alert windows closing.

Waiting on a test here
Attachment #734783 - Flags: review?(dtownsend+bugmail)
Assignee

Comment 4

6 years ago
Attachment #734783 - Attachment is obsolete: true
Attachment #747801 - Flags: review?(dtownsend+bugmail)
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.
Attachment #747801 - Flags: review?(dtownsend+bugmail) → review-
Assignee

Comment 6

6 years ago
(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.
(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?
Assignee

Comment 8

6 years ago
(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.
(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.
Assignee

Comment 10

6 years ago
Attachment #747801 - Attachment is obsolete: true
Attachment #751760 - Flags: review?(dtownsend+bugmail)
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.
Attachment #751760 - Flags: review?(dtownsend+bugmail) → review+
https://hg.mozilla.org/mozilla-central/rev/6b070df3287e
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Assignee

Comment 14

6 years ago
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
Attachment #751760 - Flags: approval-mozilla-beta?
Attachment #751760 - Flags: approval-mozilla-aurora?
Attachment #751760 - Flags: approval-mozilla-beta?
Attachment #751760 - Flags: approval-mozilla-beta+
Attachment #751760 - Flags: approval-mozilla-aurora?
Attachment #751760 - Flags: approval-mozilla-aurora+
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
You need to log in before you can comment on or make changes to this bug.