1644039 - Tests that use window.close() are prone to cause intermittent test failures

Status: NEW

(GeckoView :: Extensions, defect, P3)

Description

[Rob Wu [:robwu]]

I have investigated a test failure (https://bugzilla.mozilla.org/show_bug.cgi?id=1642967#c14), and found that it was caused by the existence of an unexpected window from a previous test, which in turn is caused by window.close() being asynchronous. This issue is hard to debug because the original test is often unaffected, but tests that don't expect unrelated windows are failing from side effects of not being closed yet.

window.close() is asynchronous, but wasn't causing issues on desktop, because the DOMWindowClose event is sent from the child process to the main process, which in turn closes the tab immediately. Test-related messages also go through the parent process, so the messages are ordered and the tab and its UI are closed in time, as expected.

On GeckoView, DOMWindowClose is not sent to the main Gecko process, but sent from content to the Java layer of GeckoView, which closes the tab (in the test activity). This communication with the Java layer is not synchronized with the extension test runner, and as a result it is possible for mochitests to proceed with the next part of the test before the test has actually finished. As a result, there may be windows that haven't been closed yet, and those windows can interfere with the following tests.

We should fix this issue to increase the reliability of tests, and then also fix bug 1565700 to make it easier to correctly detect such failures (i.e. immediately after the error-prone test rather than an unrelated other test).

Fixing that will probably fix the following:

• https://bugzilla.mozilla.org/show_bug.cgi?id=1642967#c14
• bug 1630066 - confirmed via logcat.
• bug 1618231 - maybe (partially)
• bug 1629393 - confirmed via logcat.
• bug 1523288
• bug 1549671
• bug 1617062
• bug 1523193

Once this bug is fixed, we should also fix bug 1565700 to make it easier to catch these kinds of issues.

Comment 1

[Rob Wu [:robwu]]

I'm thinking of fixing this by introducing a test helper that does a ping-pong to the GV embedder after calling window.close().

GV people - if you're reading this and can think of a better idea, please let me know.

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Rob Wu [:robwu] from comment #1)

I'm thinking of fixing this by introducing a test helper that does a ping-pong to the GV embedder after calling window.close().

GV people - if you're reading this and can think of a better idea, please let me know.

Do we expect only one window after each test is concluded? It seems like we could have the test harness hook up to nsIWindowWatcher to keep track of opened windows, and only end the test once we get to the expected state.

Comment 3

[Rob Wu [:robwu]]

(In reply to James Willcox (:snorp) ([e-mail]) (he/him) from comment #2)

(In reply to Rob Wu [:robwu] from comment #1)

I'm thinking of fixing this by introducing a test helper that does a ping-pong to the GV embedder after calling window.close().

GV people - if you're reading this and can think of a better idea, please let me know.

Do we expect only one window after each test is concluded? It seems like we could have the test harness hook up to nsIWindowWatcher to keep track of opened windows, and only end the test once we get to the expected state.

I don't know how many windows to expect, but it should be the same number as before the start of the test.
The current use of .close() gives the misleading impression that closing a window is synchronous, and that is a recipe for intermittent test failures. Awaiting close of the window between tests will help in many cases, but not all, because there are tests that open two windows within one file (and that is perfectly legitimate): https://searchfox.org/mozilla-central/rev/9ad88f80aeedcd3cd7d7f63be07f577861727054/toolkit/components/extensions/test/mochitest/test_ext_sendmessage_no_receiver.html#42,44,74,76

Comment 4

[[:owlish] 🦉 PST]

still relevant