don't allow discarding of animated images

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: tnikkel, Assigned: tnikkel)

Tracking

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
Created attachment 8846263 [details]
testcase.html

Plot twist! It is currently possible (and has been for quite a while) to discard animated images. All we need is the follow sequence of events.

1. Decode an animated image.
2. Move the animated image out of view (so it is not painted).
3. Call canvas.drawImage on the animated image (or anything else that asks for a first frame only decode). This creates a static entry in the surface cache for this first frame in addition to the animated entry. Because it is a static request we will also start a first frame decode. RasterImage::Decode calls SurfaceCache::UnlockEntries

https://dxr.mozilla.org/mozilla-central/rev/4ceb9062ea8f4113bfd1b3536ace4a840a72faa7/image/RasterImage.cpp#1166

and bam, the animated frames are now unlocked (even though the RasterImage, and it's entry in the surface cache is still locked.
4. Switch tabs, open about:memory and minimize memory to actual throw away the animated frames.
5. Switch back to the image tab, scroll the image back into view, it will not animate, it will just show the last composited frame forever.

The attached testcase reproduces this, scroll down, click the button, then change tabs and minimize memory.

This is probably responsible for some (most?) of the crashes from bug 1120279.
(Assignee)

Comment 1

2 years ago
Created attachment 8846264 [details] [diff] [review]
patch
Attachment #8846264 - Flags: review?(aosmond)
Comment on attachment 8846264 [details] [diff] [review]
patch

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

Nice catch!
Attachment #8846264 - Flags: review?(aosmond) → review+

Comment 3

2 years ago
Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/778683798a9a
Don't allow the surface cache to unlock the animated frames of an animated image (when discarding of animated images is disabled). r=aosmond

Comment 4

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/778683798a9a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.