Cherry-pick 265870.5@safari-7616-branch (a06556a). https://bugs.webki…

…t.org/show_bug.cgi?id=258712

    Crash under SVGImageChromeClient::invalidateContentsAndRootView()
    https://bugs.webkit.org/show_bug.cgi?id=258992
    rdar://111456803

    Reviewed by David Kilzer.

    Do hardening by deploying WeakPtr instead of raw pointers for
    SVGImage and ImageObserver. Also make it so that we can ref
    an ImageObserver.

    * Source/WebCore/html/ImageBitmap.cpp:
    * Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:
    (WebCore::CanvasRenderingContext2DBase::drawImage):
    * Source/WebCore/loader/cache/CachedImage.h:
    * Source/WebCore/platform/graphics/BitmapImage.cpp:
    (WebCore::BitmapImage::draw):
    (WebCore::BitmapImage::drawPattern):
    (WebCore::BitmapImage::internalAdvanceAnimation):
    (WebCore::BitmapImage::imageFrameAvailableAtIndex):
    * Source/WebCore/platform/graphics/GraphicsContextGL.cpp:
    (WebCore::GraphicsContextGL::packImageData):
    * Source/WebCore/platform/graphics/Image.cpp:
    (WebCore::Image::imageObserver const):
    (WebCore::Image::setImageObserver):
    (WebCore::Image::drawPattern):
    * Source/WebCore/platform/graphics/Image.h:
    (WebCore::Image::imageObserver const): Deleted.
    (WebCore::Image::setImageObserver): Deleted.
    * Source/WebCore/platform/graphics/ImageObserver.h:
    (WebCore::ImageObserver::ref):
    (WebCore::ImageObserver::deref):
    * Source/WebCore/platform/graphics/ImageSource.cpp:
    (WebCore::ImageSource::encodedDataStatusChanged):
    (WebCore::ImageSource::decodedSizeChanged):
    * Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:
    (WebCore::PDFDocumentImage::decodedSizeChanged):
    (WebCore::PDFDocumentImage::draw):
    * Source/WebCore/platform/graphics/texmap/TextureMapperTiledBackingStore.cpp:
    (WebCore::TextureMapperTiledBackingStore::updateContentsFromImageIfNeeded):
    * Source/WebCore/svg/graphics/SVGImage.cpp:
    (WebCore::SVGImage::drawForContainer):
    (WebCore::SVGImage::nativeImage):
    (WebCore::SVGImage::draw):
    (WebCore::SVGImage::dataChanged):
    * Source/WebCore/svg/graphics/SVGImageClients.h:
    * Tools/TestWebKitAPI/Tests/WebCore/SVGImageCasts.cpp:
    (TestWebKitAPI::TestImageObserver::create):
    (TestWebKitAPI::TEST):

    Canonical link: https://commits.webkit.org/265870.5@safari-7616-branch

Source/WebCore/html/ImageBitmap.cpp

@@ -677,7 +677,7 @@ void ImageBitmap::createCompletionHandler(ScriptExecutionContext& scriptExecutio
     completionHandler(WTFMove(imageBitmap));
 }
 
-class ImageBitmapImageObserver final : public RefCounted<ImageBitmapImageObserver>, public ImageObserver {
+class ImageBitmapImageObserver final : public ImageObserver {
 public:
     static Ref<ImageBitmapImageObserver> create(String mimeType, long long expectedContentLength, const URL& sourceUrl)
     {

Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp

@@ -1637,7 +1637,7 @@ ExceptionOr<void> CanvasRenderingContext2DBase::drawImage(Document& document, Ca
     if (!image)
         return { };
 
-    ImageObserver* observer = image->imageObserver();
+    auto observer = image->imageObserver();
 
     if (image->drawsSVGImage()) {
         image->setImageObserver(nullptr);
@@ -1673,7 +1673,7 @@ ExceptionOr<void> CanvasRenderingContext2DBase::drawImage(Document& document, Ca
     didDraw(repaintEntireCanvas, normalizedDstRect);
 
     if (image->drawsSVGImage())
-        image->setImageObserver(observer);
+        image->setImageObserver(WTFMove(observer));
 
     return { };
 }

Source/WebCore/loader/cache/CachedImage.h

@@ -140,7 +140,7 @@ class CachedImage final : public CachedResource {
     // For compatibility, images keep loading even if there are HTTP errors.
     bool shouldIgnoreHTTPStatusCodeErrors() const override { return true; }
 
-    class CachedImageObserver final : public RefCounted<CachedImageObserver>, public ImageObserver {
+    class CachedImageObserver final : public ImageObserver {
     public:
         static Ref<CachedImageObserver> create(CachedImage& image) { return adoptRef(*new CachedImageObserver(image)); }
         WeakHashSet<CachedImage>& cachedImages() { return m_cachedImages; }

Source/WebCore/platform/graphics/BitmapImage.cpp

@@ -331,8 +331,8 @@ ImageDrawResult BitmapImage::draw(GraphicsContext& context, const FloatRect& des
 
     m_currentFrameDecodingStatus = frameDecodingStatusAtIndex(m_currentFrame);
 
-    if (imageObserver())
-        imageObserver()->didDraw(*this);
+    if (auto observer = imageObserver())
+        observer->didDraw(*this);
 
     return result;
 }
@@ -357,14 +357,14 @@ void BitmapImage::drawPattern(GraphicsContext& ctxt, const FloatRect& destRect,
         if (!buffer)
             return;
 
-        ImageObserver* observer = imageObserver();
+        auto observer = imageObserver();
 
         // Temporarily reset image observer, we don't want to receive any changeInRect() calls due to this relayout.
         setImageObserver(nullptr);
 
         draw(buffer->context(), tileRect, tileRect, { options, DecodingMode::Synchronous, ImageOrientation::Orientation::FromImage });
 
-        setImageObserver(observer);
+        setImageObserver(WTFMove(observer));
         buffer->convertToLuminanceMask();
 
         m_cachedImage = ImageBuffer::sinkIntoImage(WTFMove(buffer), PreserveResolution::Yes);
@@ -541,8 +541,8 @@ void BitmapImage::internalAdvanceAnimation()
 
     callDecodingCallbacks();
 
-    if (imageObserver())
-        imageObserver()->imageFrameAvailable(*this, ImageAnimatingState::Yes, nullptr, decodingStatus);
+    if (auto observer = imageObserver())
+        observer->imageFrameAvailable(*this, ImageAnimatingState::Yes, nullptr, decodingStatus);
 
     LOG(Images, "BitmapImage::%s - %p - url: %s [m_currentFrame = %ld]", __FUNCTION__, this, sourceURL().string().utf8().data(), m_currentFrame);
 }
@@ -654,8 +654,8 @@ void BitmapImage::imageFrameAvailableAtIndex(size_t index)
     if (frameHasDecodedNativeImageCompatibleWithOptionsAtIndex(m_currentFrame, m_currentSubsamplingLevel, DecodingMode::Asynchronous))
         callDecodingCallbacks();
 
-    if (imageObserver())
-        imageObserver()->imageFrameAvailable(*this, ImageAnimatingState::No, nullptr, decodingStatus);
+    if (auto observer = imageObserver())
+        observer->imageFrameAvailable(*this, ImageAnimatingState::No, nullptr, decodingStatus);
 }
 
 DestinationColorSpace BitmapImage::colorSpace()

Source/WebCore/platform/graphics/GraphicsContextGL.cpp

@@ -484,7 +484,7 @@ bool GraphicsContextGL::packImageData(Image* image, const void* pixels, GCGLenum
 
     if (!packPixels(static_cast<const uint8_t*>(pixels), sourceFormat, sourceImageWidth, sourceImageHeight, sourceImageSubRectangle, depth, sourceUnpackAlignment, unpackImageHeight, format, type, alphaOp, data.data(), flipY))
         return false;
-    if (ImageObserver* observer = image->imageObserver())
+    if (auto observer = image->imageObserver())
         observer->didDraw(*image);
     return true;
 }

Source/WebCore/platform/graphics/Image.cpp

@@ -56,6 +56,16 @@ Image::Image(ImageObserver* observer)
 
 Image::~Image() = default;
 
+RefPtr<ImageObserver> Image::imageObserver() const
+{
+    return m_imageObserver.get();
+}
+
+void Image::setImageObserver(RefPtr<ImageObserver>&& observer)
+{
+    m_imageObserver = observer.get();
+}
+
 Image& Image::nullImage()
 {
     ASSERT(isMainThread());
@@ -152,8 +162,8 @@ void Image::drawPattern(GraphicsContext& ctxt, const FloatRect& destRect, const
 
     ctxt.drawPattern(*tileImage, destRect, tileRect, patternTransform, phase, spacing, options);
 
-    if (imageObserver())
-        imageObserver()->didDraw(*this);
+    if (auto observer = imageObserver())
+        observer->didDraw(*this);
 }
 
 ImageDrawResult Image::drawTiled(GraphicsContext& ctxt, const FloatRect& destRect, const FloatPoint& srcPoint, const FloatSize& scaledTileSize, const FloatSize& spacing, const ImagePaintingOptions& options)

Source/WebCore/platform/graphics/Image.h

@@ -77,7 +77,7 @@ struct Length;
 // This class gets notified when an image creates or destroys decoded frames and when it advances animation frames.
 class ImageObserver;
 
-class Image : public RefCounted<Image> {
+class Image : public RefCounted<Image>, public CanMakeWeakPtr<Image> {
     friend class CachedSubimage;
     friend class GraphicsContext;
 public:
@@ -153,8 +153,8 @@ class Image : public RefCounted<Image> {
     void setAllowsAnimation(std::optional<bool> allowsAnimation) { m_allowsAnimation = allowsAnimation; }
 
     // Typically the CachedImage that owns us.
-    ImageObserver* imageObserver() const { return m_imageObserver; }
-    void setImageObserver(ImageObserver* observer) { m_imageObserver = observer; }
+    RefPtr<ImageObserver> imageObserver() const;
+    void setImageObserver(RefPtr<ImageObserver>&&);
     URL sourceURL() const;
     WEBCORE_EXPORT String mimeType() const;
     long long expectedContentLength() const;
@@ -216,7 +216,7 @@ class Image : public RefCounted<Image> {
 
 private:
     RefPtr<FragmentedSharedBuffer> m_encodedImageData;
-    ImageObserver* m_imageObserver;
+    WeakPtr<ImageObserver> m_imageObserver;
 
     // A value of true or false will override the default Page::imageAnimationEnabled state.
     std::optional<bool> m_allowsAnimation { std::nullopt };

Source/WebCore/platform/graphics/ImageObserver.h

@@ -26,7 +26,9 @@
 #pragma once
 
 #include "ImageTypes.h"
+#include <wtf/RefCounted.h>
 #include <wtf/URL.h>
+#include <wtf/WeakPtr.h>
 
 namespace WebCore {
 
@@ -35,10 +37,10 @@ class IntRect;
 
 // Interface for notification about changes to an image, including decoding,
 // drawing, and animating.
-class ImageObserver {
-protected:
-    virtual ~ImageObserver() = default;
+class ImageObserver : public RefCounted<ImageObserver>, public CanMakeWeakPtr<ImageObserver> {
 public:
+    virtual ~ImageObserver() = default;
+
     virtual URL sourceUrl() const = 0;
     virtual String mimeType() const = 0;
     virtual long long expectedContentLength() const = 0;
@@ -55,6 +57,9 @@ class ImageObserver {
 
     virtual bool allowsAnimation(const Image&) const { return true; }
     virtual bool layerBasedSVGEngineEnabled() const { return false; }
+
+protected:
+    ImageObserver() = default;
 };
 
 }

Source/WebCore/platform/graphics/ImageSource.cpp

@@ -166,16 +166,20 @@ void ImageSource::encodedDataStatusChanged(EncodedDataStatus status)
     if (status >= EncodedDataStatus::SizeAvailable)
         growFrames();
 
-    if (m_image && m_image->imageObserver())
-        m_image->imageObserver()->encodedDataStatusChanged(*m_image, status);
+    if (!m_image)
+        return;
+
+    if (auto observer = m_image->imageObserver())
+        observer->encodedDataStatusChanged(*m_image, status);
 }
 
 void ImageSource::decodedSizeChanged(long long decodedSize)
 {
-    if (!decodedSize || !m_image || !m_image->imageObserver())
+    if (!decodedSize || !m_image)
         return;
 
-    m_image->imageObserver()->decodedSizeChanged(*m_image, decodedSize);
+    if (auto imageObserver = m_image->imageObserver())
+        imageObserver->decodedSizeChanged(*m_image, decodedSize);
 }
 
 void ImageSource::decodedSizeIncreased(unsigned decodedSize)

Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp

@@ -86,8 +86,8 @@ void PDFDocumentImage::decodedSizeChanged(size_t newCachedBytes)
     if (!m_cachedBytes && !newCachedBytes)
         return;
 
-    if (imageObserver())
-        imageObserver()->decodedSizeChanged(*this, -static_cast<long long>(m_cachedBytes) + newCachedBytes);
+    if (auto observer = imageObserver())
+        observer->decodedSizeChanged(*this, -static_cast<long long>(m_cachedBytes) + newCachedBytes);
 
     m_cachedBytes = newCachedBytes;
 }
@@ -154,8 +154,8 @@ ImageDrawResult PDFDocumentImage::drawPDFDocument(GraphicsContext& context, cons
     context.scale({ 1, -1 });
     drawPDFPage(context);
 
-    if (imageObserver())
-        imageObserver()->didDraw(*this);
+    if (auto observer = imageObserver())
+        observer->didDraw(*this);
 
     return ImageDrawResult::DidDraw;
 }

Source/WebCore/platform/graphics/texmap/TextureMapperTiledBackingStore.cpp

@@ -36,8 +36,8 @@ void TextureMapperTiledBackingStore::updateContentsFromImageIfNeeded(TextureMapp
 
     updateContents(textureMapper, m_image.get(), m_image->size(), enclosingIntRect(m_image->rect()));
 
-    if (m_image->imageObserver())
-        m_image->imageObserver()->didDraw(*m_image);
+    if (auto observer = m_image->imageObserver())
+        observer->didDraw(*m_image);
     m_image = nullptr;
 }
 

Source/WebCore/svg/graphics/SVGImage.cpp

@@ -190,7 +190,7 @@ ImageDrawResult SVGImage::drawForContainer(GraphicsContext& context, const Float
     if (!m_page)
         return ImageDrawResult::DidNothing;
 
-    ImageObserver* observer = imageObserver();
+    auto observer = imageObserver();
     ASSERT(observer);
 
     // Temporarily reset image observer, we don't want to receive any changeInRect() calls due to this relayout.
@@ -211,7 +211,7 @@ ImageDrawResult SVGImage::drawForContainer(GraphicsContext& context, const Float
 
     ImageDrawResult result = draw(context, dstRect, scaledSrc, options);
 
-    setImageObserver(observer);
+    setImageObserver(WTFMove(observer));
     return result;
 }
 
@@ -232,13 +232,13 @@ RefPtr<NativeImage> SVGImage::nativeImage(const DestinationColorSpace& colorSpac
     if (!imageBuffer)
         return nullptr;
 
-    ImageObserver* observer = imageObserver();
+    auto observer = imageObserver();
     setImageObserver(nullptr);
     setContainerSize(size());
 
     imageBuffer->context().drawImage(*this, FloatPoint(0, 0));
 
-    setImageObserver(observer);
+    setImageObserver(WTFMove(observer));
     return ImageBuffer::sinkIntoNativeImage(WTFMove(imageBuffer));
 }
 
@@ -327,8 +327,8 @@ ImageDrawResult SVGImage::draw(GraphicsContext& context, const FloatRect& dstRec
 
     stateSaver.restore();
 
-    if (imageObserver())
-        imageObserver()->didDraw(*this);
+    if (auto observer = imageObserver())
+        observer->didDraw(*this);
 
     return ImageDrawResult::DidDraw;
 }
@@ -481,7 +481,7 @@ EncodedDataStatus SVGImage::dataChanged(bool allDataReceived)
         m_page->settings().setShouldAllowUserInstalledFonts(false);
 
 #if ENABLE(LAYER_BASED_SVG_ENGINE)
-        if (auto* observer = imageObserver())
+        if (auto observer = imageObserver())
             m_page->settings().setLayerBasedSVGEngineEnabled(observer->layerBasedSVGEngineEnabled());
 #endif
         auto* localMainFrame = dynamicDowncast<LocalFrame>(m_page->mainFrame());

Source/WebCore/svg/graphics/SVGImageClients.h

@@ -30,6 +30,7 @@
 
 #include "EmptyClients.h"
 #include "SVGImage.h"
+#include <wtf/WeakPtr.h>
 
 namespace WebCore {
 
@@ -47,35 +48,40 @@ class SVGImageChromeClient final : public EmptyChromeClient {
     }
 
     bool isSVGImageChromeClient() const final { return true; }
-    SVGImage* image() const { return m_image; }
+    SVGImage* image() const { return m_image.get(); }
 
 private:
     void chromeDestroyed() final
     {
         m_image = nullptr;
     }
 
-    void invalidateContentsAndRootView(const IntRect& r) final
+    void invalidateContentsAndRootView(const IntRect& rect) final
     {
-        // If m_image->m_page is null, we're being destroyed.
-        if (!m_image || !m_image->m_page)
+        RefPtr image { m_image.get() };
+
+        // If m_image->internalPage() is null, we're being destroyed.
+        if (!image || !image->internalPage())
             return;
 
-        auto* imageObserver = m_image->imageObserver();
+        auto imageObserver = image->imageObserver();
         if (!imageObserver)
             return;
 
-        imageObserver->imageFrameAvailable(*m_image, m_image->isAnimating() ? ImageAnimatingState::Yes : ImageAnimatingState::No, &r);
+        imageObserver->imageFrameAvailable(*image, image->isAnimating() ? ImageAnimatingState::Yes : ImageAnimatingState::No, &rect);
     }
 
     bool scheduleRenderingUpdate() final
     {
-        if (m_image && m_image->imageObserver())
-            m_image->imageObserver()->scheduleRenderingUpdate(*m_image);
+        RefPtr image { m_image.get() };
+        if (!image)
+            return true;
+        if (auto imageObserver = image->imageObserver())
+            imageObserver->scheduleRenderingUpdate(*image);
         return true;
     }
 
-    SVGImage* m_image;
+    WeakPtr<SVGImage> m_image;
 };
 
 } // namespace WebCore

Tools/TestWebKitAPI/Tests/WebCore/SVGImageCasts.cpp

@@ -36,6 +36,14 @@ using namespace WebCore;
 
 class TestImageObserver : public ImageObserver {
 public:
+    static Ref<TestImageObserver> create()
+    {
+        return adoptRef(*new TestImageObserver);
+    }
+
+private:
+    TestImageObserver() = default;
+
     URL sourceUrl() const final
     {
         return URL();
@@ -79,7 +87,7 @@ class TestImageObserver : public ImageObserver {
 
 TEST(SVGImageCasts, SVGImageForContainerIsNotSVGImage)
 {
-    TestImageObserver imageObserver;
+    auto imageObserver = TestImageObserver::create();
     auto svgImage = SVGImage::create(imageObserver);
     Image& svgImageBase = svgImage.get();
     EXPECT_TRUE(is<SVGImage>(svgImageBase));