Skip to content
Permalink
Browse files
REGRESSION(r249162): CanvasRenderingContext2DBase::drawImage() crashe…
…s if the image is animated and the first frame cannot be decoded

https://bugs.webkit.org/show_bug.cgi?id=239113
rdar://87980543

Reviewed by Simon Fraser.

Source/WebCore:

CanvasRenderingContext2DBase::drawImage() needs to ensure the first frame
of the animated image can be decoded correctly before creating the temporary
static image. If the first frame can't be decoded, this function should return
immediately. This matches the behavior of this function before r249162.

The animated image decodes its frames asynchronously in a work queue. But
the first frame has to be decoded synchronously in the main run loop. So
to avoid running the image decoder in two different threads we are going
to keep the first and the current frame cached when we receive a memory
pressure warning. This should not increase the memory allocation of the
animated image because the numbers of cached frames increases quickly and
we keep all of them till a memory warning is received. But the memory
pressure warning will be received a little bit more often. This depends
on the memory size of the first frame.

To make the code more robust, make ImageSource take a Ref<NativeImage>
instead of taking a RefPtr<NativeImage>.

* html/canvas/CanvasRenderingContext2DBase.cpp:
(WebCore::CanvasRenderingContext2DBase::drawImage):
* platform/graphics/BitmapImage.cpp:
(WebCore::BitmapImage::BitmapImage):
(WebCore::BitmapImage::destroyDecodedData):
* platform/graphics/BitmapImage.h:
* platform/graphics/ImageSource.cpp:
(WebCore::ImageSource::ImageSource):
(WebCore::ImageSource::destroyDecodedData):
(WebCore::ImageSource::setNativeImage):
* platform/graphics/ImageSource.h:
(WebCore::ImageSource::create):
(WebCore::ImageSource::isDecoderAvailable const):
(WebCore::ImageSource::destroyAllDecodedData): Deleted.
(WebCore::ImageSource::destroyAllDecodedDataExcludeFrame): Deleted.
(WebCore::ImageSource::destroyDecodedDataBeforeFrame): Deleted.

Source/WebKit:

* GPUProcess/graphics/RemoteDisplayListRecorder.cpp:
(WebKit::RemoteDisplayListRecorder::drawSystemImage):

Canonical link: https://commits.webkit.org/250624@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@294280 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
shallawa committed May 16, 2022
1 parent 9521bac commit f7d646ec921b13baa821bb647981bb3f021cdd84
Showing 8 changed files with 94 additions and 33 deletions.
@@ -1,3 +1,46 @@
2022-05-16 Said Abou-Hallawa <said@apple.com>

REGRESSION(r249162): CanvasRenderingContext2DBase::drawImage() crashes if the image is animated and the first frame cannot be decoded
https://bugs.webkit.org/show_bug.cgi?id=239113
rdar://87980543

Reviewed by Simon Fraser.

CanvasRenderingContext2DBase::drawImage() needs to ensure the first frame
of the animated image can be decoded correctly before creating the temporary
static image. If the first frame can't be decoded, this function should return
immediately. This matches the behavior of this function before r249162.

The animated image decodes its frames asynchronously in a work queue. But
the first frame has to be decoded synchronously in the main run loop. So
to avoid running the image decoder in two different threads we are going
to keep the first and the current frame cached when we receive a memory
pressure warning. This should not increase the memory allocation of the
animated image because the numbers of cached frames increases quickly and
we keep all of them till a memory warning is received. But the memory
pressure warning will be received a little bit more often. This depends
on the memory size of the first frame.

To make the code more robust, make ImageSource take a Ref<NativeImage>
instead of taking a RefPtr<NativeImage>.

* html/canvas/CanvasRenderingContext2DBase.cpp:
(WebCore::CanvasRenderingContext2DBase::drawImage):
* platform/graphics/BitmapImage.cpp:
(WebCore::BitmapImage::BitmapImage):
(WebCore::BitmapImage::destroyDecodedData):
* platform/graphics/BitmapImage.h:
* platform/graphics/ImageSource.cpp:
(WebCore::ImageSource::ImageSource):
(WebCore::ImageSource::destroyDecodedData):
(WebCore::ImageSource::setNativeImage):
* platform/graphics/ImageSource.h:
(WebCore::ImageSource::create):
(WebCore::ImageSource::isDecoderAvailable const):
(WebCore::ImageSource::destroyAllDecodedData): Deleted.
(WebCore::ImageSource::destroyAllDecodedDataExcludeFrame): Deleted.
(WebCore::ImageSource::destroyDecodedDataBeforeFrame): Deleted.

2022-05-16 Oriol Brufau <obrufau@igalia.com>

Take intrinsicBorderForFieldset() into account in intrinsically sized fieldset
@@ -1545,8 +1545,11 @@ ExceptionOr<void> CanvasRenderingContext2DBase::drawImage(Document& document, Ca

if (image->isBitmapImage()) {
// Drawing an animated image to a canvas should draw the first frame (except for a few layout tests)
if (image->isAnimated() && !document.settings().animatedImageDebugCanvasDrawingEnabled())
if (image->isAnimated() && !document.settings().animatedImageDebugCanvasDrawingEnabled()) {
image = BitmapImage::create(image->nativeImage());
if (!image)
return { };
}
downcast<BitmapImage>(*image).updateFromSettings(document.settings());
}

@@ -52,9 +52,8 @@ BitmapImage::BitmapImage(ImageObserver* observer)
{
}

BitmapImage::BitmapImage(RefPtr<NativeImage>&& image, ImageObserver* observer)
: Image(observer)
, m_source(ImageSource::create(WTFMove(image)))
BitmapImage::BitmapImage(Ref<NativeImage>&& image)
: m_source(ImageSource::create(WTFMove(image)))
{
}

@@ -77,12 +76,15 @@ void BitmapImage::destroyDecodedData(bool destroyAll)
{
LOG(Images, "BitmapImage::%s - %p - url: %s", __FUNCTION__, this, sourceURL().string().utf8().data());

if (!destroyAll)
m_source->destroyDecodedDataBeforeFrame(m_currentFrame);
else if (!canDestroyDecodedData())
m_source->destroyAllDecodedDataExcludeFrame(m_currentFrame);
else {
m_source->destroyAllDecodedData();
if (!destroyAll) {
// Destroy all the frames between frame0 and m_currentFrame.
m_source->destroyDecodedData(1, m_currentFrame);
} else if (!canDestroyDecodedData()) {
// Destroy all the frames except frame0 and m_currentFrame.
m_source->destroyDecodedData(1, m_currentFrame);
m_source->destroyDecodedData(m_currentFrame + 1, frameCount());
} else {
m_source->destroyDecodedData(0, frameCount());
m_currentFrameDecodingStatus = DecodingStatus::Invalid;
}

@@ -229,7 +231,6 @@ ImageDrawResult BitmapImage::draw(GraphicsContext& context, const FloatRect& des
if (destRect.isEmpty() || requestedSrcRect.isEmpty())
return ImageDrawResult::DidNothing;


auto srcRect = requestedSrcRect;
auto preferredSize = size();
auto srcSize = sourceSize();
@@ -53,13 +53,19 @@ class Timer;

class BitmapImage final : public Image {
public:
static Ref<BitmapImage> create(PlatformImagePtr&& platformImage, ImageObserver* observer = nullptr)
static RefPtr<BitmapImage> create(PlatformImagePtr&& platformImage)
{
return adoptRef(*new BitmapImage(NativeImage::create(WTFMove(platformImage)), observer));
return create(NativeImage::create(WTFMove(platformImage)));
}
static Ref<BitmapImage> create(RefPtr<NativeImage>&& nativeImage, ImageObserver* observer = nullptr)
static RefPtr<BitmapImage> create(RefPtr<NativeImage>&& nativeImage)
{
return adoptRef(*new BitmapImage(WTFMove(nativeImage), observer));
if (!nativeImage)
return nullptr;
return create(nativeImage.releaseNonNull());
}
static Ref<BitmapImage> create(Ref<NativeImage>&& nativeImage)
{
return adoptRef(*new BitmapImage(WTFMove(nativeImage)));
}
static Ref<BitmapImage> create(ImageObserver* observer = nullptr)
{
@@ -154,7 +160,7 @@ class BitmapImage final : public Image {
void decode(Function<void()>&&);

private:
WEBCORE_EXPORT BitmapImage(RefPtr<NativeImage>&&, ImageObserver* = nullptr);
WEBCORE_EXPORT BitmapImage(Ref<NativeImage>&&);
WEBCORE_EXPORT BitmapImage(ImageObserver* = nullptr);

RefPtr<NativeImage> frameImageAtIndex(size_t index) { return m_source->frameImageAtIndex(index); }
@@ -44,7 +44,7 @@ ImageSource::ImageSource(BitmapImage* image, AlphaOption alphaOption, GammaAndCo
{
}

ImageSource::ImageSource(RefPtr<NativeImage>&& nativeImage)
ImageSource::ImageSource(Ref<NativeImage>&& nativeImage)
: m_runLoop(RunLoop::current())
{
m_frameCount = 1;
@@ -120,17 +120,17 @@ bool ImageSource::isAllDataReceived()
return isDecoderAvailable() ? m_decoder->isAllDataReceived() : frameCount();
}

void ImageSource::destroyDecodedData(size_t frameCount, size_t excludeFrame)
void ImageSource::destroyDecodedData(size_t begin, size_t end)
{
unsigned decodedSize = 0;
if (begin >= end)
return;

ASSERT(frameCount <= m_frames.size());
ASSERT(end <= m_frames.size());

for (size_t index = 0; index < frameCount; ++index) {
if (index == excludeFrame)
continue;
unsigned decodedSize = 0;

for (size_t index = begin; index < end; ++index)
decodedSize += m_frames[index].clearImage();
}

decodedSizeReset(decodedSize);
}
@@ -232,7 +232,7 @@ void ImageSource::growFrames()
m_frames.grow(newSize);
}

void ImageSource::setNativeImage(RefPtr<NativeImage>&& nativeImage)
void ImageSource::setNativeImage(Ref<NativeImage>&& nativeImage)
{
ASSERT(m_frames.size() == 1);
ImageFrame& frame = m_frames[0];
@@ -51,7 +51,7 @@ class ImageSource : public ThreadSafeRefCounted<ImageSource>, public CanMakeWeak
return adoptRef(*new ImageSource(image, alphaOption, gammaAndColorProfileOption));
}

static Ref<ImageSource> create(RefPtr<NativeImage>&& nativeImage)
static Ref<ImageSource> create(Ref<NativeImage>&& nativeImage)
{
return adoptRef(*new ImageSource(WTFMove(nativeImage)));
}
@@ -62,9 +62,7 @@ class ImageSource : public ThreadSafeRefCounted<ImageSource>, public CanMakeWeak
bool isAllDataReceived();

unsigned decodedSize() const { return m_decodedSize; }
void destroyAllDecodedData() { destroyDecodedData(frameCount(), frameCount()); }
void destroyAllDecodedDataExcludeFrame(size_t excludeFrame) { destroyDecodedData(frameCount(), excludeFrame); }
void destroyDecodedDataBeforeFrame(size_t beforeFrame) { destroyDecodedData(beforeFrame, beforeFrame); }
void destroyDecodedData(size_t begin, size_t end);
void destroyIncompleteDecodedData();
void clearFrameBufferCache(size_t beforeFrame);

@@ -128,7 +126,7 @@ class ImageSource : public ThreadSafeRefCounted<ImageSource>, public CanMakeWeak

private:
ImageSource(BitmapImage*, AlphaOption = AlphaOption::Premultiplied, GammaAndColorProfileOption = GammaAndColorProfileOption::Applied);
ImageSource(RefPtr<NativeImage>&&);
ImageSource(Ref<NativeImage>&&);

enum class MetadataType {
AccessibilityDescription = 1 << 0,
@@ -153,15 +151,14 @@ class ImageSource : public ThreadSafeRefCounted<ImageSource>, public CanMakeWeak

bool ensureDecoderAvailable(FragmentedSharedBuffer* data);
bool isDecoderAvailable() const { return m_decoder; }
void destroyDecodedData(size_t frameCount, size_t excludeFrame);
void decodedSizeChanged(long long decodedSize);
void didDecodeProperties(unsigned decodedPropertiesSize);
void decodedSizeIncreased(unsigned decodedSize);
void decodedSizeDecreased(unsigned decodedSize);
void decodedSizeReset(unsigned decodedSize);
void encodedDataStatusChanged(EncodedDataStatus);

void setNativeImage(RefPtr<NativeImage>&&);
void setNativeImage(Ref<NativeImage>&&);
void cacheMetadataAtIndex(size_t, SubsamplingLevel, DecodingStatus = DecodingStatus::Invalid);
void cachePlatformImageAtIndex(PlatformImagePtr&&, size_t, SubsamplingLevel, const DecodingOptions&, DecodingStatus = DecodingStatus::Invalid);
void cachePlatformImageAtIndexAsync(PlatformImagePtr&&, size_t, SubsamplingLevel, const DecodingOptions&, DecodingStatus);
@@ -1,3 +1,14 @@
2022-05-16 Said Abou-Hallawa <said@apple.com>

REGRESSION(r249162): CanvasRenderingContext2DBase::drawImage() crashes if the image is animated and the first frame cannot be decoded
https://bugs.webkit.org/show_bug.cgi?id=239113
rdar://87980543

Reviewed by Simon Fraser.

* GPUProcess/graphics/RemoteDisplayListRecorder.cpp:
(WebKit::RemoteDisplayListRecorder::drawSystemImage):

2022-05-16 Tim Horton <timothy_horton@apple.com>

Fix the build after 250556@main
@@ -302,7 +302,7 @@ void RemoteDisplayListRecorder::drawSystemImage(SystemImage& systemImage, const
ASSERT_NOT_REACHED();
return;
}
badge.setImage(BitmapImage::create(WTFMove(nativeImage)));
badge.setImage(BitmapImage::create(nativeImage.releaseNonNull()));
}
#endif
handleItem(DisplayList::DrawSystemImage(systemImage, destinationRect));

0 comments on commit f7d646e

Please sign in to comment.