1589625 - Remote browser chrome tests do not correctly clean-up after tests

Status: RESOLVED FIXED

(Remote Protocol :: Agent, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

All of the remote browser chrome tests do not correctly clean-up the tab and client, but also don't close the connection to the remote agent in case of failures.

Tests can be found here:
https://searchfox.org/mozilla-central/source/remote/test/browser/

What we actually need is a teardown method, which does that by default, or wrap all the testing and teardown code inside a try / finally block.

I will have a look how other browser chrome tests have been setup for handling teardown logic.

Maybe that a fix here is enough to re-enable debug jobs in CI as disabled by bug 1547679.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The biggest issue is actually that the Remote Agent connection isn't getting closed, and as such we leak all the used domains. Given that all the tests make use of the Remote Agent we can have the calls to listen() and close() both in the closure as part of our custom add_task() method.

For the left open tab I would suggest that we run each of the tests inside a new tab. Also because this will prevent side-effects between individual tests.

Comment 2

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Note there's a teardown method added in https://phabricator.services.mozilla.com/D47032#change-DME1OkaQJVRg

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This won't help given that it's not getting executed too in case of any failures in the test. But yes, something like that is necessary, and doesn't have to be called from the test itself.

I was able to get rid of the leaks of the Remote agent connection, and closing the tabs should be easy too.

But thanks for letting me know, so that I can refactor the code appropriately.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=92fd405e287d6ea7af9a39aee670a861ad3d7bc2

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1589625 - [remote] Add documentation for enabling logging of emitted events. r=#remote!

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1589625 - [remote] Improve handling of tabs when closing a chrome window. r=#remote!

Due to some obvious bugs in the code of TabObserver.jsm the registered
targets for each of the window's tabs haven't been unregistered when
the window has been closed.

It has the effect that when closing the Remote Agent the browsingContext
of the tab target, which has to be destroyed, cannot be retrieved.
Instead an error is raises, because the underlying frameLoader actually
doesn't exist anymore.

Given that "TabClose" events aren't fired when the window closes,
those have to be emulated.

Depends on D50230

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1589625 - [remote] Start and stop the CDP server outside of the browser chrome tests. r=#remote!

To ensure that the CDP server connection is always closed after a
test even when it is failing, its lifetime has to be handled inside
the "add_task" function.

Currently if a test fails all the registered events and observer
notifications are getting leaked. This patch ensures that all of
those events and notifications are getting unregistered.

Depends on D50231

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1589625 - [remote] Improve setup and teardown logic for browser chrome tests. r=#remote!

Currently when browser chrome tests are failing the open tabs, client,
and Remote Agent will never be closed, and as such each failing test
causes massive memory leaks.

Therefore the teardown logic needs to be moved out of the tests into
the "add_task()" function. Only that way we can make sure to run
all the clean-up steps independent of the test success state.

Depends on D50232

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e09c20baeed7
[remote] Add documentation for enabling logging of emitted events. r=remote-protocol-reviewers,ato,maja_zf
https://hg.mozilla.org/integration/autoland/rev/5b3badffcd9c
[remote] Improve handling of tabs when closing a chrome window. r=remote-protocol-reviewers,ato
https://hg.mozilla.org/integration/autoland/rev/ebeb63afc52d
[remote] Start and stop the CDP server outside of the browser chrome tests. r=remote-protocol-reviewers,ato
https://hg.mozilla.org/integration/autoland/rev/21764868072d
[remote] Improve setup and teardown logic for browser chrome tests. r=remote-protocol-reviewers,ato

Comment 10

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/e09c20baeed7
https://hg.mozilla.org/mozilla-central/rev/5b3badffcd9c
https://hg.mozilla.org/mozilla-central/rev/ebeb63afc52d
https://hg.mozilla.org/mozilla-central/rev/21764868072d