Bug 454513 fixed a case where it proved to be an issue. Note that the actually reported test failure was in fact caused by a previous test :-> It should not hurt to "fix" the other occurrences ! Maybe it would fix other (intermittent) bugs, or prevent future failures ?! *** Would it be possible that the test harness warns if a test leaves an EventListener alive ?

For normal mochitests, forgetting to call removeEventListener shouldn't be a problem. It's only a problem for tests that act on shared |window| objects. What kinds of tests do that?

Browser chrome tests - see the bug this bug depends on. They run sequentially in a single chrome window.

As a starting point, http://mxr.mozilla.org/mozilla-central/search?string=wi.*%5C..*EventListener&regexp=on&case=on&find=%2Ftest%2F browser_bug414214.js must be fixed; but what about the others ? I would say yes (to all), as it can't hurt, but I wouldn't want to spend time then get the patch refused as superfluous...

The hurt is that it'll clutter up our tests. And we'll waste peoples time (including my own) going over tests and "fixing" them. Additionally it means that we'll get less leak testing as leaks often happen through event listeners, and few real-world pages call removeEventListener. So I'm strongly opposed to having this as a general rule. In fact, I'd argue that the chrome test suite might need to get fixed if it can't deal with left-over event listeners.

Problem is chrome/browser-chrome tests don't always completely recycle the pages they test [aiui]--Where webpages do. If we need to test missing removeEventListeners lets test that, and actually practice good coding practices in our tests to increase test accuracy [in turn reducing random oranges].

IMHO anything we need to do just to satisfy the test suite means that A) we'll likely to forget to do it, especially if it only creates intermittent problems. B) We'll spend more time writing test code that is better spent elsewhere. A is also likely to add to B. I understand if there are other reasons why we're not fixing this problem, such as it would significantly slow down running the tests, or that it would be a lot of work to fix it. Or if calling removeEventListener is something that chrome code really should do then things are of course fine. However web pages aren't expected to so no need to hunt down 'missing' removeEventListener calls there.

(In reply to comment #6) > I understand if there are other reasons why we're not fixing this problem, such > as it would significantly slow down running the tests, or that it would be a > lot of work to fix it. How would you propose we fix it? We can't easily remove all event listeners (even if we had something like bug 448602) because it would be nearly impossible to differentiate between listeners added by chrome code and listeners added by test code. We could restart the browser after each test, or run each test in a new window, but that would complicate things and make test runs much slower. > However web pages aren't expected to so no need to hunt down 'missing' > removeEventListener calls there. Right, I agree. This bug only covers browser chrome tests, as far as I know.

(In reply to comment #7) > (In reply to comment #6) > > However web pages aren't expected to so no need to hunt down 'missing' > > removeEventListener calls there. > > Right, I agree. This bug only covers browser chrome tests, as far as I know. Also agreed, we shouldn't force it; but imo it is still good code, so if a test author/reviewer wants to request/write it, no-one should stop them there :-)

Probably "unrelated", but it can't hurt to fix that case. ***** I had a (detection) patch somewhere for the main issue, if I can find it...

Comment on attachment 369954 [details] [diff] [review] (Av1) Fix harness-overlay.xul [Checkin: Comment 10 & 11] http://hg.mozilla.org/mozilla-central/rev/96624298f10e

Comment on attachment 369954 [details] [diff] [review] (Av1) Fix harness-overlay.xul [Checkin: Comment 10 & 11] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3dd141941496

This is (an updated version of) what I used in the past. It seemed to (be enough to) do the job. *** [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090402 Minefield/3.6a1pre] (home, optim default) (W2Ksp4) (http://hg.mozilla.org/mozilla-central/rev/c4f6bb1db611) Currently, it reports { TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_bug422590.js | [browser-test.js, Tester.finish()] mismatched window.addEventListener() and window.removeEventListener() - Got 14, expected 12 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_bug432599.js | [browser-test.js, Tester.finish()] mismatched window.addEventListener() and window.removeEventListener() - Got 18, expected 14 } Though I had a look and a few tries at them, but couldn't find out what was wrong :-( So I don't know if it's the tests or my detection code ?

(In reply to comment #12) http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_bug422590.js http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_bug432599.js

Comment on attachment 371098 [details] [diff] [review] (Bv1) browser-test.js: monitor window EventListeners number >+ // Check that the number of EventListeners is unchanged. >+ if (this.currentTestIndex >= 0 && >+ gNbEventListener != this.nbWinEventListenerStart) >+ this.currentTest.scope.is(gNbEventListener, this.nbWinEventListenerStart, >+ "[browser-test.js, Tester.finish()] mismatched window.addEventListener() and window.removeEventListener()"); This is wrong, as chrome JS can add listeners while tests are running.