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

RESOLVED FIXED

Status

Tech Evangelism
Add-ons
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Loic, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2], URL)

(Reporter)

Description

5 years ago
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]
(Reporter)

Updated

5 years ago
Blocks: 668871, 700547
Whiteboard: [MemShrink]
Developer cc'd.

Comment 2

5 years ago
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.
(Reporter)

Comment 3

5 years ago
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

5 years ago
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.
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?
(Reporter)

Comment 6

5 years ago
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]
BTW, this is the #2 add-on on AMO and has over 7 million users.

Comment 8

5 years ago
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.
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

5 years ago
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 :)
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);
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

5 years ago
Can we consider that issue has been solved so i can roll out an update ?
We'll consider the issue solved when you roll out the update and the zombie compartments are gone.

Comment 15

5 years ago
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 ?
(Reporter)

Comment 16

5 years ago
(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

5 years ago
Many thanks Loic. I'll make an update of Video DownloadHelper shortly.
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

5 years ago
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.
Whiteboard: [MemShrink] → [MemShrink:P2]
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.