Closed Bug 1298184 Opened 8 years ago Closed 2 years ago

Clear the ImageLib surface cache, not the image cache, when a device reset occurs

Categories

(Core :: Graphics: ImageLib, defect)

defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: seth, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

gfxWindowsPlatform::HandleDeviceReset() may be called in a variety of situations where all surfaces may have become invalid (for example, due to a graphics driver cache) or where our graphics backend may have changed. In these situations, the surfaces we've decoded images into need to be regenerated.

Current, we clear the image caches in this situation, but this does not achieve the goal we're trying to achieve, because the frame tree holds on to image objects directly. That means that for visible pages, clearing the image caches won't cause the images to be redecoded. And we pay a heavy price for this, because we're forced to load formerly cached images again *from the network*.

Clearing ImageLib's surface cache is the right thing to do here. The surface cache stores, well, surfaces - the decoded versions of images. We can clear all of the surfaces without getting rid of the image data itself. This will work even for visible pages, automatically triggering a redecode when we try to draw the images again, and it won't force us to reload any data from the network.

A nice side effect of making this change is that this lets us check the content backend on the main thread, before we begin decoding an image, safe in the knowledge that if there's a device reset, the resulting surface will get evicted from the surface cache and we'll never use it. This enables us to further off-main-thread all the things.
In order for this to work, DiscardAll() has to discard even locked surfaces.

When the surface cache code was originally written, we discarded the original
image data for some images (for example, images loaded from chrome:// URIs), so
we couldn't rematerialize their surfaces. That's why there are references in the
documentation to this situation, and why DiscardAll() doesn't discard surfaces
for locked images.

For various reasons, including downscale-during-decode support, we stopped
discarding the original image data. All images now retain their image data, no
matter what. That means that there are no cases where we can't rematerialize an
image's surfaces.

Given that change in behavior, it's reasonable to discard even locked surfaces
in DiscardAll(). And we need that behavior to correctly recover from a device
reset, since even locked surfaces are affected by a driver crash or backend
change. (Indeed, this issue shows that we really couldn't have supported
non-rematerializable surfaces in the first place.)

This change also fixes an issue with the existing caller of
SurfaceCacheUtils::DiscardAll(), which intends to discard all SVG-as-image
surfaces when a theme change occurs. All such surfaces used to be unlocked, but
at some point in the past we started to support locking them, which broke the
theme change code. After this patch, it'll work correctly again.
Attachment #8785004 - Flags: review?(dholbert)
This part switches us from clear the image cache (technically image *caches*, I
suppose) to clearing the surface cache when we handle a device reset.
Attachment #8785005 - Flags: review?(dvander)
Whoops, a hunk from part 1 ended up in part 2. Reuploading.
Attachment #8785012 - Flags: review?(dholbert)
Attachment #8785004 - Attachment is obsolete: true
Attachment #8785005 - Attachment is obsolete: true
Attachment #8785004 - Flags: review?(dholbert)
Attachment #8785005 - Flags: review?(dvander)
Just to ease my uneasiness, what's the worst that can happen if we purge cache entries for an image that's locked, supposing we get a random DiscardAll() call?  We'll just have to redecode a bunch of images the next time we paint them, right?

(Whoever is keeping a cache-entry locked presumably shouldn't be holding raw pointers to its surfaces or anything like that, right?  And if they are, they've got owning references, I epxect... so it's unlikely that there are any use-after-free bugs that'd be caused by disregarding cache entries' locked status here, right?)
Comment on attachment 8785012 [details] [diff] [review]
(Part 1) - Make SurfaceCache::DiscardAll() discard even locked surfaces.

Review of attachment 8785012 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, assuming this is pretty tame operation, per comment 6
Attachment #8785012 - Flags: review?(dholbert) → review+
Attachment #8785013 - Flags: review?(dvander) → review+
(In reply to Daniel Holbert [:dholbert] from comment #6)
> Just to ease my uneasiness, what's the worst that can happen if we purge
> cache entries for an image that's locked, supposing we get a random
> DiscardAll() call?  We'll just have to redecode a bunch of images the next
> time we paint them, right?

Yes, we'll just notice that the surfaces aren't there in RasterImage::LookupFrame() and we'll trigger a redecode. It's not a big deal.

*However*: I realized after posting these patches that this will trigger an assert in debug builds for animated images, because we assert that animated images never need to be redecoded. We already detect and handle some analogous cases, so I could just post a third part to handle this in RasterImage, but I think a better idea is to just wait until I land a patch that's already in my patch queue that removes that assert. So let's not land this right away. I'll try to get that patch uploaded so I can make the bug block this one.

> (Whoever is keeping a cache-entry locked presumably shouldn't be holding raw
> pointers to its surfaces or anything like that, right?  And if they are,
> they've got owning references, I epxect... so it's unlikely that there are
> any use-after-free bugs that'd be caused by disregarding cache entries'
> locked status here, right?)

Yeah, that's definitely true. Use-after-free is not a problem here.
Sounds good. Thanks!
Blocks: 1298544

The bug assignee didn't login in Bugzilla in the last 7 months.
:aosmond, could you have a look please?
For more information, please visit auto_nag documentation.

Assignee: seth.bugzilla → nobody
Flags: needinfo?(aosmond)
Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(aosmond)
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: