Closed
Bug 479502
Opened 16 years ago
Closed 16 years ago
imgLoader::RemoveFromCache accesses freed memory calling entry->SetEvicted()
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
People
(Reporter: dbaron, Assigned: joe)
References
Details
(Keywords: crash, fixed1.9.1, valgrind)
Attachments
(2 files, 1 obsolete file)
(deleted),
text/plain; charset=UTF-8
|
Details | |
(deleted),
patch
|
vlad
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•16 years ago
|
||
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.
Assignee | ||
Comment 2•16 years ago
|
||
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)
Assignee | ||
Comment 3•16 years ago
|
||
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?
Assignee | ||
Comment 4•16 years ago
|
||
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)
Attachment #363389 -
Flags: review?(vladimir) → review+
Comment 5•16 years ago
|
||
Could this bug explain the following Firefox 3.1b3pre topcrashes?
* #15 - nsExpirationTracker<imgCacheEntry, 3>::RemoveObject(imgCacheEntry*)
* #50 - imgLoader::RemoveFromCache(imgCacheEntry*)
Assignee | ||
Comment 6•16 years ago
|
||
Maybe #50, but #15 is unlikely. :(
Flags: blocking1.9.1? → blocking1.9.1+
Updated•16 years ago
|
Priority: -- → P2
Assignee | ||
Comment 7•16 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/0015ec27f938
This'll get pushed to 1.9.1 when bug 466586 does.
Status: NEW → RESOLVED
Closed: 16 years ago
Priority: P2 → --
Resolution: --- → FIXED
Comment 8•16 years ago
|
||
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.)
Assignee | ||
Comment 9•16 years ago
|
||
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.
Description
•