Bug 167618 – The current frame of an image should not deleted if another frame is asynchronously being decoded

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

Decoding multiple frames from the same image is not thread-safe. Since the next frame may not be ready at the time BitmapImage::draw() is called, the current frame has to be drawn. If the current frame is not available, drawing the current frame will invoke the image decoding from the drawing commit thread. This may lead to a crash when two threads update the image decoded frames data at the same time.

[Said Abou-Hallawa]

Created attachment 300174 [details]
Patch

[Said Abou-Hallawa]

I will see if I can come up with a test for this patch.

[Simon Fraser (smfr)]

Comment on attachment 300174 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        be destroyed expect the current frame.

expect -> except

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:73
> +void ImageFrameCache::destroyDecodedData(bool destroyAll, size_t currentFrame)

'currentFrame' is a bit confusing here. Does it mean "just destroy data for the current frame", or "destroy data for all frames up to and including currentFrame"?

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:87
> +bool ImageFrameCache::destroyDecodedDataIfNecessary(bool destroyAll, size_t currentFrame)

Same comment.

[Said Abou-Hallawa]

Created attachment 300254 [details]
Patch

[Said Abou-Hallawa]

(In reply to comment #3)
> Comment on attachment 300174 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=300174&action=review
> 
> > Source/WebCore/ChangeLog:18
> > +        be destroyed expect the current frame.
> 
> expect -> except
> 
> > Source/WebCore/platform/graphics/ImageFrameCache.cpp:73
> > +void ImageFrameCache::destroyDecodedData(bool destroyAll, size_t currentFrame)
> 
> 'currentFrame' is a bit confusing here. Does it mean "just destroy data for
> the current frame", or "destroy data for all frames up to and including
> currentFrame"?
> 
> > Source/WebCore/platform/graphics/ImageFrameCache.cpp:87
> > +bool ImageFrameCache::destroyDecodedDataIfNecessary(bool destroyAll, size_t currentFrame)
> 
> Same comment.

I did not see these comments before uploading the new patch. Please ignore it till I fix these issues.

[Said Abou-Hallawa]

Created attachment 300279 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 300174 [details]
Patch

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

>>> Source/WebCore/ChangeLog:18
>>> +        be destroyed expect the current frame.
>> 
>> expect -> except
> 
> I did not see these comments before uploading the new patch. Please ignore it till I fix these issues.

Fixed.

>> Source/WebCore/platform/graphics/ImageFrameCache.cpp:73
>> +void ImageFrameCache::destroyDecodedData(bool destroyAll, size_t currentFrame)
> 
> 'currentFrame' is a bit confusing here. Does it mean "just destroy data for the current frame", or "destroy data for all frames up to and including currentFrame"?

All the logic is moved to BitmapImage::destroyDecodedData(). BitmapImage::destroyDecodedDataIfNecessary() also checks the decoded data  size and then decides to call BitmapImage::destroyDecodedData() or not. The ImageSource is just a wrapper for the ImageFrameCache functions. The ImageFrameCache::destroyDecodedData() deletes a range of ImageFrame unconditionally.

[Said Abou-Hallawa]

Created attachment 300557 [details]
Patch

[Jon Lee]

rdar://problem/30231732

[Simon Fraser (smfr)]

Comment on attachment 300557 [details]
Patch

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

> Source/WTF/wtf/Assertions.h:306
> +    CRASH(); \
> +} \

The indentation is off here.

> Source/WebCore/ChangeLog:16
> +        We can avoid that by destroying all the frames expect the current frame if

expect -> except

[Said Abou-Hallawa]

Created attachment 301408 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 301408 [details]
Patch

Clearing flags on attachment: 301408

Committed r212265: <http://trac.webkit.org/changeset/212265>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

<rdar://problem/30231732>