991311 - Devtools tests leak many inspector-traversal-data.html windows

Status: RESOLVED INACTIVE

(DevTools :: Inspector, defect, P3)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Looking at the shutdown logs from a mochitest-chrome test we see many lines such as

08:27:44     INFO -  --DOMWINDOW == 8 (1F39FDB0) [pid = 3588] [serial = 4178] [outer = 00000000] [url = chrome://mochitests/content/chrome/toolkit/devtools/server/tests/mochitest/inspector-traversal-data.html]

These windows are hanging around until shutdown somehow.

Comment 1

[Nicholas Nethercote [inactive]]

robc, Panos, Nick: any idea who should look into this?

Comment 2

[Joe Walker [:jwalker]]

(In reply to Nicholas Nethercote [:njn] from comment #1)
> robc, Panos, Nick: any idea who should look into this?

Patrick is probably best.

Comment 3

[Patrick Brosset <:pbro>]

I can see the DOMWINDOW lines in the logs when running the tests with a debug build locally.
It does however say at the end of the test run that there are no leaks.
What exactly do these lines mean?

I've tried cleaning up a few things around in the destructor of the WalkerActor (which is being tested here), but that didn't seem to change anything.

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #3)
> I can see the DOMWINDOW lines in the logs when running the tests with a
> debug build locally.
> It does however say at the end of the test run that there are no leaks.
> What exactly do these lines mean?

A ++DOMWINDOW line means that we're creating a DOM window in the given process with the given serial number.

A --DOMWINDOW line means that a DOM window in the given process, with the given serial number (so you can locate the matching ++DOMWINDOW) and url, is being destroyed.  It also tells you how many DOM windows are remaining (8 for the example in comment 0)

So all those windows are hanging around until shutdown; it's not clear to me why the leak checker says there are no leaks, though...

Comment 5

[Patrick Brosset <:pbro>]

Gotcha, thanks for the clarification. So it means my goal is to manage, at the end of a test run, to have as many ++ and --.

Comment 6

[Andrew McCreight [:mccr8]]

> it's not clear to me why the leak checker says there are no leaks, though...
The leak checker only checks things at shutdown.  Mochitest browser-chrome has a fancier leak checker that tests if things are leaking at an intermediate state, but I think this is for just mochitest chrome.

Comment 7

[Andrew McCreight [:mccr8]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #5)
> So it means my goal is to manage, at
> the end of a test run, to have as many ++ and --.
From comment 0, it sounds like the number of ++ and -- are already balanced.  The problem is that the windows are not going away when the test itself ends (or shortly thereafter), but all the way when the browser is shutting down.  That means we're leaking the windows, but just in a way that evades the leak detector.

Comment 8

[Nathan Froyd [:froydnj]]

I am not sure this is a strictly a devtools problem...running layout/style/test/ shows similar behavior of not releasing 95 docshells and > 180 windows until shutdown.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yes, the shutdown leak checker only reports leaks that persist *through* shutdown.  This is a leak that lasts *until* shutdown and then goes away.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Nathan Froyd (:froydnj) from comment #8)
> I am not sure this is a strictly a devtools problem...running
> layout/style/test/ shows similar behavior of not releasing 95 docshells and
> > 180 windows until shutdown.

Are you sure shutdown isn't just the first time we're GC/CCing?

Comment 11

[Patrick Brosset <:pbro>]

Not knowing a whole lot about this topic I investigated by commenting out most of the code and working my way through pieces of code, uncommenting them one by one.

Here's what I found:

1 - The test harness creates a bunch (~15) of windows when the tests start. These windows are destroyed on shutdown. This seems expected.

2 - There's a window created in between each test run (iframe-between-tests.html), which also gets destroyed on shutdown. To be expected too.

3 - For most tests, the windows that are created during the test aren't being destroyed at the end of the test, but rather at shutdown.
However, there are a few tests during which windows are destroyed, but not necessarily the windows that were created during these tests.

4 - A few of the windows created during a test run are related to the test harness itself (loading the test page in an iframe for instance).

5 - A few other windows created during a test run are created when doing:
  DebuggerServer.init(() => true);
  DebuggerServer.addBrowserActors();
This is done in inspector-helper.js, and there's a registerCleanupFunction done just after that does this:
  DebuggerServer.destroy();

6 - The rest of the windows created during a test run are created when the test uses the `attachURL` function, which is also defined in inspector-helper.js.
This function opens a new window (window.open) and loads a given URL in it.
inspector-helper.js also registers a cleanup function to close this window when the test ends:
  win.close();
  win = null;
This code is executed correctly when the test ends, but as noted in (3), the corresponding windows only get destroyed at shutdown.

7 - A lot of tests in toolkit/devtools/server/tests/mochitest` use inspector-helper.js and attachURL, and most of them also use the inspector-traversal-data.html test document, that's why we see it often in the logs.

8 - Final point, if I force a GC in a registerCleanupFunction, then windows supposed to be destroyed are indeed destroyed before the test ends.

(In reply to (Away 4/19-5/7) from comment #10)
> Are you sure shutdown isn't just the first time we're GC/CCing?
I tend to agree. As I said in (3), there are some tests though that will cause GC to run, and during these tests we see bunches of --DOMWINDOW in the logs, for windows that had been created previously but not GCd yet.

Do you think we could force GC in the test harness at the end of each test to make sure we can actually see potential leaks?

Comment 12

[Patrick Brosset <:pbro>]

Worth noting too: the inspector-traversal-data.html window, metionned in comment 0, isn't going away before shutdown, even with GC forced, and this also happens even when commenting everything related to the devtools in the test (not starting the debuggerServer, nor connecting the client, nor starting the inspector actor).

So this doesn't seem related to the devtools.

Comment 13

[Andrew McCreight [:mccr8]]

> Do you think we could force GC in the test harness at the end of each test to
make sure we can actually see potential leaks?

If you do that, please use SpecialPowers.DOMWindowUtils.runNextCollectorTimer(), which will ensure that we actually run incremental collections and not just synchronous ones (which can hide problems).

Comment 14

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Andrew McCreight [:mccr8] from comment #13)
> > Do you think we could force GC in the test harness at the end of each test to
> make sure we can actually see potential leaks?
> 

I did this in my patch for bug 983948 but we found that it increased mochitest-browser tests duration by at least 5 minutes so we chose not to land it.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

More importantly, we didn't see any real difference in the number of intermittent failures we were seeing, so there wasn't any apparent value in taking the runtime hit.

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

Inspector Bugs Triage - Filter on CLIMBING SHOES

Comment 17

[Patrick Brosset <:pbro>]

Unassigned myself as I'm not working on this.