Last Comment Bug 725875 - Gridtube 1.4.3 add-on causes a single youtube.com zombie compartment
: Gridtube 1.4.3 add-on causes a single youtube.com zombie compartment
Status: RESOLVED FIXED
[MemShrink]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: general
:
Mentors:
Depends on:
Blocks: LeakyAddons ZombieCompartments
  Show dependency treegraph
 
Reported: 2012-02-09 16:34 PST by Nils Maier [:nmaier]
Modified: 2012-02-10 15:42 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Affected version 1.4.3 (10.58 KB, application/x-xpinstall)
2012-02-09 16:34 PST, Nils Maier [:nmaier]
no flags Details

Description Nils Maier [:nmaier] 2012-02-09 16:34:32 PST
Created attachment 595907 [details]
Affected version 1.4.3

STR:
- Open a youtube search results page
- Wiggle your mouse over some result item, in particular the question mark icon that is displayed as the result item description is expanded on mouse over.
- Close the tab
- Open about:memory?verbose
- Hit Minimize memory use a couple of times
- Look for any compartments for youtube
-> Still present

This seems to happen due to implicit variable declarations in overlay.js#popupShow/popupHide: node is missing a |var|.
I cannot reproduce after adding the missing |var| declarations.

As this seems to be due to an implicit variable declaration, at most one content window + compartment will be leaked.
Using the steps above, it can be determined that in fact at most one window + compartment is leaked.

Previous versions (1.4.2) might be affected, too, as they share the same suspected culprit, but in the add-on default configuration the leak is not reproducible.
Comment 1 Nicholas Nethercote [:njn] 2012-02-09 20:11:25 PST
This is the second one this week where an add-on leak has been found to be caused by a missing |var|, and which "use strict;" would have caught.  Grr.
Comment 2 Nils Maier [:nmaier] 2012-02-09 21:06:00 PST
Well, it's usually not as easy as sticking in "use strict"; You'll actually spend a lot of time fixing your code. I'd be all in favor of a new "use noimplicitdecl"; or something similar and force public add-ons to use that or strict mode in all code via AMO policy, maybe turning it on by default for chrome code after a grace period.

Here is a long-fixed window leak I created myself. Leaked our DownThemAll! windows, not content pages:
https://bugs.downthemall.net/changeset/2744
Another one regarding references keeping windows alive:
https://bugs.downthemall.net/changeset/2734
And a very funky one:
https://bugs.downthemall.net/changeset/2750
Comment 3 Jorge Villalobos [:jorgev] 2012-02-10 07:25:21 PST
Nils reviewed the add-on and pointed out the leak to the developer. It should be fixed soon, hopefully.
Comment 4 Costas B. 2012-02-10 14:06:01 PST
I am the addon author. Thanks for noticing this bug. Indeed it caused the aforementioned memory leak problem. I didn't declare the specific variable 'node' as 'var'. I fixed this, by declaring the variable properly in both functions (popupShow/popupHide). After than I didn't get the same problem again. I will resubmit for full review as new version.
Comment 5 Nils Maier [:nmaier] 2012-02-10 15:42:16 PST
Version 1.4.4 was submitted and made public that fixes the leaks

Note You need to log in before you can comment on or make changes to this bug.