Heap-use-after-free in WTF::HashMap<int, WTF::RefPtr<WebCore::CalculationValue>,...
authormikelawther@chromium.org <mikelawther@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 14 May 2012 03:30:46 +0000 (03:30 +0000)
committermikelawther@chromium.org <mikelawther@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 14 May 2012 03:30:46 +0000 (03:30 +0000)
https://bugs.webkit.org/show_bug.cgi?id=85195

Source/WebCore:

This bug was caused by Length not understanding that calc expressions shouldn't be
blended - a Length with a calc expression handle was created without incrementing
the ref count of the expression. Length no longer attempts to blend calc expressions,
http://webkit.org/b/86160 has been filed to track expression blending. Fixing this fixed
the crash.

Once this was fixed, the RenderStyle diff checker thought the style was changing,
as Length didn't know how to compare calc expressions, resulting in an infinite
loop of style recalcs. Expressions can now compare themselves.

Reviewed by Darin Adler.

Tests: css3/calc/transition-crash.html
       css3/calc/transition-crash2.html

* platform/CalculationValue.h:
(WebCore::CalcExpressionNode::CalcExpressionNode):
(CalcExpressionNode):
(WebCore::CalcExpressionNode::type):
(CalculationValue):
(WebCore::CalculationValue::operator==):
(WebCore::CalcExpressionNumber::CalcExpressionNumber):
(WebCore::CalcExpressionNumber::operator==):
(CalcExpressionNumber):
(WebCore::CalcExpressionLength::CalcExpressionLength):
(WebCore::CalcExpressionLength::operator==):
(CalcExpressionLength):
(WebCore::CalcExpressionBinaryOperation::CalcExpressionBinaryOperation):
(WebCore::CalcExpressionBinaryOperation::operator==):
(CalcExpressionBinaryOperation):
* platform/Length.cpp:
(WebCore::Length::isCalculatedEqual):
(WebCore):
* platform/Length.h:
(WebCore::Length::operator==):
(Length):
(WebCore::Length::blend):

LayoutTests:

Reviewed by Darin Adler.

* css3/calc/transition-crash-expected.txt: Added.
* css3/calc/transition-crash.html: Added.
* css3/calc/transition-crash2-expected.txt: Added.
* css3/calc/transition-crash2.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/css3/calc/transition-crash-expected.txt [new file with mode: 0644]
LayoutTests/css3/calc/transition-crash.html [new file with mode: 0644]
LayoutTests/css3/calc/transition-crash2-expected.txt [new file with mode: 0644]
LayoutTests/css3/calc/transition-crash2.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/platform/CalculationValue.h
Source/WebCore/platform/Length.cpp
Source/WebCore/platform/Length.h

index b37a105..61590fc 100644 (file)
@@ -1,3 +1,15 @@
+2012-05-13  Mike Lawther  <mikelawther@chromium.org>
+
+        Heap-use-after-free in WTF::HashMap<int, WTF::RefPtr<WebCore::CalculationValue>, WTF::IntHash<unsigned int>, WTF::HashTrait
+        https://bugs.webkit.org/show_bug.cgi?id=85195
+
+        Reviewed by Darin Adler.
+
+        * css3/calc/transition-crash-expected.txt: Added.
+        * css3/calc/transition-crash.html: Added.
+        * css3/calc/transition-crash2-expected.txt: Added.
+        * css3/calc/transition-crash2.html: Added.
+
 2012-05-13  Philippe Normand  <pnormand@igalia.com>
 
         Unreviewed GTK test_expectations update.
diff --git a/LayoutTests/css3/calc/transition-crash-expected.txt b/LayoutTests/css3/calc/transition-crash-expected.txt
new file mode 100644 (file)
index 0000000..d60a471
--- /dev/null
@@ -0,0 +1,2 @@
+This test checks class changes affecting sibling selectors happening during transitions over calculated lengths. The test passes if it does not crash.
+
diff --git a/LayoutTests/css3/calc/transition-crash.html b/LayoutTests/css3/calc/transition-crash.html
new file mode 100644 (file)
index 0000000..e94e854
--- /dev/null
@@ -0,0 +1,36 @@
+<!DOCTYPE html>
+<style>
+.transition { 
+    height: -webkit-calc(100% - 10px); 
+    -webkit-transition: height 50ms; 
+}
+.flim + .sibling { 
+}
+</style>
+
+<body>
+    This test checks class changes affecting sibling selectors happening during transitions over calculated lengths.
+    The test passes if it does not crash.
+</body>
+
+<script>
+    if (window.layoutTestController) {
+        layoutTestController.dumpAsText(true);        
+        layoutTestController.waitUntilDone(); 
+    }
+
+    div = document.createElement('div');
+    div.setAttribute('class', 'sibling');
+    document.body.appendChild(div);
+
+    th = document.createElement('th'); // td, tr also cause test to fail
+    th.setAttribute('class', 'transition');
+    document.body.appendChild(th);
+
+    function boom() {
+        div.setAttribute('class', 'stix'); 
+        if (window.layoutTestController) 
+            layoutTestController.notifyDone();
+    }
+    setTimeout(boom, 1);
+</script>
diff --git a/LayoutTests/css3/calc/transition-crash2-expected.txt b/LayoutTests/css3/calc/transition-crash2-expected.txt
new file mode 100644 (file)
index 0000000..2ee1d4d
--- /dev/null
@@ -0,0 +1,2 @@
+This tests transitioning of elements containing a calc expression. The test passes if it does not crash.
+
diff --git a/LayoutTests/css3/calc/transition-crash2.html b/LayoutTests/css3/calc/transition-crash2.html
new file mode 100644 (file)
index 0000000..898c15e
--- /dev/null
@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+This tests transitioning of elements containing a calc expression. The test passes if it does not crash.
+<div>
+    <div style="width: -webkit-calc(-100px + 100%);"></div>
+</div>
+<script>
+    if (window.layoutTestController) {
+        layoutTestController.dumpAsText(true);        
+        layoutTestController.waitUntilDone(); 
+    }
+
+    function boom() {
+        head = document.getElementsByTagName("head")[0];
+        var style = document.createElement("style");
+        style.innerHTML=":last-child     {-webkit-transition-duration:.1s;}";
+        head.appendChild(style);
+        if (window.layoutTestController) 
+            layoutTestController.notifyDone();
+    }
+
+    setTimeout(boom, 10);
+</script>
\ No newline at end of file
index 98599c8..771c332 100644 (file)
@@ -1,3 +1,46 @@
+2012-05-13  Mike Lawther  <mikelawther@chromium.org>
+
+        Heap-use-after-free in WTF::HashMap<int, WTF::RefPtr<WebCore::CalculationValue>, WTF::IntHash<unsigned int>, WTF::HashTrait
+        https://bugs.webkit.org/show_bug.cgi?id=85195
+
+        This bug was caused by Length not understanding that calc expressions shouldn't be 
+        blended - a Length with a calc expression handle was created without incrementing
+        the ref count of the expression. Length no longer attempts to blend calc expressions,
+        http://webkit.org/b/86160 has been filed to track expression blending. Fixing this fixed
+        the crash.
+
+        Once this was fixed, the RenderStyle diff checker thought the style was changing,
+        as Length didn't know how to compare calc expressions, resulting in an infinite
+        loop of style recalcs. Expressions can now compare themselves.
+
+        Reviewed by Darin Adler.
+
+        Tests: css3/calc/transition-crash.html
+               css3/calc/transition-crash2.html
+
+        * platform/CalculationValue.h:
+        (WebCore::CalcExpressionNode::CalcExpressionNode):
+        (CalcExpressionNode):
+        (WebCore::CalcExpressionNode::type):
+        (CalculationValue):
+        (WebCore::CalculationValue::operator==):
+        (WebCore::CalcExpressionNumber::CalcExpressionNumber):
+        (WebCore::CalcExpressionNumber::operator==):
+        (CalcExpressionNumber):
+        (WebCore::CalcExpressionLength::CalcExpressionLength):
+        (WebCore::CalcExpressionLength::operator==):
+        (CalcExpressionLength):
+        (WebCore::CalcExpressionBinaryOperation::CalcExpressionBinaryOperation):
+        (WebCore::CalcExpressionBinaryOperation::operator==):
+        (CalcExpressionBinaryOperation):
+        * platform/Length.cpp:
+        (WebCore::Length::isCalculatedEqual):
+        (WebCore):
+        * platform/Length.h:
+        (WebCore::Length::operator==):
+        (Length):
+        (WebCore::Length::blend):
+
 2012-05-13  Darin Adler  <darin@apple.com>
 
         Roll out local changes accidentally landed in r116905.
index 9339b10..9be00e9 100755 (executable)
@@ -51,20 +51,43 @@ enum CalculationPermittedValueRange {
     CalculationRangeAll,
     CalculationRangeNonNegative
 };
+
+enum CalcExpressionNodeType {
+    CalcExpressionNodeUndefined,
+    CalcExpressionNodeNumber,
+    CalcExpressionNodeLength,
+    CalcExpressionNodeBinaryOperation
+};
         
 class CalcExpressionNode {
 public:
+    CalcExpressionNode()
+        : m_type(CalcExpressionNodeUndefined)
+    {
+    }
+    
     virtual ~CalcExpressionNode()
     {
     }
     
     virtual float evaluate(float maxValue) const = 0;
+    virtual bool operator==(const CalcExpressionNode&) const = 0;
+
+    CalcExpressionNodeType type() const { return m_type; }
+    
+protected:
+    CalcExpressionNodeType m_type;
 };
     
 class CalculationValue : public RefCounted<CalculationValue> {
 public:
     static PassRefPtr<CalculationValue> create(PassOwnPtr<CalcExpressionNode> value, CalculationPermittedValueRange);
     float evaluate(float maxValue) const;
+
+    bool operator==(const CalculationValue& o) const 
+    { 
+        return m_value == o.m_value || *(m_value.get()) == *(o.m_value.get());
+    }
     
 private:
     CalculationValue(PassOwnPtr<CalcExpressionNode> value, CalculationPermittedValueRange range)
@@ -82,8 +105,19 @@ public:
     explicit CalcExpressionNumber(float value)
         : m_value(value)
     {
+        m_type = CalcExpressionNodeNumber;
     }
 
+    bool operator==(const CalcExpressionNumber& o) const
+    {
+        return m_value == o.m_value;
+    }
+
+    virtual bool operator==(const CalcExpressionNode& o) const
+    {
+        return type() == o.type() && *this == static_cast<const CalcExpressionNumber&>(o);
+    }
+    
     virtual float evaluate(float) const 
     {
         return m_value;
@@ -98,8 +132,19 @@ public:
     explicit CalcExpressionLength(Length length)
         : m_length(length)
     {
+        m_type = CalcExpressionNodeLength;
     }
 
+    bool operator==(const CalcExpressionLength& o) const
+    {
+        return m_length == o.m_length;
+    }
+    
+    virtual bool operator==(const CalcExpressionNode& o) const
+    {
+        return type() == o.type() && *this == static_cast<const CalcExpressionLength&>(o);
+    }
+    
     virtual float evaluate(float maxValue) const
     {
         return floatValueForLength(m_length, maxValue);
@@ -116,8 +161,20 @@ public:
         , m_rightSide(rightSide)
         , m_operator(op)
     {
+        m_type = CalcExpressionNodeBinaryOperation;
     }
 
+    bool operator==(const CalcExpressionBinaryOperation& o) const
+    {
+        return m_operator == o.m_operator && *m_leftSide == *o.m_leftSide && *m_rightSide == *o.m_rightSide;
+    }
+
+    virtual bool operator==(const CalcExpressionNode& o) const
+    {
+        return type() == o.type() && *this == static_cast<const CalcExpressionBinaryOperation&>(o);
+    }
+    
+    
     virtual float evaluate(float) const;
 
 private:
index 5166c0d..3811d4d 100644 (file)
@@ -232,6 +232,11 @@ float Length::nonNanCalculatedValue(int maxValue) const
     return result;
 }
 
+bool Length::isCalculatedEqual(const Length& o) const
+{
+    return isCalculated() && (calculationValue() == o.calculationValue() || *calculationValue() == *o.calculationValue());
+}
+
 class SameSizeAsLength {
     int32_t value;
     int32_t metaData;
index 0c963dd..e6cc02d 100644 (file)
@@ -49,21 +49,25 @@ public:
     Length(LengthType t)
         : m_intValue(0), m_quirk(false), m_type(t), m_isFloat(false)
     {
+        ASSERT(t != Calculated);
     }
 
     Length(int v, LengthType t, bool q = false)
         : m_intValue(v), m_quirk(q), m_type(t), m_isFloat(false)
     {
+        ASSERT(t != Calculated);
     }
     
     Length(FractionalLayoutUnit v, LengthType t, bool q = false)
         : m_floatValue(v.toFloat()), m_quirk(q), m_type(t), m_isFloat(true)
     {
+        ASSERT(t != Calculated);
     }
     
     Length(float v, LengthType t, bool q = false)
-    : m_floatValue(v), m_quirk(q), m_type(t), m_isFloat(true)
+        : m_floatValue(v), m_quirk(q), m_type(t), m_isFloat(true)
     {
+        ASSERT(t != Calculated);
     }
 
     Length(double v, LengthType t, bool q = false)
@@ -91,7 +95,7 @@ public:
             decrementCalculatedRef();
     }  
     
-    bool operator==(const Length& o) const { return (m_type == o.m_type) && (m_quirk == o.m_quirk) && (isUndefined() || (getFloatValue() == o.getFloatValue())); }
+    bool operator==(const Length& o) const { return (m_type == o.m_type) && (m_quirk == o.m_quirk) && (isUndefined() || (getFloatValue() == o.getFloatValue()) || isCalculatedEqual(o)); }
     bool operator!=(const Length& o) const { return !(*this == o); }
 
     const Length& operator*=(float v)
@@ -212,6 +216,7 @@ public:
     bool isIntrinsicOrAuto() const { return type() == Auto || type() == MinIntrinsic || type() == Intrinsic; }
     bool isSpecified() const { return type() == Fixed || type() == Percent || type() == Calculated || isViewportPercentage(); }
     bool isCalculated() const { return type() == Calculated; }
+    bool isCalculatedEqual(const Length&) const;
 
     Length blend(const Length& from, double progress) const
     {
@@ -221,6 +226,10 @@ public:
 
         if (from.isZero() && isZero())
             return *this;
+
+        // FIXME http://webkit.org/b/86160 - Blending doesn't work with calculated expressions
+        if (type() == Calculated)
+            return *this;
         
         LengthType resultType = type();
         if (isZero())