Bug 177406 - Images may render partial frames even after loading all the encoded data
Summary: Images may render partial frames even after loading all the encoded data
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-09-23 01:38 PDT by Said Abou-Hallawa
Modified: 2017-09-25 13:13 PDT (History)
9 users (show)

See Also:


Attachments
Patch (10.20 KB, patch)
2017-09-23 01:56 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (10.19 KB, patch)
2017-09-23 13:11 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2017-09-23 01:38:27 PDT
This bug can happen if the image decoding thread is closed after it finished decoding the partially loaded frame. The dead decoding thread can shuts down itself if the SynchronizedFixedQueue notifies it that it has been closed. This can happen if the dispatcher makes this thread active. But before this happens, new image data is received, the image renderer is invalidated and a new decoding request is made. So a new decoding thread is created and a frame request is pushed to the SynchronizedFixedQueue. The dead thread still can access the SynchronizedFixedQueue but it did not get a chance to be active between closing the SynchronizedFixedQueue and reopening it. So the dead thread dequeues the new decoding request. When it finishes decoding, it realizes it is already dead so it drops the decoded frame. So although we received new image data we do not draw a newer image frame. If this happens after receiving all the data, the full image full won't be rendered.

This is the calling stack that can lead to this bug:

// Receiving image data and render repaint.
BitmapImage::draw()
ImageSource::requestFrameAsyncDecodingAtIndex()
ImageFrameCache::requestFrameAsyncDecodingAtIndex()
ImageFrameCache::startAsyncDecodingQueue()
WorkQueue::create()                                 // Creates a WorkQueue, say WQ1.
  ImageDecoder::createFrameImageAtIndex()           // Happens in the decoding thread WQ1.
  callOnMainThread()                                // Gets dispatched from WQ1.
    ImageFrameCache::cacheNativeImageAtIndexAsync() // Get called from callOnMainThread() from the decoding thread WQ1.
    BitmapImage::imageFrameAvailableAtIndex()       // The renderer is invalidated
    ImageSource::stopAsyncDecodingQueue()
    ImageFrameCache::stopAsyncDecodingQueue()
    SynchronizedFixedQueue::close()
BitmapImage::draw()                                 // The new frame is drawn.

// Receiving image data and render repaint.
BitmapImage::draw()
ImageSource::requestFrameAsyncDecodingAtIndex()
ImageFrameCache::requestFrameAsyncDecodingAtIndex()
ImageFrameCache::startAsyncDecodingQueue()
WorkQueue::create()                                 // Creates a WorkQueue, say WQ2
  ImageDecoder::createFrameImageAtIndex()           // Happens in the dead decoding thread WQ1
  callOnMainThread()                                // Gets dispatched from WQ1.
     m_decodingQueue != protectedQueue              // This frame is dropped to the floor and the render does not get repainted.
Comment 1 Said Abou-Hallawa 2017-09-23 01:56:29 PDT
Created attachment 321622 [details]
Patch
Comment 2 Said Abou-Hallawa 2017-09-23 01:57:43 PDT
<rdar://problem/34588529>
Comment 3 Simon Fraser (smfr) 2017-09-23 10:13:35 PDT
Comment on attachment 321622 [details]
Patch

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

> Source/WTF/wtf/SynchronizedFixedQueue.h:43
>      SynchronizedFixedQueue()

This constructor should be private now.

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:327
> +    ASSERT(m_frameRequestQueue);
> +    m_frameRequestQueue->enqueue({ index, subsamplingLevel, sizeForDrawing, decodingStatus });

The ASSERT is pointless because you'll crash on the next line anyway.
Comment 4 Said Abou-Hallawa 2017-09-23 13:11:26 PDT
Created attachment 321636 [details]
Patch
Comment 5 Said Abou-Hallawa 2017-09-23 13:17:45 PDT
Comment on attachment 321622 [details]
Patch

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

>> Source/WTF/wtf/SynchronizedFixedQueue.h:43
>>      SynchronizedFixedQueue()
> 
> This constructor should be private now.

This wil require changing TestWebKitAPI/Tests/WTF/SynchronizedFixedQueue.cpp. And I wanted to make the smallest change to fix this bug.

>> Source/WebCore/platform/graphics/ImageFrameCache.cpp:327
>> +    m_frameRequestQueue->enqueue({ index, subsamplingLevel, sizeForDrawing, decodingStatus });
> 
> The ASSERT is pointless because you'll crash on the next line anyway.

Done. And it should not be null since ImageFrameCache::requestFrameAsyncDecodingAtIndex() ensures the decoding thread is created by calling ImageFrameCache::startAsyncDecodingQueue(). This function calls ImageFrameCache::frameRequestQueue() which ensures the m_frameRequestQueue is created.
Comment 6 Simon Fraser (smfr) 2017-09-23 13:46:56 PDT
(In reply to Said Abou-Hallawa from comment #5)
> Comment on attachment 321622 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=321622&action=review
> 
> >> Source/WTF/wtf/SynchronizedFixedQueue.h:43
> >>      SynchronizedFixedQueue()
> > 
> > This constructor should be private now.
> 
> This wil require changing
> TestWebKitAPI/Tests/WTF/SynchronizedFixedQueue.cpp. And I wanted to make the
> smallest change to fix this bug.

Then you should change the test. The way it is, someone can make a SynchronizedFixedQueue on the stack which makes no sense for a ref-counted object (and is dangerous).
Comment 7 WebKit Commit Bot 2017-09-23 14:05:29 PDT
The commit-queue encountered the following flaky tests while processing attachment 321636 [details]:

http/tests/security/cross-origin-xsl-BLOCKED.html bug 51054 (authors: abarth@webkit.org, jochen@chromium.org, and rniwa@webkit.org)
The commit-queue is continuing to process your patch.
Comment 8 WebKit Commit Bot 2017-09-23 14:05:56 PDT
Comment on attachment 321636 [details]
Patch

Clearing flags on attachment: 321636

Committed r222427: <http://trac.webkit.org/changeset/222427>
Comment 9 WebKit Commit Bot 2017-09-23 14:05:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Darin Adler 2017-09-23 14:42:34 PDT
Comment on attachment 321636 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageFrameCache.h:142
>      Ref<WorkQueue> decodingQueue();

This likely should just return WorkQueue&, not Ref<WorkQueue>, unless there is some case where this creates a newly created queue or there is some major risk of it being deallocated while being used.

> Source/WebCore/platform/graphics/ImageFrameCache.h:143
> +    Ref<SynchronizedFixedQueue<ImageFrameRequest, BufferSize>> frameRequestQueue();

I think this should return just a plain reference, not a Ref<>, for the same reasons I cited above.
Comment 11 Said Abou-Hallawa 2017-09-25 13:13:00 PDT
The followup bug is https://bugs.webkit.org/show_bug.cgi?id=177458