ImageDecoder can be deleted while the async decoder thread is still using it

• Component: Platform
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

As explained by Miguel in bug #167304

"* The second crash affects multiplatform code, and it happens because the decoder being used in the decoding thread can be deleted while there's still decoding happening.
The decoder is a unique_ptr stored in ImageSource, and that class is the one that sets it to ImageFrameCache, which is the one using it (it's stored as a simple pointer in ImageFrameCache).
When ImageSource::clear() gets called, it sets the ImageFrameCache decoder to null, without ensuring somehow that it's not being used at that point by the decoding thread, and this causes the crash.

I see 2 ways to fix this:

* Using a lock to ensure that ImageFrameCache::stopAsyncDecodingQueue() doesn't return until the decoding thread has stopped processing the requested frames. That function gets called before ImageSource::clear() and its mission is to stop the decoding in the secondary thread, but currently it's possible that there's a frame being decoded when that function returns. If we add a lock there and in the function that performs the decoding in the secondary thread, we can be sure that there's no decoding happening and we can delete the decoder safely. I've tested this fix and it seems to work properly.

* Another option is to turn the decoder into a RefPtr inside ImageSource, pass that RefPtr to ImageFrameCache and use it inside the function that performs the decoding in the secondary thread. This way ImageSource can unref the decoder when it wants, but it won't be destroyed until the decoding thread finishes using it.

Which do you think is the best option?"

[Michael Catanzaro]

*** Bug 166838 has been marked as a duplicate of this bug. ***

[Michael Catanzaro]

*** Bug 168589 has been marked as a duplicate of this bug. ***

[Michael Catanzaro]

*** Bug 169132 has been marked as a duplicate of this bug. ***

[Miguel Gomez]

Created attachment 303905 [details]
Patch

[WebKit Commit Bot]

Attachment 303905 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/image-decoders/ImageDecoder.h:50:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Miguel Gomez]

Created attachment 303909 [details]
Patch

[WebKit Commit Bot]

Attachment 303909 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/image-decoders/ImageDecoder.h:50:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Said Abou-Hallawa]

Comment on attachment 303909 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:263
> -    decodingQueue()->dispatch([this, protectedThis = WTFMove(protectedThis), protectedQueue = WTFMove(protectedQueue)] {
> +    decodingQueue()->dispatch([this, protectedThis = makeRef(*this), protectedQueue = decodingQueue(), protectedDecoder = m_decoder] {

I was doing something similar to what you are doing here and smfr did not like it, https://bugs.webkit.org/show_bug.cgi?id=155546#c54. I think I agree with him that protecting ref pointers should not be buried inside a function call.

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:376
> +        return (m_decoder.get()->*functor)();

It can also be written like this:
    return (*m_decoder.*functor)();

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:378
> +    *cachedValue = (m_decoder.get()->*functor)();

Ditto.

> Source/WebCore/platform/graphics/ImageFrameCache.h:39
> +#if USE(CG)
> +#include "ImageDecoderCG.h"
> +#elif USE(DIRECT2D)
> +#include "ImageDecoderDirect2D.h"
> +#include <WinCodec.h>
> +#else
> +#include "ImageDecoder.h"
> +#endif
> +

Instead of moving these headers to the header file, you can move the implementation of setDecoder() and decoder() to the source file and just keep the ImageDecoder forward declaration.

> Source/WebCore/platform/graphics/ImageFrameCache.h:66
> +    void setDecoder(RefPtr<ImageDecoder> decoder) { m_decoder = decoder; }

Can't this be?

    void setDecoder(const RefPtr<ImageDecoder>&);

[Miguel Gomez]

Created attachment 303939 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 303939 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageFrameCache.h:58
> +    void setDecoder(const RefPtr<ImageDecoder>&);

It doesn't make sense to pass a const RefPtr. Pass a RefPtr<>&& to transfer ownership, or a ImageDecoder& to just retain.

[Said Abou-Hallawa]

Comment on attachment 303939 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:75
> +    m_decoder = decoder;

This function should look this:

     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 protected
     // copy of the old decoder.
     stopAsyncDecodingQueue();
     m_decoder = decoder;

[Miguel Gomez]

(In reply to comment #10)
> Comment on attachment 303939 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303939&action=review
> 
> > Source/WebCore/platform/graphics/ImageFrameCache.h:58
> > +    void setDecoder(const RefPtr<ImageDecoder>&);
> 
> It doesn't make sense to pass a const RefPtr. Pass a RefPtr<>&& to transfer
> ownership, or a ImageDecoder& to just retain.

Sorry, I haven't fully mastered RefPtr yet.
I don't want to transfer ownership, but the parameter to setDecoder() can be nullptr, so I guess ImageDecoder& is not an option either.
I guess it should just be ImageDecoder* then, and then assign it to the RefPtr inside ImageFrameCache.

[Miguel Gomez]

Created attachment 303979 [details]
Patch

[WebKit Commit Bot]

Attachment 303979 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/image-decoders/ImageDecoder.h:50:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Miguel Gomez]

Guys, is this ok now? Should I make any other modification? Thanks for your comments.

[Carlos Garcia Campos]

Comment on attachment 303979 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:80
> +    // copy of the old decoder.

I would say reference instead of copy here.

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:290
> +    RefPtr<ImageDecoder> protectedDecoder = m_decoder;

We could use a ref for this one too, I guess m_decoder can't be nullptr at this point, we are using protectedDecoder inside the lambda without any null check.

> Source/WebCore/platform/image-decoders/ImageDecoder.h:63
> +        // Returns 0 if we can't sniff a supported type from the provided data (possibly

0 -> nullptr.

[Miguel Gomez]

Created attachment 304249 [details]
Patch

[WebKit Commit Bot]

Attachment 304249 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/image-decoders/ImageDecoder.h:50:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 304249 [details]
Patch

Clearing flags on attachment: 304249

Committed r213833: <http://trac.webkit.org/changeset/213833>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

Comment on attachment 304249 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        No new tests.

I think you need to add a new test. You can add an internal API which forces the ImageDecoder to be deleted. A function like this needs to be called from an internal API.

void BitmapImage::clearDecoder()
{
    m_source.clear(data());
}

And then a new frame is requested. Without your patch this scenario should crash. To expose a BitmapImage function through Internals, you may look at Internals::imageFrameIndex(). And to write a test for animated images, you may look fast/images/animated-image-loop-count.html.

[Jon Lee]

<rdar://problem/30888034>

[Jon Lee]

Can we file a new bug to track the need for a test for this patch?

[Said Abou-Hallawa]

(In reply to comment #23)
> Can we file a new bug to track the need for a test for this patch?

https://bugs.webkit.org/show_bug.cgi?id=169576.