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.
Created attachment 295048 [details] Patch
*** Bug 164866 has been marked as a duplicate of this bug. ***
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.
(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.
(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?
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>.
(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?
(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!
Committed r208881: <http://trac.webkit.org/changeset/208881>
(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.