Last Comment Bug 795633 - Zombie compartment with add-on Video DownloadHelper 4.9.10 after visiting tumblr.com
: Zombie compartment with add-on Video DownloadHelper 4.9.10 after visiting tum...
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Tech Evangelism
Classification: Other
Component: Add-ons (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
http://urbisetorbis.tumblr.com/
Depends on:
Blocks: LeakyAddons ZombieCompartments
  Show dependency treegraph
 
Reported: 2012-09-29 13:31 PDT by Loic
Modified: 2012-11-01 16:37 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments

Description Loic 2012-09-29 13:31:15 PDT
The issue is reproducible with FF15+.

STR:
1) Start a new profile
2) Install add-on Video DownloadHelper 4.9.10 https://addons.mozilla.org/en-US/firefox/addon/video-downloadhelper/
3) Open about:compartments and about:memory
4) Visit http://urbisetorbis.tumblr.com/ a few seconds then close the tab

Results on FF18:

User Compartments
about:blank
http://urbisetorbis.tumblr.com/post/29832864305/photoset_iframe/glam-val/tumblr_m92aj9B0m61qbd8lf/500/false, about:blank
null-principal
resource://gre-resources/hiddenWindow.html

Ghost Windows
http://urbisetorbis.tumblr.com/post/29832864305/photoset_iframe/glam-val/tumblr_m92aj9B0m61qbd8lf/500/false [2]

├─────251,896 B (00.38%) -- top(none)/ghost/window(http://urbisetorbis.tumblr.com/post/29832864305/photoset_iframe/glam-val/tumblr_m92aj9B0m61qbd8lf/500/false)
│  │     ├──250,968 B (00.38%) -- js/compartment(http://urbisetorbis.tumblr.com/post/29832864305/photoset_iframe/glam-val/tumblr_m92aj9B0m61qbd8lf/500/false, about:blank)
│  │     │  ├──130,400 B (00.20%) ── analysis-temporary
│  │     │  ├───94,208 B (00.14%) -- gc-heap
│  │     │  │   ├──51,160 B (00.08%) ── unused-gc-things
│  │     │  │   ├──15,576 B (00.02%) ── shapes/dict
│  │     │  │   ├──14,768 B (00.02%) ── sundries
│  │     │  │   └──12,704 B (00.02%) ── objects/function
│  │     │  └───26,360 B (00.04%) ── other-sundries
│  │     └──────928 B (00.00%) ── dom/other [2]
Comment 1 Jorge Villalobos [:jorgev] 2012-10-01 08:10:18 PDT
Developer cc'd.
Comment 2 Michel Gutierrez 2012-10-01 08:23:50 PDT
If you check the memory while the download is still available (the DownloadHelper icon is still spinning), even if the corresponding tab has been closed, then it is normal to see this in the memory dump as a reference has been kept by the download entry.

By default, the download entry is released after one minute at the next tab event (opening/closing/switching any tab).

If the memory blocks remain while the DownloadHelper icon is grey, then this is a bug to be fixed.
Comment 3 Loic 2012-10-01 09:15:03 PDT
I didn't download anything with VDH in my STR: I just loaded the offending URL and closed the tab after 5 sec. The memory log has been recorded after I closed the tab 1 hour ago.
Comment 4 Michel Gutierrez 2012-10-01 09:25:14 PDT
The fact that you download the video or not does not affect the behaviour i described. It's the fact the VDH sees the video as been downloadable that counts.

But if you see the memory compartment after 1 hour, then yes, there is a problem here. I am going to look into it.
Comment 5 Kris Maglione [:kmag] 2012-10-01 16:25:23 PDT
Whether intentional or not, it's not allowed. Is there some technical reason that you need a reference to objects from that document? Also, how are you storing the reference that the wrappers aren't being cut when the window is destroyed?
Comment 6 Loic 2012-10-02 18:32:13 PDT
Another example with this page:
http://www.pcinpact.com/news/74246-microsoft-propose-typescript-pour-developpement-projets-javascript.htm

After 30 min, the JS compartment is still here:

├───1,172,216 B (01.40%) -- top(none)/ghost/window(http://channel9.msdn.com/posts/Anders-Hejlsberg-Introducing-TypeScript/player?w=512&h=288&wmode=transparent)
│  │   ├──1,171,288 B (01.40%) -- js/compartment(http://channel9.msdn.com/posts/Anders-Hejlsberg-Introducing-TypeScript/player?w=512&h=288&wmode=transparent)
│  │   │  ├────585,728 B (00.70%) -- gc-heap
│  │   │  │    ├──170,672 B (00.20%) -- objects
│  │   │  │    │  ├──110,912 B (00.13%) ── function
│  │   │  │    │  └───59,760 B (00.07%) ── non-function
│  │   │  │    ├──157,856 B (00.19%) -- shapes
│  │   │  │    │  ├───76,848 B (00.09%) ── dict
│  │   │  │    │  ├───55,056 B (00.07%) ── tree
│  │   │  │    │  └───25,952 B (00.03%) ── base
│  │   │  │    ├──124,544 B (00.15%) ── unused-gc-things
│  │   │  │    ├───97,920 B (00.12%) ── scripts
│  │   │  │    ├───29,440 B (00.04%) ── type-objects
│  │   │  │    └────5,296 B (00.01%) ── sundries
│  │   │  ├────333,928 B (00.40%) ── script-data
│  │   │  ├────115,408 B (00.14%) ── analysis-temporary
│  │   │  ├─────67,168 B (00.08%) -- shapes-extra
│  │   │  │     ├──31,072 B (00.04%) ── dict-tables
│  │   │  │     ├──23,168 B (00.03%) ── tree-tables
│  │   │  │     └──12,928 B (00.02%) ── compartment-tables
│  │   │  ├─────35,072 B (00.04%) ── objects/slots
│  │   │  ├─────18,320 B (00.02%) ── other-sundries
│  │   │  └─────15,664 B (00.02%) ── type-inference/object-main
│  │   └────────928 B (00.00%) ── dom/other [2]
Comment 7 Nicholas Nethercote [:njn] 2012-10-02 19:08:25 PDT
BTW, this is the #2 add-on on AMO and has over 7 million users.
Comment 8 Michel Gutierrez 2012-10-03 10:43:44 PDT
I do have a fix that works ok for me: http://www.downloadhelper.net/install-beta.php?version=4.9.11b1

Basically, the problem happened when DownloadHelper saw a downloadable gallery inside a page frame/iframe. The cleanup was not done properly as it is when the gallery was at top page level.

After fixing that problem, i ran into another issue still in the gallery download feature where a mouse listener was keeping a reference to the document through a closure. It now uses a weak reference properly.

Let me know how that fix works in your environment.
Comment 9 Kris Maglione [:kmag] 2012-10-03 12:05:28 PDT
Ok, so the reason the wrapper cutting seems not to be working here is that for some reason the add-on is using a nsIMutableArray rather than a JS object to store its references to web content. The updated version does seem to address the issues, but I'd recommend removing all unnecessary nsIMutableArray usage.

Also, there should be no need to ever query a DOM node to nsIDOMEventTarget. It has nothing to do with this problem, just a nit...
Comment 10 Michel Gutierrez 2012-10-03 12:17:11 PDT
As far as i remember, i don't make any explicit usage of nsIMutableArray, just plain JS objects and some nsIProperties too.

About nsIDOMEventTarget, you are right. It's just that this add-on has been around for 6 years. Some things are not necessary anymore but change is dangerous with millions as a user base :)
Comment 11 Kris Maglione [:kmag] 2012-10-03 12:24:37 PDT
The entry object that you reference in the changed code comes from the following in one of the *-probe.jsm files:

	var fdescs=Components.classes["@mozilla.org/array;1"].
		createInstance(Components.interfaces.nsIMutableArray);
Comment 12 Kris Maglione [:kmag] 2012-10-03 12:26:52 PDT
Ah, sorry, you're right, the entries object the following:

	var desc=Components.classes["@mozilla.org/properties;1"].
		createInstance(Components.interfaces.nsIProperties);

The nsIMutableArray is a different (still culpable) object. The issue is the same. It's best to avoid these types of objects unless you need them to communicate with native code.
Comment 13 Michel Gutierrez 2012-10-15 00:50:46 PDT
Can we consider that issue has been solved so i can roll out an update ?
Comment 14 Kris Maglione [:kmag] 2012-10-15 09:13:09 PDT
We'll consider the issue solved when you roll out the update and the zombie compartments are gone.
Comment 15 Michel Gutierrez 2012-10-15 09:26:40 PDT
Thanks Kris, but the purpose of this bugzilla service is to check bugs are fixed before rolling out a version to users, and in this case it involves ~10 million people.

So far, no one in this bug thread said "i was seeing the zombie issue in 4.9.10 and no longer in 4.9.11b1" except me but it doesn't count since i'm the developer :)

Loic, can you confirm that bug has gone so i can release the update and we'll be able to close the bug ?
Comment 16 Loic 2012-10-15 09:55:46 PDT
(In reply to Michel Gutierrez from comment #15) 
> Loic, can you confirm that bug has gone so i can release the update and
> we'll be able to close the bug ?

I tried with VDH 4.9.11b1 and I'm not able to reproduce the zombie compartments with links posted in comment #0 and comment #6. So I guess it's fixed.
Comment 17 Michel Gutierrez 2012-10-15 14:28:52 PDT
Many thanks Loic. I'll make an update of Video DownloadHelper shortly.
Comment 18 Kris Maglione [:kmag] 2012-10-19 16:46:08 PDT
Michel, when are you going to have a new version ready on AMO? Please let me know when you do so that I can review it.
Comment 19 Michel Gutierrez 2012-10-20 00:16:37 PDT
I don't know for sure as i have a few other minor bugs i'd like to see in the new release. Probably next week or the week after.

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