Bug 172061 – Synchronize access to ImageDecoder

• Component: Images
• Status: NEW

[Said Abou-Hallawa]

For BitmapImage, ImageFrameCache is the only caller to ImageDecoder::createFrameImageAtIndex(). One call is made by the decoding thread in ImageFrameCache::startAsyncDecodingQueue(). The other call is made by ImageFrameCache::frameAtIndexCacheIfNeeded() which is called on the main thread.

First, all the image decoders WebKit uses, are not thread-safe. Second, there was not a problem before enabling the async image decoding for large images. An animated image can either be decoded synchronously or asynchronously. There was not conflict between the main thread and the decoding thread trying to decode the same frame or different frame from the same image at the same time. After enabling the asynchronous decoding for large images, we realize that for some cases the image has to be decoded synchronously, e.g drawing on a canvas. That means, if an image is set to be the background of an element but at the same time it's drawn on a canvas. The same image frame may be decoded from the main thread and from the decoding thread. This situation leads to crash in the image decoder when it tries to write to one of its members in two thread. For example, one thread realloc a vector memory and the other thread tries to read it.

Consider the following timeline:

1) BitmapImage::draw(DecodingMode::Asynchronous): 
       1.1) Calls ImageFrameCache::requestFrameAsyncDecodingAtIndex() which appends the requested from to m_frameCommitQueue.
2) DecodingThread in ImageFrameCache::startAsyncDecodingQueue(): 
       2.1) Calls ImageDecoder::createFrameImageAtIndex()
3) ImageSource::clear(): 
       3.1) Calls ImageFrameCache::setDecoder()
           3.1.1) Calls ImageFrameCache::stopAsyncDecodingQueue() which clears m_frameCommitQueue.
4) BitmapImage::draw(DecodingMode::Synchronous):
       4.1) Calls frameIsBeingDecodedAndIsCompatibleWithOptionsAtIndex()
           4.1.1) Calls ImageFrameCache::frameIsBeingDecodedAndIsCompatibleWithOptionsAtIndex() which returns false because m_frameCommitQueue is empty.
       4.2) Calls frameImageAtIndexCacheIfNeeded()
           4.2.1) Calls ImageFrameCache::frameAtIndexCacheIfNeeded()
               4.2.1.1) Calls ImageDecoder::createFrameImageAtIndex()

If the call 2.1 did not finish before 4.2.1.1 starts the crash will happen.

[Said Abou-Hallawa]

Created attachment 309986 [details]
Patch

[Said Abou-Hallawa]

<rdar://problem/31857571>

[Said Abou-Hallawa]

It turned out I was wrong. All the native image decoders which are used by WebKit are not thread safe. I have seen crash reports which crashing because the decoding thread is crating a native image frame while the main thread is querying the image metadata or the frame metadata.

We need to protect not only the  ImageDecoder::createFrameImageAtIndex() but also all the other methods of the native decoder. Or simply we need to protect the calls ImageFrameCache make to the ImageDecoder methods.

[Said Abou-Hallawa]

Created attachment 310155 [details]
Patch for EWS

[Said Abou-Hallawa]

Created attachment 310158 [details]
Patch for review

[Simon Fraser (smfr)]

Comment on attachment 310158 [details]
Patch for review

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

> Source/WebCore/platform/graphics/BitmapImage.cpp:-222
> -        if (frameIsBeingDecodedAndIsCompatibleWithOptionsAtIndex(m_currentFrame, DecodingMode::Asynchronous)) {
> -            // FIXME: instead of showing the yellow rectangle and returning we need to wait for this the frame to finish decoding.
> -            if (m_showDebugBackground) {
> -                fillWithSolidColor(context, destRect, Color(Color::yellow).colorWithAlpha(0.5), op);
> -                LOG(Images, "BitmapImage::%s - %p - url: %s [waiting for async decoding to finish]", __FUNCTION__, this, sourceURL().string().utf8().data());
> -            }
> -            return;
> -        }

Why did this disappear?

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:329
> +            NativeImagePtr nativeImage = createFrameImageAtIndex(protectedDecoder, frameRequest.index, frameRequest.subsamplingLevel, frameRequest.decodingOptions);

If we had to extend the lifetime of protectedDecoder before, how does locking on m_decoderLock work now? Can we go back to using m_decoder?

[Maciej Stachowiak]

Comment on attachment 310158 [details]
Patch for review

Unfortunately, this old patch no longer applies.