Changeset 207471 in webkit

Timestamp:

Oct 18, 2016 10:41:14 AM (8 years ago)

Author:

Said Abou-Hallawa

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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/css/implicit-property-restore-expected.txt (added)
• LayoutTests/fast/css/implicit-property-restore.html (added)
• LayoutTests/fast/css/remove-shorthand-expected.txt (modified) (1 diff)
• Source/WTF/ChangeLog (modified) (1 diff)
• Source/WTF/wtf/TemporaryChange.h (modified) (2 diffs)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/css/parser/CSSParser.cpp (modified) (9 diffs)
• Source/WebCore/css/parser/SVGCSSParser.cpp (modified) (3 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2016-10-18  Said Abou-Hallawa  <sabouhallawa@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  <ryanhaddad@apple.com>
 

trunk/LayoutTests/fast/css/remove-shorthand-expected.txt

@@ -49 +49 @@
 and adds "".
 Removing list-style
-removes "list-style-type, list-style-position, list-style-image"
+removes "list-style"
 and adds "".
 Removing margin

trunk/Source/WTF/ChangeLog

@@ +1 @@
+2016-10-18  Said Abou-Hallawa  <sabouhallawa@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.fraser@apple.com>
 

trunk/Source/WTF/wtf/TemporaryChange.h

@@ -44 +44 @@
     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 +59 @@
         m_scopedVariable = m_originalValue;
     }
-
 
 private:

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2016-10-18  Said Abou-Hallawa  <sabouhallawa@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  <cdumez@apple.com>
 

trunk/Source/WebCore/css/parser/CSSParser.cpp

@@ -110 +110 @@
 #include <wtf/NeverDestroyed.h>
 #include <wtf/StdLibExtras.h>
+#include <wtf/TemporaryChange.h>
 #include <wtf/dtoa.h>
 #include <wtf/text/StringBuffer.h>
@@ -138 +139 @@
 
 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 {
@@ -2135 +2109 @@
             result = true;
         }
-        m_implicitShorthand = false;
         return result;
     }
@@ -3374 +3347 @@
 
     ShorthandScope scope(this, propId);
+    TemporaryChange<bool> change(m_implicitShorthand);
 
     bool parsedProperty[cMaxFillProperties] = { false };
@@ -3769 +3743 @@
     // 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) {
@@ -3806 +3780 @@
                 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);
@@ -3816 +3790 @@
                 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();
@@ -3826 +3800 @@
                 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;
@@ -8511 +8485 @@
         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);

trunk/Source/WebCore/css/parser/SVGCSSParser.cpp

@@ -31 +31 @@
 #include "RenderTheme.h"
 #include "SVGPaint.h"
+
+#include <wtf/TemporaryChange.h>
 
 namespace WebCore {
@@ -212 +214 @@
     {
         ShorthandScope scope(this, propId);
-        m_implicitShorthand = true;
+        TemporaryChange<bool> change(m_implicitShorthand, true);
         if (!parseValue(CSSPropertyMarkerStart, important))
             return false;
@@ -222 +224 @@
         addProperty(CSSPropertyMarkerMid, value, important);
         addProperty(CSSPropertyMarkerEnd, value, important);
-        m_implicitShorthand = false;
         return true;
     }