454513 - browser_bug453896.js fails on all SeaMonkey unit test boxes

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Serge Gautherie (:sgautherie)]

Since that (bug) test landed, it fails for SeaMonkey on all 3 platforms:
[
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1220968348.1220974177.20273.gz
Linux comm-central dep unit test on 2008/09/09 06:52:28

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1220971042.1220976375.25536.gz
MacOSX 10.4 comm-central dep unit test on 2008/09/09 07:37:22

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1220969059.1220973180.17477.gz
Win2k3 comm-central dep unit test on 2008/09/09 07:04:19
]

Comment 1

[Robert Kaiser]

The bug mentioned above is bug 453896 and the failure line is:
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/layout/style/test/browser_bug453896.js | Timed out

Now that we have this noted in comments, adjusting summary to be more easily parseable by a human :)

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hmmm.  I thought the directory I was exporting it to was for Firefox browser tests.  I'm surprised that SeaMonkey is using the same directory.

I don't know if it's easy to make the test work in both.  If not, probably the right thing to do is ifdef the makefile.

Comment 3

[Robert Kaiser]

The directory is actually for "tests running with browser chrome" and as SeaMonkey also has browser chrome, we're running all we can run from that suite as well.
Making that test |ifdef MOZ_PHOENIX| is probably the right thing to do if it tests Firefox specifics even though it lives in Gecko code.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

For what it's worth, all it really tests is "run the following mochitests in a Web page that's been opened in a new tab and *not* switched to".

Comment 5

[Robert Kaiser]

It looks like the test itself actually runs well, it's some other test before it that causes us to fail on this one.

Comment 6

[Robert Kaiser]

Actually, what I see here is two event listeners that are added but never removed, in http://mxr.mozilla.org/mozilla-central/source/docshell/test/browser/browser_bug441169.js#16 and http://mxr.mozilla.org/mozilla-central/source/layout/style/test/browser_bug453896.js#16 - when I remove the test of the former from the suite, the latter succeeds.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yeah, probably both listeners ought to be removed.  The first is worse since it's on Window rather than the tab (which is only used once).

Comment 8

[Robert Kaiser]

Attachment: remove listeners in the right places [Checkin: Comment 10]

This patch removes the listeners and makes the tests succeed. I actually guess that the listener in the new test isn't the problem as it's browser goes away with the tab, but it's cleaner to remove the listener anyway. The one the window is surely something we must remove, I don't know why Firefox doesn't run into a failure somewhere with that.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 337992 [details] [diff] [review]
remove listeners in the right places
[Checkin: Comment 10]

r=dbaron, although you might also want to run it by whoever maintains the browser test harness (which I probably should have done the first time, too...)  (though if whoever that is isn't responsive, it's probably ok just to land it...)

Comment 10

[Robert Kaiser]

not sure who owns the browser harness, but I figured when the other tests in http://mxr.mozilla.org/mozilla-central/source/docshell/test/browser (at least those SeaMonkey builds) are removing their listeners, it should be the correct thing to do - esp. when it fixes the tests locally here.

Pushed as http://hg.mozilla.org/mozilla-central/rev/c9482d51ef47 - marking FIXED on the assumption that unit test boxes see the same success as my local machine, please reopen if that's not happening.

Comment 11

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

Comment on attachment 337992 [details] [diff] [review]
remove listeners in the right places
[Checkin: Comment 10]

Yeah, this is fine. Tests are supposed to clean up after themselves as much as they can.

(Though I do wonder why these were causing test failures only in SeaMonkey...)

Comment 12

[Serge Gautherie (:sgautherie)]

(In reply to comment #6)
> http://mxr.mozilla.org/mozilla-central/source/docshell/test/browser/browser_bug441169.js#16
> http://mxr.mozilla.org/mozilla-central/source/layout/style/test/browser_bug453896.js#16

Confirmed:
http://mxr.mozilla.org/mozilla-central/search?string=EventListener&case=on&find=%2Fdocshell%2Ftest%2Fbrowser%2F
http://mxr.mozilla.org/mozilla-central/search?string=EventListener&case=on&find=%2Flayout%2Fstyle%2Ftest%2F

(In reply to comment #10)
> Pushed as http://hg.mozilla.org/mozilla-central/rev/c9482d51ef47

V.Fixed, per
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1221092919.1221097646.9005.gz
Win2k3 comm-central dep unit test on 2008/09/10 17:28:39

(In reply to comment #11)
> Tests are supposed to clean up after themselves as much as they can.

I filed bug 454717 ;-)