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.
Created attachment 309565 [details] Patch
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.
Created attachment 309614 [details] Patch
<rdar://problem/32109397>
Created attachment 309721 [details] Patch
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
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
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
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
Created attachment 309759 [details] Patch
Created attachment 309761 [details] Patch
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.
Created attachment 309831 [details] Patch
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 }.
Created attachment 309841 [details] Patch
Created attachment 309899 [details] Patch
Created attachment 309914 [details] Patch
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?
Created attachment 310169 [details] Patch
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().
Comment on attachment 310169 [details] Patch Clearing flags on attachment: 310169 Committed r216882: <http://trac.webkit.org/changeset/216882>
All reviewed patches have been landed. Closing bug.