607639 - Check for any window left open after a browser-chrome test

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[(no longer active)]

I've noticed this a few times since yesterday.  See <http://tbpl.mozilla.org/?tree=MozillaTry&rev=d458b3eebe13> for an example (you may need to go back in time a few times to reach the revision.)  In the right bottom pane one sees:

    mochitest-chrome: 14062/0/112
    mochitest-browser-chrome: 13815/0/7
    mochitest-a11y: 15074/0/15
    mochitest-ipcplugins: 458/0/0

But 4 browser chrome tests have actually failed...

Comment 1

[Phil Ringnalda (:philor)]

From the log:

INFO TEST-START | Shutdown
Browser Chrome Test Summary
	Passed: 13815
	Failed: 0
	Todo: 7

*** End BrowserChrome Test Results ***
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_uninstalling.js | Tried to remove an add-on that wasn't registered with the mock provider
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_uninstalling.js | Tried to remove an add-on that wasn't registered with the mock provider
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_uninstalling.js | Tried to remove an add-on that wasn't registered with the mock provider
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_uninstalling.js | Tried to remove an add-on that wasn't registered with the mock provider
gned.xpi to file C:\Users\cltbld\AppData\Local\Temp\tmp-jfu.xpi

Comment 2

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

That's really odd. No idea how this could happen, unless it was caused by some JS bug with .map() or something. I can't seem to reproduce it.

Comment 3

[Phil Ringnalda (:philor)]

Forgot to mention the STR:

1. hg up -r 965dc117adc0 (up through e7a1b719e090) (or revert http://hg.mozilla.org/mozilla-central/rev/1c42dfb39b1b)
2. have done that on Windows

Comment 4

[(no longer active)]

(In reply to comment #3)
> Forgot to mention the STR:
> 
> 1. hg up -r 965dc117adc0 (up through e7a1b719e090) (or revert
> http://hg.mozilla.org/mozilla-central/rev/1c42dfb39b1b)

Do you mean this is a regression from 1c42dfb39b1b?

> 2. have done that on Windows

...and?

Comment 5

[(no longer active)]

(In reply to comment #2)
> That's really odd. No idea how this could happen, unless it was caused by some
> JS bug with .map() or something. I can't seem to reproduce it.

Actually, unless I'm on crack, those four failures seem to have happened *after* this call was made: <http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js#126>, so that very well explains the 0... ;-)

Comment 6

[(no longer active)]

Mossop might have some ideas here...

Comment 7

[Dave Townsend [:mossop]]

The problem is quite simple, the failures are happening after the browser-chrome test run has completed and the summary is printed out. In the case here one of the tests left an add-ons manager window open that was still holding some event listeners keeping the old test scope alive. When the window got closed by the application shutting down the test scope triggered a couple of failures.

Comment 8

[Dave Townsend [:mossop]]

Unless you make run_tests.py create the summary you really don't have a way to avoid this. I don't think it is worth fixing tbh, it was still turning the boxes orange ok, after that the summary count is just a nicety

Comment 9

[(no longer active)]

(In reply to comment #7)
> The problem is quite simple, the failures are happening after the
> browser-chrome test run has completed and the summary is printed out. In the
> case here one of the tests left an add-ons manager window open that was still
> holding some event listeners keeping the old test scope alive. When the window
> got closed by the application shutting down the test scope triggered a couple
> of failures.

Is it possible to use registerCleanupFuncton in add-on manager tests so that we catch the test which failed to close the window?  A window left open might lead to other failures as well, I guess.

Comment 10

[Dave Townsend [:mossop]]

(In reply to comment #9)
> (In reply to comment #7)
> > The problem is quite simple, the failures are happening after the
> > browser-chrome test run has completed and the summary is printed out. In the
> > case here one of the tests left an add-ons manager window open that was still
> > holding some event listeners keeping the old test scope alive. When the window
> > got closed by the application shutting down the test scope triggered a couple
> > of failures.
> 
> Is it possible to use registerCleanupFuncton in add-on manager tests so that we
> catch the test which failed to close the window?  A window left open might lead
> to other failures as well, I guess.

Yes, I did so and fixed the test that was doing it in http://hg.mozilla.org/mozilla-central/rev/1c42dfb39b1b.

I was a little surprised that the browser chrome harness itself wasn't failing on extra windows though, I thought it did that anyway

Comment 11

[(no longer active)]

So does this mean that we shouldn't see this problem any more?

Comment 12

[Dave Townsend [:mossop]]

I don't believe so. The changeset references in comment 0 contains the change that caused the failure and not the change that fixed it. If this is still happening at all then there is some other reason for it that I'm not aware of.

Comment 13

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

waitForWindowState should be catching windows left open by broken/timedout tests:

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js#92

Comment 14

[Dave Townsend [:mossop]]

(In reply to comment #13)
> waitForWindowState should be catching windows left open by broken/timedout
> tests:
> 
> http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js#92

That checks for browser windows, not for any other kind though.

Comment 15

[(no longer active)]

(In reply to comment #14)
> (In reply to comment #13)
> > waitForWindowState should be catching windows left open by broken/timedout
> > tests:
> > 
> > http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js#92
> 
> That checks for browser windows, not for any other kind though.

Can't we pass null to getEnumerator there to look for every window type (excluding the browser-chrome tests window itself)?

Comment 16

[(no longer active)]

Attachment: Patch (v1)

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > waitForWindowState should be catching windows left open by broken/timedout
> > > tests:
> > > 
> > > http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js#92
> > 
> > That checks for browser windows, not for any other kind though.
> 
> Can't we pass null to getEnumerator there to look for every window type
> (excluding the browser-chrome tests window itself)?

Let me put my patch where my mouth is!

Comment 17

[(no longer active)]

Gavin: ping?

Comment 18

[(no longer active)]

Gavin: ping times two? ;)

Comment 19

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

Comment on attachment 486960 [details] [diff] [review]
Patch (v1)

>diff --git a/testing/mochitest/browser-test.js b/testing/mochitest/browser-test.js

>+        let type = win.document.documentElement.getAttribute("windowtype");
>+        switch (type) {
>+        case "navigator:browser":
>+          type = "browser window";
>+          break;
>+        case null:
>+          type = "unknown window";

type = "unknown window with type: " + type; // ?

Comment 20

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/678e2a2d2195