Bug 171900 – REGRESSION (216471): Infinite repaint-drawing loop when asynchronously decoding incomplete image frames

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

[Said Abou-Hallawa]

The change <http://trac.webkit.org/changeset/216471> can cause infinite repaint-drawing loop when asynchronously decoding incomplete image frames. It has two flaws:

1. In BitmapImage::draw() we check frameIsCompleteAtIndex() and if it is false, we request decoding the image frame. When the image frame finishes decoding, the image element is repainted and hence the BitmapImage::draw is called back. We check again if frameIsCompleteAtIndex() and we find it false, so we request decoding the image frame although we may not received new data. This can cause an infinite repaint-draw loop. With small number of images, the loop can be broken if the main thread gets a chance to set new data in the ImageDecoder and eventually it answesr frameIsCompleteAtIndex() with true. But if the page has many large images, the main will be busy repainting and drawing the incomplete large images. And it may not have a chance to complete setting the images' data.

2. The decoding thread caches the ImageFrame metadata on the main thread in ImageFrameCache::cacheFrameMetadataAtIndex(). It calls ImageDecoder::frameIsCompleteAtIndex() to check whether this ImageFrame has a partial decoded ImageFrame or a complete one. This actually might return the wrong answer. If while decoding the image frame in the decoding thread or while waiting the callOnMainThread() to be dispatched, the rest of data are set in the ImageDecoder. The ImageDecoder::frameIsCompleteAtIndex() will return true although the decoded image frame is actually incomplete.

[Said Abou-Hallawa]

Created attachment 309565 [details]
Patch

[Said Abou-Hallawa]

I could not write an HTTP test for this patch. I could only reproduce the infinite loop using a page with many large images. The main thread was busy doing repaint-draw for the same set of incomplete images. With one image or two the infinite loop can't happen.

[Said Abou-Hallawa]

Created attachment 309614 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/32109397>

[Said Abou-Hallawa]

Created attachment 309721 [details]
Patch

[Build Bot]

Comment on attachment 309721 [details]
Patch

Attachment 309721 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3719194

New failing tests:
workers/bomb.html

[Build Bot]

Created attachment 309738 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 309721 [details]
Patch

Attachment 309721 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3719212

New failing tests:
http/tests/misc/slow-loading-animated-image.html

[Build Bot]

Created attachment 309745 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Said Abou-Hallawa]

Created attachment 309759 [details]
Patch

[Said Abou-Hallawa]

Created attachment 309761 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 309761 [details]
Patch

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

> Source/WebCore/platform/graphics/BitmapImage.h:210
> +    enum class ImageDecodingStatus { Invalid, Decoding, Ready };

Can we avoid having both ImageDecodingStatus and the ImageFrame::Decoding enum? They are so similar, and I really want ImageFrame::Decoding to be ImageFrame::DecodingStatus.

[Said Abou-Hallawa]

Created attachment 309831 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 309761 [details]
Patch

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

>> Source/WebCore/platform/graphics/BitmapImage.h:210
>> +    enum class ImageDecodingStatus { Invalid, Decoding, Ready };
> 
> Can we avoid having both ImageDecodingStatus and the ImageFrame::Decoding enum? They are so similar, and I really want ImageFrame::Decoding to be ImageFrame::DecodingStatus.

Done. ImageFrame::DecodingStatus has the following values:  { Invalid, Partial, Complete, Decoding }.

[Said Abou-Hallawa]

Created attachment 309841 [details]
Patch

[Said Abou-Hallawa]

Created attachment 309899 [details]
Patch

[Said Abou-Hallawa]

Created attachment 309914 [details]
Patch

[Tim Horton]

Comment on attachment 309914 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        'Decoding'. This new value will not never be cached in the ImageFrame::
> +        m_decodingStatus. Add a member m_currentFrameDecodingStatus to BitmapImage.

not never? Can we de-double-negativeify this?

> Source/WebCore/ChangeLog:34
> +        which is: does the ImageFrame of the nextFrame has a native image with 
> +        decoded with the native size or not?

Grammar here goes downhill as the sentence goes on.

> Source/WebCore/ChangeLog:66
> +        decides whether another request for the same image frame is allowed or not.

s/decides/decide/

> Source/WebCore/platform/graphics/ImageFrameCache.h:137
> +    void cacheAsyncFrameNativeImageAtIndex(NativeImagePtr&&, size_t, SubsamplingLevel, const DecodingOptions&, ImageFrame::DecodingStatus);

I think the Async should have come at the end of this method name (cacheFrameNativeImageAtIndex{Async}), but that's not relevant to this patch. Also could probably drop the "frame" because this is the "ImageFrameCache" so it's implied, but w/e.

> Source/WebCore/platform/graphics/ImageSource.cpp:-116
> -    m_frameCache->destroyIncompleteDecodedData();

Is BitmapImage the only way to get here? Or are there other paths to get here which won't destroyIncompleteDecodedData now?

[Said Abou-Hallawa]

Created attachment 310169 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 309914 [details]
Patch

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

>> Source/WebCore/ChangeLog:15
>> +        m_decodingStatus. Add a member m_currentFrameDecodingStatus to BitmapImage.
> 
> not never? Can we de-double-negativeify this?

The ChangeLog statement was changed. I added assertions in ImageFrame::setDecodingStatus() and ImageFrame::decodingStatus() to ensure the current or the new value isn't equals to ImageFrame::DecodingStatus::Decoding.

>> Source/WebCore/ChangeLog:34
>> +        decoded with the native size or not?
> 
> Grammar here goes downhill as the sentence goes on.

Fixed.

>> Source/WebCore/ChangeLog:66
>> +        decides whether another request for the same image frame is allowed or not.
> 
> s/decides/decide/

Fixed.

>> Source/WebCore/platform/graphics/ImageFrameCache.h:137
>> +    void cacheAsyncFrameNativeImageAtIndex(NativeImagePtr&&, size_t, SubsamplingLevel, const DecodingOptions&, ImageFrame::DecodingStatus);
> 
> I think the Async should have come at the end of this method name (cacheFrameNativeImageAtIndex{Async}), but that's not relevant to this patch. Also could probably drop the "frame" because this is the "ImageFrameCache" so it's implied, but w/e.

The three functions were renamed to the following:

cacheMetadataAtIndex()
cacheNativeImageAtIndex()
cacheNativeImageAtIndexAsync()

>> Source/WebCore/platform/graphics/ImageSource.cpp:-116
>> -    m_frameCache->destroyIncompleteDecodedData();
> 
> Is BitmapImage the only way to get here? Or are there other paths to get here which won't destroyIncompleteDecodedData now?

BitmapImage::dataChanged() is the only way to change the ImageDecoder data and it is the only caller to ImageSource::dataChanged().

[WebKit Commit Bot]

Comment on attachment 310169 [details]
Patch

Clearing flags on attachment: 310169

Committed r216882: <http://trac.webkit.org/changeset/216882>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.