Changeset 235537 in webkit

Timestamp:

Aug 30, 2018, 9:27:44 PM (8 years ago)

Author:

Alan Bujtas

Message:

CounterMaps should hold a unique_ptr of CounterMap.
​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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/css/counters/crash-when-cloning-body-expected.txt (added)
• LayoutTests/fast/css/counters/crash-when-cloning-body.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/rendering/RenderCounter.cpp (modified) (10 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/rendering/RenderCounter.cpp

@@ -48 +48 @@
 
 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);
@@ -297 +297 @@
     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;
     }
@@ -313 +313 @@
         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);
 
@@ -327 +327 @@
         if (!currentRenderer->hasCounterNodeMap())
             continue;
-        auto* currentCounter = maps.find(currentRenderer)->value.get(identifier);
+        auto* currentCounter = maps.find(currentRenderer)->value->get(identifier);
         if (!currentCounter)
             continue;
@@ -437 +437 @@
         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())
@@ -449 +449 @@
     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);
 }
@@ -459 +460 @@
     if (map == counterMaps().end())
         return;
-    auto node = map->value.take(identifier);
+    auto node = map->value->take(identifier);
     if (!node)
         return;
@@ -500 +501 @@
     }
     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);
@@ -508 +509 @@
         }
         auto place = findPlaceForCounter(renderer, key, node->hasResetType());
-        if (node != counterMap.get(key))
+        if (node != counterMap->get(key))
             continue;
         CounterNode* parent = node->parent();
@@ -602 +603 @@
             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);