Bug 169199 - ImageDecoder can be deleted while the async decoder thread is still using it
Summary: ImageDecoder can be deleted while the async decoder thread is still using it
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Miguel Gomez
URL:
Keywords: Gtk, InRadar
: 166838 168589 169132 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-03-06 05:36 PST by Carlos Garcia Campos
Modified: 2017-03-13 17:04 PDT (History)
9 users (show)

See Also:


Attachments
Patch (12.49 KB, patch)
2017-03-09 03:42 PST, Miguel Gomez
no flags Details | Formatted Diff | Diff
Patch (13.02 KB, patch)
2017-03-09 05:37 PST, Miguel Gomez
no flags Details | Formatted Diff | Diff
Patch (12.94 KB, patch)
2017-03-09 11:29 PST, Miguel Gomez
no flags Details | Formatted Diff | Diff
Patch (12.61 KB, patch)
2017-03-09 13:35 PST, Miguel Gomez
no flags Details | Formatted Diff | Diff
Patch (12.59 KB, patch)
2017-03-13 03:29 PDT, Miguel Gomez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2017-03-06 05:36:41 PST
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?"
Comment 1 Michael Catanzaro 2017-03-06 10:29:57 PST
*** Bug 166838 has been marked as a duplicate of this bug. ***
Comment 2 Michael Catanzaro 2017-03-06 10:30:03 PST
*** Bug 168589 has been marked as a duplicate of this bug. ***
Comment 3 Michael Catanzaro 2017-03-06 10:30:10 PST
*** Bug 169132 has been marked as a duplicate of this bug. ***
Comment 4 Miguel Gomez 2017-03-09 03:42:56 PST
Created attachment 303905 [details]
Patch
Comment 5 WebKit Commit Bot 2017-03-09 03:44:59 PST
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.
Comment 6 Miguel Gomez 2017-03-09 05:37:28 PST
Created attachment 303909 [details]
Patch
Comment 7 WebKit Commit Bot 2017-03-09 05:40:10 PST
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.
Comment 8 Said Abou-Hallawa 2017-03-09 08:50:54 PST
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>&);
Comment 9 Miguel Gomez 2017-03-09 11:29:37 PST
Created attachment 303939 [details]
Patch
Comment 10 Simon Fraser (smfr) 2017-03-09 11:31:54 PST
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.
Comment 11 Said Abou-Hallawa 2017-03-09 11:42:17 PST
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;
Comment 12 Miguel Gomez 2017-03-09 12:33:24 PST
(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.
Comment 13 Miguel Gomez 2017-03-09 13:35:55 PST
Created attachment 303979 [details]
Patch
Comment 14 WebKit Commit Bot 2017-03-09 13:37:26 PST
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.
Comment 15 Miguel Gomez 2017-03-13 01:53:24 PDT
Guys, is this ok now? Should I make any other modification? Thanks for your comments.
Comment 16 Carlos Garcia Campos 2017-03-13 02:09:12 PDT
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.
Comment 17 Miguel Gomez 2017-03-13 03:29:22 PDT
Created attachment 304249 [details]
Patch
Comment 18 WebKit Commit Bot 2017-03-13 03:34:01 PDT
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.
Comment 19 WebKit Commit Bot 2017-03-13 05:01:05 PDT
Comment on attachment 304249 [details]
Patch

Clearing flags on attachment: 304249

Committed r213833: <http://trac.webkit.org/changeset/213833>
Comment 20 WebKit Commit Bot 2017-03-13 05:01:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Said Abou-Hallawa 2017-03-13 09:01:25 PDT
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.
Comment 22 Jon Lee 2017-03-13 15:07:44 PDT
<rdar://problem/30888034>
Comment 23 Jon Lee 2017-03-13 15:08:09 PDT
Can we file a new bug to track the need for a test for this patch?
Comment 24 Said Abou-Hallawa 2017-03-13 17:04:50 PDT
(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.