736918 - Intermittent "test_nodb_pluschanges.js | false == true"

Status: VERIFIED FIXED

(Firefox :: Search, defect, P2)

Description

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1332097443.1332100322.29124.gz
OS X 10.6 comm-central-trunk debug test xpcshell on 2012/03/18 12:04:03
{
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nodb_pluschanges.js | test failed (with xpcshell return code: 0), see following log:

TEST-UNEXPECTED-FAIL | /builds/slave/test/build/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nodb_pluschanges.js | false == true - See following stack:
JS frame :: /builds/slave/test/build/xpcshell/head.js :: do_throw :: line 462
JS frame :: /builds/slave/test/build/xpcshell/head.js :: _do_check_eq :: line 556
JS frame :: /builds/slave/test/build/xpcshell/head.js :: do_check_eq :: line 577
JS frame :: /builds/slave/test/build/xpcshell/head.js :: do_check_true :: line 591
JS frame :: /builds/slave/test/build/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nodb_pluschanges.js :: <TOP_LEVEL> :: line 59
JS frame :: /builds/slave/test/build/xpcshell/tests/toolkit/components/search/tests/xpcshell/head_search.js :: <TOP_LEVEL> :: line 96
JS frame :: resource:///components/nsSearchService.js :: <TOP_LEVEL> :: line 3744
JS frame :: resource://gre/modules/NetUtil.jsm :: <TOP_LEVEL> :: line 119
}

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Investigating. Does this happen only on MacOS X? I see that this is a perma-orange, what does it mean exactly?

Comment 2

[[e-mail]]

(In reply to David Rajchenbach Teller from comment #1)
> Investigating. Does this happen only on MacOS X? I see that this is a
> perma-orange, what does it mean exactly?
On Firefox, most tests are green, and a couple are random-orange, which means that they don't usually fail and even when they do we don't know why yet. Any patch that causes tests to fail consistently gets backed out pretty quickly.

Unfortunately core test authors either don't realise that SeaMonkey is going to try to run their test as well and/or don't care whether it fails, even if it is consistent, which we call perma-orange. Sometimes the test relies on differences between Firefox and SeaMonkey, but sometimes the test is just buggy.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

I have the impression that my test relies on an event that is sent only once on FF but twice on SM. First time the event is received, the test succeeds and cleans up after itself, and the second time, which is unexpected, it fails.

I am currently trytesting a fix.

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #1)
> Investigating. Does this happen only on MacOS X? I see that this is a
> perma-orange, what does it mean exactly?

Yes, OSX 10.6 only, afaict.
perma means it happens at every run.

*****

Fwiw, first time I notice it on OSX 10.5: (ftb, I assume it's random there.)

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1332140061.1332143030.6826.gz
OS X 10.5 comm-central-trunk debug test xpcshell on 2012/03/18 23:54:21
s: cb-sea-miniosx02

Comment 5

[Legacy TBPL/Treeherder Robot]

mbrubeck
https://tbpl.mozilla.org/php/getParsedLog.php?id=10174218&tree=Mozilla-Inbound
Rev3 Fedora 12x64 mozilla-inbound opt test xpcshell on 2012-03-19 01:29:28

TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nodb_pluschanges.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nodb_pluschanges.js | false == true - See following stack:

Comment 6

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

Hrm, does this test never remove it's observer at all? That seems bad...

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Attachment: Fix

Attaching a candidate fix. At the moment, I have not succeeded at getting a SeaMonkey testing environment (even through TryServer) so I unfortunately cannot test it against SeaMonkey, but this one-liner does not break FF and should solve the issue.

Comment 8

[Serge Gautherie (:sgautherie)]

Comment on attachment 607479 [details] [diff] [review]
Fix

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

::: toolkit/components/search/tests/xpcshell/head_search.js
@@ +92,5 @@
>  function afterCommit(callback)
>  {
>    let obs = function(result, topic, verb) {
>      if (verb == "write-metadata-to-disk-complete") {
> +      Services.obs.removeObserver(obs, topic);

Nit: "browser-search-service" would make it easier to match them, etc.

Comment 9

[Serge Gautherie (:sgautherie)]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #6)
> Hrm, does this test never remove it's observer at all? That seems bad...

Should not be needed in xpcshell, should it?


(In reply to David Rajchenbach Teller [:Yoric] from comment #7)
> At the moment, I have not succeeded at getting a
> SeaMonkey testing environment (even through TryServer) so I unfortunately

SM xpcshell: one can build or download, but no Try (yet).

> cannot test it against SeaMonkey, but this one-liner does not break FF and
> should solve the issue.

More than not break FF, the question is "will it fix the random oranges"?

Comment 10

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

(In reply to Serge Gautherie (:sgautherie) from comment #9)
> Should not be needed in xpcshell, should it?

Right, I forgot these were xpcshell.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #10)
> (In reply to Serge Gautherie (:sgautherie) from comment #9)
> > Should not be needed in xpcshell, should it?
> 
> Right, I forgot these were xpcshell.

Indeed. Here, the issue is that I have to send "quit-application" to instruct the search engine to flush of search metadata. If I diagnosed the issue properly, SM and FF do not seem to have the same behavior here, and "quit-application" is sent a second time on SM. Regardless, removing the listener after we have received the first occurrence of our event seems the right thing to do, and I hope that it solves the issue.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/25460df0838f

Please don't checkin-needed patches with Try syntax in them in the future.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

My apologies. I feel a little stupid.

Comment 14

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/25460df0838f

Comment 15

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1332387115.1332390072.14578.gz
OS X 10.6 comm-central-trunk debug test xpcshell on 2012/03/21 20:31:55

V.Fixed