237153 - Image gallery memory leak

Status: VERIFIED FIXED

(SeaMonkey :: General, defect)

Description

[Cesar Eduardo Barros]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7a) Gecko/20040218
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7a) Gecko/20040218

This is an easy to reproduce memory leak (at least I can reproduce it at will)
which eats memory quickly. I don't know the exact conditions which can trigger
it, but I know at least that opening lots of images on tabs with scaling enabled
and closing them before they finish loading seems to leak as much memory as the
tab would use while open. I have not tested other variants (like letting them
finish loading, turning off scaling, using new windows, opening images within
html pages, using another image format, etc), so I cannot say where the leak is
(thus Browser-General).

Reproducible: Always
Steps to Reproduce:
This is what I actually tested to reproduce the problem. Some details might not
be really needed. YMMV.

You need a somewhat slow connection (so the images won't finish loading before
you can cancel them).

1.Enable "Resize large images to fit the broser window" and opening tabs on
middle click
2.Go to a jpeg image gallery with text links (no thumbnails)
3.Middle click the ten first images
4.Watch the images load, and when each image is about half loaded close the tab
5.Repeat steps 3-4 with another set of 10 images. Watch the memory usage rise
monotonically while you do it, both mozilla-bin's and the X server's. Repeat as
needed until it you feel it's enough. Be careful to not eat all the memory,
unless you have a "blame Mozilla" OOM killer module.
Actual Results:  
Before starting, X was using about 107M and mozilla-bin about 76M
After opening ~40 images, X was using about 303M and mozilla-bin was using about
228M
Closing Mozilla made X go back to about 106M
(values from the VIRT column in Debian's version of "top" in the testing
release, with a 2.6 kernel. YMMV)

Expected Results:  
After closing the tabs, the memory usage should go back to the original values
both in X and mozilla-bin, give or take a few megabytes due to mozilla's memory
cache.
It should stop growing at some point instead of eating enough memory to cause a
OOM (Out Of Memory).

I will soon try to reproduce again, this time letting all the images finish
loading, and post the results here.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So if letting the images load all the way fixes the leaks, I have a theory as to
what's going on...

Right now, we have a reference cycle between the image document and the image
node.  The document holds two refs to the node (one through mImageElement, one
through the node tree).  The node holds two refs to the document (one as
mDocument, one as an image observer).

Setting the document's script global object to null sets mImageElement to null
and sets the node's mDocument to null.  That still leaves a reference cycle,
though.  The image observer ref is currently removed in
ImageListener::OnStopRequest.  But if OnStopRequest fires after we have set the
script global object to null, there is no mImageElement and the image observer
ref cannot be removed...

So if waiting till the load is done does fix the leak, we need to remove the
image observer ref in SetScriptGlobalObject before setting mImageElement to null.

Comment 2

[Cesar Eduardo Barros]

(mid-air collision, retrying)

Tried again the exact same situation, but now letting all tabs finish loading
before closing the set of tabs. Mozilla's memory usage stayed constant (after
finishing each set of tabs, the memory usage was the same -- it is slightly
higher while loading). X's memory usage was about constant (lost in the normal
noise of other apps using it).

So, the bug happens when cancelling the image loading (by closing the tab before
it finishes). But it's still not enough for me to known which component it is (I
do not know enough about Moz's structure), so I can't set it to something better.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 143631 [details] [diff] [review]
Proposed patch

I forgot to add that the leaking cycle is between the image observer ref (node
to document) and the node tree ref (document to node), since the node tree is
not released until ~nsDocument.

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 143631 [details] [diff] [review]
Proposed patch

r+sr=jst

I think this is worth taking for 1.7b, it's a large leak (though not an all
that common one), and the fix seems as safe as they come.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 143631 [details] [diff] [review]
Proposed patch

a=[e-mail] for 1.7b.

/be

Comment 7

[Boris Zbarsky [:bzbarsky]]

Patch checked in for 1.7b.  Cesar, could you possibly try tomorrow's nightly and
verify the bug?

Comment 8

[Cesar Eduardo Barros]

Still leaking for me on 2004031208, will try again tomorrow.

Comment 9

[Cesar Eduardo Barros]

Still happens for me on 2004031308, reopening.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Ok, I've finally figured out a way to reproduce this bug (the NASA site loads
too fast over here, but I hacked together a script to serve an image slowly...).

Looking into it.

Comment 11

[Oleg Sidletskiy]

not limited to Linux, can reproduce with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7b) Gecko/20040312
Firefox/0.8.0+

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

All hail the refcount logger!

So it looks like OnStopRequest actually breaks _two_ cycles.  My first patch
fixed one of them.  The second one is that the document holds a ref to the
image node via the DOM tree.  The image node holds a ref to the
imgRequestProxy, which holds a ref to the necko nsIChannel (via imgRequest),
which holds a ref to the ImageListener (as its streamlistener), which holds a
ref to the document.  This last ref is removed in
nsMediaDocumentStreamListener::OnStopRequest, but that wasn't getting called in
cases when mImageElement was null (as it is when the page is closed before the
image finishes loading).

I've verified that with this patch image-related stuff no longer appears in the
list of leaks generated by XPCOM_MEM_LEAK_LOG (as it does without this patch).

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 143828 [details] [diff] [review]
Fix

jst, can you do one more easy review here?

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 143828 [details] [diff] [review]
Fix

r+sr=jst.

Since we took the previous one for 1.7b, we'd probably want this one too.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Checked in.  Cesar, would you test again with tomorrow's builds?  Thank you for
the help!

Comment 16

[Cesar Eduardo Barros]

Verified 2004031508

Closing the tabs free the memory both in X and mozilla-bin