Closed Bug 691102 Opened 13 years ago Closed 12 years ago

zombie compartments if Image Zoom in use

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: aryx, Unassigned)

References

(
URL
)

Details

(Whiteboard: [MemShrink:P3]) 

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.
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....
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.
CCing the Image Zoom developer.
Great analysis, bz.  I'll pre-triage this as a P3 because that's what we do for memory leaks in add-ons.
Whiteboard: [MemShrink:P3]
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?
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.
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?
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?
> 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.  :(
How do I test? Any references to articles or tools would be appreciated
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.  :(
OK reproduced the problem, thanks for reporting
(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 :-(
(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.
(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.
(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...
(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.
(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?
(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.
The add-on has been downgraded to preliminary approval pending the memory leak fix.
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.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
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
The leak in this addon was probably fixed by Bug 695480 if nothing else ...
Since I can't reproduce this bug anymore, I'm moving the add-on to Fully Reviewed again.
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.
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.
You need to log in before you can comment on or make changes to this bug.