Closed Bug 237153 Opened 20 years ago Closed 20 years ago
Image gallery memory leak
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.
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.
(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 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 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.
Assignee: general → bzbarsky
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #143631 - Flags: approval1.7b? → approval1.7b+
Comment on attachment 143631 [details] [diff] [review] Proposed patch email@example.com for 1.7b. /be
Patch checked in for 1.7b. Cesar, could you possibly try tomorrow's nightly and verify the bug?
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.7beta
Still leaking for me on 2004031208, will try again tomorrow.
Still happens for me on 2004031308, reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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+
OS: Linux → All
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).
Attachment #143631 - Attachment is obsolete: true
Comment on attachment 143828 [details] [diff] [review] Fix jst, can you do one more easy review here?
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.
Attachment #143828 - Flags: approval1.7b? → approval1.7b+
Checked in. Cesar, would you test again with tomorrow's builds? Thank you for the help!
Status: REOPENED → RESOLVED
Closed: 20 years ago → 20 years ago
Resolution: --- → FIXED
Verified 2004031508 Closing the tabs free the memory both in X and mozilla-bin
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.