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

RESOLVED FIXED

Status

()

Core
ImageLib
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: dbaron, Assigned: Joe Drew (not getting mail))

Tracking

({crash, fixed1.9.1, valgrind})

Trunk
crash, fixed1.9.1, valgrind
Points:
---
Bug Flags:
blocking1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
Created attachment 363360 [details]
valgrind warnings

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

9 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

9 years ago
Created attachment 363386 [details] [diff] [review]
Hold on to a reference to the entry when NotifyExpired is called

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

9 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

9 years ago
Created attachment 363389 [details] [diff] [review]
 Hold on to a reference to the entry when NotifyExpired is called (fixed)

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+

Updated

9 years ago
Keywords: valgrind

Comment 5

9 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

9 years ago
Maybe #50, but #15 is unlikely. :(
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
(Assignee)

Comment 7

9 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
Last Resolved: 9 years ago
Priority: P2 → --
Resolution: --- → FIXED

Updated

9 years ago
Blocks: 466586

Comment 8

9 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

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