Fix a crash caused by AVIF decoding failure

• Component: Images
• Status: RESOLVED FIXED

[ChangSeok Oh]

https://gif2avif.com/browser-test/ crashes where AVIF support is enabled.

#0  0x00007fa33d834cf0 in WebCore::AVIFImageReader::imageCount() const ()
   from target:/app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#1  0x00007fa33d834441 in WebCore::AVIFImageDecoder::tryDecodeSize(bool) ()
   from target:/app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#2  0x00007fa33d813ef3 in WebCore::ScalableImageDecoder::setData(WebCore::SharedBuffer&, bool) ()
   from target:/app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#3  0x00007fa33cfa765c in WebCore::ImageSource::dataChanged(WebCore::SharedBuffer*, bool) ()
   from target:/app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#4  0x00007fa33cd07363 in WebCore::CachedImage::updateImageData(bool) ()
   from target:/app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#5  0x00007fa33cd0d0e0 in WebCore::CachedImage::updateBufferInternal(WebCore::SharedBuffer&) ()
   from target:/app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#6  0x00007fa33cd0d332 in WebCore::CachedImage::updateBuffer(WebCore::SharedBuffer&) ()
   from target:/app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#7  0x00007fa33c9dfcad in WebCore::ImageDocumentParser::appendBytes(WebCore::DocumentWriter&, char const*, unsigned long) () from target:/app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#8  0x00007fa33b550696 in WebKit::WebFrameLoaderClient::committedLoad(WebCore::DocumentLoader*, char const*, int) () from target:/app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#9  0x00007fa33cc527aa in WebCore::DocumentLoader::commitLoad(char const*, int) ()
   from target:/app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#10 0x00007fa33cd1b6d8 in WebCore::CachedRawResource::notifyClientsDataWasReceived(char const*, unsigned int) [clone .part.0] () from target:/app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#11 0x00007fa33cd1bd5a in WebCore::CachedRawResource::updateBuffer(WebCore::SharedBuffer&) [clone .part.0] () from target:/app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#12 0x00007fa33ccd6c22 in WebCore::SubresourceLoader::didReceiveDataOrBuffer(char const*, int, WTF::RefPtr<WebCore::SharedBuffer, WTF::RawPtrTraits<WebCore::SharedBuffer>, WTF::DefaultRefDerefTraits<WebCore::Shar--Type <RET> for more, q to quit, c to continue without paging--
edBuffer> >&&, long long, WebCore::DataPayloadType) ()
   from target:/app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#13 0x00007fa33ccd6dc5 in WebCore::SubresourceLoader::didReceiveData(char const*, unsigned int, long long, WebCore::DataPayloadType) () from target:/app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
Backtrace stopped: Cannot access memory at address 0x7fff96d4b698

[ChangSeok Oh]

Created attachment 424467 [details]
Patch

[Philippe Normand]

Comment on attachment 424467 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        No new tests since no functionality changed.

I think a test ensuring we no longer crash when loading a specific file could still make sense. There's a few already for the video-as-img-tag scenario.

[ChangSeok Oh]

Comment on attachment 424467 [details]
Patch

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

>> Source/WebCore/ChangeLog:14
>> +        No new tests since no functionality changed.
> 
> I think a test ensuring we no longer crash when loading a specific file could still make sense. There's a few already for the video-as-img-tag scenario.

O.K. Let me look into if we can create a reliable test.

[ChangSeok Oh]

The minimal test case that reproduces the crash is
<iframe src='https://gif2avif.com/sample.avif'></iframe>

However, the crash does not happen when the sample.avif is loaded from the local.
<iframe src='sample.avif'></iframe>

Is there a way to mimic a network delay in webkit test infrastructure?

[ChangSeok Oh]

I found bug 194506.
There was testRunner.setDefersLoading but it isn't for wk2 anymore.

[ChangSeok Oh]

I could make a simplified test here. https://shivamidow.github.io/avif-crash.html
But I don't know how to mimic it in the webkit test environment.
It is hard to reproduce the crash because there is no way to reliably simulate the necessary network delay of the avif image transfer.

[Philippe Normand]

One option could be to throttle the HTTP request? There´s a CGI script for videos: LayoutTests/http/tests/media/video-throttled-load.cgi

Maybe a similar approach could be used?

[Radar WebKit Bug Importer]

<rdar://problem/76205084>

[Said Abou-Hallawa]

You may use LayoutTests/http/tests/images/resources/load-and-stall.php. It is used to mimic the network delay in many http layout tests.

[ChangSeok Oh]

(In reply to Said Abou-Hallawa from comment #9)
> You may use LayoutTests/http/tests/images/resources/load-and-stall.php. It
> is used to mimic the network delay in many http layout tests.

@Philippe and  @Said, Thanks. Let me give a shot.

[ChangSeok Oh]

Created attachment 426989 [details]
Patch

[ChangSeok Oh]

Created attachment 426990 [details]
Patch

[Philippe Normand]

Comment on attachment 426990 [details]
Patch

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

> LayoutTests/TestExpectations:125
>  fast/images/avif-image-decoding.html [ Skip ]
>  fast/images/avif-as-image.html [ Skip ]
>  fast/images/animated-avif.html [ Skip ]
> +http/tests/images/avif-partial-load-crash.html [ Skip ]

Oh, I realize now I forgot to unskip these in the glib TestExpections! They should pass now that the avif decoder is enabled on the bots. Can you check it please? Either in this patch or a new one :)

[ChangSeok Oh]

Comment on attachment 426990 [details]
Patch

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

Thanks!

>> LayoutTests/TestExpectations:125
>> +http/tests/images/avif-partial-load-crash.html [ Skip ]
> 
> Oh, I realize now I forgot to unskip these in the glib TestExpections! They should pass now that the avif decoder is enabled on the bots. Can you check it please? Either in this patch or a new one :)

I will upload a separate patch to handle this.

[EWS]

Committed r276578 (237014@main): <https://commits.webkit.org/237014@main>

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