Last Comment Bug 691102 - zombie compartments if Image Zoom in use
: zombie compartments if Image Zoom in use
Status: RESOLVED WONTFIX
[MemShrink:P3]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Windows XP
: -- normal with 2 votes (vote)
: ---
Assigned To: general
:
Mentors:
https://addons.mozilla.org/firefox/ad...
Depends on:
Blocks: LeakyAddons ZombieCompartments
  Show dependency treegraph
 
Reported: 2011-10-01 15:33 PDT by Sebastian H. [:aryx][:archaeopteryx]
Modified: 2013-04-01 12:27 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description Sebastian H. [:aryx][:archaeopteryx] 2011-10-01 15:33:01 PDT
Mozilla/5.0 (Windows NT 5.1; rv:10.0a1) Gecko/20111001 Firefox/10.0a1
Mozilla/5.0 (Windows NT 5.1; rv:7.0.1) Gecko/20100101 Firefox/7.0.1

With the Image Zoom add-on installed ( https://addons.mozilla.org/firefox/addon/image-zoom/ ), using it's functionality - right-clicking an image, keeping the mouse button pressed and using the scroll wheel to zoom - makes this compartment never going away after closing the tab.

Steps to reproduce:
1. Install the Image Zoom add-on verison 0.4.6 from https://addons.mozilla.org/firefox/addon/image-zoom/ (Nightly users have to increase the maxVersion in the install.rdf for compatibility).
2. Open http://shaverfacts.com/images/shaver4.jpg?1269630575
3. Right-click the image, hold the button and use the mouse wheel to zoom into the image.
4. Close the tab.
5. Open about:memory?verbose
6. Click "Minimize memory usage" a few times.

Actual result: This still exists.
│   ├──────89,858 B (00.06%) -- compartment(http://shaverfacts.com/images/shaver4.jpg?1269630575)
│   │      ├──86,016 B (00.06%) -- gc-heap
│   │      │  ├──50,424 B (00.04%) -- arena-unused
│   │      │  ├──18,592 B (00.01%) -- objects
│   │      │  ├──16,520 B (00.01%) -- shapes
│   │      │  ├─────336 B (00.00%) -- arena-headers
│   │      │  ├─────144 B (00.00%) -- arena-padding
│   │      │  ├───────0 B (00.00%) -- strings
│   │      │  └───────0 B (00.00%) -- xml
│   │      ├───3,728 B (00.00%) -- object-slots
│   │      ├─────114 B (00.00%) -- scripts
│   │      ├───────0 B (00.00%) -- string-chars
│   │      ├───────0 B (00.00%) -- mjit-code
│   │      ├───────0 B (00.00%) -- mjit-data
│   │      ├───────0 B (00.00%) -- tjit-code
│   │      └───────0 B (00.00%) -- tjit-data
│   │              ├──0 B (00.00%) -- allocators-main
│   │              └──0 B (00.00%) -- allocators-reserve

Expected result:
Not this compartment anymore.
Comment 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-01 21:25:09 PDT
The extension has code like this, at global scope:

  if (!net) var net = {};
  if (!net.yellowgorilla) net.yellowgorilla = {};
  if (!net.yellowgorilla.imagezoom) net.yellowgorilla.imagezoom = {};
  net.yellowgorilla.imagezoom.overlay = new function () {
  ...
      function izImage(oImage) {
          var pImage = oImage;
          // Define a bunch of functions that close over pImage
          // Stick references on all those functions on izImage.prototype
      }
  ...
    this.izZoomIn = function () {
        //Create the object and invoke its zoom method passing the factor to zoom
        var oizImage = new izImage(document.popupNode);
        oizImage.zoom(nsIPrefBranchObj.getIntPref("zoomvalue") / 100);
        reportStatus(oizImage);
    }
  }

So now consider what happens when the net.yellowgorilla.imagezoom.overlay.izZoomIn function is called.  It will take document.popupNode (that is, the image element), and change izImage.prototype to hold references to functions that close over that image element.  So as long as the izImage function exists (which is as long as as the net.yellowgorilla.imagezoom.overlay object sticks around) it keeps its .prototype alive, which keeps the functions stored on it alive, which keep the closed-over image node alive.

In other words, the net effect of all this code is that this extension is just sticking a reference to the last image it operated on in a global variable in the chrome scope.  So of course that compartment will stick around until the extension operates on some other image.  Unless, of course, there's some critical subtlety that I'm missing somewhere in this code where izImage.prototype gets cleared.

Jorge, how can we get in touch with this addon author to get them to fix their extension?  :(

I wonder whether we can catch cases in which a compartment is only kept alive due to cross-compartment wrappers from chrome and report them somehow....
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-01 21:27:36 PDT
Oh, a slight correction.  It's not the net.yellowgorilla.imagezoom.overlay object that keeps the izImage function alive.  It's all the properties on it that point to function that close over the value of izImage from the original function invocation that created the net.yellowgorilla.imagezoom.overlay object.  The distinction is pretty academic, though, since it's not like anything removes all those member functions from the overlay object.
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-01 21:32:19 PDT
Posted http://imagezoom.yellowgorilla.net/forums/viewtopic.php?pid=2348
Comment 4 Jorge Villalobos [:jorgev] 2011-10-03 08:32:49 PDT
CCing the Image Zoom developer.
Comment 5 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-10-17 18:33:02 PDT
Great analysis, bz.  I'll pre-triage this as a P3 because that's what we do for memory leaks in add-ons.
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-17 19:19:27 PDT
Jorge, I've had no response from the Image Zoom developer...  Could you try getting in touch with him directly?  Do we have some way to attach memory leak metadata to addons in AMO?
Comment 7 Jason Adams 2011-10-17 19:31:44 PDT
Image Zoom Developer Here, I understand there is a memory leak however I am a simple javascript developer and don't know anything about memory management.
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-17 19:59:08 PDT
Jason, comment 1 explains what the issue is.  Basically, when you redefine the functions on izImage.prototype all those function objects you create hold references to the image in question and keep it alive.

Why do you need to redefine those functions every time, exactly?  Can you avoid doing that?
Comment 9 Jason Adams 2011-10-17 20:20:59 PDT
To tell you the truth, its been 7 years since I developed Image Zoom so I can't remember the details off the top of my head on what and why I am doing certain things. I will look into it. So you are saying if I avoid redefining the prototype functions each time, the memory leak will be solved? Do I basically need to ensure that I am not referencing the image anywhere after I have finished with it?
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-17 20:24:16 PDT
> So you are saying if I avoid redefining the prototype functions each time, the memory
> leak will be solved?

I can't guarantee it, but that looked like the only path via which the extension keeps referencing the image.

> Do I basically need to ensure that I am not referencing the image anywhere after I have
> finished with it?

Yes, exactly.  I know this is a pain.  :(
Comment 11 Jason Adams 2011-10-17 20:33:30 PDT
How do I test? Any references to articles or tools would be appreciated
Comment 12 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-17 20:38:06 PDT
As far as testing whether the leak is fixed, just following the steps in comment 0 in a current Firefox nightly should work.

For finding references to the image.... there is no good tooling so far that I know of.  :(
Comment 13 Jason Adams 2011-10-17 21:13:00 PDT
OK reproduced the problem, thanks for reporting
Comment 14 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-10-18 04:04:47 PDT
(In reply to Boris Zbarsky (:bz) from comment #12)
> As far as testing whether the leak is fixed, just following the steps in
> comment 0 in a current Firefox nightly should work.
> 
> For finding references to the image.... there is no good tooling so far that
> I know of.  :(

The only workable tool we have is DEBUG_CC, which is a bit much to ask addon developers to use :-(
Comment 15 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2011-10-18 08:13:21 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #14)
> The only workable tool we have is DEBUG_CC, which is a bit much to ask addon
> developers to use :-(

CC graph dumping is not super easy to use, but it can be done in a release build.  Of course, that only helps if the CC is keeping the image alive, whereas it is probably JS.  In an ideal future, you would be able to dump the JS heap from an optimized build, too.
Comment 16 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-10-18 08:15:50 PDT
(In reply to Andrew McCreight [:mccr8] from comment #15)
> CC graph dumping is not super easy to use, but it can be done in a release
> build.  Of course, that only helps if the CC is keeping the image alive,
> whereas it is probably JS.  In an ideal future, you would be able to dump
> the JS heap from an optimized build, too.

Yeah, the problem is that generally if it's an addon or page bug it's going to be JS keeping it alive.  Dumping the CC graph can help us, but I don't think it's generally useful to others.  Being able to traverse the JS heap though like DEBUG_CC does would likely be quite useful for them.
Comment 17 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2011-10-18 08:18:46 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #16)
> Being able to traverse the JS heap
> though like DEBUG_CC does would likely be quite useful for them.

A graph dump always traces through marked JS objects, just like DEBUG_CC.  The problem is that neither traces from JS roots, so you can't tell why JS is kept alive.

I wonder how hard it would be to hook up my new JS heap dumping tools to create some kind of "zombie hunter" tool that tells you why a compartment is alive...
Comment 18 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2011-10-18 09:06:02 PDT
(In reply to Andrew McCreight [:mccr8] from comment #17)
> I wonder how hard it would be to hook up my new JS heap dumping tools to
> create some kind of "zombie hunter" tool that tells you why a compartment is
> alive...
File Bug 695348 for this.
Comment 19 Jorge Villalobos [:jorgev] 2012-02-22 10:51:00 PST
(In reply to Jason Adams from comment #13)
> OK reproduced the problem, thanks for reporting

Jason, can you give us an estimate on when will the fixed version be uploaded to AMO?
Comment 20 Andrew Williamson [:eviljeff] 2012-02-29 09:42:43 PST
(In reply to Jason Adams from comment #13)
> OK reproduced the problem, thanks for reporting

Hi Jason, can you let us know if you're still working on a new version to fix the leak?  If not we'll have to downgrade the existing versions on AMO to preliminary reviewed.
Thanks.
Comment 21 Jorge Villalobos [:jorgev] 2012-03-07 14:17:28 PST
The add-on has been downgraded to preliminary approval pending the memory leak fix.
Comment 22 Andrew Williamson [:eviljeff] 2012-03-14 05:44:44 PDT
Closing bug for now as there isn't any activity on a fix.  If an updated version is submitted or Jason wants to discuss further we can re-open.
Comment 23 Jason Adams 2013-03-30 15:43:48 PDT
I just tried to revisit this bug however I cannot reproduce it on Firefox 19. I would appreciate some help on progressing this so I can get my addon out of preliminary status. Regards, Jason
Comment 24 Kyle Huey [:khuey] (khuey@mozilla.com) 2013-03-30 15:56:39 PDT
The leak in this addon was probably fixed by Bug 695480 if nothing else ...
Comment 25 Jorge Villalobos [:jorgev] 2013-04-01 10:31:24 PDT
Since I can't reproduce this bug anymore, I'm moving the add-on to Fully Reviewed again.
Comment 26 Jason Adams 2013-04-01 10:44:10 PDT
There are a few incompatibilities with recent versions of firefox. I have fixed these and going through the testing phase now. I should have a new version to submit soon. Not sure if you want to set it to fully reviewed given these circumstances.
Comment 27 Jorge Villalobos [:jorgev] 2013-04-01 11:22:15 PDT
If they are big problems, I can set your add-on as incompatible with the versions where the problems occur. If it's not a big deal, it's fine for it to be public while the new version is submitted and reviewed.

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