481753 - Another potential imgRequest-not-in-cache bug

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Joe Drew (not getting mail)]

After bug 481553 didn't solve the crash in mozilla-central, I went back to the source code with the ideas I found in that bug, and found another. This is a race condition. 

1. Load an image normally.
2. Browsing happens, and there are no more proxies for this image cache entry. The image cache entry starts being tracked for removal.
3. The image is re-requested, and gets set up with its imgCacheEntry again, but *does not get removed from the time-based expiry notice.* Imagelib needs to go out to the network to validate the cache entry.
4. Enough time passes, and the image cache entry is removed from the cache by the nsExpiryTracker.
5. The validation results finally come back, and the image needs to be re-loaded. The old image gets removed from the cache -- but wait! it's already removed!

Step 5 is a bit iffy, since the only stacks I've ever seen for bug 480352 involve Cancel(). I'd expect a mix between Cancel() and the explicit RemoveFromCache in imgCacheValidator, unless it's really unusual for images to need to be re-loaded when they're being validated.

I have no idea how to test this in a repeatable way.

Comment 1

[Joe Drew (not getting mail)]

Attachment: Make sure imgRequest always knows when it's in the cache

This patch takes the knowledge about cache status bug 481553 taught imgRequest and extends it to a complete set: when the imgRequest is present in the cache, mIsInCache is true, and when it isn't, it's false. If this bug occurs 'in the wild,' this should fix it.

Comment 2

[Joe Drew (not getting mail)]

Attachment: Make not leak when Cancel() is called

The previous patch wouldn't clear the hard reference to mCacheEntry if it thought it wasn't in the cache. This turns out to be a bad idea, because hard references to cache entries would be held around if errors occurred or the request was canceled partway through.

Comment 3

[Joe Drew (not getting mail)]

Attachment: Fix leak when we failed to create channel

Though I was never able to reproduce the leak, through code inspection I found that it was possible for cache entries to be added to the expiry queue without being added to the cache. This means they'd only be freed on exit() time, not at XPCOM shutdown time.

This is clearly incorrect - the expiry queue should be a subset of the cache's hash table - so now PutIntoCache() handles putting into the queue. This works because imgCacheEntry::mHasNoProxies is true for new cache entries.

Somewhat unfortunately, the best way to implement this fix is to include the fix for bug 479328 in it. Thus, I'm going to ask Boris for sr, since I'm asking him for sr in bug 479328.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 367218 [details] [diff] [review]
Fix leak when we failed to create channel

Minor style nit: in SetHasNoProxies, put back the early PR_FALSE return?  Unless you explicitly meant to write it that way.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Joe, could you please post an interdiff against the fix for bug 479328 instead of a patch that includes that one?  That would be a lot simpler to review...

Comment 6

[Joe Drew (not getting mail)]

Attachment: Interdiff from bug 479328

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 367218 [details] [diff] [review]
Fix leak when we failed to create channel

Add assertions in the cases when mIsInCache but !mKeyURI or !mCacheEntry, as discussed on irc, and remove the runtests.py change.  With that, looks good.

Comment 8

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/d3c2bf145fb8

I wasn't able to add the assertions because mCacheEntry can be null if we have no observers, which is entirely separate from being in the cache.

I am currently crossing every part of myself to ensure this fixes the topcrash.

Comment 9

[Joe Drew (not getting mail)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3bdd315ea739