Changeset 269046 in webkit

Timestamp:

Oct 27, 2020, 10:08:50 AM (6 years ago)

Author:

Chris Lord

Message:

[GLIB] imported/w3c/web-platform-tests/html/canvas/offscreen/line-styles/2d.line.width.transformed.html is a flaky crash
​https://bugs.webkit.org/show_bug.cgi?id=217986

Reviewed by Simon Fraser.

Fix race condition caused by use of OffscreenCanvas::scriptExecutionContext() on main thread.
Instead of passing a reference to the OffscreenCanvas object when dealing with the placeholder
canvas on the main thread, encapsulate the necessary data in a separate, ThreadSafeRefCounted
object and pass that instead, negating the need to call back to the Worker thread to release
the reference.

Covered by existing tests.

• html/OffscreenCanvas.cpp:

(WebCore::OffscreenCanvas::create):
(WebCore::OffscreenCanvas::OffscreenCanvas):
(WebCore::OffscreenCanvas::detach):
(WebCore::OffscreenCanvas::setPlaceholderCanvas):
(WebCore::OffscreenCanvas::pushBufferToPlaceholder):
(WebCore::OffscreenCanvas::commitToPlaceholderCanvas):

• html/OffscreenCanvas.h:

Location:

trunk

Files:

• LayoutTests/platform/glib/TestExpectations (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/html/OffscreenCanvas.cpp (modified) (5 diffs)
• Source/WebCore/html/OffscreenCanvas.h (modified) (2 diffs)

trunk/LayoutTests/platform/glib/TestExpectations

@@ -392 +392 @@
 
 webkit.org/b/215462 imported/w3c/web-platform-tests/html/canvas/offscreen/manual/the-offscreen-canvas/offscreencanvas.commit.html [ Failure Pass ]
-
-# Flaky crash on pushBufferToPlaceholder().
-webkit.org/b/217986 imported/w3c/web-platform-tests/html/canvas/offscreen/line-styles/2d.line.width.transformed.html [ Crash Pass ]
-webkit.org/b/217986 imported/w3c/web-platform-tests/html/canvas/offscreen/path-objects/2d.path.arc.selfintersect.1.worker.html [ Crash Pass ]
-webkit.org/b/217986 imported/w3c/web-platform-tests/html/canvas/offscreen/path-objects/2d.path.arc.selfintersect.1.html [ Crash Pass ]
 
 #////////////////////////////////////////////////////////////////////////////////////////

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2020-10-27  Chris Lord  <clord@igalia.com>
+
+        [GLIB] imported/w3c/web-platform-tests/html/canvas/offscreen/line-styles/2d.line.width.transformed.html is a flaky crash
+        https://bugs.webkit.org/show_bug.cgi?id=217986
+
+        Reviewed by Simon Fraser.
+
+        Fix race condition caused by use of OffscreenCanvas::scriptExecutionContext() on main thread.
+        Instead of passing a reference to the OffscreenCanvas object when dealing with the placeholder
+        canvas on the main thread, encapsulate the necessary data in a separate, ThreadSafeRefCounted
+        object and pass that instead, negating the need to call back to the Worker thread to release
+        the reference.
+
+        Covered by existing tests.
+
+        * html/OffscreenCanvas.cpp:
+        (WebCore::OffscreenCanvas::create):
+        (WebCore::OffscreenCanvas::OffscreenCanvas):
+        (WebCore::OffscreenCanvas::detach):
+        (WebCore::OffscreenCanvas::setPlaceholderCanvas):
+        (WebCore::OffscreenCanvas::pushBufferToPlaceholder):
+        (WebCore::OffscreenCanvas::commitToPlaceholderCanvas):
+        * html/OffscreenCanvas.h:
+
 2020-10-27  Antti Koivisto  <antti@apple.com>
 

trunk/Source/WebCore/html/OffscreenCanvas.cpp

@@ -86 +86 @@
         clone->setOriginTainted();
 
-    callOnMainThread([detachedCanvas = WTFMove(detachedCanvas), offscreenCanvas = makeRef(clone.get())] () mutable {
-        offscreenCanvas->m_placeholderCanvas = detachedCanvas->takePlaceholderCanvas();
-        if (offscreenCanvas->m_placeholderCanvas) {
-            auto& placeholderContext = downcast<PlaceholderRenderingContext>(*offscreenCanvas->m_placeholderCanvas->renderingContext());
+    callOnMainThread([detachedCanvas = WTFMove(detachedCanvas), placeholderData = makeRef(*clone->m_placeholderData)] () mutable {
+        placeholderData->canvas = detachedCanvas->takePlaceholderCanvas();
+        if (placeholderData->canvas) {
+            auto& placeholderContext = downcast<PlaceholderRenderingContext>(*placeholderData->canvas->renderingContext());
             auto& imageBufferPipe = placeholderContext.imageBufferPipe();
             if (imageBufferPipe)
-                offscreenCanvas->m_bufferPipeSource = imageBufferPipe->source();
+                placeholderData->bufferPipeSource = imageBufferPipe->source();
         }
-        offscreenCanvas->scriptExecutionContext()->postTask({ ScriptExecutionContext::Task::CleanupTask,
-            [releaseOffscreenCanvas = WTFMove(offscreenCanvas)] (ScriptExecutionContext&) { } });
     });
 
@@ -111 +109 @@
     : CanvasBase(IntSize(width, height))
     , ContextDestructionObserver(&context)
+    , m_placeholderData(PlaceholderData::create())
 {
 }
@@ -390 +389 @@
 
     auto detached = makeUnique<DetachedOffscreenCanvas>(takeImageBuffer(), size(), originClean());
-    detached->m_placeholderCanvas = std::exchange(m_placeholderCanvas, nullptr);
+    detached->m_placeholderCanvas = std::exchange(m_placeholderData->canvas, nullptr);
 
     return detached;
@@ -399 +398 @@
     ASSERT(!m_context);
     ASSERT(isMainThread());
-    m_placeholderCanvas = makeWeakPtr(canvas);
+    m_placeholderData->canvas = makeWeakPtr(canvas);
     auto& placeholderContext = downcast<PlaceholderRenderingContext>(*canvas.renderingContext());
     auto& imageBufferPipe = placeholderContext.imageBufferPipe();
     if (imageBufferPipe)
-        m_bufferPipeSource = imageBufferPipe->source();
+        m_placeholderData->bufferPipeSource = imageBufferPipe->source();
 }
 
 void OffscreenCanvas::pushBufferToPlaceholder()
 {
-    callOnMainThread([protectedThis = makeRef(*this), this] () mutable {
-        {
-            auto locker = holdLock(m_commitLock);
-            if (m_placeholderCanvas && m_pendingCommitBuffer)
-                m_placeholderCanvas->setImageBufferAndMarkDirty(WTFMove(m_pendingCommitBuffer));
-            m_pendingCommitBuffer = nullptr;
-        }
-
-        scriptExecutionContext()->postTask({ ScriptExecutionContext::Task::CleanupTask,
-            [releaseThis = WTFMove(protectedThis)] (ScriptExecutionContext&) { } });
+    callOnMainThread([placeholderData = makeRef(*m_placeholderData)] () mutable {
+        auto locker = holdLock(placeholderData->bufferLock);
+        if (placeholderData->canvas && placeholderData->pendingCommitBuffer)
+            placeholderData->canvas->setImageBufferAndMarkDirty(WTFMove(placeholderData->pendingCommitBuffer));
+        placeholderData->pendingCommitBuffer = nullptr;
     });
 }
@@ -431 +425 @@
         m_context->paintRenderingResultsToCanvas();
 
-    if (m_bufferPipeSource) {
+    if (m_placeholderData->bufferPipeSource) {
         auto bufferCopy = imageBuffer->copyRectToBuffer(FloatRect(FloatPoint(), imageBuffer->logicalSize()), ColorSpace::SRGB, imageBuffer->context());
         if (bufferCopy)
-            m_bufferPipeSource->handle(WTFMove(bufferCopy));
-    }
-
-    auto locker = holdLock(m_commitLock);
-    bool shouldPushBuffer = !m_pendingCommitBuffer;
-    m_pendingCommitBuffer = imageBuffer->copyRectToBuffer(FloatRect(FloatPoint(), imageBuffer->logicalSize()), ColorSpace::SRGB, imageBuffer->context());
-    if (m_pendingCommitBuffer && shouldPushBuffer)
+            m_placeholderData->bufferPipeSource->handle(WTFMove(bufferCopy));
+    }
+
+    auto locker = holdLock(m_placeholderData->bufferLock);
+    bool shouldPushBuffer = !m_placeholderData->pendingCommitBuffer;
+    m_placeholderData->pendingCommitBuffer = imageBuffer->copyRectToBuffer(FloatRect(FloatPoint(), imageBuffer->logicalSize()), ColorSpace::SRGB, imageBuffer->context());
+    if (m_placeholderData->pendingCommitBuffer && shouldPushBuffer)
         pushBufferToPlaceholder();
 }

trunk/Source/WebCore/html/OffscreenCanvas.h

@@ -40 +40 @@
 #include <wtf/Forward.h>
 #include <wtf/RefCounted.h>
+#include <wtf/ThreadSafeRefCounted.h>
 #include <wtf/WeakPtr.h>
 #include <wtf/text/WTFString.h>
@@ -188 +189 @@
 
     bool m_hasScheduledCommit { false };
-    WeakPtr<HTMLCanvasElement> m_placeholderCanvas;
-    RefPtr<ImageBufferPipe::Source> m_bufferPipeSource;
-
-    mutable Lock m_commitLock;
-    std::unique_ptr<ImageBuffer> m_pendingCommitBuffer;
+
+    class PlaceholderData : public ThreadSafeRefCounted<PlaceholderData> {
+    public:
+        static Ref<PlaceholderData> create()
+        {
+            return adoptRef(*new PlaceholderData);
+        }
+
+        WeakPtr<HTMLCanvasElement> canvas;
+        RefPtr<ImageBufferPipe::Source> bufferPipeSource;
+        std::unique_ptr<ImageBuffer> pendingCommitBuffer;
+        mutable Lock bufferLock;
+    };
+
+    RefPtr<PlaceholderData> m_placeholderData;
 };