557477 - Content/Chrome tabs leak javascript objects/functions

Status: RESOLVED FIXED

Description

[Mark Banner (:standard8) (afk 15 - 18 Oct)]

Created attachment 437265 [details] [diff] [review]
The fix

I've just found that leak monitor was updated last year (having given up as it wasn't updated for absolutely ages).

As a result, it shows that we're leaking various things in the main window. Most of these aren't to do with content tabs and need other bugs filing, but I'll do that later.

STR:

1) Install Leak monitor
2) Run Thunderbird
3) Open the what's new page (or migration assistant for chrome tabs)
4) Close the tab
5) Close the main window (if you're not on Mac you may want to open the Address Book first to prevent shut-down).
6) Wait for leak monitor windows to appear.

Reported leaks:
Leaks in window 0x158ee840:
[ ] [leaked object] (1e5c1860, chrome://global/content/bindings/browser.xml, 481-498) = [Function]
[ ] [leaked object] (1e27f440, chrome://messenger/content/threadPane.js, 42-90) = [Function]
[+] [leaked object] (18ab4120) = [XULElement]
[+] [leaked object] (215aea80) = [XULElement]
[ ] [leaked object] (1dede860, chrome://messenger/content/msgMail3PaneWindow.js, 936-947) = [Function]
[ ] [leaked object] (1e5c18a0, chrome://global/content/bindings/browser.xml, 515-531) = [Function]
[ ] [leaked object] (158ee840) = [ChromeWindow]
[+] [leaked object] (1dedbbe0) = [Object]
[-] [leaked object] (18ab41c0) = [XULElement]
[ ] [leaked object] (1e5c1060, chrome://global/content/charsetOverlay.js, 237-245) = [Function]
[+] [leaked object] (1f02a520) = [Object]
576-581) = [Function]
[ ] [leaked object] (196be9e0, chrome://messenger/content/tabmail.xml, 1810-1818) = [Function]
[-] [leaked object] (18ab4220) = [XULElement]
[-] [leaked object] (18ab4240) = [XULElement]
[+] [leaked object] (18ab4260) = [Object]
[+] [leaked object] (1dedb600) = [Object]
 [ ] _kAboutRightsVersion = 1
 [ ] _protocolSvc = true
 [ ] openSpecialTabsOnStartup (1dedb640, chrome://messenger/content/specialTabs.js, 147-176) = [Function]
 [+] contentTabType (1dedb660) = [Object]
...
[ ] [leaked object] (20852680, chrome://messenger/content/msgMail3PaneWindow.js, 785-791) = [Function]
[+] [leaked object] (18ab4140) = [XULElement]
[+] [leaked object] (18ab4100) = [XULElement]
[+] [leaked object] (1ea4a900) = [XULElement]
[ ] [leaked object] (1e5c1840, chrome://global/content/bindings/browser.xml, 456-474) = [Function]
[+] [leaked object] (215aea20) = [XULElement]

For this bug, I'm looking specifically at the browser.xml functions and the specialTabs object.

The specialTabs object leak should be once per a main window. The browser.xml functions will be per content/chrome tab opened.

Thankfully, looking at the Firefox specific source code, it reveals that their tabbrowser calls browser.destroy() when closing a tab - this ensures the browser binding cleans up correctly as due to the wonders of xbl, it doesn't always.

So a couple of calls to destroy() fixes the browser.xml leaks. Adding a call to remove the mail-startup-done listener once it has been fired resolves the specialTabs object leak.

I don't think we can explicitly test for this at the moment as we don't have a harness suitable for this type of leak test.

Comment 1

[Andrew Sutherland [:asuth]]

Comment on attachment 437265 [details] [diff] [review]
The fix

We should be able to test for a leak no longer happening by explicitly obtaining a weak reference to the object in question, forcing a cycle-collecting GC, and then verifying that the weak reference is now empty.

Having said that, I agree that the proper solution is a more systemic memory leak detector harness like integrating the leak detector extension, not a bunch of one-off piecemeal tests.  (At least not unless we keep regressing the exact same leak before we have the proper solution...)

Comment 2

[Mark Banner (:standard8) (afk 15 - 18 Oct)]

I've raised bug 557764 to investigate the global nature of this. I agree that we don't really want one-off piecemeal tests, especially when it comes to functions or xul elements being held onto.

Comment 3

[Mark Banner (:standard8) (afk 15 - 18 Oct)]

Checked in: http://hg.mozilla.org/comm-central/rev/436e299ed6a9

Comment 4

[Mark Banner (:standard8) (afk 15 - 18 Oct)]

Comment on attachment 437265 [details] [diff] [review]
The fix

Although this is only a small leak, I'm still going to consider it for the 3.0 branch.

Comment 5

[Mark Banner (:standard8) (afk 15 - 18 Oct)]

Comment on attachment 437265 [details] [diff] [review]
The fix

Not had any issues raised about this, and its a minor improvement for users, therefore taking for 3.0.5.

Comment 6

[Mark Banner (:standard8) (afk 15 - 18 Oct)]

I checked in a reduced version of this to branch:

http://hg.mozilla.org/releases/comm-1.9.1/rev/3cb9509da408

The mail-startup-done registration wasn't present there as we didn't have the xpinstall notifications like we do on TB 3.1.