CounterMaps should hold a unique_ptr of CounterMap.
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 31 Aug 2018 04:27:44 +0000 (04:27 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 31 Aug 2018 04:27:44 +0000 (04:27 +0000)
https://bugs.webkit.org/show_bug.cgi?id=189174
<rdar://problem/43686458>

Reviewed by Ryosuke Niwa.

Source/WebCore:

In certain cases calls to CounterMaps might lead to unexpected deletion of the CounterMap object.

Test: fast/css/counters/crash-when-cloning-body.html

* rendering/RenderCounter.cpp:
(WebCore::makeCounterNode):
(WebCore::destroyCounterNodeWithoutMapRemoval):
(WebCore::RenderCounter::destroyCounterNodes):
(WebCore::RenderCounter::destroyCounterNode):
(WebCore::updateCounters):
(showCounterRendererTree):

LayoutTests:

* fast/css/counters/crash-when-cloning-body-expected.txt: Added.
* fast/css/counters/crash-when-cloning-body.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/css/counters/crash-when-cloning-body-expected.txt [new file with mode: 0644]
LayoutTests/fast/css/counters/crash-when-cloning-body.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderCounter.cpp

index 34abd26..0b7089a 100644 (file)
@@ -1,3 +1,14 @@
+2018-08-30  Zalan Bujtas  <zalan@apple.com>
+
+        CounterMaps should hold a unique_ptr of CounterMap.
+        https://bugs.webkit.org/show_bug.cgi?id=189174
+        <rdar://problem/43686458>
+
+        Reviewed by Ryosuke Niwa.
+
+        * fast/css/counters/crash-when-cloning-body-expected.txt: Added.
+        * fast/css/counters/crash-when-cloning-body.html: Added.
+
 2018-08-30  Truitt Savell  <tsavell@apple.com>
 
         Unreviewed, rolling out r235516.
diff --git a/LayoutTests/fast/css/counters/crash-when-cloning-body-expected.txt b/LayoutTests/fast/css/counters/crash-when-cloning-body-expected.txt
new file mode 100644 (file)
index 0000000..5ae0cfd
--- /dev/null
@@ -0,0 +1,2 @@
+Pass if no crash.
+Pass if no crash.
diff --git a/LayoutTests/fast/css/counters/crash-when-cloning-body.html b/LayoutTests/fast/css/counters/crash-when-cloning-body.html
new file mode 100644 (file)
index 0000000..d245cd3
--- /dev/null
@@ -0,0 +1,41 @@
+<!DOCTYPE html>\r
+<html>\r
+<head>\r
+<style>                        \r
+*:first-child {\r
+       display: table;\r
+       counter-increment: foo2;\r
+}\r
+\r
+* {\r
+       display: inline;\r
+       -webkit-appearance: unset;\r
+       visibility: hidden;\r
+}\r
+                       \r
+:nth-last-child(1) {\r
+       counter-reset: foo1;\r
+}\r
+</style>\r
+</head>\r
+<body>\r
+<div id=firstDiv></div>\r
+<div id=secondDiv></div>\r
+<div style="visibility: visible">Pass if no crash.</div>\r
+</body>\r
+\r
+<script>\r
+if (window.testRunner)\r
+    testRunner.dumpAsText();\r
+    \r
+document.body.offsetHeight;\r
+\r
+let div = document.createElement('div');\r
+div.appendChild(document.body.cloneNode(true))\r
+\r
+document.styleSheets[0].addRule('#firstDiv::after','content: counter(foobar)');\r
+\r
+secondDiv.appendChild(document.createElement('input'))\r
+firstDiv.appendChild(div);\r
+</script>\r
+</html>\r
index 967cdf9..6940e6e 100644 (file)
@@ -1,3 +1,23 @@
+2018-08-30  Zalan Bujtas  <zalan@apple.com>
+
+        CounterMaps should hold a unique_ptr of CounterMap.
+        https://bugs.webkit.org/show_bug.cgi?id=189174
+        <rdar://problem/43686458>
+
+        Reviewed by Ryosuke Niwa.
+
+        In certain cases calls to CounterMaps might lead to unexpected deletion of the CounterMap object.
+
+        Test: fast/css/counters/crash-when-cloning-body.html
+
+        * rendering/RenderCounter.cpp:
+        (WebCore::makeCounterNode):
+        (WebCore::destroyCounterNodeWithoutMapRemoval):
+        (WebCore::RenderCounter::destroyCounterNodes):
+        (WebCore::RenderCounter::destroyCounterNode):
+        (WebCore::updateCounters):
+        (showCounterRendererTree):
+
 2018-08-30  Ross Kirsling  <ross.kirsling@sony.com>
 
         Speculative build fix for WPE after r235531.
index 76c0113..11e547b 100644 (file)
@@ -47,7 +47,7 @@ using namespace HTMLNames;
 WTF_MAKE_ISO_ALLOCATED_IMPL(RenderCounter);
 
 using CounterMap = HashMap<AtomicString, Ref<CounterNode>>;
-using CounterMaps = HashMap<const RenderElement*, CounterMap>;
+using CounterMaps = HashMap<const RenderElement*, std::unique_ptr<CounterMap>>;
 
 static CounterNode* makeCounterNode(RenderElement&, const AtomicString& identifier, bool alwaysCreateCounter);
 
@@ -296,7 +296,7 @@ static CounterNode* makeCounterNode(RenderElement& renderer, const AtomicString&
 {
     if (renderer.hasCounterNodeMap()) {
         ASSERT(counterMaps().contains(&renderer));
-        if (auto* node = counterMaps().find(&renderer)->value.get(identifier))
+        if (auto* node = counterMaps().find(&renderer)->value->get(identifier))
             return node;
     }
 
@@ -312,7 +312,7 @@ static CounterNode* makeCounterNode(RenderElement& renderer, const AtomicString&
     if (place.parent)
         place.parent->insertAfter(newNode, place.previousSibling.get(), identifier);
 
-    maps.add(&renderer, CounterMap { }).iterator->value.add(identifier, newNode.copyRef());
+    maps.add(&renderer, std::make_unique<CounterMap>()).iterator->value->add(identifier, newNode.copyRef());
     renderer.setHasCounterNodeMap(true);
 
     if (newNode->parent())
@@ -326,7 +326,7 @@ static CounterNode* makeCounterNode(RenderElement& renderer, const AtomicString&
         skipDescendants = false;
         if (!currentRenderer->hasCounterNodeMap())
             continue;
-        auto* currentCounter = maps.find(currentRenderer)->value.get(identifier);
+        auto* currentCounter = maps.find(currentRenderer)->value->get(identifier);
         if (!currentCounter)
             continue;
         skipDescendants = true;
@@ -436,8 +436,8 @@ static void destroyCounterNodeWithoutMapRemoval(const AtomicString& identifier,
     for (RefPtr<CounterNode> child = node.lastDescendant(); child && child != &node; child = WTFMove(previous)) {
         previous = child->previousInPreOrder();
         child->parent()->removeChild(*child);
-        ASSERT(counterMaps().find(&child->owner())->value.get(identifier) == child);
-        counterMaps().find(&child->owner())->value.remove(identifier);
+        ASSERT(counterMaps().find(&child->owner())->value->get(identifier) == child);
+        counterMaps().find(&child->owner())->value->remove(identifier);
     }
     if (auto* parent = node.parent())
         parent->removeChild(node);
@@ -448,8 +448,9 @@ void RenderCounter::destroyCounterNodes(RenderElement& owner)
     ASSERT(owner.hasCounterNodeMap());
     auto& maps = counterMaps();
     ASSERT(maps.contains(&owner));
-    for (auto& keyValue : maps.take(&owner))
-        destroyCounterNodeWithoutMapRemoval(keyValue.key, keyValue.value);
+    auto counterMap = maps.take(&owner);
+    for (auto& counterMapEntry : *counterMap)
+        destroyCounterNodeWithoutMapRemoval(counterMapEntry.key, counterMapEntry.value);
     owner.setHasCounterNodeMap(false);
 }
 
@@ -458,7 +459,7 @@ void RenderCounter::destroyCounterNode(RenderElement& owner, const AtomicString&
     auto map = counterMaps().find(&owner);
     if (map == counterMaps().end())
         return;
-    auto node = map->value.take(identifier);
+    auto node = map->value->take(identifier);
     if (!node)
         return;
     destroyCounterNodeWithoutMapRemoval(identifier, *node);
@@ -499,15 +500,15 @@ static void updateCounters(RenderElement& renderer)
         return;
     }
     ASSERT(counterMaps().contains(&renderer));
-    auto& counterMap = counterMaps().find(&renderer)->value;
+    auto* counterMap = counterMaps().find(&renderer)->value.get();
     for (auto& key : directiveMap->keys()) {
-        RefPtr<CounterNode> node = counterMap.get(key);
+        RefPtr<CounterNode> node = counterMap->get(key);
         if (!node) {
             makeCounterNode(renderer, key, false);
             continue;
         }
         auto place = findPlaceForCounter(renderer, key, node->hasResetType());
-        if (node != counterMap.get(key))
+        if (node != counterMap->get(key))
             continue;
         CounterNode* parent = node->parent();
         if (place.parent == parent && place.previousSibling == node->previousSibling())
@@ -601,7 +602,7 @@ void showCounterRendererTree(const WebCore::RenderObject* renderer, const char*
         fprintf(stderr, "%p N:%p P:%p PS:%p NS:%p C:%p\n",
             current, current->node(), current->parent(), current->previousSibling(),
             current->nextSibling(), downcast<WebCore::RenderElement>(*current).hasCounterNodeMap() ?
-            counterName ? WebCore::counterMaps().find(downcast<WebCore::RenderElement>(current))->value.get(identifier) : (WebCore::CounterNode*)1 : (WebCore::CounterNode*)0);
+            counterName ? WebCore::counterMaps().find(downcast<WebCore::RenderElement>(current))->value->get(identifier) : (WebCore::CounterNode*)1 : (WebCore::CounterNode*)0);
     }
     fflush(stderr);
 }