REGRESSION (r262643): DumpRenderTree at com.apple.WebCore: WebCore::Document::prepare...
authordino@apple.com <dino@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 17 Jun 2020 00:44:23 +0000 (00:44 +0000)
committerdino@apple.com <dino@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 17 Jun 2020 00:44:23 +0000 (00:44 +0000)
https://bugs.webkit.org/show_bug.cgi?id=213221
rdar://64260400

Reviewed by Simon Fraser.

Source/WebCore:

A Document could still be holding a pointer to an HTMLCanvasElement after the
canvas had been deleted because the CanvasObserver protocol was disconnected
too early. The fix is to explicitly clear the canvas from the Document as it
stops observing.

Test: webgl/preparation-removed-from-document.html

* dom/Document.cpp:
(WebCore::Document::prepareCanvasesForDisplayIfNeeded): Copy the HashSet to a Vector
just in case something weird happens to the set during iteration.
(WebCore::Document::clearCanvasPreparation): Remove the canvas from the list of
of elements that need preparation.
* dom/Document.h: Add the new clearCanvasPreparation method.

* html/HTMLCanvasElement.cpp:
(WebCore::HTMLCanvasElement::~HTMLCanvasElement): Clear the document.
(WebCore::HTMLCanvasElement::didMoveToNewDocument): Ditto.
(WebCore::HTMLCanvasElement::removedFromAncestor): Ditto.

LayoutTests:

Test that triggers a rendering on a canvas, then rips it out of
the document before drawing.

* webgl/preparation-removed-from-document-expected.txt: Added.
* webgl/preparation-removed-from-document.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@263128 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/webgl/preparation-removed-from-document-expected.txt [new file with mode: 0644]
LayoutTests/webgl/preparation-removed-from-document.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/dom/Document.h
Source/WebCore/html/HTMLCanvasElement.cpp

index 8393249..8135182 100644 (file)
@@ -1,3 +1,17 @@
+2020-06-16  Dean Jackson  <dino@apple.com>
+
+        REGRESSION (r262643): DumpRenderTree at com.apple.WebCore: WebCore::Document::prepareCanvasesForDisplayIfNeeded
+        https://bugs.webkit.org/show_bug.cgi?id=213221
+        rdar://64260400
+
+        Reviewed by Simon Fraser.
+
+        Test that triggers a rendering on a canvas, then rips it out of
+        the document before drawing.
+
+        * webgl/preparation-removed-from-document-expected.txt: Added.
+        * webgl/preparation-removed-from-document.html: Added.
+
 2020-06-16  Peng Liu  <peng.liu6@apple.com>
 
         REGRESSION: [iOS wk2] media/modern-media-controls/media-controller/ios/media-controller-stop-updates-in-fullscreen.html is failing consistently
diff --git a/LayoutTests/webgl/preparation-removed-from-document-expected.txt b/LayoutTests/webgl/preparation-removed-from-document-expected.txt
new file mode 100644 (file)
index 0000000..8b13789
--- /dev/null
@@ -0,0 +1 @@
+
diff --git a/LayoutTests/webgl/preparation-removed-from-document.html b/LayoutTests/webgl/preparation-removed-from-document.html
new file mode 100644 (file)
index 0000000..6a98f3a
--- /dev/null
@@ -0,0 +1,46 @@
+<!DOCTYPE html>
+<style>
+canvas {
+    width: 200px;
+    height: 200px;
+}
+</style>
+<script>
+if (window.testRunner) {
+    testRunner.dumpAsText();
+    testRunner.waitUntilDone();
+}
+
+let canvas;
+
+function startTest()
+{
+    canvas = document.createElement("canvas");
+    canvas.width = 200;
+    canvas.height = 200;
+    document.body.appendChild(canvas);
+    requestAnimationFrame(updateAndRemoveCanvas);
+}
+
+function updateAndRemoveCanvas()
+{
+    let gl = canvas.getContext("webgl");
+    gl.clearColor(0, 1, 0, 1);
+    gl.clear(gl.COLOR_BUFFER_BIT);
+    canvas.remove();
+    canvas = null;
+    gl = null;
+    if (window.GCController)
+        GCController.collect();
+
+    requestAnimationFrame(finishTest);
+}
+
+function finishTest()
+{
+    if (window.testRunner)
+        testRunner.notifyDone();
+}
+
+window.addEventListener("DOMContentLoaded", startTest, false);
+</script>
index 8d8bea4..7cc1388 100644 (file)
@@ -1,5 +1,32 @@
 2020-06-16  Dean Jackson  <dino@apple.com>
 
+        REGRESSION (r262643): DumpRenderTree at com.apple.WebCore: WebCore::Document::prepareCanvasesForDisplayIfNeeded
+        https://bugs.webkit.org/show_bug.cgi?id=213221
+        rdar://64260400
+
+        Reviewed by Simon Fraser.
+
+        A Document could still be holding a pointer to an HTMLCanvasElement after the
+        canvas had been deleted because the CanvasObserver protocol was disconnected
+        too early. The fix is to explicitly clear the canvas from the Document as it
+        stops observing.
+
+        Test: webgl/preparation-removed-from-document.html
+
+        * dom/Document.cpp:
+        (WebCore::Document::prepareCanvasesForDisplayIfNeeded): Copy the HashSet to a Vector
+        just in case something weird happens to the set during iteration.
+        (WebCore::Document::clearCanvasPreparation): Remove the canvas from the list of
+        of elements that need preparation.
+        * dom/Document.h: Add the new clearCanvasPreparation method.
+
+        * html/HTMLCanvasElement.cpp:
+        (WebCore::HTMLCanvasElement::~HTMLCanvasElement): Clear the document.
+        (WebCore::HTMLCanvasElement::didMoveToNewDocument): Ditto.
+        (WebCore::HTMLCanvasElement::removedFromAncestor): Ditto.
+
+2020-06-16  Dean Jackson  <dino@apple.com>
+
         [WebGL] Lose the context if IOSurface allocation fails
         https://bugs.webkit.org/show_bug.cgi?id=213265
         <rdar://problem/64424742>
index f8ed647..9f1e823 100644 (file)
@@ -8589,7 +8589,7 @@ void Document::prepareCanvasesForDisplayIfNeeded()
 {
     // Some canvas contexts need to do work when rendering has finished but
     // before their content is composited.
-    for (auto* canvas : m_canvasesNeedingDisplayPreparation) {
+    for (auto* canvas : copyToVector(m_canvasesNeedingDisplayPreparation)) {
         // However, if they are not in the document body, then they won't
         // be composited and thus don't need preparation. Unfortunately they
         // can't tell at the time they were added to the list, since they
@@ -8603,6 +8603,11 @@ void Document::prepareCanvasesForDisplayIfNeeded()
     m_canvasesNeedingDisplayPreparation.clear();
 }
 
+void Document::clearCanvasPreparation(HTMLCanvasElement* canvas)
+{
+    m_canvasesNeedingDisplayPreparation.remove(canvas);
+}
+
 void Document::canvasChanged(CanvasBase& canvasBase, const FloatRect&)
 {
     if (is<HTMLCanvasElement>(canvasBase)) {
index 7eabf65..6f7d2c0 100644 (file)
@@ -1596,6 +1596,7 @@ public:
     const FrameSelection& selection() const { return m_selection; }
 
     void prepareCanvasesForDisplayIfNeeded();
+    void clearCanvasPreparation(HTMLCanvasElement*);
     void canvasChanged(CanvasBase&, const FloatRect&) final;
     void canvasResized(CanvasBase&) final { };
     void canvasDestroyed(CanvasBase&) final;
index 49ba33f..1a10ebe 100644 (file)
@@ -146,8 +146,8 @@ HTMLCanvasElement::~HTMLCanvasElement()
     // FIXME: This has to be called here because CSSCanvasValue::CanvasObserverProxy::canvasDestroyed()
     // downcasts the CanvasBase object to HTMLCanvasElement. That invokes virtual methods, which should be
     // avoided in destructors, but works as long as it's done before HTMLCanvasElement destructs completely.
-    // This will also cause the document to remove itself as an observer.
     notifyObserversCanvasDestroyed();
+    document().clearCanvasPreparation(this);
 
     m_context = nullptr; // Ensure this goes away before the ImageBuffer.
     setImageBuffer(nullptr);
@@ -1001,6 +1001,7 @@ void HTMLCanvasElement::eventListenersDidChange()
 
 void HTMLCanvasElement::didMoveToNewDocument(Document& oldDocument, Document& newDocument)
 {
+    oldDocument.clearCanvasPreparation(this);
     removeObserver(oldDocument);
     addObserver(newDocument);
 
@@ -1009,8 +1010,10 @@ void HTMLCanvasElement::didMoveToNewDocument(Document& oldDocument, Document& ne
 
 void HTMLCanvasElement::removedFromAncestor(RemovalType removalType, ContainerNode& oldParentOfRemovedTree)
 {
-    if (removalType.disconnectedFromDocument)
+    if (removalType.disconnectedFromDocument) {
+        oldParentOfRemovedTree.document().clearCanvasPreparation(this);
         removeObserver(oldParentOfRemovedTree.document());
+    }
 
     HTMLElement::removedFromAncestor(removalType, oldParentOfRemovedTree);
 }