Closed Bug 479502 Opened 16 years ago Closed 16 years ago

imgLoader::RemoveFromCache accesses freed memory calling entry->SetEvicted()

Categories

(Core :: Graphics: ImageLib, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dbaron, Assigned: joe)

References

Details

(Keywords: crash, fixed1.9.1, valgrind)

Attachments

(2 files, 1 obsolete file)

Attached file valgrind warnings (deleted) —
I was just doing a valgrind run on the testcase in bug 399994: I started up firefox with attachment 290918 [details] on the command line and then did File -> Print Preview with the mouse. This warning had showed up either while going into print preview or before; what I was looking for in that bug was what happened when leaving print preview. While doing this valgrind run, I got the attached valgrind warning, which I think is pretty self-explanatory. imgLoader::RemoveFromCache calls entry->SetEvicted() on an entry that was freed inside the call three lines above.
Well, maybe it's not completely self-explanatory since one would expect the caller of NotifyExpired to be holding a reference to the object it's passing. I'm not sure if that's actually the case, though.
The expiration tracker doesn't hold on to references to imgCacheEntries, so when an image is expiring, it'll actually be deleted if we don't have an explicit ref to it. This patch adds that ref.
Assignee: nobody → joe
Attachment #363386 - Flags: review?(vladimir)
This should block, because it's a problem that's been in since the new image cache, and it's made much worse since bug 466586 landed.
Flags: blocking1.9.1?
Accidentally mashed bits of two patches together. Here's the corrected patch.
Attachment #363386 - Attachment is obsolete: true
Attachment #363389 - Flags: review?(vladimir)
Attachment #363386 - Flags: review?(vladimir)
Keywords: valgrind
Could this bug explain the following Firefox 3.1b3pre topcrashes? * #15 - nsExpirationTracker<imgCacheEntry, 3>::RemoveObject(imgCacheEntry*) * #50 - imgLoader::RemoveFromCache(imgCacheEntry*)
Maybe #50, but #15 is unlikely. :(
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Status: NEW → RESOLVED
Closed: 16 years ago
Priority: P2 → --
Resolution: --- → FIXED
Blocks: 466586
You were either lucky or unlucky that you called SetEvicted after you dropped all your references. (It was unlucky because if you'd called SetEvicted earlier then you would still have a cache entry and so wouldn't crash; it was lucky because someone else might not realise and add some code that would then crash.)
Because there's a crash on 1.9.1 which _might_ be caused by this, and because it's clearly something that needed fixing regardless of the status of bug 466586 (that bug depends on this bug, not the other way around), I pushed this to 1.9.1. http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0be041c49b4b
Keywords: crash, fixed1.9.1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: