Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Bug 792636 - Add some automated leak detection #598

Closed
wants to merge 13 commits into
from

Conversation

Projects
None yet
3 participants
Member

Mossop commented Oct 4, 2012

I'd like to get some feedback on this @ochameau. It adds some very basic leak detection that displays if it thinks something has leaked at the end of the test run. It works using memory reporters. Before running tests it makes a list of all the JS compartments and windows that are in memory. After the test run it checks to see if anything new is there (ignoring a blacklist of chrome stuff). If there is it logs it.

Currently though it is not very reliable. Sometimes it will find a bunch of leaks, sometimes it won't. I ran it for every test individually and in the cases where it was reliably claiming a leak I always found a real cause for it. It now only reliably claims a leak for the private browsing module, and I think that is a real leak in platform code.

To try to make it more reliable I tried having it delay and re-check for up to a second, running GC each time, but still no joy. Do you have any ideas what might be going on?

The next step of this would probably be to also run the leak check after each test file, or even each individual test in the file to make it easier to narrow down the problems but that is pointless unless it gets more reliable.

Member

ochameau commented Oct 14, 2012

Currently though it is not very reliable. Sometimes it will find a bunch of leaks, sometimes it won't. I ran it for every test individually and in the cases where it was reliably claiming a leak I always found a real cause for it. It now only reliably claims a leak for the private browsing module, and I think that is a real leak in platform code.

To try to make it more reliable I tried having it delay and re-check for up to a second, running GC each time, but still no joy. Do you have any ideas what might be going on?

Wouldn't it because the leaks are intermittent? I can easily imagine two cases: we leak only when a race condition happens (or the other way around ;)). And the platform code do stuff only on some runs, or only after xx seconds after firefox startup so that we will see more leaks on cfx testall just because it takes more time to execute.

Otherwise, I think that something simplier should already be reliable:

foceGC();
setTimeout(function () {
  forceGC();
  setTimeout(function () {
    checkLeaks();
  });
});

I would be interested so see cases where we need more forceGC.
Then, there is the nsIDOMWindowUtils method, I've always wondered if it was usefull to call it in addition to Cu.forceGC:
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIDOMWindowUtils#garbageCollect
Might be worth giving it a try?

Finally, I think that such feature can already be usefull if optional.

@Mossop Mossop Merge branch 'master' into check-memory
Conflicts:
	test/test-addon-installer.js
	test/test-content-symbiont.js
a2bf516
Contributor

erikvold commented Jan 19, 2013

@Mossop I think we should cherry-pick the leak fixes that you've made at least.

@erikvold erikvold and 1 other commented on an outdated diff Jan 19, 2013

lib/sdk/deprecated/unit-test-finder.js
@@ -11,6 +11,8 @@ module.metadata = {
const file = require("../io/file");
const memory = require('./memory');
const suites = require('@test/options').allTestModules;
+const { Loader } = require('../test/loader');
+const { ensure } = require('../system/unload');
@erikvold

erikvold Jan 19, 2013

Contributor

can this line be removed?

@Mossop

Mossop Jan 19, 2013

Member

Yeah looks like it is left over from a previous iteration

Contributor

erikvold commented Jan 19, 2013

can we make this a optional feature for now and land it?

Member

Mossop commented Jan 19, 2013

I've already cherry-picked the leak fixes and landed them elsewhere. I have been intending to turn this into an optional feature and get it landed, just haven't found the time yet. If you feel like taking it then by all means!

Mossop added some commits Apr 22, 2013

@Mossop Mossop Merge branch 'master' into check-memory
Conflicts:
	lib/sdk/deprecated/unit-test-finder.js
	lib/sdk/test/harness.js
	lib/sdk/test/loader.js
	test/test-addon-installer.js
	test/test-page-mod.js
50e5e66
@Mossop Mossop Make memory checking optional. 4111415
Member

Mossop commented Apr 22, 2013

I've updated this branch to master and made memory checking optional

Member

ochameau commented Apr 23, 2013

error: reddit-panel: An exception occurred.
TypeError: panel is undefined
resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.path/sdk/
panel/utils.js 91
Traceback (most recent call last):
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/timers.js", line 31, in notify
    callback.apply(null, args);
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/test/harness.js", line 226, in cleanup
    loader.unload();
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/unload.js", line 41, in unloadWrapper
    originalDestructor.call(obj, reason);
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/test/loader.js", line 31, in wrapper<.unload
    unload(loader, reason);
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/toolkit/loader.js", line 379, in unload
    notifyObservers(subject, 'sdk:loader:destroy', reason);
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/events.js", line 62, in
    data: data
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/unload.js", line 77, in onunload
    unload(reason);
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/unload.js", line 56, in unload
    observers.forEach(function(observer) {
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/unload.js", line 58, in
    observer(reason);
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/unload.js", line 67, in
    unloaders.slice().forEach(function(unloadWrapper) {
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/unload.js", line 68, in
    unloadWrapper(reason);
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/unload.js", line 41, in unloadWrapper
    originalDestructor.call(obj, reason);
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/test/loader.js", line 31, in
    unload(loader, reason);
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/toolkit/loader.js", line 379, in unload
    notifyObservers(subject, 'sdk:loader:destroy', reason);
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/events.js", line 62, in
    data: data
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/unload.js", line 77, in onunload
    unload(reason);
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/unload.js", line 56, in unload
    observers.forEach(function(observer) {
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/unload.js", line 58, in
    observer(reason);
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/unload.js", line 67, in
    unloaders.slice().forEach(function(unloadWrapper) {
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/unload.js", line 68, in
    unloadWrapper(reason);
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/unload.js", line 41, in unloadWrapper
    originalDestructor.call(obj, reason);
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/widget.js", line 338, in destroy
    this.panel.destroy();
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/core/disposable.js", line 70, in destroy
    this.dispose();
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/panel.js", line 170, in dispose
    this.hide();
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/panel.js", line 229, in hide
    domPanel.close(viewFor(this));
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/panel/utils.js", line 91, in close
    return panel.hidePopup && panel.hidePopup();
info: reddit-panel: [JavaScript Error: "reddit-panel: An exception occurred.
TypeError: panel is undefined
resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.path/sdk/
panel/utils.js 91
Traceback (most recent call last):
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/timers.js", line 31, in notify
    callback.apply(null, args);
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/test/harness.js", line 226, in cleanup
    loader.unload();
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/unload.js", line 41, in unloadWrapper
    originalDestructor.call(obj, reason);
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/test/loader.js", line 31, in wrapper<.unload
    unload(loader, reason);
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/toolkit/loader.js", line 379, in unload
    notifyObservers(subject, 'sdk:loader:destroy', reason);
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/events.js", line 62, in
    data: data
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/unload.js", line 77, in onunload
    unload(reason);
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/unload.js", line 56, in unload
    observers.forEach(function(observer) {
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/unload.js", line 58, in
    observer(reason);
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/unload.js", line 67, in
    unloaders.slice().forEach(function(unloadWrapper) {
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/unload.js", line 68, in
    unloadWrapper(reason);
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/unload.js", line 41, in unloadWrapper
    originalDestructor.call(obj, reason);
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/test/loader.js", line 31, in
    unload(loader, reason);
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/toolkit/loader.js", line 379, in unload
    notifyObservers(subject, 'sdk:loader:destroy', reason);
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/events.js", line 62, in
    data: data
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/unload.js", line 77, in onunload
    unload(reason);
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/unload.js", line 56, in unload
    observers.forEach(function(observer) {
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/unload.js", line 58, in
    observer(reason);
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/unload.js", line 67, in
    unloaders.slice().forEach(function(unloadWrapper) {
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/unload.js", line 68, in
    unloadWrapper(reason);
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/system/unload.js", line 41, in unloadWrapper
    originalDestructor.call(obj, reason);
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/widget.js", line 338, in destroy
    this.panel.destroy();
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/core/disposable.js", line 70, in destroy
    this.dispose();
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/panel.js", line 170, in dispose
    this.hide();
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/panel.js", line 229, in hide
    domPanel.close(viewFor(this));
  File "resource://extensions.modules.anonid0-reddit-panel-at-jetpack.commonjs.p
ath/sdk/panel/utils.js", line 91, in close
    return panel.hidePopup && panel.hidePopup();
"]

r+ with this exception being fixed and commits squashed before merging.
(This exception happens when running cfx test -o in reddit-panel.)

Otherwise, I'm wondering if the memory check done in mochitest-browser can be done after each test without slowing down tests massively? (I'm excepting it to go through the whole CC graph...)
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js#321
We can identify any kind of leaks thanks to the CCAnalyzer helper. Here it is used to look for GlobalWindow, but we can track other objects as well, like Sandboxes...

Member

Mossop commented Apr 23, 2013

Ok the exception is because my patch makes us properly unload the loader used for tests. This ends up calling panel.destroy twice, once through Disposable listening for unload, and once from widget.destroy. I'll fix it in https://bugzilla.mozilla.org/show_bug.cgi?id=864986.

I do like the idea of checking memory after each test, but for now I just want to get this landed and out of my queue.

@Mossop Mossop closed this Apr 25, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment