Bug 239113

Summary: REGRESSION(r249162): CanvasRenderingContext2DBase::drawImage() crashes if the image is animated and the first frame cannot be decoded
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: ImagesAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, changseok, esprehn+autocc, ews-watchlist, gyuyoung.kim, heycam, sabouhallawa, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=74779
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Said Abou-Hallawa 2022-04-12 00:38:13 PDT 
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.
Comment 1 Said Abou-Hallawa 2022-04-12 00:38:42 PDT 
rdar://87980543
Comment 2 Said Abou-Hallawa 2022-04-12 02:33:30 PDT 
Created attachment 457318 [details]
Patch
Comment 3 Said Abou-Hallawa 2022-04-12 12:04:43 PDT 
Created attachment 457350 [details]
Patch
Comment 4 Simon Fraser (smfr) 2022-04-12 14:12:52 PDT 
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)?
Comment 5 Said Abou-Hallawa 2022-04-25 13:53:45 PDT 
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.
Comment 6 Said Abou-Hallawa 2022-04-25 18:05:11 PDT 
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.
Comment 7 Said Abou-Hallawa 2022-04-25 18:35:50 PDT 
Created attachment 458320 [details]
Patch
Comment 8 Said Abou-Hallawa 2022-05-11 21:11:05 PDT 
Created attachment 459207 [details]
Patch
Comment 9 Said Abou-Hallawa 2022-05-12 00:40:06 PDT 
Created attachment 459208 [details]
Patch
Comment 10 EWS 2022-05-16 16:46:10 PDT 
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].