Closed Bug 468160 Opened 12 years ago Closed 12 years ago

Leak imgRequest with <object> whose data attribute redirects to an image

Categories

(Core :: ImageLib, defect)

x86
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9.1b3

People

(Reporter: jruderman, Assigned: joe)

References

(Blocks 1 open bug)

Details

(Keywords: memory-leak, testcase, verified1.9.1)

Attachments

(3 files, 3 obsolete files)

Attached file testcase
Leaks the following:

gfxImageFrame         
imgCacheEntry         
imgContainer          
imgRequest            
nsBaseAppShell        
nsBaseURLParser       
nsPrincipal           
nsProperties          
nsRect                
nsRunnable            
nsStandardURL         
nsStringBuffer        
nsSupportsCStringImpl 
nsTArray_base         
nsThebesImage         
nsThread              
nsTimerImpl           
nsVoidArray           
nsWeakReference
Hmm.  Leaking the imgCacheEntry is not a good sign.  Joe, would that leak the imgRequest?

Unfortunately, refcount logging on mac seems to be lying to me (or at least not finding the actual leaked objects).  :(

I do wonder why the redirect matters; it doesn't really affect what nsObjectLoadingContent does....
Yeah, leaking the imgCacheEntry would leak the imgRequest. If the imgCacheEntry is leaked, it'd account for all those classes leaking.

I wonder if the new redirect handling is behind this.
Note that in this case imagelib doesn't see the redirects happen.  It just gets a LoadImageWithChannel with the final "after all the redirects" channel.
OK, I poked around some more.  It looks like the reason refcount logging was lying is that the objects _are_ released.... but after XPCOM shutdown.  In particular, the bottom of that stack is:

nsRefPtrHashtable<nsCStringHashKey, imgCacheEntry>::~nsRefPtrHashtable() (/usr/include/libkern/i386/_OSByteOrder.h:)
__tcf_0 (/usr/include/libkern/i386/_OSByteOrder.h:)
__cxa_finalize (/usr/lib/libSystem.B.dylib)
exit (/usr/lib/libSystem.B.dylib)

That looks like the static image cache hashtable having its destructor called.  So at that point the image cache is nonempty.

We could probably clear the hashtables in ~imgLoader, right?

Or are they expected to be empty by then (in which case we should assert and find why they're not empty).
All the image caches are explicitly cleared in imgLoader::Shutdown(), which is called from XPCOM shutdown. So entries lying around after that would definitely be a weird case.
Hmm.  So LoadImageWithChannel puts into the cache using the channel's GetURI, but inits the imgRequest with the channel's GetOriginalURI.  In the redirect case, those would be different.

When we go to evict from Shutdown, we use the imgIRequest URI for the eviction, right?

I suspect that inconsistency is the problem.

One immediate solution would be to just clear the hashtables in addition (or instead of) to evicting as we do now.  But we should still fix the LoadImageWithChannel caching stuff.
To fix this inconsistency, I'm going to have to add a field to imgRequest, mKeyURI. This will also come up with the patch to bug 89419; for that reason, I'll base a patch to fix this bug on the patch in that bug.
This patch fixes the leak.
Assignee: nobody → joe
Attachment #352248 - Flags: superreview?(vladimir)
Attachment #352248 - Flags: review?(bzbarsky)
Comment on attachment 352248 [details] [diff] [review]
Keep track of the keyed URI, and use it where necessary

>diff --git a/modules/libpr0n/src/imgLoader.cpp b/modules/libpr0n/src/imgLoader.cpp

>   nsCOMPtr<nsIURI> uri;
>   newChannel->GetURI(getter_AddRefs(uri));
>+  newChannel->GetURI(getter_AddRefs(mKeyURI));
> 
>   // If we don't still have a cache entry, we don't want to refresh the cache.
>   if (uri && mCacheEntry)
>     imgLoader::PutIntoCache(uri, mCacheEntry);

This code is kind of ugly, but it's going to change a little bit once I put in the GetOriginalURI asked of me in bug 89419. So just pretend that it's cleaned up.
Component: Networking → ImageLib
QA Contact: networking → imagelib
Attachment #352248 - Flags: superreview?(vladimir) → superreview+
Comment on attachment 352248 [details] [diff] [review]
Keep track of the keyed URI, and use it where necessary

Looks ok to me code-wise.
Comment on attachment 352248 [details] [diff] [review]
Keep track of the keyed URI, and use it where necessary

Looks good.  Fixes eviction of redirected loads that don't go through LoadImageWithChannel too, right?
Attachment #352248 - Flags: review?(bzbarsky) → review+
Right. All eviction should work properly, because we're keeping around a copy of the URL we're keying on.
Attachment #352248 - Flags: approval1.9.1?
Pushed to mozilla-central in http://hg.mozilla.org/mozilla-central/rev/dd314af60eda
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
I backed out this and the changes for bug 414259 due to leaks on the tinderbox.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Oh, speaking of which, this bug's testcase needs to be added to our leak tests.
This is a mochitest for this bug. It doesn't directly test for leaks, but the Mochitest framework does test for leaks, so it will do the job.
Attached patch unbitrotted patch (obsolete) — Splinter Review
Attached patch real unbitrotted patch (obsolete) — Splinter Review
This is the patch that I'm going to apply.
Attachment #352248 - Attachment is obsolete: true
Attachment #354359 - Attachment is obsolete: true
Attachment #352248 - Flags: approval1.9.1?
Whoops - forgot to qrefresh.
Attachment #354360 - Attachment is obsolete: true
Duplicate of this bug: 469564
This should block 1.9.1, since it is a duplicate of bug 469564, and that bug blocked 1.9.1.

This bug is fixed with the following pushes to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/967ae997cf22 and http://hg.mozilla.org/mozilla-central/rev/8dce9cf66103

If it is approved for 1.9.1, I will push there once this has baked on m-c for a while.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Flags: blocking1.9.1?
Resolution: --- → FIXED
Duplicate of this bug: 460652
Flags: blocking1.9.1? → blocking1.9.1+
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5c25cbd03bf2 was a quick follow-up, since I forgot that sdwilsh's quick way of porting between branches doesn't do git-style diffs.
V.Fixed, as leak decreased on Tinderboxes:
http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox&maxdate=1230087245
{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1230075915.1230080515.22639.gz&fulltext=1
Linux mozilla-central unit test on 2008/12/23 15:45:15
TEST-PASS | runtests-leaks | WARNING leaked 73718 bytes during test execution (threshold set at 75000 bytes)

...
TEST-PASS | runtests-leaks | leaked 2 instances of gfxImageFrame with size 56 bytes each (112 bytes total)
TEST-PASS | runtests-leaks | leaked 3 instances of imgCacheEntry with size 32 bytes each (96 bytes total)
TEST-PASS | runtests-leaks | leaked 3 instances of imgContainer with size 84 bytes each (252 bytes total)
TEST-PASS | runtests-leaks | leaked 3 instances of imgRequest with size 156 bytes each (468 bytes total)
...
etc
...

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1230082999.1230087037.7261.gz&fulltext=1
Linux mozilla-central unit test on 2008/12/23 17:43:19
TEST-PASS | runtests-leaks | WARNING leaked 71574 bytes during test execution (threshold set at 75000 bytes)
}
Fix timeframe:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=82fd9752b6e1&tochange=967ae997cf22
Blocks: mlkTests
Status: RESOLVED → VERIFIED
Target Milestone: --- → mozilla1.9.1b3
(In reply to comment #26)
> Linux mozilla-central unit test on 2008/12/23 17:43:19
> TEST-PASS | runtests-leaks | WARNING leaked 71574 bytes during test execution
> (threshold set at 75000 bytes)

Ftr, MacOSX and Windows boxes were not affected:
[I don't know if that was expected...]

TEST-PASS | runtests-leaks | WARNING leaked 71254 bytes during test execution (threshold set at 75000 bytes)
TEST-PASS | runtests-leaks | WARNING leaked 71950 bytes during test execution (threshold set at 75000 bytes)

Anyway, Linux seems to have now the "same" leak(s) as the other two platforms :-)
You need to log in before you can comment on or make changes to this bug.