The default bug view has changed. See this FAQ.

data:text/html,basic%20style%20inspector%20tests leaked during mochitest-browser-chrome

RESOLVED FIXED in Firefox 9

Status

()

Firefox
Developer Tools
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dao, Assigned: miker)

Tracking

({mlk})

Trunk
Firefox 9
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [minotaur][styleinspector])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
Status: NEW → ASSIGNED
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.
Attachment #557111 - Flags: review?(dao)
(Reporter)

Comment 3

6 years ago
> 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.
(Reporter)

Comment 5

6 years ago
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).
(Reporter)

Comment 7

6 years ago
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.
Attachment #557111 - Flags: review?(dao)
(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.
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.
Attachment #557111 - Attachment is obsolete: true
Attachment #557468 - Flags: review?(mihai.sucan)
Blocks: 683672
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!
Attachment #557468 - Flags: review?(mihai.sucan) → review+
Try is green:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=3ba2b5869de8

Ready to land
Whiteboard: [minotaur][styleinspector][has-patch][review+]
Blocks: 683737

Updated

6 years ago
Whiteboard: [minotaur][styleinspector][has-patch][review+] → [minotaur][styleinspector][has-patch][review+][land-in-fx-team]
No longer blocks: 683672
(Reporter)

Updated

6 years ago
Whiteboard: [minotaur][styleinspector][has-patch][review+][land-in-fx-team] → [minotaur][styleinspector][inbound]
http://hg.mozilla.org/mozilla-central/rev/4771660af6b2
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
(Reporter)

Updated

6 years ago
Whiteboard: [minotaur][styleinspector][inbound] → [minotaur][styleinspector]
(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
You need to log in before you can comment on or make changes to this bug.