Closed Bug 557477 Opened 10 years ago Closed 10 years ago

Content/Chrome tabs leak javascript objects/functions

Categories

(Thunderbird :: Toolbars and Tabs, defect)

defect
Not set

Tracking

(thunderbird3.1 beta2-fixed, thunderbird3.0 .5-fixed)

RESOLVED FIXED
Thunderbird 3.1b2
Tracking Status
thunderbird3.1 --- beta2-fixed
thunderbird3.0 --- .5-fixed

People

(Reporter: standard8, Assigned: standard8)

Details

(Keywords: memory-leak)

Attachments

(1 file)

Attached patch The fixSplinter Review
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.
Attachment #437265 - Flags: review?(bugmail)
Keywords: mlk
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...)
Attachment #437265 - Flags: review?(bugmail) → review+
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.
Checked in: http://hg.mozilla.org/comm-central/rev/436e299ed6a9
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.1b2
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.
Attachment #437265 - Flags: approval-thunderbird3.0.5?
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.
Attachment #437265 - Flags: approval-thunderbird3.0.5? → approval-thunderbird3.0.5+
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.
You need to log in before you can comment on or make changes to this bug.