REGRESSION(r294536): NativeImages copied from ImageBufferIOSurfaceBackend 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: http://svn.webkit.org/repository/webkit/trunk@295746 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/LayoutTests/fast/canvas/canvas-pattern-from-modified-canvas-expected.txt b/LayoutTests/fast/canvas/canvas-pattern-from-modified-canvas-expected.txt
new file mode 100644
index 0000000..af0177f
--- /dev/null
+++ 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
new file mode 100644
index 0000000..75f8f5f
--- /dev/null
+++ 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
index f054d02..d13fdc7 100644
--- a/Source/WebCore/platform/graphics/ImageBuffer.h
+++ b/Source/WebCore/platform/graphics/ImageBuffer.h
@@ -101,7 +101,7 @@
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
index 56fa057..a10d329 100644
--- a/Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.cpp
+++ b/Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.cpp
@@ -125,6 +125,7 @@
ImageBufferIOSurfaceBackend::~ImageBufferIOSurfaceBackend()
{
+ ensureNativeImagesHaveCopiedBackingStore();
IOSurface::moveToPool(WTFMove(m_surface), m_ioSurfacePool.get());
}
@@ -161,10 +162,12 @@
// 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::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
index bc44e9b..09232e6 100644
--- a/Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.h
+++ b/Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.h
@@ -87,7 +87,7 @@
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
index 5d34167..33fded2 100644
--- a/Source/WebKit/WebProcess/GPU/graphics/RemoteDisplayListRecorderProxy.h
+++ b/Source/WebKit/WebProcess/GPU/graphics/RemoteDisplayListRecorderProxy.h
@@ -53,7 +53,7 @@
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
index 25cee95..d951cfc 100644
--- a/Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h
+++ b/Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h
@@ -66,7 +66,7 @@
~RemoteImageBufferProxy()
{
if (!m_remoteRenderingBackendProxy || m_remoteRenderingBackendProxy->isGPUProcessConnectionClosed()) {
- setNeedsFlush(false);
+ m_needsFlush = false;
return;
}
@@ -116,9 +116,23 @@
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 @@
void clearBackend() final
{
- setNeedsFlush(false);
+ m_needsFlush = false;
didFlush(m_sentFlushIdentifier);
+ prepareForBackingStoreChange();
BaseConcreteImageBuffer::clearBackend();
}
@@ -254,8 +269,8 @@
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 @@
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 @@
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;
