Diff
Modified: trunk/LayoutTests/ChangeLog (127825 => 127826)
--- trunk/LayoutTests/ChangeLog 2012-09-07 05:09:04 UTC (rev 127825)
+++ trunk/LayoutTests/ChangeLog 2012-09-07 05:22:09 UTC (rev 127826)
@@ -1,3 +1,16 @@
+2012-09-06 Elliott Sprehn <[email protected]>
+
+ Add methods to CounterDirectives to clean up StyleBuilder and RenderCounter.
+ https://bugs.webkit.org/show_bug.cgi?id=94587
+
+ Reviewed by Julien Chaffraix.
+
+ Tests for bug 94642 exposing an unitialized read when using counter-reset and
+ counter-increment: inherit.
+
+ * fast/css/counters/counter-reset-inherit-bug-94642-expected.html: Added.
+ * fast/css/counters/counter-reset-inherit-bug-94642.html: Added.
+
2012-09-06 Joanmarie Diggs <[email protected]>
[Gtk] accessibility/canvas-description-and-role expected results needed
Added: trunk/LayoutTests/fast/css/counters/counter-reset-inherit-bug-94642-expected.html (0 => 127826)
--- trunk/LayoutTests/fast/css/counters/counter-reset-inherit-bug-94642-expected.html (rev 0)
+++ trunk/LayoutTests/fast/css/counters/counter-reset-inherit-bug-94642-expected.html 2012-09-07 05:22:09 UTC (rev 127826)
@@ -0,0 +1,3 @@
+<!doctype html>
+
+<div>0</div>
Added: trunk/LayoutTests/fast/css/counters/counter-reset-inherit-bug-94642.html (0 => 127826)
--- trunk/LayoutTests/fast/css/counters/counter-reset-inherit-bug-94642.html (rev 0)
+++ trunk/LayoutTests/fast/css/counters/counter-reset-inherit-bug-94642.html 2012-09-07 05:22:09 UTC (rev 127826)
@@ -0,0 +1,15 @@
+<!doctype html>
+
+<style>
+ body { counter-reset: x 0; }
+
+ div {
+ counter-increment: inherit;
+ }
+
+ div:before {
+ content: counter(x);
+ }
+</style>
+
+<div></div>
Modified: trunk/Source/WebCore/ChangeLog (127825 => 127826)
--- trunk/Source/WebCore/ChangeLog 2012-09-07 05:09:04 UTC (rev 127825)
+++ trunk/Source/WebCore/ChangeLog 2012-09-07 05:22:09 UTC (rev 127826)
@@ -1,3 +1,57 @@
+2012-09-06 Elliott Sprehn <[email protected]>
+
+ Add methods to CounterDirectives to clean up StyleBuilder and RenderCounter.
+ https://bugs.webkit.org/show_bug.cgi?id=94587
+
+ Reviewed by Julien Chaffraix.
+
+ Adds several methods to CounterDirectives and an accessor method to RenderStyle for
+ getting the CounterDirectives by idenfitier and uses those methods to clean up
+ the code in StyleBuilder and RenderCounter. This also switches to using AtomicString
+ directly instead of AtomicStringImpl and calling get() everywhere.
+
+ The refactor fixes the unitialized read in WKBug 94642.
+
+ Test: fast/css/counters/counter-reset-inherit-bug-94642.html
+
+ * css/CSSComputedStyleDeclaration.cpp:
+ (WebCore::counterToCSSValue): Use new accessors.
+ * css/StyleBuilder.cpp:
+ (WebCore::ApplyPropertyCounter::applyInheritValue): Use new inherit methods.
+ (WebCore::ApplyPropertyCounter::applyValue): Use new setters.
+ * rendering/RenderCounter.cpp:
+ (WebCore):
+ (WebCore::planCounter):
+ (WebCore::makeCounterNode):
+ (WebCore::destroyCounterNodeWithoutMapRemoval):
+ (WebCore::RenderCounter::destroyCounterNodes):
+ (WebCore::RenderCounter::destroyCounterNode):
+ (WebCore::updateCounters):
+ (WebCore::RenderCounter::rendererStyleChanged):
+ (showCounterRendererTree):
+ * rendering/style/CounterDirectives.cpp:
+ (WebCore::operator==):
+ * rendering/style/CounterDirectives.h: Added new accessors and switched to using AtomicString directly.
+ (CounterDirectives):
+ (WebCore::CounterDirectives::CounterDirectives):
+ (WebCore::CounterDirectives::isReset):
+ (WebCore::CounterDirectives::resetValue):
+ (WebCore::CounterDirectives::setResetValue):
+ (WebCore::CounterDirectives::clearReset):
+ (WebCore::CounterDirectives::inheritReset):
+ (WebCore::CounterDirectives::isIncrement):
+ (WebCore::CounterDirectives::incrementValue):
+ (WebCore::CounterDirectives::addIncrementValue):
+ (WebCore::CounterDirectives::clearIncrement):
+ (WebCore::CounterDirectives::inheritIncrement):
+ (WebCore::CounterDirectives::isDefined): If either reset or increment is used.
+ (WebCore::CounterDirectives::combinedValue): Combined local value of the counter.
+ (WebCore):
+ * rendering/style/RenderStyle.cpp:
+ (WebCore::RenderStyle::getCounterDirectives): New method which always returns a CounterDirectives instance by identifier.
+ (WebCore):
+ * rendering/style/RenderStyle.h:
+
2012-09-06 Joanmarie Diggs <[email protected]>
[Gtk] accessibility/canvas-description-and-role expected results needed
Modified: trunk/Source/WebCore/css/CSSComputedStyleDeclaration.cpp (127825 => 127826)
--- trunk/Source/WebCore/css/CSSComputedStyleDeclaration.cpp 2012-09-07 05:09:04 UTC (rev 127825)
+++ trunk/Source/WebCore/css/CSSComputedStyleDeclaration.cpp 2012-09-07 05:22:09 UTC (rev 127826)
@@ -1287,8 +1287,8 @@
RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated();
for (CounterDirectiveMap::const_iterator it = map->begin(); it != map->end(); ++it) {
- list->append(cssValuePool().createValue(it->first.get(), CSSPrimitiveValue::CSS_STRING));
- short number = propertyID == CSSPropertyCounterIncrement ? it->second.m_incrementValue : it->second.m_resetValue;
+ list->append(cssValuePool().createValue(it->first, CSSPrimitiveValue::CSS_STRING));
+ short number = propertyID == CSSPropertyCounterIncrement ? it->second.incrementValue() : it->second.resetValue();
list->append(cssValuePool().createValue((double)number, CSSPrimitiveValue::CSS_NUMBER));
}
return list.release();
Modified: trunk/Source/WebCore/css/StyleBuilder.cpp (127825 => 127826)
--- trunk/Source/WebCore/css/StyleBuilder.cpp 2012-09-07 05:09:04 UTC (rev 127825)
+++ trunk/Source/WebCore/css/StyleBuilder.cpp 2012-09-07 05:22:09 UTC (rev 127826)
@@ -1013,20 +1013,9 @@
for (Iterator it = parentMap.begin(); it != end; ++it) {
CounterDirectives& directives = map.add(it->first, CounterDirectives()).iterator->second;
if (counterBehavior == Reset) {
- directives.m_reset = it->second.m_reset;
- directives.m_resetValue = it->second.m_resetValue;
+ directives.inheritReset(it->second);
} else {
- // Inheriting a counter-increment means taking the parent's current value for the counter
- // and adding it to itself.
- directives.m_increment = it->second.m_increment;
- directives.m_incrementValue = 0;
- if (directives.m_increment) {
- float incrementValue = directives.m_incrementValue;
- directives.m_incrementValue = clampToInteger(incrementValue + it->second.m_incrementValue);
- } else {
- directives.m_increment = true;
- directives.m_incrementValue = it->second.m_incrementValue;
- }
+ directives.inheritIncrement(it->second);
}
}
}
@@ -1043,9 +1032,9 @@
Iterator end = map.end();
for (Iterator it = map.begin(); it != end; ++it)
if (counterBehavior == Reset)
- it->second.m_reset = false;
+ it->second.clearReset();
else
- it->second.m_increment = false;
+ it->second.clearIncrement();
int length = list ? list->length() : 0;
for (int i = 0; i < length; ++i) {
@@ -1059,20 +1048,12 @@
AtomicString identifier = static_cast<CSSPrimitiveValue*>(pair->first())->getStringValue();
int value = static_cast<CSSPrimitiveValue*>(pair->second())->getIntValue();
- CounterDirectives& directives = map.add(identifier.impl(), CounterDirectives()).iterator->second;
+ CounterDirectives& directives = map.add(identifier, CounterDirectives()).iterator->second;
if (counterBehavior == Reset) {
- directives.m_reset = true;
- directives.m_resetValue = value;
+ directives.setResetValue(value);
} else {
- if (directives.m_increment) {
- float incrementValue = directives.m_incrementValue;
- directives.m_incrementValue = clampToInteger(incrementValue + value);
- } else {
- directives.m_increment = true;
- directives.m_incrementValue = value;
- }
+ directives.addIncrementValue(value);
}
-
}
}
static PropertyHandler createHandler() { return PropertyHandler(&applyInheritValue, &emptyFunction, &applyValue); }
Modified: trunk/Source/WebCore/rendering/RenderCounter.cpp (127825 => 127826)
--- trunk/Source/WebCore/rendering/RenderCounter.cpp 2012-09-07 05:09:04 UTC (rev 127825)
+++ trunk/Source/WebCore/rendering/RenderCounter.cpp 2012-09-07 05:22:09 UTC (rev 127826)
@@ -41,7 +41,7 @@
using namespace HTMLNames;
-typedef HashMap<RefPtr<AtomicStringImpl>, RefPtr<CounterNode> > CounterMap;
+typedef HashMap<AtomicString, RefPtr<CounterNode> > CounterMap;
typedef HashMap<const RenderObject*, OwnPtr<CounterMap> > CounterMaps;
static CounterNode* makeCounterNode(RenderObject*, const AtomicString& identifier, bool alwaysCreateCounter);
@@ -242,20 +242,11 @@
return false; // Counters are forbidden from all other pseudo elements.
}
- if (const CounterDirectiveMap* directivesMap = style->counterDirectives()) {
- CounterDirectives directives = directivesMap->get(identifier.impl());
- if (directives.m_reset) {
- value = directives.m_resetValue;
- if (directives.m_increment)
- value += directives.m_incrementValue;
- isReset = true;
- return true;
- }
- if (directives.m_increment) {
- value = directives.m_incrementValue;
- isReset = false;
- return true;
- }
+ const CounterDirectives directives = style->getCounterDirectives(identifier);
+ if (directives.isDefined()) {
+ value = directives.combinedValue();
+ isReset = directives.isReset();
+ return true;
}
if (identifier == "list-item") {
@@ -421,7 +412,7 @@
if (object->hasCounterNodeMap()) {
if (CounterMap* nodeMap = counterMaps().get(object)) {
- if (CounterNode* node = nodeMap->get(identifier.impl()).get())
+ if (CounterNode* node = nodeMap->get(identifier).get())
return node;
}
}
@@ -444,7 +435,7 @@
counterMaps().set(object, adoptPtr(nodeMap));
object->setHasCounterNodeMap(true);
}
- nodeMap->set(identifier.impl(), newNode);
+ nodeMap->set(identifier, newNode);
if (newNode->parent())
return newNode.get();
// Checking if some nodes that were previously counter tree root nodes
@@ -456,7 +447,7 @@
skipDescendants = false;
if (!currentRenderer->hasCounterNodeMap())
continue;
- CounterNode* currentCounter = maps.get(currentRenderer)->get(identifier.impl()).get();
+ CounterNode* currentCounter = maps.get(currentRenderer)->get(identifier).get();
if (!currentCounter)
continue;
skipDescendants = true;
@@ -564,8 +555,8 @@
for (RefPtr<CounterNode> child = node->lastDescendant(); child && child != node; child = previous) {
previous = child->previousInPreOrder();
child->parent()->removeChild(child.get());
- ASSERT(counterMaps().get(child->owner())->get(identifier.impl()) == child);
- counterMaps().get(child->owner())->remove(identifier.impl());
+ ASSERT(counterMaps().get(child->owner())->get(identifier) == child);
+ counterMaps().get(child->owner())->remove(identifier);
}
if (CounterNode* parent = node->parent())
parent->removeChild(node);
@@ -580,8 +571,7 @@
CounterMap* map = mapsIterator->second.get();
CounterMap::const_iterator end = map->end();
for (CounterMap::const_iterator it = map->begin(); it != end; ++it) {
- AtomicString identifier(it->first.get());
- destroyCounterNodeWithoutMapRemoval(identifier, it->second.get());
+ destroyCounterNodeWithoutMapRemoval(it->first, it->second.get());
}
maps.remove(mapsIterator);
owner->setHasCounterNodeMap(false);
@@ -592,7 +582,7 @@
CounterMap* map = counterMaps().get(owner);
if (!map)
return;
- CounterMap::iterator mapIterator = map->find(identifier.impl());
+ CounterMap::iterator mapIterator = map->find(identifier);
if (mapIterator == map->end())
return;
destroyCounterNodeWithoutMapRemoval(identifier, mapIterator->second.get());
@@ -635,22 +625,22 @@
CounterDirectiveMap::const_iterator end = directiveMap->end();
if (!renderer->hasCounterNodeMap()) {
for (CounterDirectiveMap::const_iterator it = directiveMap->begin(); it != end; ++it)
- makeCounterNode(renderer, AtomicString(it->first.get()), false);
+ makeCounterNode(renderer, it->first, false);
return;
}
CounterMap* counterMap = counterMaps().get(renderer);
ASSERT(counterMap);
for (CounterDirectiveMap::const_iterator it = directiveMap->begin(); it != end; ++it) {
- RefPtr<CounterNode> node = counterMap->get(it->first.get());
+ RefPtr<CounterNode> node = counterMap->get(it->first);
if (!node) {
- makeCounterNode(renderer, AtomicString(it->first.get()), false);
+ makeCounterNode(renderer, it->first, false);
continue;
}
RefPtr<CounterNode> newParent = 0;
RefPtr<CounterNode> newPreviousSibling = 0;
- findPlaceForCounter(renderer, AtomicString(it->first.get()), node->hasResetType(), newParent, newPreviousSibling);
- if (node != counterMap->get(it->first.get()))
+ findPlaceForCounter(renderer, it->first, node->hasResetType(), newParent, newPreviousSibling);
+ if (node != counterMap->get(it->first))
continue;
CounterNode* parent = node->parent();
if (newParent == parent && newPreviousSibling == node->previousSibling())
@@ -658,7 +648,7 @@
if (parent)
parent->removeChild(node.get());
if (newParent)
- newParent->insertAfter(node.get(), newPreviousSibling.get(), it->first.get());
+ newParent->insertAfter(node.get(), newPreviousSibling.get(), it->first);
}
}
@@ -694,17 +684,17 @@
if (oldMapIt != oldMapEnd) {
if (oldMapIt->second == it->second)
continue;
- RenderCounter::destroyCounterNode(renderer, it->first.get());
+ RenderCounter::destroyCounterNode(renderer, it->first);
}
// We must create this node here, because the changed node may be a node with no display such as
// as those created by the increment or reset directives and the re-layout that will happen will
// not catch the change if the node had no children.
- makeCounterNode(renderer, it->first.get(), false);
+ makeCounterNode(renderer, it->first, false);
}
// Destroying old counters that do not exist in the new counterDirective map.
for (CounterDirectiveMap::const_iterator it = oldCounterDirectives->begin(); it !=oldMapEnd; ++it) {
if (!newCounterDirectives->contains(it->first))
- RenderCounter::destroyCounterNode(renderer, it->first.get());
+ RenderCounter::destroyCounterNode(renderer, it->first);
}
} else {
if (renderer->hasCounterNodeMap())
@@ -716,7 +706,7 @@
// We must create this node here, because the added node may be a node with no display such as
// as those created by the increment or reset directives and the re-layout that will happen will
// not catch the change if the node had no children.
- makeCounterNode(renderer, it->first.get(), false);
+ makeCounterNode(renderer, it->first, false);
}
}
}
@@ -741,7 +731,7 @@
fprintf(stderr, "%p N:%p P:%p PS:%p NS:%p C:%p\n",
current, current->node(), current->parent(), current->previousSibling(),
current->nextSibling(), current->hasCounterNodeMap() ?
- counterName ? WebCore::counterMaps().get(current)->get(identifier.impl()).get() : (WebCore::CounterNode*)1 : (WebCore::CounterNode*)0);
+ counterName ? WebCore::counterMaps().get(current)->get(identifier).get() : (WebCore::CounterNode*)1 : (WebCore::CounterNode*)0);
}
fflush(stderr);
}
Modified: trunk/Source/WebCore/rendering/style/CounterDirectives.cpp (127825 => 127826)
--- trunk/Source/WebCore/rendering/style/CounterDirectives.cpp 2012-09-07 05:09:04 UTC (rev 127825)
+++ trunk/Source/WebCore/rendering/style/CounterDirectives.cpp 2012-09-07 05:22:09 UTC (rev 127826)
@@ -27,13 +27,10 @@
bool operator==(const CounterDirectives& a, const CounterDirectives& b)
{
- if (a.m_reset != b.m_reset || a.m_increment != b.m_increment)
- return false;
- if (a.m_reset && a.m_resetValue != b.m_resetValue)
- return false;
- if (a.m_increment && a.m_incrementValue != b.m_incrementValue)
- return false;
- return true;
+ return a.isIncrement() == b.isIncrement()
+ && a.incrementValue() == b.incrementValue()
+ && a.isReset() == b.isReset()
+ && a.resetValue() == b.resetValue();
}
PassOwnPtr<CounterDirectiveMap> clone(const CounterDirectiveMap& counterDirectives)
Modified: trunk/Source/WebCore/rendering/style/CounterDirectives.h (127825 => 127826)
--- trunk/Source/WebCore/rendering/style/CounterDirectives.h 2012-09-07 05:09:04 UTC (rev 127825)
+++ trunk/Source/WebCore/rendering/style/CounterDirectives.h 2012-09-07 05:22:09 UTC (rev 127826)
@@ -26,20 +26,75 @@
#define CounterDirectives_h
#include <wtf/HashMap.h>
+#include <wtf/MathExtras.h>
#include <wtf/RefPtr.h>
-#include <wtf/text/AtomicStringImpl.h>
+#include <wtf/text/AtomicString.h>
+#include <wtf/text/AtomicStringHash.h>
namespace WebCore {
-struct CounterDirectives {
+class CounterDirectives {
+public:
CounterDirectives()
- : m_reset(false)
- , m_increment(false)
+ : m_isResetSet(false)
+ , m_isIncrementSet(false)
+ , m_resetValue(0)
+ , m_incrementValue(0)
{
}
- bool m_reset;
- bool m_increment;
+ // FIXME: The code duplication here could possibly be replaced by using two
+ // maps, or by using a container that held two generic Directive objects.
+
+ bool isReset() const { return m_isResetSet; }
+ int resetValue() const { return m_resetValue; }
+ void setResetValue(int value)
+ {
+ m_resetValue = value;
+ m_isResetSet = true;
+ }
+ void clearReset()
+ {
+ m_resetValue = 0;
+ m_isResetSet = false;
+ }
+ void inheritReset(CounterDirectives& parent)
+ {
+ m_resetValue = parent.m_resetValue;
+ m_isResetSet = parent.m_isResetSet;
+ }
+
+ bool isIncrement() const { return m_isIncrementSet; }
+ int incrementValue() const { return m_incrementValue; }
+ void addIncrementValue(int value)
+ {
+ m_incrementValue = clampToInteger((double)m_incrementValue + value);
+ m_isIncrementSet = true;
+ }
+ void clearIncrement()
+ {
+ m_incrementValue = 0;
+ m_isIncrementSet = false;
+ }
+ void inheritIncrement(CounterDirectives& parent)
+ {
+ m_incrementValue = parent.m_incrementValue;
+ m_isIncrementSet = parent.m_isIncrementSet;
+ }
+
+ bool isDefined() const { return isReset() || isIncrement(); }
+
+ int combinedValue() const
+ {
+ ASSERT(m_isResetSet || !m_resetValue);
+ ASSERT(m_isIncrementSet || !m_incrementValue);
+ // FIXME: Shouldn't allow overflow here.
+ return m_resetValue + m_incrementValue;
+ }
+
+private:
+ bool m_isResetSet;
+ bool m_isIncrementSet;
int m_resetValue;
int m_incrementValue;
};
@@ -47,7 +102,7 @@
bool operator==(const CounterDirectives&, const CounterDirectives&);
inline bool operator!=(const CounterDirectives& a, const CounterDirectives& b) { return !(a == b); }
-typedef HashMap<RefPtr<AtomicStringImpl>, CounterDirectives> CounterDirectiveMap;
+typedef HashMap<AtomicString, CounterDirectives> CounterDirectiveMap;
PassOwnPtr<CounterDirectiveMap> clone(const CounterDirectiveMap&);
Modified: trunk/Source/WebCore/rendering/style/RenderStyle.cpp (127825 => 127826)
--- trunk/Source/WebCore/rendering/style/RenderStyle.cpp 2012-09-07 05:09:04 UTC (rev 127825)
+++ trunk/Source/WebCore/rendering/style/RenderStyle.cpp 2012-09-07 05:22:09 UTC (rev 127826)
@@ -1036,6 +1036,13 @@
return *map;
}
+const CounterDirectives RenderStyle::getCounterDirectives(const AtomicString& identifier) const
+{
+ if (const CounterDirectiveMap* directives = counterDirectives())
+ return directives->get(identifier);
+ return CounterDirectives();
+}
+
const AtomicString& RenderStyle::hyphenString() const
{
ASSERT(hyphens() != HyphensNone);
Modified: trunk/Source/WebCore/rendering/style/RenderStyle.h (127825 => 127826)
--- trunk/Source/WebCore/rendering/style/RenderStyle.h 2012-09-07 05:09:04 UTC (rev 127825)
+++ trunk/Source/WebCore/rendering/style/RenderStyle.h 2012-09-07 05:22:09 UTC (rev 127826)
@@ -1494,6 +1494,7 @@
const CounterDirectiveMap* counterDirectives() const;
CounterDirectiveMap& accessCounterDirectives();
+ const CounterDirectives getCounterDirectives(const AtomicString& identifier) const;
QuotesData* quotes() const { return rareInheritedData->quotes.get(); }
void setQuotes(PassRefPtr<QuotesData>);
