[Webkit-unassigned] [Bug 203353] [WinCairo] GIFImageReader::m_data is ref/deref in different threads

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Oct 30 19:10:31 PDT 2019


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

--- Comment #9 from Fujii Hironori <Hironori.Fujii at sony.com> ---
Comment on attachment 382289
  --> https://bugs.webkit.org/attachment.cgi?id=382289
Patch

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

Thanks for the review.

>> Source/WebCore/platform/image-decoders/ScalableImageDecoder.h:75
>> +        return *m_data.get();
> 
> m_data can be null and this is why it isn't a Ref<SharedBuffer>.

Will do.
However, the decorders don't do the null check of m_data. For example, GIFImageReader::decode dereferences m_data->size() without the null check.
The reason of Ref<SharedBuffer> is that it is not set in the time of ScalableImageDecoder construction.

>> Source/WebCore/platform/image-decoders/bmp/BMPImageReader.cpp:128
>> +    if ((m_decodedOffset > m_parent->data().size()) || ((m_parent->data().size() - m_decodedOffset) < 4))
> 
> The decoder here is called m_parent while it is called m_client in GIFImageReader. Can't we use the same name for both? Or can't we add a base class for both of them named "ImageReader"? In ImageReader, we can define the relationship between the reader and the decoder.

Implementations of *ImageReaders varies.
PNGImageReader and JPEGImageReader don't have m_data, and PNGImageReader::decode and JPEGImageReader::decode take 'const SharedBuffer&' as the argument.
I think this is better than BMPImageReader and GIFImageReader because ScalableImageDecoder::m_data is guarded by m_mutex.
Anyway, I'll do it in follow-up patches.

>> Source/WebCore/platform/image-decoders/bmp/BMPImageReader.cpp:160
>> +    if ((m_decodedOffset > m_parent->data().size()) || ((m_parent->data().size() - m_decodedOffset) < m_infoHeader.biSize) || !readInfoHeader())
> 
> Since m_parent->data() is repeated many times in this file, can't we add a wrapper: SharedBuffer* data() { returns m_parent->data(); }?

Will do.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20191031/3d1b2092/attachment-0001.htm>


More information about the webkit-unassigned mailing list