[webkit-reviews] review canceled: [Bug 239113] REGRESSION(r249162): CanvasRenderingContext2DBase::drawImage() crashes if the image is animated and the first frame cannot be decoded : [Attachment 457350] Patch

[bugzilla-daemon at webkit.org]

Said Abou-Hallawa <sabouhallawa at apple.com> has canceled  review:
Bug 239113: REGRESSION(r249162): CanvasRenderingContext2DBase::drawImage()
crashes if the image is animated and the first frame cannot be decoded
https://bugs.webkit.org/show_bug.cgi?id=239113

Attachment 457350: Patch

https://bugs.webkit.org/attachment.cgi?id=457350&action=review




--- Comment #5 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
Comment on attachment 457350
  --> https://bugs.webkit.org/attachment.cgi?id=457350
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457350&action=review

>> Source/WebCore/ChangeLog:11
>> +	    static image. If the first frame can't be decoded, it should return
> 
> Do we know when this happens (when the first frame can't be decoded)?

I think I misunderstood this bug. The null check will prevent the crash but it
will leave a racy behavior while decoding the frames of the animated image.

The problem is the animated image is decoded asynchronously. But drawing the
animated image to the canvas requires decoding the first frame. So we decode
the first frame synchronously. But ImageIO is not thread safe with regarding to
the frame decoding. Also this will mess all ImageIO calculations since each
frame is composed by adding a delta to the previous frame. ImageIO has to
decode the frames 0, 1, ... n-1 to be able to decode frame n. So if we want to
decode frame zero for canvas drawing while the current frame is n, ImageIO will
delete all the frames and decode frame zero. Then it has to decode all the
other frames from frame 1 till frame n-1 again before returning frame n.

So it looks like we need to cache frame zero always and never delete it even
under memory pressure. We use this rule with the current frame and we just need
to extend to the frame zero as well.