Crash in RenderTableCol::willBeRemovedFromTree()
authorajuma@chromium.org <ajuma@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 6 Feb 2020 19:54:37 +0000 (19:54 +0000)
committerajuma@chromium.org <ajuma@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 6 Feb 2020 19:54:37 +0000 (19:54 +0000)
https://bugs.webkit.org/show_bug.cgi?id=207031

Reviewed by Antti Koivisto.

Source/WebCore:

A RenderTableCol's table() can be null during willBeRemovedFromTree. This can
happen when the RenderTableCol's table is an ancestor rather than a direct
parent. If RenderTreeBuilder::destroy is then called on an ancestor of the
the RenderTableCol's table, RenderTreeBuilder::destroy proceeds down the ancestor
chain, detaching each node along the way. By the time the RenderTableCol is
reached, the table (a non-parent ancestor) has already been detached, so
table() is null and we crash while trying to use it.

The underlying bug is that RenderTreeBuilder::destroyAndCleanUpAnonymousWrappers
is getting called on the RenderTableCol's ancestor before its descendants (including
the RenderTableCol) are destroyed.

Fix this by changing the order of operations in RenderTreeUpdater::tearDownRenderers
so that tearDownLeftoverShadowHostChildren happens before destroyAndCleanUpAnonymousWrappers.
This ensures that the RenderTableCol is destroyed before destroyAndCleanUpAnonymousWrappers is
called on its ancestor.

Test: tables/table-col-indent-crash.html

* rendering/updating/RenderTreeUpdater.cpp:
(WebCore::RenderTreeUpdater::tearDownRenderers):

LayoutTests:

* tables/table-col-indent-crash-expected.txt: Added.
* tables/table-col-indent-crash.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/tables/table-col-indent-crash-expected.txt [new file with mode: 0644]
LayoutTests/tables/table-col-indent-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/updating/RenderTreeUpdater.cpp

index 254ab85..54315c6 100644 (file)
@@ -1,3 +1,13 @@
+2020-02-06  Ali Juma  <ajuma@chromium.org>
+
+        Crash in RenderTableCol::willBeRemovedFromTree()
+        https://bugs.webkit.org/show_bug.cgi?id=207031
+
+        Reviewed by Antti Koivisto.
+
+        * tables/table-col-indent-crash-expected.txt: Added.
+        * tables/table-col-indent-crash.html: Added.
+
 2020-02-06  Jason Lawrence  <lawrence.j@apple.com>
 
         Regression: (r255150?) [ Mac wk1 ] http/wpt/css/css-animations/start-animation-001.html is flaky failing.
diff --git a/LayoutTests/tables/table-col-indent-crash-expected.txt b/LayoutTests/tables/table-col-indent-crash-expected.txt
new file mode 100644 (file)
index 0000000..4fa608e
--- /dev/null
@@ -0,0 +1,3 @@
+This test passes if it doesn't crash.
+
diff --git a/LayoutTests/tables/table-col-indent-crash.html b/LayoutTests/tables/table-col-indent-crash.html
new file mode 100644 (file)
index 0000000..5fc94f7
--- /dev/null
@@ -0,0 +1,25 @@
+<style>
+   body { -webkit-user-modify: read-write; }
+</style>
+
+<script>
+window.onload = function() {
+    if (window.testRunner)
+        testRunner.dumpAsText();
+    window.getSelection().setPosition(tableElement);
+    document.execCommand("indent", false);
+    document.execCommand("indent", false);
+}
+</script>
+
+<body>
+  <template></template>
+  <details id="details">
+    <summary>
+      <table id="tableElement">
+        <br/>
+        <col>This test passes if it doesn't crash.</col>
+      </table>
+    </summary>
+  </details>
+</body>
\ No newline at end of file
index 578e76f..db48052 100644 (file)
@@ -1,3 +1,32 @@
+2020-02-06  Ali Juma  <ajuma@chromium.org>
+
+        Crash in RenderTableCol::willBeRemovedFromTree()
+        https://bugs.webkit.org/show_bug.cgi?id=207031
+
+        Reviewed by Antti Koivisto.
+
+        A RenderTableCol's table() can be null during willBeRemovedFromTree. This can
+        happen when the RenderTableCol's table is an ancestor rather than a direct
+        parent. If RenderTreeBuilder::destroy is then called on an ancestor of the
+        the RenderTableCol's table, RenderTreeBuilder::destroy proceeds down the ancestor
+        chain, detaching each node along the way. By the time the RenderTableCol is
+        reached, the table (a non-parent ancestor) has already been detached, so
+        table() is null and we crash while trying to use it.
+
+        The underlying bug is that RenderTreeBuilder::destroyAndCleanUpAnonymousWrappers
+        is getting called on the RenderTableCol's ancestor before its descendants (including
+        the RenderTableCol) are destroyed.
+
+        Fix this by changing the order of operations in RenderTreeUpdater::tearDownRenderers
+        so that tearDownLeftoverShadowHostChildren happens before destroyAndCleanUpAnonymousWrappers.
+        This ensures that the RenderTableCol is destroyed before destroyAndCleanUpAnonymousWrappers is
+        called on its ancestor.
+
+        Test: tables/table-col-indent-crash.html
+
+        * rendering/updating/RenderTreeUpdater.cpp:
+        (WebCore::RenderTreeUpdater::tearDownRenderers):
+
 2020-02-06  Brent Fulgham  <bfulgham@apple.com>
 
         Prevent navigating top level frames to Data URLs
index 28d59ee..d2a8cb8 100644 (file)
@@ -561,6 +561,10 @@ void RenderTreeUpdater::tearDownRenderers(Element& root, TeardownType teardownTy
         while (teardownStack.size() > depth) {
             auto& element = *teardownStack.takeLast();
 
+            // Make sure we don't leave any renderers behind in nodes outside the composed tree.
+            if (element.shadowRoot())
+                tearDownLeftoverShadowHostChildren(element, builder);
+
             switch (teardownType) {
             case TeardownType::Full:
             case TeardownType::RendererUpdateCancelingAnimations:
@@ -589,10 +593,6 @@ void RenderTreeUpdater::tearDownRenderers(Element& root, TeardownType teardownTy
                 element.setRenderer(nullptr);
             }
 
-            // Make sure we don't leave any renderers behind in nodes outside the composed tree.
-            if (element.shadowRoot())
-                tearDownLeftoverShadowHostChildren(element, builder);
-
             if (element.hasCustomStyleResolveCallbacks())
                 element.didDetachRenderers();
         }