[css-grid] Crash on debug removing a positioned child
authorrego@igalia.com <rego@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 16 Mar 2017 13:13:01 +0000 (13:13 +0000)
committerrego@igalia.com <rego@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 16 Mar 2017 13:13:01 +0000 (13:13 +0000)
https://bugs.webkit.org/show_bug.cgi?id=169739

Reviewed by Sergio Villar Senin.

Source/WebCore:

When we add or remove a positioned item we don't need to mark
the grid as dirty, because positioned items do not affect the layout
of the grid at all.

This was causing a crash when a positioned item was removed
after a layout. As after the positioned item was removed,
the method RenderGrid::layoutBlock() was not called,
so when the grid was repainted we got a crash.

Test: fast/css-grid-layout/grid-crash-remove-positioned-item.html

* rendering/RenderGrid.cpp:
(WebCore::RenderGrid::addChild): Add early return to avoid marking
the grid as dirty for positioned grid items.
(WebCore::RenderGrid::removeChild): Ditto.

LayoutTests:

Add new test that checks that adding and removing a positioned grid item
doesn't cause any crashes.

* fast/css-grid-layout/grid-crash-remove-positioned-item-expected.txt: Added.
* fast/css-grid-layout/grid-crash-remove-positioned-item.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/css-grid-layout/grid-crash-remove-positioned-item-expected.txt [new file with mode: 0644]
LayoutTests/fast/css-grid-layout/grid-crash-remove-positioned-item.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderGrid.cpp

index e9abb0c..7ee5896 100644 (file)
@@ -1,3 +1,16 @@
+2017-03-16  Manuel Rego Casasnovas  <rego@igalia.com>
+
+        [css-grid] Crash on debug removing a positioned child
+        https://bugs.webkit.org/show_bug.cgi?id=169739
+
+        Reviewed by Sergio Villar Senin.
+
+        Add new test that checks that adding and removing a positioned grid item
+        doesn't cause any crashes.
+
+        * fast/css-grid-layout/grid-crash-remove-positioned-item-expected.txt: Added.
+        * fast/css-grid-layout/grid-crash-remove-positioned-item.html: Added.
+
 2017-03-16  Caio Lima  <ticaiolima@gmail.com>
 
         [ESnext] Implement Object Spread
diff --git a/LayoutTests/fast/css-grid-layout/grid-crash-remove-positioned-item-expected.txt b/LayoutTests/fast/css-grid-layout/grid-crash-remove-positioned-item-expected.txt
new file mode 100644 (file)
index 0000000..94919e9
--- /dev/null
@@ -0,0 +1,5 @@
+webkit.org/b/169739 - [css-grid] Crash on debug removing a positioned child
+
+This test has PASSED if it does not CRASH on debug.
+
+item
diff --git a/LayoutTests/fast/css-grid-layout/grid-crash-remove-positioned-item.html b/LayoutTests/fast/css-grid-layout/grid-crash-remove-positioned-item.html
new file mode 100644 (file)
index 0000000..79c0f90
--- /dev/null
@@ -0,0 +1,20 @@
+<!DOCTYPE html>
+<script>
+  if (window.testRunner)
+    testRunner.dumpAsText();
+</script>
+<p>webkit.org/b/169739 - [css-grid] Crash on debug removing a positioned child</p>
+<p>This test has PASSED if it does not CRASH on debug.</p>
+<div id="grid" style="display: grid;">
+  <!-- This grid item with some text is needed, otherwise RenderGrid::paintChildren()
+       won't be called after removing the positioned item. -->
+  <div>item</div>
+</div>
+<script>
+  var abspositem = document.createElement("div");
+  abspositem.style.position = "absolute";
+  var grid = document.getElementById("grid");
+  grid.appendChild(abspositem);
+  document.body.offsetLeft;
+  grid.removeChild(abspositem);
+</script>
index 5184274..6f01145 100644 (file)
@@ -1,3 +1,26 @@
+2017-03-16  Manuel Rego Casasnovas  <rego@igalia.com>
+
+        [css-grid] Crash on debug removing a positioned child
+        https://bugs.webkit.org/show_bug.cgi?id=169739
+
+        Reviewed by Sergio Villar Senin.
+
+        When we add or remove a positioned item we don't need to mark
+        the grid as dirty, because positioned items do not affect the layout
+        of the grid at all.
+
+        This was causing a crash when a positioned item was removed
+        after a layout. As after the positioned item was removed,
+        the method RenderGrid::layoutBlock() was not called,
+        so when the grid was repainted we got a crash.
+
+        Test: fast/css-grid-layout/grid-crash-remove-positioned-item.html
+
+        * rendering/RenderGrid.cpp:
+        (WebCore::RenderGrid::addChild): Add early return to avoid marking
+        the grid as dirty for positioned grid items.
+        (WebCore::RenderGrid::removeChild): Ditto.
+
 2017-03-16  Carlos Alberto Lopez Perez  <clopez@igalia.com>
 
         [WebRTC] SDP sess-id in the "o=" line should be a value between 0 and LLONG_MAX.
index d90946d..5af068a 100644 (file)
@@ -69,6 +69,11 @@ void RenderGrid::addChild(RenderObject* newChild, RenderObject* beforeChild)
 {
     RenderBlock::addChild(newChild, beforeChild);
 
+    // Positioned grid items do not take up space or otherwise participate in the layout of the grid,
+    // for that reason we don't need to mark the grid as dirty when they are added.
+    if (newChild->isOutOfFlowPositioned())
+        return;
+
     // The grid needs to be recomputed as it might contain auto-placed items that
     // will change their position.
     dirtyGrid();
@@ -78,6 +83,11 @@ void RenderGrid::removeChild(RenderObject& child)
 {
     RenderBlock::removeChild(child);
 
+    // Positioned grid items do not take up space or otherwise participate in the layout of the grid,
+    // for that reason we don't need to mark the grid as dirty when they are removed.
+    if (child.isOutOfFlowPositioned())
+        return;
+
     // The grid needs to be recomputed as it might contain auto-placed items that
     // will change their position.
     dirtyGrid();