795633 - Zombie compartment with add-on Video DownloadHelper 4.9.10 after visiting tumblr.com

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[Loic]

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] (he/him)]

Developer cc'd.

Comment 2

[Michel Gutierrez]

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]

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]

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]]

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]

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 [inactive]]

BTW, this is the #2 add-on on AMO and has over 7 million users.

Comment 8

[Michel Gutierrez]

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]]

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]

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]]

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]]

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]

Can we consider that issue has been solved so i can roll out an update ?

Comment 14

[Kris Maglione [:kmag]]

We'll consider the issue solved when you roll out the update and the zombie compartments are gone.

Comment 15

[Michel Gutierrez]

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]

(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]

Many thanks Loic. I'll make an update of Video DownloadHelper shortly.

Comment 18

[Kris Maglione [:kmag]]

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]

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.