Bug 239113 – REGRESSION(r249162): CanvasRenderingContext2DBase::drawImage() crashes if the image is animated and the first frame cannot be decoded

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Said Abou-Hallawa]

Some pages may draw the BitmapImage before loading the encoded data is complete. This case is already handled by BitmapImage::draw() which returns peacefully if the "current frame" cannot be decoded. 

To draw an animated BitmapImage to a canvas, r249162 changed the old behavior to draw the first frame always. To do that, this revision creates another temporary static BitmapImage and draws this temporary BitmapImage instead of drawing the original image. This temporary static BitmapImage is created from the first frame of the original image.

The problem is CanvasRenderingContext2DBase::drawImage() does not check whether the first frame is decoded with a valid NativeImage or not before creating the temporary static BitmapImage. If the NativeImage is null, BitmapImage will create an ImageSource with a null NativeImage. But because ImageSource does not take into account the case of the null NativeImage, a segmentation fault crash will happen once the members of the null NativeImage is accessed.

[Said Abou-Hallawa]

rdar://87980543

[Said Abou-Hallawa]

Created attachment 457318 [details]
Patch

[Said Abou-Hallawa]

Created attachment 457350 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 457350 [details]
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)?

[Said Abou-Hallawa]

Comment on attachment 457350 [details]
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.

[Said Abou-Hallawa]

Comment on attachment 457350 [details]
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.

I tried to write a test case with multiple large animated images and I was hoping it will crash on macOS or on iOS but it did not. It looks like my theory about ImageIO being thread unsafe when decoding frames from different threads is incorrect.

[Said Abou-Hallawa]

Created attachment 458320 [details]
Patch

[Said Abou-Hallawa]

Created attachment 459207 [details]
Patch

[Said Abou-Hallawa]

Created attachment 459208 [details]
Patch

[EWS]

Committed r294280 (250624@main): <https://commits.webkit.org/250624@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 459208 [details].