When the image decoding thread makes a callOnMainThread(), ensure all the objects...
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 17 May 2017 20:58:42 +0000 (20:58 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 17 May 2017 20:58:42 +0000 (20:58 +0000)
https://bugs.webkit.org/show_bug.cgi?id=171614

Patch by Said Abou-Hallawa <sabouhallawa@apple.com> on 2017-05-17
Reviewed by David Kilzer.

The asynchronous image decoding was designed to not block the main thread if
the image is deleted. To achieve that we allow decoding the current frame
even if it is not going to be used after closing the decoding queue. We
protect all the objects which the decoding thread uses. But when a frame
finishes decoding the native image frame is cached on the main thread. Not
all of the objects are protected when the callOnMainThread() is dispatched.
The ImageFrameCache and the ImageDecoder objects are not protected.

This might lead to two kinds of crashes:
1. A segfault inside the ImageDecoder trying to access one of its member
2. A segfault inside the ImageFrameCache trying to access one of its frames

The fix is to protect the ImageFrameCache and the ImageDecoder when the
decoding thread makes a callOnMainThread(). Also switch all the pointers
the decoding threads protect to be ThreadSafeRefCounted.

* platform/graphics/ImageFrameCache.cpp:
(WebCore::ImageFrameCache::startAsyncDecodingQueue):
* platform/graphics/ImageFrameCache.h:
* platform/graphics/cg/ImageDecoderCG.h:
* platform/graphics/win/ImageDecoderDirect2D.h:
* platform/image-decoders/ImageDecoder.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@216998 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/ImageFrameCache.cpp
Source/WebCore/platform/graphics/ImageFrameCache.h
Source/WebCore/platform/graphics/cg/ImageDecoderCG.h
Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.h
Source/WebCore/platform/image-decoders/ImageDecoder.h

index 6c528c0..1145b17 100644 (file)
@@ -1,3 +1,33 @@
+2017-05-17  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        When the image decoding thread makes a callOnMainThread(), ensure all the objects it needs are protected
+        https://bugs.webkit.org/show_bug.cgi?id=171614
+
+        Reviewed by David Kilzer.
+
+        The asynchronous image decoding was designed to not block the main thread if
+        the image is deleted. To achieve that we allow decoding the current frame
+        even if it is not going to be used after closing the decoding queue. We 
+        protect all the objects which the decoding thread uses. But when a frame
+        finishes decoding the native image frame is cached on the main thread. Not
+        all of the objects are protected when the callOnMainThread() is dispatched.
+        The ImageFrameCache and the ImageDecoder objects are not protected.
+
+        This might lead to two kinds of crashes:
+        1. A segfault inside the ImageDecoder trying to access one of its member
+        2. A segfault inside the ImageFrameCache trying to access one of its frames
+
+        The fix is to protect the ImageFrameCache and the ImageDecoder when the
+        decoding thread makes a callOnMainThread(). Also switch all the pointers
+        the decoding threads protect to be ThreadSafeRefCounted.
+
+        * platform/graphics/ImageFrameCache.cpp:
+        (WebCore::ImageFrameCache::startAsyncDecodingQueue):
+        * platform/graphics/ImageFrameCache.h:
+        * platform/graphics/cg/ImageDecoderCG.h:
+        * platform/graphics/win/ImageDecoderDirect2D.h:
+        * platform/image-decoders/ImageDecoder.h:
+
 2017-05-17  Wenson Hsieh  <wenson_hsieh@apple.com>
 
         A URL type is vended for a non-URL plain text string when starting data interaction
index be24ba8..dd4940f 100644 (file)
@@ -281,28 +281,30 @@ void ImageFrameCache::startAsyncDecodingQueue()
     Ref<ImageDecoder> protectedDecoder = Ref<ImageDecoder>(*m_decoder);
 
     // We need to protect this, m_decodingQueue and m_decoder from being deleted while we are in the decoding loop.
-    decodingQueue()->dispatch([this, protectedThis = WTFMove(protectedThis), protectedQueue = WTFMove(protectedQueue), protectedDecoder = WTFMove(protectedDecoder)] {
+    decodingQueue()->dispatch([protectedThis = WTFMove(protectedThis), protectedQueue = WTFMove(protectedQueue), protectedDecoder = WTFMove(protectedDecoder)] {
         ImageFrameRequest frameRequest;
 
-        while (m_frameRequestQueue.dequeue(frameRequest)) {
+        while (protectedThis->m_frameRequestQueue.dequeue(frameRequest)) {
             TraceScope tracingScope(AsyncImageDecodeStart, AsyncImageDecodeEnd);
 
             // Get the frame NativeImage on the decoding thread.
             NativeImagePtr nativeImage = protectedDecoder->createFrameImageAtIndex(frameRequest.index, frameRequest.subsamplingLevel, frameRequest.decodingOptions);
             if (nativeImage)
-                LOG(Images, "ImageFrameCache::%s - %p - url: %s [frame %ld has been decoded]", __FUNCTION__, this, sourceURL().string().utf8().data(), frameRequest.index);
-            else
-                LOG(Images, "ImageFrameCache::%s - %p - url: %s [decoding for frame %ld has failed]", __FUNCTION__, this, sourceURL().string().utf8().data(), frameRequest.index);
+                LOG(Images, "ImageFrameCache::%s - %p - url: %s [frame %ld has been decoded]", __FUNCTION__, protectedThis.ptr(), protectedThis->sourceURL().string().utf8().data(), frameRequest.index);
+            else {
+                LOG(Images, "ImageFrameCache::%s - %p - url: %s [decoding for frame %ld has failed]", __FUNCTION__, protectedThis.ptr(), protectedThis->sourceURL().string().utf8().data(), frameRequest.index);
+                continue;
+            }
 
             // Update the cached frames on the main thread to avoid updating the MemoryCache from a different thread.
-            callOnMainThread([this, protectedQueue = protectedQueue.copyRef(), nativeImage, frameRequest] () mutable {
-                // The queue may be closed if after we got the frame NativeImage, stopAsyncDecodingQueue() was called
-                if (protectedQueue.ptr() == m_decodingQueue) {
-                    ASSERT(m_frameCommitQueue.first() == frameRequest);
-                    m_frameCommitQueue.removeFirst();
-                    cacheNativeImageAtIndexAsync(WTFMove(nativeImage), frameRequest.index, frameRequest.subsamplingLevel, frameRequest.decodingOptions, frameRequest.decodingStatus);
+            callOnMainThread([protectedThis = protectedThis.copyRef(), protectedQueue = protectedQueue.copyRef(), protectedDecoder = protectedDecoder.copyRef(), nativeImage = WTFMove(nativeImage), frameRequest] () mutable {
+                // The queue may have been closed if after we got the frame NativeImage, stopAsyncDecodingQueue() was called.
+                if (protectedQueue.ptr() == protectedThis->m_decodingQueue && protectedDecoder.ptr() == protectedThis->m_decoder) {
+                    ASSERT(protectedThis->m_frameCommitQueue.first() == frameRequest);
+                    protectedThis->m_frameCommitQueue.removeFirst();
+                    protectedThis->cacheNativeImageAtIndexAsync(WTFMove(nativeImage), frameRequest.index, frameRequest.subsamplingLevel, frameRequest.decodingOptions, frameRequest.decodingStatus);
                 } else
-                    LOG(Images, "ImageFrameCache::%s - %p - url: %s [frame %ld will not cached]", __FUNCTION__, this, sourceURL().string().utf8().data(), frameRequest.index);
+                    LOG(Images, "ImageFrameCache::%s - %p - url: %s [frame %ld will not cached]", __FUNCTION__, protectedThis.ptr(), protectedThis->sourceURL().string().utf8().data(), frameRequest.index);
             });
         }
     });
index 52d3603..c718af9 100644 (file)
@@ -41,7 +41,7 @@ class Image;
 class ImageDecoder;
 class URL;
 
-class ImageFrameCache : public RefCounted<ImageFrameCache> {
+class ImageFrameCache : public ThreadSafeRefCounted<ImageFrameCache> {
     friend class ImageSource;
 public:
     static Ref<ImageFrameCache> create(Image* image)
index c8ddb6d..1283c16 100644 (file)
@@ -35,7 +35,7 @@ typedef const struct __CFData* CFDataRef;
 
 namespace WebCore {
 
-class ImageDecoder : public RefCounted<ImageDecoder> {
+class ImageDecoder : public ThreadSafeRefCounted<ImageDecoder> {
     WTF_MAKE_FAST_ALLOCATED;
 public:
     ImageDecoder(const URL& sourceURL, AlphaOption, GammaAndColorProfileOption);
index bf5ee46..6436c06 100644 (file)
@@ -36,7 +36,7 @@ interface IWICImagingFactory;
 
 namespace WebCore {
 
-class ImageDecoder : public RefCounted<ImageDecoder> {
+class ImageDecoder : public ThreadSafeRefCounted<ImageDecoder> {
     WTF_MAKE_FAST_ALLOCATED;
 public:
     ImageDecoder();
index 13d8d0b..f5bada8 100644 (file)
@@ -49,7 +49,7 @@ class URL;
 // ENABLE(IMAGE_DECODER_DOWN_SAMPLING) allows image decoders to downsample
 // at decode time. Image decoders will downsample any images larger than
 // |m_maxNumPixels|. FIXME: Not yet supported by all decoders.
-class ImageDecoder : public RefCounted<ImageDecoder> {
+class ImageDecoder : public ThreadSafeRefCounted<ImageDecoder> {
     WTF_MAKE_NONCOPYABLE(ImageDecoder); WTF_MAKE_FAST_ALLOCATED;
 public:
     ImageDecoder(AlphaOption alphaOption, GammaAndColorProfileOption gammaAndColorProfileOption)