Bug 172122 – Make ImageDecoder be a member of ImageFrameCache only

• Status: NEW
• Component: Images

[Said Abou-Hallawa]

All the native image decoders, which are used by WebKit, are not thread safe. Many crashes have been reported because of read/write or write/write scenarios which are invoked concurrently from the main thread and the decoding thread. To fix these crashes, we have to enforce that an ImageDecoder be accessed by a single thread at any time. 

To make locking ImageDecoder easier to implement and recognize, we need first to make ImageDecoder belongs to a single class only. Currently ImageDecoder is created by ImagSource and a copy of it is passed to ImageFrameCache. Most of the calls to the ImageDeooder methods are made from ImageFrameCache. Only a few calls are made from ImageSource. It make sense to move the ownership of the ImageDecoder to ImageFrameCache.

[Said Abou-Hallawa]

Created attachment 310142 [details]
Patch

[Said Abou-Hallawa]

Created attachment 310147 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 310147 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:244
> +    frame.m_isSubsamplingAllowed = m_decoder->frameAllowSubsamplingAtIndex(index);

Why is this different from ImageFrameCache::isSubsamplingAllowed()?

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:594
> +    if (isDecoderAvailable())
> +        m_decoder->setTargetContext(targetContext);

I think we should keep the "set a GraphicsContext* on the decoder" concept specific to Windows, with #ifdefs.

> Source/WebCore/platform/graphics/ImageFrameCache.h:63
> +    void clear(SharedBuffer*);

Why does clear() have a parameter that passes in data?

> Source/WebCore/platform/graphics/ImageFrameCache.h:151
>      RefPtr<ImageDecoder> m_decoder;

If the decoder is a RefPtr, what's the guarantee that a reference is not handed out to some other class? a unique_ptr would be better.

> Source/WebCore/platform/image-decoders/ImageDecoder.h:200
> +    void setTargetContext(const GraphicsContext*) { }

It's a shame to make this windows-only ugliness used for all platforms.

[Maciej Stachowiak]

Comment on attachment 310147 [details]
Patch

Unfortunately, this old patch no longer applies.