Since bug 582596 landed on the fx-team repo, we leak data:text/html,basic%20style%20inspector%20tests.

This can only be from browser/devtools/styleinspector/test/browser/browser_styleinspector.js but that test is very simple (almost identical to other tests). It does not leak locally according to the test log. Still investigating.

Without a way to reproduce this I am working blindly but I have added a destructor to the style inspector and this hopefully plugs the hole.

> Without a way to reproduce this I am working blindly This was on tinderbox, so you can push it to the try server (try: -b d -p linux64 -u mochitest-o -t none).

This is pretty easy to reproduce with a debug build and running the whole styleinspector suite. Without the patch: [browser/devtools/styleinspector/test/browser/browser_styleinspector.js] 2x [chrome://browser/content/csshtmltree.xhtml] 1x [data:text/html,basic%20style%20inspector%20tests] 1x [about:blank] 1x docShells With the patch applied: No leaks at all.

Thanks Tim! Michael, why are you adding a destroy method that needs to be called explicitly -- why not do this automatically when the panel closes?

Interestingly, running the whole styleinspector suite didn't show any leaks but I am glad that somebody could test this. Because we don't always want to destroy it straight away when it closes ... we have to preserve state whilst looking at other nodes. e.g. When the style inspector is registered with the highlighter: 1. Highlight a child node 2. Expand the color property 3. Close the style inspector 4. Using the highlighter click on the parent node The color property should still be expanded and the panel should be scrolled to the same location as with the child node (this comes as part of a later patch).

Comment on attachment 557111 [details] [diff] [review] Mochitest leak patch 1 I'm not familiar with the things mentioned in comment 6, so I don't feel comfortable reviewing this. It looks wrong to me that browser_styleinspector.js needs to explicitly call stylePanel.destroy.

(In reply to Michael Ratcliffe from comment #6) > Interestingly, running the whole styleinspector suite didn't show any leaks > but I am glad that somebody could test this. These are not "leaks" that will show in the leak statistics at the end of the test run but these are objects that are kept alive until the whole test suites shuts down. So you can observe this by running the whole test suite and after "TEST-START Shutdown" there is a series of "--DOMWINDOW" and "--DOCSHELL" lines. If they occur after the test shutdown then there's something that keeps those alive.

Of course Däo is correct that the style inspector panels should destroy themselves onpopuphide so this is now implemented. Because we need to preserve tool state and the tools themselves when they are registered with the highlighter I have added a toggle that allows us to preserve the tool on hide. Of course, the highlighter will need to ensure that it destroys all registered tools to prevent any further leaks.

Comment on attachment 557468 [details] [diff] [review] Mochitest leak patch 2 Review of attachment 557468 [details] [diff] [review]: ----------------------------------------------------------------- Confirmed. I reproduced the memleak locally, and this patch fixes the memleak. Giving it r+. Thank you! One comment on the StyleInspector.jsm: it's not ideal to return the xul:panel and overload the DOM element with properties and methods. In the future we should have a StyleInspector object returned that has its own properties, methods, etc, as we need, including a .panel property that points to the xul:panel DOM element. This would bring the StyleInspector closer to how the Tree Panel works - see bug 650794. Maybe a follow up bug report should be opened. Thanks!

Try is green: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=3ba2b5869de8 Ready to land

(In reply to Mihai Sucan [:msucan] from comment #10) > Comment on attachment 557468 [details] [diff] [review] > Mochitest leak patch 2 > > Review of attachment 557468 [details] [diff] [review]: > ----------------------------------------------------------------- > > Confirmed. I reproduced the memleak locally, and this patch fixes the > memleak. Giving it r+. Thank you! > > One comment on the StyleInspector.jsm: it's not ideal to return the > xul:panel and overload the DOM element with properties and methods. In the > future we should have a StyleInspector object returned that has its own > properties, methods, etc, as we need, including a .panel property that > points to the xul:panel DOM element. This would bring the StyleInspector > closer to how the Tree Panel works - see bug 650794. Maybe a follow up bug > report should be opened. Thanks! BHug 685893 logged