For every call to getSystem [1] you should call releaseSystem [2] to make sure the System object is correctly released when no one is using it. However the Developer Toolbar calls getSystem every time it receives a 'TabSelected' or 'load' event [3] but only calls releaseSystem once when the toolbar is destroyed [4]. So in the end you have a system object with bunch of alleged references causing the object to never be fully released. [1] https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/gcli/commands/index.js#134 [2] https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/gcli/commands/index.js#165 [3] https://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/DeveloperToolbar.jsm#649 [4] https://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/DeveloperToolbar.jsm#628

Here's a simple patch that fixes the reference counting. I don't really know how to go about testing this. Do you have any ideas or does this even need one? Try run for good measure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b63bacc984a7

(In reply to Sami Jaktholm from comment #1) > Created attachment 8616151 [details] [diff] [review] > bug-1168679-toolbar-refcounts.patch > > Here's a simple patch that fixes the reference counting. I don't really know > how to go about testing this. Do you have any ideas or does this even need > one? I think if we export linksForTarget here: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/gcli/commands/index.js#122, then we should be able to do assertions on the expected value of links. We could just have a test that uses some of the helpers here like openToolbar, openTab, closeTab to do some basic validation that the references are properly cleaned up: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/commandline/test/helpers.js#222. Thanks for taking this on - even a really simple test would be a great start.

Comment on attachment 8616151 [details] [diff] [review] bug-1168679-toolbar-refcounts.patch Review of attachment 8616151 [details] [diff] [review]: ----------------------------------------------------------------- Seems fine, please add tests as mentioned in Comment 2. Let me know if that turns out to be tricky

Oh great, releasing the system properly caused the existing tests to blow up and leak the entire world in debug tests. It might not be that simple after all. If it's OK to export the link map just for testing tests should be doable. I'll try to come up with something that works and has some tests.

This seems to be a bigger can of worms I though of. First of all, I found the test that was failing. It was browser_cmd_csscoverage_startstop.js. Here's relevant parts of the log: > console.log: RELEASE SYSTEM: > console.trace: > _i/commands/index.js 169 exports.releaseSystem > _eveloperToolbar.jsm 649 DeveloperToolbar.prototype.handleEvent > 0 > console.log: GETSYSTEM: > console.trace: > _i/commands/index.js 136 exports.getSystem > _eveloperToolbar.jsm 653 DeveloperToolbar.prototype.handleEvent > 0 > TEST-INFO | load from browser_cmd_csscoverage_page2.html > console.log: RELEASE SYSTEM: > console.trace: > _i/commands/index.js 169 exports.releaseSystem > _eveloperToolbar.jsm 649 DeveloperToolbar.prototype.handleEvent > 0 > JavaScript error: resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/system.js, line 303: TypeError: front is undefined > TEST-INFO | load from browser_cmd_csscoverage_page1.html > 7 INFO TEST-PASS | browser/devtools/commandline/test/browser_cmd_csscoverage_startstop.js | page 1 loaded The tab seems to load a new page but releaseSystem for the old one fails due to front is undefined because it hasn't finished loading. This causes all kinds of weird issues and havoc later on especially as the test happens to open the toolbox which creates its own requisition, systems, fronts etc. Now, we could just wait for the front to load before destroying it but that will lead to a new class of possible problems. For example, if the about to be destroyed front is needed again (it might very well be as every tab switch destroys previous front and creates a new one) what will happen then? I don't know and I don't have time to figure it out just now. To me the current situation looks a lot better than the alternative universe where the refcounting was working as expected.

Per bug 1491875, this component has been closed, and the affected code is being removed from Firefox. Closing this bug as incomplete.