Bug 164864 – REGRESSION(r208511): ImageDecoders: Crash decoding GIF images since r208511

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Carlos Garcia Campos]

This happens sometimes since r208511 because the same decoder is decoded by more than one thread at the same time and the decoders are not thread-safe. Several methods in ImageDecoder need to decode partially the image, so it's possible that one method calls frameBufferAtIndex at the same times as createFrameImageAtIndex that now can be called from the image decoder thread.

[Carlos Garcia Campos]

Created attachment 295048 [details]
Patch

[Carlos Alberto Lopez Perez]

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

[Simon Fraser (smfr)]

Comment on attachment 295048 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        This happens sometimes since r208511 because the same decoder is decoded by more than one thread at the same

same decoder is decoded by -> same decoder is used by

> Source/WebCore/platform/image-decoders/ImageDecoder.h:218
> +        Lock m_lock;

Would be nice to name this so that it indicates what it protects.

[Carlos Garcia Campos]

(In reply to comment #3)
> Comment on attachment 295048 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295048&action=review

Thanks for the review.

> > Source/WebCore/ChangeLog:8
> > +        This happens sometimes since r208511 because the same decoder is decoded by more than one thread at the same
> 
> same decoder is decoded by -> same decoder is used by

Oops.

> > Source/WebCore/platform/image-decoders/ImageDecoder.h:218
> > +        Lock m_lock;
> 
> Would be nice to name this so that it indicates what it protects.

Actually I named this m_decoderLock at first, but sounded redundant to me because the class is ImageDecoder. Maybe m_decodeLock, since we are protecting the decode() method in the end.

[Simon Fraser (smfr)]

(In reply to comment #4)
> Actually I named this m_decoderLock at first, but sounded redundant to me
> because the class is ImageDecoder. Maybe m_decodeLock, since we are
> protecting the decode() method in the end.

Locks don't protect code, they protect data. m_lock is probably OK here.

What needs to happen to make the decoders thread safe?

[Said Abou-Hallawa]

Comment on attachment 295048 [details]
Patch

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

Would it be better to rename the existing virtual frameBufferAtIndex() functions to be internalFrameBufferAtIndex() and create a non virtual function called frameBufferAtIndex() in imageDecoder class which does the locking and call internalFrameBufferAtIndex()?

> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:174
> +    Locker<Lock> locker(m_lock);

You can use LockHolder instead of Locker<Lock>.

[Carlos Garcia Campos]

(In reply to comment #5)
> (In reply to comment #4)
> > Actually I named this m_decoderLock at first, but sounded redundant to me
> > because the class is ImageDecoder. Maybe m_decodeLock, since we are
> > protecting the decode() method in the end.
> 
> Locks don't protect code, they protect data. m_lock is probably OK here.

Good point.

> What needs to happen to make the decoders thread safe?

You mean making every decoder thread safe instead of protecting them from ImageDecoder?

[Carlos Garcia Campos]

(In reply to comment #6)
> Comment on attachment 295048 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295048&action=review
> 
> Would it be better to rename the existing virtual frameBufferAtIndex()
> functions to be internalFrameBufferAtIndex() and create a non virtual
> function called frameBufferAtIndex() in imageDecoder class which does the
> locking and call internalFrameBufferAtIndex()?

I thought about that but I also want to make sure the returned ImageFrame is not modified by another thread, that's why don't release the lock until the end of the function.

> > Source/WebCore/platform/image-decoders/ImageDecoder.cpp:174
> > +    Locker<Lock> locker(m_lock);
> 
> You can use LockHolder instead of Locker<Lock>.

Sure!

[Carlos Garcia Campos]

Committed r208881: <http://trac.webkit.org/changeset/208881>

[Said Abou-Hallawa]

(In reply to comment #8)
> (In reply to comment #6)
> > Comment on attachment 295048 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=295048&action=review
> > 
> > Would it be better to rename the existing virtual frameBufferAtIndex()
> > functions to be internalFrameBufferAtIndex() and create a non virtual
> > function called frameBufferAtIndex() in imageDecoder class which does the
> > locking and call internalFrameBufferAtIndex()?
> 
> I thought about that but I also want to make sure the returned ImageFrame is
> not modified by another thread, that's why don't release the lock until the
> end of the function.
> 

But the only function that changes the ImageFrame or the ImageDecoder:: m_frameBufferCache is ImageDecoder::frameBufferAtIndex(). It does that via calling ImageDecoder::decode() which calls ImageReader::decode(). The ImageReader::decode() is the one that is responsible for caching the ImageFrames in ImageDecoder:: m_frameBufferCache.

There is another calls to ImageDecoder::decode() from ImageDecoder::isSizeAvailable() but this calls decodes the size of the image frame only. It does not cache any ImageFrame.

So I think it's cleaner to lock m_frameBufferCache for writing in the one place that changes it instead of locking every place that makes a call to the same function.