Closed Bug 689160 Opened 9 years ago Closed 9 years ago

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

Categories

(DevTools :: General, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 10

People

(Reporter: rcampbell, Assigned: rcampbell)

References

Details

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

Attachments

(1 file, 1 obsolete file)

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.
Depends on: 689164
Assignee: nobody → rcampbell
Attached patch csshtmltree leak fix (obsolete) — Splinter Review
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+
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+
Whiteboard: [fixed-in-fx-team]
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
https://hg.mozilla.org/mozilla-central/rev/294ed82810e1
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 10
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.