Jesse Ruderman Reporter
	Description • 16 years ago

Leaks the following:

gfxImageFrame         
imgCacheEntry         
imgContainer          
imgRequest            
nsBaseAppShell        
nsBaseURLParser       
nsPrincipal           
nsProperties          
nsRect                
nsRunnable            
nsStandardURL         
nsStringBuffer        
nsSupportsCStringImpl 
nsTArray_base         
nsThebesImage         
nsThread              
nsTimerImpl           
nsVoidArray           
nsWeakReference

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 16 years ago

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....

	Joe Drew (not getting mail) Assignee
	Comment 2 • 16 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 16 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 16 years ago

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).

	Joe Drew (not getting mail) Assignee
	Comment 5 • 16 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 16 years ago

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.

	Joe Drew (not getting mail) Assignee
	Comment 7 • 16 years ago

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.

	Joe Drew (not getting mail) Assignee
	Comment 8 • 16 years ago

This patch fixes the leak.

	Joe Drew (not getting mail) Assignee
	Comment 9 • 16 years ago

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.

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 10 • 16 years ago

Comment on attachment 352248 [details] [diff] [review]
Keep track of the keyed URI, and use it where necessary

Looks ok to me code-wise.

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 16 years ago

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?

	Joe Drew (not getting mail) Assignee
	Comment 12 • 16 years ago

Right. All eviction should work properly, because we're keeping around a copy of the URL we're keying on.

	Joe Drew (not getting mail) Assignee
	Comment 13 • 16 years ago

Pushed to mozilla-central in http://hg.mozilla.org/mozilla-central/rev/dd314af60eda

	Robert Sayre
	Comment 14 • 16 years ago

I backed out this and the changes for bug 414259 due to leaks on the tinderbox.

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 16 years ago

Oh, speaking of which, this bug's testcase needs to be added to our leak tests.

	Joe Drew (not getting mail) Assignee
	Comment 16 • 16 years ago

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.

	Joe Drew (not getting mail) Assignee
	Comment 18 • 16 years ago

This is the patch that I'm going to apply.

	Joe Drew (not getting mail) Assignee
	Comment 19 • 16 years ago

Whoops - forgot to qrefresh.

	Joe Drew (not getting mail) Assignee
	Comment 21 • 16 years ago

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.

	Jeff Walden [:Waldo]
	Comment 23 • 16 years ago

http://hg.mozilla.org/mozilla-central/rev/8dce9cf66103

	Joe Drew (not getting mail) Assignee
	Comment 24 • 16 years ago

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6f65a925a8c3
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0c5c78519dae

	Joe Drew (not getting mail) Assignee
	Comment 25 • 16 years ago

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.

	Serge Gautherie (:sgautherie)
	Comment 26 • 16 years ago

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

	Serge Gautherie (:sgautherie)
	Comment 27 • 16 years ago

(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 :-)

	Serge Gautherie (:sgautherie)
	Comment 28 • 16 years ago

verified1.9.1, per bug 460548 comment 18.
See http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox3.1&maxdate=1231207933