Last Comment Bug 683320 - data:text/html,basic%20style%20inspector%20tests leaked during mochitest-browser-chrome
: data:text/html,basic%20style%20inspector%20tests leaked during mochitest-brow...
Status: RESOLVED FIXED
[minotaur][styleinspector]
: mlk
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 9
Assigned To: Michael Ratcliffe [:miker] [:mratcliffe]
:
Mentors:
Depends on:
Blocks: 582596 bc-leaks 683737
  Show dependency treegraph
 
Reported: 2011-08-30 14:22 PDT by Dão Gottwald [:dao]
Modified: 2011-09-09 09:12 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Mochitest leak patch 1 (4.71 KB, patch)
2011-08-31 02:48 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Mochitest leak patch 2 (4.87 KB, patch)
2011-09-01 05:23 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
mihai.sucan: review+
Details | Diff | Splinter Review

Description Dão Gottwald [:dao] 2011-08-30 14:22:02 PDT
Since bug 582596 landed on the fx-team repo, we leak data:text/html,basic%20style%20inspector%20tests.
Comment 1 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-31 00:58:05 PDT
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.
Comment 2 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-31 02:48:45 PDT
Created attachment 557111 [details] [diff] [review]
Mochitest leak patch 1

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.
Comment 3 Dão Gottwald [:dao] 2011-08-31 02:53:30 PDT
> 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).
Comment 4 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-08-31 03:29:51 PDT
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.
Comment 5 Dão Gottwald [:dao] 2011-08-31 03:34:19 PDT
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?
Comment 6 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-31 04:17:26 PDT
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 7 Dão Gottwald [:dao] 2011-08-31 04:31:25 PDT
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.
Comment 8 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-08-31 06:13:10 PDT
(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.
Comment 9 Michael Ratcliffe [:miker] [:mratcliffe] 2011-09-01 05:23:38 PDT
Created attachment 557468 [details] [diff] [review]
Mochitest leak patch 2

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 10 Mihai Sucan [:msucan] 2011-09-02 05:51:40 PDT
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!
Comment 11 Michael Ratcliffe [:miker] [:mratcliffe] 2011-09-02 15:08:04 PDT
Try is green:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=3ba2b5869de8

Ready to land
Comment 12 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-09-06 04:07:58 PDT
http://hg.mozilla.org/mozilla-central/rev/4771660af6b2
Comment 13 Michael Ratcliffe [:miker] [:mratcliffe] 2011-09-09 09:12:59 PDT
(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

Note You need to log in before you can comment on or make changes to this bug.