611540 - [SeaMonkey] mochitest-plain-5: intermittent test_alerts.html failure "Observer is notified within 15 seconds", leaking to test_alerts_noobserve.html or test_Microformats.html

Status: VERIFIED FIXED

(Core :: Widget: Gtk, defect)

Description

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1289512445.1289514310.13299.gz&fulltext=1
OS X 10.5 comm-central-trunk debug test mochitests-5/5 on 2010/11/11 13:54:05
{
1360 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/alerts/test/test_alerts_noobserve.html | Observer is notified within 15 seconds
}

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1289515555.1289516609.24308.gz&fulltext=1
Linux comm-central-trunk debug test mochitests-5/5 on 2010/11/11 14:45:55
{
1357 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/alerts/test/test_alerts.html | Observer is notified within 15 seconds
...
1363 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/microformats/tests/test_Microformats.html | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - is is not defined at http://mochi.test:8888/tests/toolkit/components/alerts/test/test_alerts.html:25
JavaScript error: http://mochi.test:8888/tests/toolkit/components/alerts/test/test_alerts.html, line 25: is is not defined
}

Actually, I already noticed the "leak" potential of this test while investigating bug 589471 and I have some local fixes for that.

I assume the "timeout" itself is just due to some slowness: waiting longer should prevent that.

Comment 1

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1289541369.1289542441.10862.gz
Linux comm-central-trunk debug test mochitests-5/5 on 2010/11/11 21:56:09
(timeout, on the right test)

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Use executeSoon(), Wait for 20 seconds instead of 15, Remove cookie in "noobserve" case, Have more explicit check messages

Comment 3

[Serge Gautherie (:sgautherie)]

Comment on attachment 490113 [details] [diff] [review]
(Av1) Use executeSoon(), Wait for 20 seconds instead of 15, Remove cookie in "noobserve" case, Have more explicit check messages

http://hg.mozilla.org/try/pushloghtml?changeset=87b8ed6323d6
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-87b8ed6323d6

MacOSX fails in an (un)expected way:
{
231 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/alerts/test/test_alerts.html | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - uncaught exception: [Exception... "Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]"  nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)"  location: "JS frame :: http://mochi.test:8888/tests/toolkit/components/alerts/test/test_alerts.html :: <TOP_LEVEL> :: line 60"  data: no] at :0
234 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/alerts/test/test_alerts_noobserve.html | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - uncaught exception: [Exception... "Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]"  nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)"  location: "JS frame :: http://mochi.test:8888/tests/toolkit/components/alerts/test/test_alerts_noobserve.html :: <TOP_LEVEL> :: line 33"  data: no] at :0
}

Cc["@mozilla.org/alerts-service;1"] is available,
getService(Ci.nsIAlertsService) is unvailable, <== NS_ERROR_XPC_GS_RETURNED_FAILURE !
showAlertNotification(...) is not yet called.

I wanted to try this because I read
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIAlertsService
{
showAlertNotification()
...
Exceptions thrown
NS_ERROR_NOT_AVAILABLE
    Unable to display the notification; this may happen, for example, on Mac OS X if Growl is not installed.
}
and
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/alerts/public/nsIAlertsService.idl
{
65     * @throws NS_ERROR_NOT_AVAILABLE If the notification cannot be displayed.
...
74    void showAlertNotification(in AString  imageUrl, 
}
so I thought the failure would happen on the function call, not the service getter.

As I'm not too familiar with such a case,
*is the current behavior actually wrong?
*are (one of) the docs wrong?
*did I misunderstand which behavior to expect?

Anyway, I'll attach another patch to support (again) current behavior.

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) Add executeSoon(), Wait for 20 seconds instead of 15, Remove cookie in "noobserve" case, Have more explicit check messages

Av1, with getService() failure handling (added back).

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why do we need timeoutSafety? Why can't we just let the normal mochitest timeout occur?

Comment 6

[Serge Gautherie (:sgautherie)]

Attachment: (Av2a) Add executeSoon(), Wait for 30 seconds instead of 15, Remove cookie in "noobserve" case, Have more explicit check messages

Av2, with a (doubled =) 30 s timeout,
and an explicit comment which should have been added in bug 567931.


Fwiw, I set the safety up _after_ calling showAlertNotification():
should I rather (continue to) expect that the callback can be called in the meantime?

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to comment #3)
> As I'm not too familiar with such a case,
> *is the current behavior actually wrong?
> *are (one of) the docs wrong?
> *did I misunderstand which behavior to expect?

Fwiw, unanswered bug 492477 comment 3 was:
{
Dão Gottwald      2009-05-12 11:35:58 PDT

(In reply to comment #1)
> This is simple, the test boxes don't have Growl installed so there is no alerts
> system to test.

Do you know why we're not falling back to the custom implementation?
}

Maybe one can explain the other?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+// Mochitest harness default timeout is 5 mn (+ 30 s),
+// but this test is trivial and shouldn't need that much time:
+// assume that an in-test timeout of 30 s should be enough. (Bug 567931)

What is the point of having the separate timeout though? Sure, the test shouldn't take 5 minutes, but what is the point of cutting it short?

Comment 9

[Serge Gautherie (:sgautherie)]

(In reply to comment #8)
> What is the point of having the separate timeout though?

There is no answer of mine to that:
this was bug 567931, pushed by mats without review.

I wonder as you do, yet I would prefer to fix this bug and reopen(+backout) bug 567931 separately, unless mats (and you) can explain and agree here soon.

Comment 10

[Mats Palmgren (inactive)]

Even 15 seconds is a ridiculously long time to wait for this simple
test to complete.  If 20 or 30 seconds makes it succeed, but not 15,
then we really should file a bug on IT to figure out what's wrong in
the test environment.  Even on an extremely overloaded machine I
would expect the test to succeed within a few seconds.

The reason we want a shorter than 5 minutes timeout for this test
is that it tests a feature that's only expected to work on some
systems where the necessary notification service is present (as far
as I understand it).  On system where it's not present it's a
guaranteed 5 minute hang which is unacceptable IMHO.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #10)
> The reason we want a shorter than 5 minutes timeout for this test
> is that it tests a feature that's only expected to work on some
> systems where the necessary notification service is present (as far
> as I understand it).  On system where it's not present it's a
> guaranteed 5 minute hang which is unacceptable IMHO.

Are you sure? On such systems, shouldn't we fail to get nsIAlertsService, or fail the call to showAlertNotification, and not even set up the timer?

Comment 12

[Mats Palmgren (inactive)]

Yes, I'm pretty sure I wouldn't have added the timer unless it was necessary.
I've updated my Linux system since then and now I apparently have the
necessary system service so I can't verify.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If the timeout fires, the test fails. Shouldn't it pass instead, or at least be todo, if the timeout is actually expected on some systems?

If we can't verify that the timeout is needed, how about we take it out until someone can confirm it is needed?

Comment 14

[Mats Palmgren (inactive)]

I don't think there's a way to tell if it was "expected fail due to a
misconfigured/missing system service" or just "fail".  So being
conservative and flagging orange seems appropriate (I'd expect IT
would like to know about such things; and developers running this
test locally don't care I think).

I'm ok with removing the timer, but I thought Serge increased the time
to avoid orange? (which I took as evidence it's still needed)

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's not failing 100% of the time so I assume our test machines have the necessary software installed and the test will pass if we take out the timer (so it effectively gets 5 minutes to run). So let's do that.

If we discover systems where nsIAlertsService can be started, and showAlertNotification succeeds, but we never get the alertfinished callback, then I claim that's a bug we should fix, perhaps by detecting earlier that the system framework is broken and failing the showAlertNotification call.

Comment 16

[Mats Palmgren (inactive)]

If we need to increase the timer to more than 15 seconds for the
test to succeed than I claim that is a bug we should fix.  :p

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why do you say that? Isn't it possible for some system notification service to leave an alert showing for more than 15 seconds?

Comment 18

[Mats Palmgren (inactive)]

I wouldn't expect that long timeout by default, but I could be wrong.
Also, who knows how a busy system prioritizes such things... so let's
just remove the timeout as you suggested and see if someone complains...

Comment 19

[Mats Palmgren (inactive)]

FWIW, on a lightly loaded Ubuntu 10.10 system, the test completes in 4 seconds.

Comment 20

[Serge Gautherie (:sgautherie)]

Attachment: (Av3) Add executeSoon(), Remove in-test timeout, Remove cookie in "noobserve" case, Have more (explicit) check messages [Checked in: See comment 22 & 23]

Av2a, with timeout safety removed,
plus better messages.

To make it short:
we, all 3, agree to remove the timeout now;
it seems none of us has an explicit answer to comment 3;
then, we'll just see if/when bugs are filed.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 490509 [details] [diff] [review]
(Av3) Add executeSoon(), Remove in-test timeout, Remove cookie in "noobserve" case, Have more (explicit) check messages
[Checked in: See comment 22 & 23]

+var gNotificationIsAvailable = true;

Would be simpler to set gNotificationIsAvailable to false here and set it to true after a successful call to showAlertNotification.

r+ with that

Comment 22

[Serge Gautherie (:sgautherie)]

Comment on attachment 490509 [details] [diff] [review]
(Av3) Add executeSoon(), Remove in-test timeout, Remove cookie in "noobserve" case, Have more (explicit) check messages
[Checked in: See comment 22 & 23]

http://hg.mozilla.org/mozilla-central/rev/491b922beb05
Av3, with comment 21 suggestion(s).

Comment 23

[Serge Gautherie (:sgautherie)]

Comment on attachment 490509 [details] [diff] [review]
(Av3) Add executeSoon(), Remove in-test timeout, Remove cookie in "noobserve" case, Have more (explicit) check messages
[Checked in: See comment 22 & 23]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/69222ff4552e
(Bv1-192) Add executeSoon(), Remove cookie in "noobserve" case, Have more (explicit) check messages.

Comment 24

[Serge Gautherie (:sgautherie)]

V.Fixed, per 3-platform FF tinderboxes.

Comment 25

[Serge Gautherie (:sgautherie)]

(In reply to comment #7)
> Maybe one can explain the other?

I eventually filed bug 612411.

Comment 26

[Serge Gautherie (:sgautherie)]

verified1.9.2, per bug 612411 comment 1.

Comment 27

[LegNeato]

Not sure if this is an issue on 1.9.1. Marking it as nofix in any case.