ImageDecoder can be deleted while the async decoder thread is still using it
authormagomez@igalia.com <magomez@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 13 Mar 2017 12:01:02 +0000 (12:01 +0000)
committermagomez@igalia.com <magomez@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 13 Mar 2017 12:01:02 +0000 (12:01 +0000)
https://bugs.webkit.org/show_bug.cgi?id=169199

Reviewed by Carlos Garcia Campos.

Make the image decoder used by ImageSource and ImageFrameCache into a RefPtr instead of
and unique_ptr, and pass a reference to the decoder thread. This ensures that the decoder
will stay alive as long as the decoding thread is processing frames. Also, stop the async
decoding queue if a new decoder is set to ImageFrameCache.

No new tests.

* platform/graphics/ImageFrameCache.cpp:
(WebCore::ImageFrameCache::setDecoder):
(WebCore::ImageFrameCache::decoder):
(WebCore::ImageFrameCache::startAsyncDecodingQueue):
(WebCore::ImageFrameCache::metadata):
* platform/graphics/ImageFrameCache.h:
(WebCore::ImageFrameCache::setDecoder): Deleted.
Moved to source file so we can keep the ImageDecoder forward declaration.
(WebCore::ImageFrameCache::decoder): Deleted.
Moved to source file so we can keep the ImageDecoder forward declaration.
* platform/graphics/ImageSource.h:
* platform/graphics/cg/ImageDecoderCG.h:
(WebCore::ImageDecoder::create):
* platform/graphics/win/ImageDecoderDirect2D.h:
(WebCore::ImageDecoder::create):
* platform/image-decoders/ImageDecoder.cpp:
(WebCore::ImageDecoder::create):
* platform/image-decoders/ImageDecoder.h:

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

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

index 72a9d6a..b1b6f84 100644 (file)
@@ -1,3 +1,36 @@
+2017-03-13  Miguel Gomez  <magomez@igalia.com>
+
+        ImageDecoder can be deleted while the async decoder thread is still using it
+        https://bugs.webkit.org/show_bug.cgi?id=169199
+
+        Reviewed by Carlos Garcia Campos.
+
+        Make the image decoder used by ImageSource and ImageFrameCache into a RefPtr instead of
+        and unique_ptr, and pass a reference to the decoder thread. This ensures that the decoder
+        will stay alive as long as the decoding thread is processing frames. Also, stop the async
+        decoding queue if a new decoder is set to ImageFrameCache.
+
+        No new tests.
+
+        * platform/graphics/ImageFrameCache.cpp:
+        (WebCore::ImageFrameCache::setDecoder):
+        (WebCore::ImageFrameCache::decoder):
+        (WebCore::ImageFrameCache::startAsyncDecodingQueue):
+        (WebCore::ImageFrameCache::metadata):
+        * platform/graphics/ImageFrameCache.h:
+        (WebCore::ImageFrameCache::setDecoder): Deleted.
+        Moved to source file so we can keep the ImageDecoder forward declaration.
+        (WebCore::ImageFrameCache::decoder): Deleted.
+        Moved to source file so we can keep the ImageDecoder forward declaration.
+        * platform/graphics/ImageSource.h:
+        * platform/graphics/cg/ImageDecoderCG.h:
+        (WebCore::ImageDecoder::create):
+        * platform/graphics/win/ImageDecoderDirect2D.h:
+        (WebCore::ImageDecoder::create):
+        * platform/image-decoders/ImageDecoder.cpp:
+        (WebCore::ImageDecoder::create):
+        * platform/image-decoders/ImageDecoder.h:
+
 2017-03-13  Manuel Rego Casasnovas  <rego@igalia.com>
 
         Unprefix -webkit-min-content, -webkit-max-content and -webkit-fit-content
index 55b8b9f..9eb30da 100644 (file)
@@ -70,6 +70,23 @@ ImageFrameCache::~ImageFrameCache()
     ASSERT(!hasAsyncDecodingQueue());
 }
 
+void ImageFrameCache::setDecoder(ImageDecoder* decoder)
+{
+    if (m_decoder == decoder)
+        return;
+
+    // Changing the decoder has to stop the decoding thread. The current frame will
+    // continue decoding safely because the decoding thread has its own
+    // reference of the old decoder.
+    stopAsyncDecodingQueue();
+    m_decoder = decoder;
+}
+
+ImageDecoder* ImageFrameCache::decoder() const
+{
+    return m_decoder.get();
+}
+
 void ImageFrameCache::destroyDecodedData(size_t frameCount, size_t excludeFrame)
 {
     unsigned decodedSize = 0;
@@ -270,14 +287,15 @@ void ImageFrameCache::startAsyncDecodingQueue()
 
     Ref<ImageFrameCache> protectedThis = Ref<ImageFrameCache>(*this);
     Ref<WorkQueue> protectedQueue = decodingQueue();
+    Ref<ImageDecoder> protectedDecoder = Ref<ImageDecoder>(*m_decoder);
 
-    // We need to protect this and m_decodingQueue from being deleted while we are in the decoding loop.
-    decodingQueue()->dispatch([this, protectedThis = WTFMove(protectedThis), protectedQueue = WTFMove(protectedQueue)] {
+    // 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)] {
         ImageFrameRequest frameRequest;
 
         while (m_frameRequestQueue.dequeue(frameRequest)) {
             // Get the frame NativeImage on the decoding thread.
-            NativeImagePtr nativeImage = m_decoder->createFrameImageAtIndex(frameRequest.index, frameRequest.subsamplingLevel, frameRequest.sizeForDrawing);
+            NativeImagePtr nativeImage = protectedDecoder->createFrameImageAtIndex(frameRequest.index, frameRequest.subsamplingLevel, frameRequest.sizeForDrawing);
 
             // 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 {
@@ -385,9 +403,9 @@ T ImageFrameCache::metadata(const T& defaultValue, std::optional<T>* cachedValue
         return defaultValue;
 
     if (!cachedValue)
-        return (m_decoder->*functor)();
+        return (*m_decoder.*functor)();
 
-    *cachedValue = (m_decoder->*functor)();
+    *cachedValue = (*m_decoder.*functor)();
     didDecodeProperties(m_decoder->bytesDecodedToDetermineProperties());
     return cachedValue->value();
 }
index dcdabfe..bdd7bae 100644 (file)
@@ -55,8 +55,8 @@ public:
 
     ~ImageFrameCache();
 
-    void setDecoder(ImageDecoder* decoder) { m_decoder = decoder; }
-    ImageDecoder* decoder() const { return m_decoder; }
+    void setDecoder(ImageDecoder*);
+    ImageDecoder* decoder() const;
 
     unsigned decodedSize() const { return m_decodedSize; }
     void destroyAllDecodedData() { destroyDecodedData(frameCount(), frameCount()); }
@@ -136,7 +136,7 @@ private:
     const ImageFrame& frameAtIndexCacheIfNeeded(size_t, ImageFrame::Caching, const std::optional<SubsamplingLevel>& = { }, const std::optional<IntSize>& sizeForDrawing = { });
 
     Image* m_image { nullptr };
-    ImageDecoder* m_decoder { nullptr };
+    RefPtr<ImageDecoder> m_decoder;
     unsigned m_decodedSize { 0 };
     unsigned m_decodedPropertiesSize { 0 };
 
index 55101f7..b347356 100644 (file)
@@ -112,7 +112,7 @@ private:
     void setDecoderTargetContext(const GraphicsContext*);
 
     Ref<ImageFrameCache> m_frameCache;
-    std::unique_ptr<ImageDecoder> m_decoder;
+    RefPtr<ImageDecoder> m_decoder;
 
     std::optional<SubsamplingLevel> m_maximumSubsamplingLevel;
 
index 5ffcd9e..c0071a4 100644 (file)
@@ -35,14 +35,14 @@ typedef const struct __CFData* CFDataRef;
 
 namespace WebCore {
 
-class ImageDecoder {
+class ImageDecoder : public RefCounted<ImageDecoder> {
     WTF_MAKE_FAST_ALLOCATED;
 public:
     ImageDecoder(AlphaOption, GammaAndColorProfileOption);
 
-    static std::unique_ptr<ImageDecoder> create(const SharedBuffer&, AlphaOption alphaOption, GammaAndColorProfileOption gammaAndColorProfileOption)
+    static Ref<ImageDecoder> create(const SharedBuffer&, AlphaOption alphaOption, GammaAndColorProfileOption gammaAndColorProfileOption)
     {
-        return std::make_unique<ImageDecoder>(alphaOption, gammaAndColorProfileOption);
+        return adoptRef(*new ImageDecoder(alphaOption, gammaAndColorProfileOption));
     }
     
     static size_t bytesDecodedToDetermineProperties();
index 64add0b..301ffad 100644 (file)
@@ -36,14 +36,14 @@ interface IWICImagingFactory;
 
 namespace WebCore {
 
-class ImageDecoder {
+class ImageDecoder : public RefCounted<ImageDecoder> {
     WTF_MAKE_FAST_ALLOCATED;
 public:
     ImageDecoder();
     
-    static std::unique_ptr<ImageDecoder> create(const SharedBuffer&, AlphaOption, GammaAndColorProfileOption)
+    static Ref<ImageDecoder> create(const SharedBuffer&, AlphaOption, GammaAndColorProfileOption)
     {
-        return std::make_unique<ImageDecoder>();
+        return adoptRef(*new ImageDecoder());
     }
     
     static size_t bytesDecodedToDetermineProperties();
index 74e6604..ad6e402 100644 (file)
@@ -96,7 +96,7 @@ bool matchesCURSignature(char* contents)
 
 }
 
-std::unique_ptr<ImageDecoder> ImageDecoder::create(const SharedBuffer& data, AlphaOption alphaOption, GammaAndColorProfileOption gammaAndColorProfileOption)
+RefPtr<ImageDecoder> ImageDecoder::create(const SharedBuffer& data, AlphaOption alphaOption, GammaAndColorProfileOption gammaAndColorProfileOption)
 {
     static const unsigned lengthOfLongestSignature = 14; // To wit: "RIFF????WEBPVP"
     char contents[lengthOfLongestSignature];
@@ -105,24 +105,24 @@ std::unique_ptr<ImageDecoder> ImageDecoder::create(const SharedBuffer& data, Alp
         return nullptr;
 
     if (matchesGIFSignature(contents))
-        return std::unique_ptr<ImageDecoder> { std::make_unique<GIFImageDecoder>(alphaOption, gammaAndColorProfileOption) };
+        return adoptRef(*new GIFImageDecoder(alphaOption, gammaAndColorProfileOption));
 
     if (matchesPNGSignature(contents))
-        return std::unique_ptr<ImageDecoder> { std::make_unique<PNGImageDecoder>(alphaOption, gammaAndColorProfileOption) };
+        return adoptRef(*new PNGImageDecoder(alphaOption, gammaAndColorProfileOption));
 
     if (matchesICOSignature(contents) || matchesCURSignature(contents))
-        return std::unique_ptr<ImageDecoder> { std::make_unique<ICOImageDecoder>(alphaOption, gammaAndColorProfileOption) };
+        return adoptRef(*new ICOImageDecoder(alphaOption, gammaAndColorProfileOption));
 
     if (matchesJPEGSignature(contents))
-        return std::unique_ptr<ImageDecoder> { std::make_unique<JPEGImageDecoder>(alphaOption, gammaAndColorProfileOption) };
+        return adoptRef(*new JPEGImageDecoder(alphaOption, gammaAndColorProfileOption));
 
 #if USE(WEBP)
     if (matchesWebPSignature(contents))
-        return std::unique_ptr<ImageDecoder> { std::make_unique<WEBPImageDecoder>(alphaOption, gammaAndColorProfileOption) };
+        return adoptRef(*new WEBPImageDecoder(alphaOption, gammaAndColorProfileOption));
 #endif
 
     if (matchesBMPSignature(contents))
-        return std::unique_ptr<ImageDecoder> { std::make_unique<BMPImageDecoder>(alphaOption, gammaAndColorProfileOption) };
+        return adoptRef(*new BMPImageDecoder(alphaOption, gammaAndColorProfileOption));
 
     return nullptr;
 }
index 6302130..758987c 100644 (file)
@@ -47,7 +47,7 @@ namespace WebCore {
     // 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 {
+    class ImageDecoder : public RefCounted<ImageDecoder> {
         WTF_MAKE_NONCOPYABLE(ImageDecoder); WTF_MAKE_FAST_ALLOCATED;
     public:
         ImageDecoder(AlphaOption alphaOption, GammaAndColorProfileOption gammaAndColorProfileOption)
@@ -60,10 +60,9 @@ namespace WebCore {
         {
         }
 
-        // Returns a caller-owned decoder of the appropriate type.  Returns 0 if
-        // we can't sniff a supported type from the provided data (possibly
+        // Returns nullptr if we can't sniff a supported type from the provided data (possibly
         // because there isn't enough data yet).
-        static std::unique_ptr<ImageDecoder> create(const SharedBuffer& data, AlphaOption, GammaAndColorProfileOption);
+        static RefPtr<ImageDecoder> create(const SharedBuffer& data, AlphaOption, GammaAndColorProfileOption);
 
         virtual String filenameExtension() const = 0;