Investigate and Fix leaked csshtmltree.xhtml windows in Highlighter+Style Inspector

RESOLVED FIXED in Firefox 10

Status

()

Firefox
Developer Tools
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: rc, Assigned: rc)

Tracking

unspecified
Firefox 10
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
During the landing of bug 663831 and associates, it was found to leak csshtmltree.xhtml tree during the course of the inspector tests. We should investigate and fix this leak as well as provide a proper test of the Style Inspector from the Highlighter toolbar.
(Assignee)

Updated

6 years ago
Depends on: 689164
(Assignee)

Updated

6 years ago
Assignee: nobody → rcampbell
(Assignee)

Comment 1

6 years ago
Created attachment 565015 [details] [diff] [review]
csshtmltree leak fix

A somewhat hacky patch that does manage to clear out the references to csshtmltree.xhtml on inspector close. All tests passed locally with no additional leaked windows.

This should be made better when the work for bug 685893 is completed but should at least let us turn on the style inspector in the interim.
Attachment #565015 - Flags: review?(mihai.sucan)
Comment on attachment 565015 [details] [diff] [review]
csshtmltree leak fix

Review of attachment 565015 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks good and the memleaks are gone. Thank you!

As previously discussed, this patch should have a mochitest for opening and closing the Style Inspector from within the Highlighter.
Attachment #565015 - Flags: review?(mihai.sucan) → review+
(Assignee)

Comment 3

6 years ago
Created attachment 565521 [details] [diff] [review]
[in-fx-team] csshtmltree leak fix + test

added a simple test to browser_highlighter_initialization.js.
Attachment #565015 - Attachment is obsolete: true
Attachment #565521 - Flags: review?(mihai.sucan)
Comment on attachment 565521 [details] [diff] [review]
[in-fx-team] csshtmltree leak fix + test

Review of attachment 565521 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks fine! All tests pass.

::: browser/devtools/styleinspector/CssHtmlTree.jsm
@@ +328,5 @@
>      CssHtmlTree.propertyNames.push.apply(CssHtmlTree.propertyNames,
>        mozProps.sort());
>    },
> +
> +  destroy: function CssHtmlTree_destroy()

Maybe a jsdoc comment here?
Attachment #565521 - Flags: review?(mihai.sucan) → review+
(Assignee)

Updated

6 years ago
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 5

6 years ago
Comment on attachment 565521 [details] [diff] [review]
[in-fx-team] csshtmltree leak fix + test

https://hg.mozilla.org/integration/fx-team/rev/294ed82810e1
Attachment #565521 - Attachment description: csshtmltree leak fix + test → [in-fx-team] csshtmltree leak fix + test
(Assignee)

Comment 6

6 years ago
https://hg.mozilla.org/mozilla-central/rev/294ed82810e1
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 10
(Assignee)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.