Title: [207471] trunk
Revision
207471
Author
s...@apple.com
Date
2016-10-18 10:41:14 -0700 (Tue, 18 Oct 2016)

Log Message

SVGCSSParser: m_implicitShorthand value is not reset after adding the shorthand property
https://bugs.webkit.org/show_bug.cgi?id=116470

Reviewed by Simon Fraser.

Source/WebCore:

When we encounter a shorthand css property, we set m_implicitShorthand
to true to tell addProperty() later that the individual properties are
all set through a short hand one. We need to make sure that setting 
m_implicitShorthand to true will not be leaked after finishing parsing
the short hand property.

Test: fast/css/implicit-property-restore.html

* css/parser/CSSParser.cpp:
(WebCore::CSSParser::parseValue):
(WebCore::CSSParser::parseFillShorthand):
(WebCore::CSSParser::parseShorthand):
(WebCore::CSSParser::parse4Values):
(WebCore::CSSParser::parseBorderRadius):
(WTF::ImplicitScope::ImplicitScope): Deleted.
(WTF::ImplicitScope::~ImplicitScope): Deleted.
Get rid of ImplicitScope and replace its calls by TemporaryChange<bool>.
        
* css/parser/SVGCSSParser.cpp:
(WebCore::CSSParser::parseSVGValue):
Restore m_implicitShorthand value after setting it temporarily to true.

Source/WTF:

* wtf/TemporaryChange.h:
(WTF::TemporaryChange::TemporaryChange):
Add a new constructor to make TemporaryChange work as a restorer. The
temporary change will happen after we construct the object.

LayoutTests:

* fast/css/implicit-property-restore-expected.txt: Added.
* fast/css/implicit-property-restore.html: Added.

* fast/css/remove-shorthand-expected.txt:
Rebase-line the test expected results because of fixing the leak of
m_implicitShorthand. The bug was happening because "background: ..." property
comes immediately before the "list-style: ...." property.

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (207470 => 207471)


--- trunk/LayoutTests/ChangeLog	2016-10-18 17:39:47 UTC (rev 207470)
+++ trunk/LayoutTests/ChangeLog	2016-10-18 17:41:14 UTC (rev 207471)
@@ -1,3 +1,18 @@
+2016-10-18  Said Abou-Hallawa  <sabouhall...@apple.com>
+
+        SVGCSSParser: m_implicitShorthand value is not reset after adding the shorthand property
+        https://bugs.webkit.org/show_bug.cgi?id=116470
+
+        Reviewed by Simon Fraser.
+
+        * fast/css/implicit-property-restore-expected.txt: Added.
+        * fast/css/implicit-property-restore.html: Added.
+
+        * fast/css/remove-shorthand-expected.txt:
+        Rebase-line the test expected results because of fixing the leak of
+        m_implicitShorthand. The bug was happening because "background: ..." property
+        comes immediately before the "list-style: ...." property.
+
 2016-10-18  Ryan Haddad  <ryanhad...@apple.com>
 
         Marking inspector/debugger/breakpoint-action-eval.html as a flaky timeout on mac-wk2.

Added: trunk/LayoutTests/fast/css/implicit-property-restore-expected.txt (0 => 207471)


--- trunk/LayoutTests/fast/css/implicit-property-restore-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/fast/css/implicit-property-restore-expected.txt	2016-10-18 17:41:14 UTC (rev 207471)
@@ -0,0 +1,12 @@
+This test verifies the property implicit flag is reset after adding any css property.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS element1.style.isPropertyImplicit('padding-top') is false
+PASS removedProperties.length is 1
+PASS removedProperties[0] is "list-style"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+

Added: trunk/LayoutTests/fast/css/implicit-property-restore.html (0 => 207471)


--- trunk/LayoutTests/fast/css/implicit-property-restore.html	                        (rev 0)
+++ trunk/LayoutTests/fast/css/implicit-property-restore.html	2016-10-18 17:41:14 UTC (rev 207471)
@@ -0,0 +1,33 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <script src=""
+</head>
+<body>
+    <div id="element-1" style="background:url() 100% 100% repeat-y; padding-top: 0px;"></div>
+    <div id="element-2" style="background:url() 100% 100% repeat-y; list-style:square outside url();"></div>
+    <script>
+        description("This test verifies the property implicit flag is reset after adding any css property.");
+        
+        var element1 = document.getElementById("element-1");
+        var element2 = document.getElementById("element-2");
+        
+        shouldBe("element1.style.isPropertyImplicit('padding-top')", "false");
+        
+        var allProperties = element2.style.cssText.concat(" ").split("; ");
+        element2.style.removeProperty("list-style");
+        
+        var remainingProperties = element2.style.cssText.concat(" ").split("; ");
+        var removedProperties = [];
+        
+        for (i in allProperties) {
+            if (remainingProperties.indexOf(allProperties[i]) < 0)
+                removedProperties.push(allProperties[i].replace(/\:.*/,""));
+        }
+
+        shouldBe("removedProperties.length", "1");
+        shouldBeEqualToString("removedProperties[0]", "list-style");
+    </script>
+    <script src=""
+</body>
+</html>

Modified: trunk/LayoutTests/fast/css/remove-shorthand-expected.txt (207470 => 207471)


--- trunk/LayoutTests/fast/css/remove-shorthand-expected.txt	2016-10-18 17:39:47 UTC (rev 207470)
+++ trunk/LayoutTests/fast/css/remove-shorthand-expected.txt	2016-10-18 17:41:14 UTC (rev 207471)
@@ -48,7 +48,7 @@
 removes "column-rule-width, column-rule-style, column-rule-color"
 and adds "".
 Removing list-style
-removes "list-style-type, list-style-position, list-style-image"
+removes "list-style"
 and adds "".
 Removing margin
 removes "margin"

Modified: trunk/Source/WTF/ChangeLog (207470 => 207471)


--- trunk/Source/WTF/ChangeLog	2016-10-18 17:39:47 UTC (rev 207470)
+++ trunk/Source/WTF/ChangeLog	2016-10-18 17:41:14 UTC (rev 207471)
@@ -1,3 +1,15 @@
+2016-10-18  Said Abou-Hallawa  <sabouhall...@apple.com>
+
+        SVGCSSParser: m_implicitShorthand value is not reset after adding the shorthand property
+        https://bugs.webkit.org/show_bug.cgi?id=116470
+
+        Reviewed by Simon Fraser.
+
+        * wtf/TemporaryChange.h:
+        (WTF::TemporaryChange::TemporaryChange):
+        Add a new constructor to make TemporaryChange work as a restorer. The
+        temporary change will happen after we construct the object.
+
 2016-10-17  Simon Fraser  <simon.fra...@apple.com>
 
         Implement DOMRect/DOMRectReadOnly

Modified: trunk/Source/WTF/wtf/TemporaryChange.h (207470 => 207471)


--- trunk/Source/WTF/wtf/TemporaryChange.h	2016-10-18 17:39:47 UTC (rev 207470)
+++ trunk/Source/WTF/wtf/TemporaryChange.h	2016-10-18 17:41:14 UTC (rev 207471)
@@ -43,10 +43,14 @@
 class TemporaryChange {
     WTF_MAKE_NONCOPYABLE(TemporaryChange);
 public:
-    TemporaryChange(T& scopedVariable, T newValue)
+    TemporaryChange(T& scopedVariable)
         : m_scopedVariable(scopedVariable)
         , m_originalValue(scopedVariable)
     {
+    }
+    TemporaryChange(T& scopedVariable, T newValue)
+        : TemporaryChange(scopedVariable)
+    {
         m_scopedVariable = newValue;
     }
 
@@ -55,7 +59,6 @@
         m_scopedVariable = m_originalValue;
     }
 
-
 private:
     T& m_scopedVariable;
     T m_originalValue;

Modified: trunk/Source/WebCore/ChangeLog (207470 => 207471)


--- trunk/Source/WebCore/ChangeLog	2016-10-18 17:39:47 UTC (rev 207470)
+++ trunk/Source/WebCore/ChangeLog	2016-10-18 17:41:14 UTC (rev 207471)
@@ -1,3 +1,32 @@
+2016-10-18  Said Abou-Hallawa  <sabouhall...@apple.com>
+
+        SVGCSSParser: m_implicitShorthand value is not reset after adding the shorthand property
+        https://bugs.webkit.org/show_bug.cgi?id=116470
+
+        Reviewed by Simon Fraser.
+
+        When we encounter a shorthand css property, we set m_implicitShorthand
+        to true to tell addProperty() later that the individual properties are
+        all set through a short hand one. We need to make sure that setting 
+        m_implicitShorthand to true will not be leaked after finishing parsing
+        the short hand property.
+
+        Test: fast/css/implicit-property-restore.html
+
+        * css/parser/CSSParser.cpp:
+        (WebCore::CSSParser::parseValue):
+        (WebCore::CSSParser::parseFillShorthand):
+        (WebCore::CSSParser::parseShorthand):
+        (WebCore::CSSParser::parse4Values):
+        (WebCore::CSSParser::parseBorderRadius):
+        (WTF::ImplicitScope::ImplicitScope): Deleted.
+        (WTF::ImplicitScope::~ImplicitScope): Deleted.
+        Get rid of ImplicitScope and replace its calls by TemporaryChange<bool>.
+        
+        * css/parser/SVGCSSParser.cpp:
+        (WebCore::CSSParser::parseSVGValue):
+        Restore m_implicitShorthand value after setting it temporarily to true.
+
 2016-10-18  Chris Dumez  <cdu...@apple.com>
 
         Update TrackEvent to stop using legacy [ConstructorTemplate=Event]

Modified: trunk/Source/WebCore/css/parser/CSSParser.cpp (207470 => 207471)


--- trunk/Source/WebCore/css/parser/CSSParser.cpp	2016-10-18 17:39:47 UTC (rev 207470)
+++ trunk/Source/WebCore/css/parser/CSSParser.cpp	2016-10-18 17:41:14 UTC (rev 207471)
@@ -109,6 +109,7 @@
 #include <wtf/HexNumber.h>
 #include <wtf/NeverDestroyed.h>
 #include <wtf/StdLibExtras.h>
+#include <wtf/TemporaryChange.h>
 #include <wtf/dtoa.h>
 #include <wtf/text/StringBuffer.h>
 #include <wtf/text/StringBuilder.h>
@@ -138,33 +139,6 @@
 
 using namespace WTF;
 
-namespace {
-
-enum PropertyType {
-    PropertyExplicit,
-    PropertyImplicit
-};
-
-class ImplicitScope {
-    WTF_MAKE_NONCOPYABLE(ImplicitScope);
-public:
-    ImplicitScope(WebCore::CSSParser& parser, PropertyType propertyType)
-        : m_parser(parser)
-    {
-        m_parser.m_implicitShorthand = propertyType == PropertyImplicit;
-    }
-
-    ~ImplicitScope()
-    {
-        m_parser.m_implicitShorthand = false;
-    }
-
-private:
-    WebCore::CSSParser& m_parser;
-};
-
-} // namespace
-
 namespace WebCore {
 
 const unsigned CSSParser::invalidParsedPropertiesCount = std::numeric_limits<unsigned>::max();
@@ -2134,7 +2108,6 @@
                 addProperty(propId2, val2.releaseNonNull(), important);
             result = true;
         }
-        m_implicitShorthand = false;
         return result;
     }
     case CSSPropertyListStyleImage:     // <uri> | none | inherit
@@ -3373,6 +3346,7 @@
         return false;
 
     ShorthandScope scope(this, propId);
+    TemporaryChange<bool> change(m_implicitShorthand);
 
     bool parsedProperty[cMaxFillProperties] = { false };
     RefPtr<CSSValue> values[cMaxFillProperties];
@@ -3768,7 +3742,7 @@
 
     // Fill in any remaining properties with the initial value.
     auto& cssValuePool = CSSValuePool::singleton();
-    ImplicitScope implicitScope(*this, PropertyImplicit);
+    TemporaryChange<bool> change(m_implicitShorthand, true);
     const StylePropertyShorthand* propertiesForInitialization = shorthand.propertiesForInitialization();
     for (unsigned i = 0; i < shorthand.length(); ++i) {
         if (propertyFound[i])
@@ -3805,7 +3779,7 @@
             if (!parseValue(properties[0], important))
                 return false;
             CSSValue* value = m_parsedProperties.last().value();
-            ImplicitScope implicitScope(*this, PropertyImplicit);
+            TemporaryChange<bool> change(m_implicitShorthand, true);
             addProperty(properties[1], value, important);
             addProperty(properties[2], value, important);
             addProperty(properties[3], value, important);
@@ -3815,7 +3789,7 @@
             if (!parseValue(properties[0], important) || !parseValue(properties[1], important))
                 return false;
             CSSValue* value = m_parsedProperties[m_parsedProperties.size() - 2].value();
-            ImplicitScope implicitScope(*this, PropertyImplicit);
+            TemporaryChange<bool> change(m_implicitShorthand, true);
             addProperty(properties[2], value, important);
             value = m_parsedProperties[m_parsedProperties.size() - 2].value();
             addProperty(properties[3], value, important);
@@ -3825,7 +3799,7 @@
             if (!parseValue(properties[0], important) || !parseValue(properties[1], important) || !parseValue(properties[2], important))
                 return false;
             CSSValue* value = m_parsedProperties[m_parsedProperties.size() - 2].value();
-            ImplicitScope implicitScope(*this, PropertyImplicit);
+            TemporaryChange<bool> change(m_implicitShorthand, true);
             addProperty(properties[3], value, important);
             break;
         }
@@ -8510,7 +8484,7 @@
     } else
         completeBorderRadii(radii[1]);
 
-    ImplicitScope implicitScope(*this, PropertyImplicit);
+    TemporaryChange<bool> change(m_implicitShorthand, true);
     addProperty(CSSPropertyBorderTopLeftRadius, createPrimitiveValuePair(WTFMove(radii[0][0]), WTFMove(radii[1][0])), important);
     addProperty(CSSPropertyBorderTopRightRadius, createPrimitiveValuePair(WTFMove(radii[0][1]), WTFMove(radii[1][1])), important);
     addProperty(CSSPropertyBorderBottomRightRadius, createPrimitiveValuePair(WTFMove(radii[0][2]), WTFMove(radii[1][2])), important);

Modified: trunk/Source/WebCore/css/parser/SVGCSSParser.cpp (207470 => 207471)


--- trunk/Source/WebCore/css/parser/SVGCSSParser.cpp	2016-10-18 17:39:47 UTC (rev 207470)
+++ trunk/Source/WebCore/css/parser/SVGCSSParser.cpp	2016-10-18 17:41:14 UTC (rev 207471)
@@ -31,6 +31,8 @@
 #include "RenderTheme.h"
 #include "SVGPaint.h"
 
+#include <wtf/TemporaryChange.h>
+
 namespace WebCore {
 
 static bool isValidSystemControlColorValue(CSSValueID id)
@@ -211,7 +213,7 @@
     case CSSPropertyMarker:
     {
         ShorthandScope scope(this, propId);
-        m_implicitShorthand = true;
+        TemporaryChange<bool> change(m_implicitShorthand, true);
         if (!parseValue(CSSPropertyMarkerStart, important))
             return false;
         if (m_valueList->current()) {
@@ -221,7 +223,6 @@
         CSSValue* value = m_parsedProperties.last().value();
         addProperty(CSSPropertyMarkerMid, value, important);
         addProperty(CSSPropertyMarkerEnd, value, important);
-        m_implicitShorthand = false;
         return true;
     }
     case CSSPropertyCx:
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to