REGRESSION(r294536): NativeImages copied from ImageBufferIOSurfaceBac…

…kend are mutated after copy https://bugs.webkit.org/show_bug.cgi?id=241367 Patch by Kimmo Kinnunen <kkinnunen@apple.com> on 2022-06-22 Reviewed by Said Abou-Hallawa. With GPUP, ImageBuffer IOSurface modifications happen in the GPUP. In r294536 it was changed that WP creates NativeImage instances with CGImage instances created from IOSurfaces mapped in WP. These CGImages do not know that the underlying IOSurface changes in the other process. Detach the CGImages from the IOSurface before the first write to the ImageBuffer is being sent to GPUP. This is done with the ensureNativeImagesHaveCopiedBackingStore() call. * LayoutTests/fast/canvas/canvas-pattern-from-modified-canvas-expected.txt: Added. * LayoutTests/fast/canvas/canvas-pattern-from-modified-canvas.html: Added. * Source/WebCore/platform/graphics/ImageBuffer.h: (WebCore::ImageBuffer::backingStoreWillChange): (WebCore::ImageBuffer::setNeedsFlush): Deleted. * Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.cpp: (WebCore::ImageBufferIOSurfaceBackend::~ImageBufferIOSurfaceBackend): (WebCore::ImageBufferIOSurfaceBackend::invalidateCachedNativeImage const): (WebCore::ImageBufferIOSurfaceBackend::copyNativeImage const): (WebCore::ImageBufferIOSurfaceBackend::ensureNativeImagesHaveCopiedBackingStore): * Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.h: * Source/WebKit/WebProcess/GPU/graphics/RemoteDisplayListRecorderProxy.h: (WebKit::RemoteDisplayListRecorderProxy::send): * Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h: (WebKit::RemoteImageBufferProxy::setNeedsFlush): (WebKit::RemoteImageBufferProxy::prepareForBackingStoreChange): Canonical link: https://commits.webkit.org/251751@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@295746 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Jun 22, 2022 · 6c003d7df0ee682e4e5b3e80f248f20da9d1ff7c · 6c003d7
1 parent 1d79030
commit 6c003d7df0ee682e4e5b3e80f248f20da9d1ff7c
Showing 7 changed files with 103 additions and 10 deletions.
diff --git a/LayoutTests/fast/canvas/canvas-pattern-from-modified-canvas-expected.txt b/LayoutTests/fast/canvas/canvas-pattern-from-modified-canvas-expected.txt
@@ -0,0 +1,15 @@
+Test to ensure that modifications to canvas are not visible from already captured patterns.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS c[0] is 255
+PASS c[1] is 0
+PASS c[2] is 0
+PASS c[0] is 0
+PASS c[1] is 255
+PASS c[2] is 0
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/canvas/canvas-pattern-from-modified-canvas.html b/LayoutTests/fast/canvas/canvas-pattern-from-modified-canvas.html
@@ -0,0 +1,47 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src="../../resources/js-test-pre.js"></script>
+</head>
+<body>
+<script>
+description("Test to ensure that modifications to canvas are not visible from already captured patterns.");
+// At the time of writing this would fail for Cocoa in case where Canvas backing store image was accessed as
+// mapped in WP but the modifications would happen in GPUP. At the time, GPUP DOM rendering == off, GPUP Canvas
+// rendering == on.
+var ctx = document.createElement('canvas').getContext('2d');
+var pcanvas = document.createElement('canvas');
+pcanvas.width = 100;
+pcanvas.height = 100;
+var pctx = pcanvas.getContext('2d');
+
+var c;
+
+pctx.fillStyle = 'lime';
+pctx.fillRect(0, 0, 50, 50);
+
+// 1. Capture pattern before modification (green).
+var pattern = ctx.createPattern(pcanvas, 'repeat');
+
+// 2. Modify the canvas that was the pattern source (the source is red, the pattern is expected to be green).
+pctx.fillStyle = 'red';
+pctx.fillRect(0, 0, 50, 50);
+c = pctx.getImageData(0, 0, 1, 1).data;
+shouldBe("c[0]", "255");
+shouldBe("c[1]", "0");
+shouldBe("c[2]", "0");
+
+// 3. Assert that the modification is not visible through the captured pattern.
+ctx.fillStyle = pattern;
+ctx.fillRect(0, 0, 100, 100);
+c = ctx.getImageData(0, 0, 1, 1).data;
+shouldBe("c[0]", "0");
+shouldBe("c[1]", "255");
+shouldBe("c[2]", "0");
+
+</script>
+<script src="../../resources/js-test-post.js"></script>
+</body>
+</html>
+
+
diff --git a/Source/WebCore/platform/graphics/ImageBuffer.h b/Source/WebCore/platform/graphics/ImageBuffer.h
@@ -101,7 +101,7 @@ class ImageBuffer : public ThreadSafeRefCounted<ImageBuffer, WTF::DestructionThr
     virtual void flushDrawingContext() { }
     virtual bool flushDrawingContextAsync() { return false; }
     virtual void didFlush(GraphicsContextFlushIdentifier) { }
-    virtual void setNeedsFlush(bool) { }
+    virtual void backingStoreWillChange() { }
 
     virtual FloatSize logicalSize() const = 0;
     virtual IntSize truncatedLogicalSize() const = 0; // This truncates the real size. You probably should be calling logicalSize() instead.

diff --git a/Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.cpp b/Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.cpp
@@ -125,6 +125,7 @@ ImageBufferIOSurfaceBackend::ImageBufferIOSurfaceBackend(const Parameters& param
 
 ImageBufferIOSurfaceBackend::~ImageBufferIOSurfaceBackend()
 {
+    ensureNativeImagesHaveCopiedBackingStore();
     IOSurface::moveToPool(WTFMove(m_surface), m_ioSurfacePool.get());
 }
 
@@ -161,10 +162,12 @@ void ImageBufferIOSurfaceBackend::invalidateCachedNativeImage() const
     // current state of the IOSurface.
     // See https://webkit.org/b/157966 and https://webkit.org/b/228682 for more context.
     context().fillRect({ });
+    m_mayHaveOutstandingBackingStoreReferences = false;
 }
 
 RefPtr<NativeImage> ImageBufferIOSurfaceBackend::copyNativeImage(BackingStoreCopy) const
 {
+    m_mayHaveOutstandingBackingStoreReferences = true;
     return NativeImage::create(m_surface->createImage());
 }
 
@@ -248,6 +251,8 @@ void ImageBufferIOSurfaceBackend::setVolatilityState(VolatilityState volatilityS
 
 void ImageBufferIOSurfaceBackend::ensureNativeImagesHaveCopiedBackingStore()
 {
+    if (!m_mayHaveOutstandingBackingStoreReferences)
+        return;
     invalidateCachedNativeImage();
     flushContext();
 }

diff --git a/Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.h b/Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.h
@@ -87,7 +87,7 @@ class WEBCORE_EXPORT ImageBufferIOSurfaceBackend : public ImageBufferCGBackend {
 
     std::unique_ptr<IOSurface> m_surface;
     mutable bool m_requiresDrawAfterPutPixelBuffer { false };
-
+    mutable bool m_mayHaveOutstandingBackingStoreReferences { false };
     mutable bool m_needsSetupContext { false };
     VolatilityState m_volatilityState { VolatilityState::NonVolatile };
 

diff --git a/Source/WebKit/WebProcess/GPU/graphics/RemoteDisplayListRecorderProxy.h b/Source/WebKit/WebProcess/GPU/graphics/RemoteDisplayListRecorderProxy.h
@@ -53,7 +53,7 @@ class RemoteDisplayListRecorderProxy : public WebCore::DisplayList::Recorder {
         if (UNLIKELY(!(m_renderingBackend && m_imageBuffer)))
             return;
 
-        m_imageBuffer->setNeedsFlush(true);
+        m_imageBuffer->backingStoreWillChange();
         m_renderingBackend->sendToStream(WTFMove(message), m_destinationBufferIdentifier);
     }
 

diff --git a/Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h b/Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h
@@ -66,7 +66,7 @@ class RemoteImageBufferProxy : public WebCore::ConcreteImageBuffer<BackendType>
     ~RemoteImageBufferProxy()
     {
         if (!m_remoteRenderingBackendProxy || m_remoteRenderingBackendProxy->isGPUProcessConnectionClosed()) {
-            setNeedsFlush(false);
+            m_needsFlush = false;
             return;
         }
 
@@ -116,9 +116,23 @@ class RemoteImageBufferProxy : public WebCore::ConcreteImageBuffer<BackendType>
         m_receivedFlushIdentifierChangedCondition.notifyAll();
     }
 
-    void setNeedsFlush(bool needsFlush) final
+    void backingStoreWillChange() final
     {
-        m_needsFlush = needsFlush;
+        if (m_needsFlush)
+            return;
+        m_needsFlush = true;
+
+        // Prepare for the backing store change the first time this notification comes after flush has
+        // completed.
+
+        // If we already need a flush, this cannot be the first notification for change,
+        // handled by the m_needsFlush case above.
+
+        // If we already have a pending flush, this cannot be the first notification for change.
+        if (hasPendingFlush())
+            return;
+
+        prepareForBackingStoreChange();
     }
 
     void waitForDidFlushWithTimeout()
@@ -230,8 +244,9 @@ class RemoteImageBufferProxy : public WebCore::ConcreteImageBuffer<BackendType>
 
     void clearBackend() final
     {
-        setNeedsFlush(false);
+        m_needsFlush = false;
         didFlush(m_sentFlushIdentifier);
+        prepareForBackingStoreChange();
         BaseConcreteImageBuffer::clearBackend();
     }
 
@@ -254,8 +269,8 @@ class RemoteImageBufferProxy : public WebCore::ConcreteImageBuffer<BackendType>
         ASSERT(resolutionScale() == 1);
         auto& mutableThis = const_cast<RemoteImageBufferProxy&>(*this);
         mutableThis.flushDrawingContextAsync();
+        backingStoreWillChange();
         m_remoteRenderingBackendProxy->putPixelBufferForImageBuffer(m_renderingResourceIdentifier, pixelBuffer, srcRect, destPoint, destFormat);
-        setNeedsFlush(true);
     }
 
     void convertToLuminanceMask() final
@@ -296,11 +311,11 @@ class RemoteImageBufferProxy : public WebCore::ConcreteImageBuffer<BackendType>
 
         if (!m_needsFlush)
             return hasPendingFlush();
-        
+
         m_sentFlushIdentifier = WebCore::GraphicsContextFlushIdentifier::generate();
         LOG_WITH_STREAM(SharedDisplayLists, stream << "RemoteImageBufferProxy " << m_renderingResourceIdentifier << " flushDrawingContextAsync - flush " << m_sentFlushIdentifier);
         m_remoteDisplayList.flushContext(m_sentFlushIdentifier);
-        setNeedsFlush(false);
+        m_needsFlush = false;
         return true;
     }
 
@@ -327,6 +342,17 @@ class RemoteImageBufferProxy : public WebCore::ConcreteImageBuffer<BackendType>
         return WTF::makeUnique<ThreadSafeRemoteImageBufferFlusher<BackendType>>(*this);
     }
 
+    void prepareForBackingStoreChange()
+    {
+        ASSERT(!hasPendingFlush());
+        // If the backing store is mapped in the process and the changes happen in the other
+        // process, we need to prepare for the backing store change before we let the change happen.
+        if (!canMapBackingStore())
+            return;
+        if (auto* backend = ensureBackendCreated())
+            backend->ensureNativeImagesHaveCopiedBackingStore();
+    }
+
     WebCore::GraphicsContextFlushIdentifier m_sentFlushIdentifier;
     Lock m_receivedFlushIdentifierLock;
     Condition m_receivedFlushIdentifierChangedCondition;