Bug 168586 – Make all the SVGElements' attributes be RefCounted objects

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: SVG

[Said Abou-Hallawa]

This is a step towards changing the raw pointers which is held by SVGAnimatedType and the SVGAnimatedPropertyTearOff object to be RefCounted pointers.

[Said Abou-Hallawa]

Created attachment 302119 [details]
Patch

[Sam Weinig]

Comment on attachment 302119 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302119&action=review

> Source/WebCore/svg/SVGAnimatedType.h:33
> +        return std::unique_ptr<SVGAnimatedType>(new SVGAnimatedType(SVGPropertyTraits<PropertyType>::initialValue()));

Can this use make_unique to avoid the redundant type name.

> Source/WebCore/svg/SVGAnimatedType.h:39
> +        return std::unique_ptr<SVGAnimatedType>(new SVGAnimatedType(WTFMove(property)));

make_unique?

> Source/WebCore/svg/SVGAnimatedType.h:50
> +        return std::unique_ptr<SVGAnimatedType>(new SVGAnimatedType(SVGPropertyTraits<std::pair<PropertyType1, PropertyType2>>::initialValue()));

make_unique? (I'll stop repeating now).

> Source/WebCore/svg/SVGAnimatedType.h:79
> +        return std::pair<PropertyType1&, PropertyType2&>(m_values[0]->property<PropertyType1>(), m_values[1]->property<PropertyType2>());

Would be good to have some assertion that m_values[1] is not null.

> Source/WebCore/svg/SVGAnimatedType.h:112
> +    RefPtr<SVGAnimatedValue> m_values[2];

What's the value of making this an array rather than just m_value1, and m_value2?

> Source/WebCore/svg/SVGAnimatedValue.h:41
> +#include "SVGTransformListValues.h"
> +
> +#include <wtf/Variant.h>

No reason to separate these.

> Source/WebCore/svg/SVGAnimatedValue.h:64
> +    public WTF::RefCounted<SVGAnimatedValue>,

WTF:: not needed. Should all be on one line.

> Source/WebCore/svg/SVGAnimatedValue.h:68
> +    static RefPtr<SVGAnimatedValue> create(const PropertyType& value)

Can this return a Ref<>?

> Source/WebCore/svg/SVGAnimatedValue.h:74
> +    static RefPtr<SVGAnimatedValue> create(PropertyType&& value)

Ditto.

> Source/WebCore/svg/SVGAnimatedValue.h:80
> +    static RefPtr<SVGAnimatedValue> create(const SVGAnimatedValue& value)

Ditto.

> Source/WebCore/svg/SVGAnimatedValue.h:86
> +    template<typename PropertyType>
> +    bool holdsAlternative() const

Despite it being the name chosen by the variant class, I don't think holdsAlternative is that great a name.  Perhaps just good old is<>?

> Source/WebCore/svg/SVGPathByteStream.h:26
> +#include "SVGPropertyTraits.h"
> +
>  #include <wtf/Noncopyable.h>

No need to separate these.

> Source/WebCore/svg/SVGPathByteStream.h:49
> +class SVGPathByteStream final : public Vector<unsigned char, 1> {
>      WTF_MAKE_FAST_ALLOCATED;
>  public:

I'm generally not a fan of using inheritance for things like this, as it tends to expose much more API surface area than necessary.  I'd prefer to see this as kept as composition.

> Source/WebCore/svg/properties/SVGPropertyTraits.h:29
> +#include "SVGParserUtilities.h"
> +
> +#include <wtf/text/StringBuilder.h>

No need to separate these.

> Source/WebCore/svg/properties/SVGPropertyTraits.h:78
> +        return std::pair<int, int>(static_cast<int>(roundf(firstNumber)), static_cast<int>(roundf(secondNumber)));

Will make_pair work here?

> Source/WebCore/svg/properties/SVGPropertyTraits.h:146
> +    static bool parse(String& type, const QualifiedName&, const String& string)
> +    {
> +        type = string;
> +        return true;
> +    }

These parse methods should be converted to return optional<Foo> at some point.

[Said Abou-Hallawa]

Created attachment 302319 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 302119 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302119&action=review

>> Source/WebCore/svg/SVGAnimatedType.h:33
>> +        return std::unique_ptr<SVGAnimatedType>(new SVGAnimatedType(SVGPropertyTraits<PropertyType>::initialValue()));
> 
> Can this use make_unique to avoid the redundant type name.

Done.

What made me use unique_ptr is I wanted to have the constructors to be private. make_unique requires the constructor to be public or it is declared to be a friend to the class. Since there are many create() functions it was not worthy adding these friend functions.

>> Source/WebCore/svg/SVGAnimatedType.h:39
>> +        return std::unique_ptr<SVGAnimatedType>(new SVGAnimatedType(WTFMove(property)));
> 
> make_unique?

Done.

>> Source/WebCore/svg/SVGAnimatedType.h:50
>> +        return std::unique_ptr<SVGAnimatedType>(new SVGAnimatedType(SVGPropertyTraits<std::pair<PropertyType1, PropertyType2>>::initialValue()));
> 
> make_unique? (I'll stop repeating now).

Done. All the constructors were changed to be public and all the create() functions were changed to use make_unique.

>> Source/WebCore/svg/SVGAnimatedType.h:79
>> +        return std::pair<PropertyType1&, PropertyType2&>(m_values[0]->property<PropertyType1>(), m_values[1]->property<PropertyType2>());
> 
> Would be good to have some assertion that m_values[1] is not null.

Done.

>> Source/WebCore/svg/SVGAnimatedType.h:112
>> +    RefPtr<SVGAnimatedValue> m_values[2];
> 
> What's the value of making this an array rather than just m_value1, and m_value2?

Done.

>> Source/WebCore/svg/SVGAnimatedValue.h:41
>> +#include <wtf/Variant.h>
> 
> No reason to separate these.

Empty line was removed.

>> Source/WebCore/svg/SVGAnimatedValue.h:64
>> +    public WTF::RefCounted<SVGAnimatedValue>,
> 
> WTF:: not needed. Should all be on one line.

WTF:: was removed and all were made on one line.

>> Source/WebCore/svg/SVGAnimatedValue.h:68
>> +    static RefPtr<SVGAnimatedValue> create(const PropertyType& value)
> 
> Can this return a Ref<>?

Done.

>> Source/WebCore/svg/SVGAnimatedValue.h:74
>> +    static RefPtr<SVGAnimatedValue> create(PropertyType&& value)
> 
> Ditto.

Done.

>> Source/WebCore/svg/SVGAnimatedValue.h:80
>> +    static RefPtr<SVGAnimatedValue> create(const SVGAnimatedValue& value)
> 
> Ditto.

Done.

>> Source/WebCore/svg/SVGAnimatedValue.h:86
>> +    bool holdsAlternative() const
> 
> Despite it being the name chosen by the variant class, I don't think holdsAlternative is that great a name.  Perhaps just good old is<>?

Function was renamed to be is<>().

>> Source/WebCore/svg/SVGPathByteStream.h:26
>>  #include <wtf/Noncopyable.h>
> 
> No need to separate these.

Empty line was removed.

>> Source/WebCore/svg/SVGPathByteStream.h:49
>>  public:
> 
> I'm generally not a fan of using inheritance for things like this, as it tends to expose much more API surface area than necessary.  I'd prefer to see this as kept as composition.

Done.

What convince me to do it this way was SVGLengthListValues is derived from Vector<SVGLengthValue> and SVGPointListValues is derived Vector<FloatPoint>.

>> Source/WebCore/svg/properties/SVGPropertyTraits.h:29
>> +#include <wtf/text/StringBuilder.h>
> 
> No need to separate these.

Empty line was removed.

>> Source/WebCore/svg/properties/SVGPropertyTraits.h:78
>> +        return std::pair<int, int>(static_cast<int>(roundf(firstNumber)), static_cast<int>(roundf(secondNumber)));
> 
> Will make_pair work here?

Done.

>> Source/WebCore/svg/properties/SVGPropertyTraits.h:146
>> +    }
> 
> These parse methods should be converted to return optional<Foo> at some point.

All SVGPropertyTraits<Foo>::parse() functions were changed to return std::optional<Foo>. SVGAnimatedValue::parse() was changed to check the returned std::optional<>.

[Said Abou-Hallawa]

Created attachment 302332 [details]
Patch

[Said Abou-Hallawa]

Created attachment 333616 [details]
Patch

[Said Abou-Hallawa]

Created attachment 333621 [details]
Patch

[EWS Watchlist]

Attachment 333621 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGValue.h:49:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:50:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:51:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:52:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:53:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:54:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:55:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:56:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:57:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:58:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:59:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:60:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:61:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:62:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:63:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:64:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:65:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:66:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:67:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:68:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:69:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:81:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/svg/SVGValue.h:108:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/ChangeLog:16:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free, use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 24 in 110 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Said Abou-Hallawa]

Created attachment 333629 [details]
Patch

[EWS Watchlist]

Attachment 333629 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGValue.h:49:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:50:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:51:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:52:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:53:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:54:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:55:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:56:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:57:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:58:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:59:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:60:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:61:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:62:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:63:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:64:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:65:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:66:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:67:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:68:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:69:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:81:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/svg/SVGValue.h:108:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/ChangeLog:16:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free, use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 24 in 110 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Said Abou-Hallawa]

Created attachment 333638 [details]
Patch

[EWS Watchlist]

Attachment 333638 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGValue.h:49:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:50:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:51:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:52:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:53:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:54:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:55:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:56:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:57:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:58:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:59:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:60:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:61:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:62:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:63:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:64:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:65:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:66:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:67:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:68:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:69:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:81:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/svg/SVGValue.h:108:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/ChangeLog:16:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free, use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 24 in 110 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Said Abou-Hallawa]

Created attachment 333646 [details]
Patch

[EWS Watchlist]

Attachment 333646 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGValue.h:49:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:50:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:51:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:52:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:53:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:54:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:55:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:56:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:57:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:58:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:59:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:60:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:61:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:62:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:63:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:64:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:65:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:66:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:67:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:68:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:69:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:81:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/svg/SVGValue.h:108:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/ChangeLog:16:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free, use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 24 in 110 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Said Abou-Hallawa]

Created attachment 333728 [details]
Patch

[EWS Watchlist]

Attachment 333728 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGValue.h:49:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:50:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:51:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:52:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:53:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:54:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:55:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:56:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:57:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:58:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:59:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:60:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:61:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:62:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:63:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:64:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:65:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:66:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:67:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:68:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:69:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:81:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/svg/SVGValue.h:108:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/ChangeLog:16:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free, use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 24 in 111 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Said Abou-Hallawa]

Created attachment 333814 [details]
Patch

[EWS Watchlist]

Attachment 333814 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGValue.h:49:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:50:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:51:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:52:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:53:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:54:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:55:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:56:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:57:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:58:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:59:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:60:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:61:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:62:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:63:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:64:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:65:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:66:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:67:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:68:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:69:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:81:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/svg/SVGValue.h:108:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/ChangeLog:16:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free, use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 24 in 111 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Comment on attachment 333814 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=333814&action=review

> Source/WebCore/ChangeLog:3
> +        Make all the SVGElements' attributes be RefCounted objects

This is a crazy big patch. I wonder if you can factor out *at least* the “fix header dependency problems” bits, and maybe the toString/setValueAsString renames?

> Source/WebCore/ChangeLog:26
> +        Creating different classes for every data type an SVG attribute does not
> +        look the cleanest solution. Besides, there are cases where the union

First sentence here has a grammar problem.

> Source/WebCore/ChangeLog:35
> +        -- Change SVGAnimatedType to hold two SVGValues. The second one is optional
> +        and only used for animated types which requires two data types.

Still not 100% sure why we don’t just make a new type (it can probably even be an alias for std::pair) for the two types that need two values, and just use SVGValue for everything?

> Source/WebCore/svg/SVGNumberListValues.h:35
> +    

:)

> Source/WebCore/svg/SVGParserUtilities.cpp:243
> +    // disallow anything except spaces at the end

Comments should be sentences with capitals and punctuation.

> Source/WebCore/svg/SVGValue.h:153
> +        return adoptRef(*new SVGValue(reinterpret_cast<const unsigned&>(property)));

What if the enum is bigger than unsigned? Can we make everyone use enum class and use the real type?

[Said Abou-Hallawa]

Created attachment 333931 [details]
Patch

[EWS Watchlist]

Attachment 333931 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGValue.h:49:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:50:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:51:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:52:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:53:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:54:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:55:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:56:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:57:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:58:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:59:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:60:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:61:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:62:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:63:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:64:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:65:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:66:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:67:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:68:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:69:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:81:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/svg/SVGValue.h:108:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/ChangeLog:16:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free, use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 24 in 108 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Said Abou-Hallawa]

Comment on attachment 333814 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=333814&action=review

>> Source/WebCore/ChangeLog:3
>> +        Make all the SVGElements' attributes be RefCounted objects
> 
> This is a crazy big patch. I wonder if you can factor out *at least* the “fix header dependency problems” bits, and maybe the toString/setValueAsString renames?

I tried by reming some of the re-factoring but I do not think this helped much reducing the size of the patch.

>> Source/WebCore/ChangeLog:26
>> +        look the cleanest solution. Besides, there are cases where the union
> 
> First sentence here has a grammar problem.

Fixed.

>> Source/WebCore/ChangeLog:35
>> +        and only used for animated types which requires two data types.
> 
> Still not 100% sure why we don’t just make a new type (it can probably even be an alias for std::pair) for the two types that need two values, and just use SVGValue for everything?

Please have a look at SVGAnimatedTypeAnimator::constructFromBaseValues() and notice that we have to call executeAction(StartAnimationAction, ...) twice one for each type. Every time we have to pass a separate SVGValue since this is going to be attached to a separate tear-off object. If the SVGValue has an alternative like std::pair<SVGAngleValue,SVGMarkerOrientType> for example, we will not be able to split it into RefCounted object each is referenced by a tear-off object.

>> Source/WebCore/svg/SVGNumberListValues.h:35
>> +    
> 
> :)

Space was removed.

>> Source/WebCore/svg/SVGParserUtilities.cpp:243
>> +    // disallow anything except spaces at the end
> 
> Comments should be sentences with capitals and punctuation.

Fixed.

>> Source/WebCore/svg/SVGValue.h:153
>> +        return adoptRef(*new SVGValue(reinterpret_cast<const unsigned&>(property)));
> 
> What if the enum is bigger than unsigned? Can we make everyone use enum class and use the real type?

Please look at SVGAnimatedEnumerationPropertyTearOff and search the code for highestEnumValue() and you will see that we already have the assumption that highestEnumValue() of any SVG enum has to fit inside unsigned.

[Said Abou-Hallawa]

Created attachment 333938 [details]
Patch

[EWS Watchlist]

Attachment 333938 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGValue.h:49:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:50:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:51:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:52:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:53:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:54:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:55:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:56:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:57:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:58:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:59:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:60:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:61:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:62:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:63:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:64:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:65:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:66:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:67:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:68:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:69:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/svg/SVGValue.h:81:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/svg/SVGValue.h:108:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/ChangeLog:16:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free, use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 24 in 108 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Said Abou-Hallawa]

Created attachment 333961 [details]
Patch

[Dean Jackson]

Comment on attachment 333814 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=333814&action=review

Can you add SVGType in a separate patch, without hooking it up to anything?

Some early comments.

>> Source/WebCore/ChangeLog:3
>> +        Make all the SVGElements' attributes be RefCounted objects
> 
> This is a crazy big patch. I wonder if you can factor out *at least* the “fix header dependency problems” bits, and maybe the toString/setValueAsString renames?

Yeah. It would be nice to split out the robotic changes from the real code changes.

> Source/WebCore/ChangeLog:16
> +        was relaxed by using WeakPtr or raw pointer from one side to the other, a
> +        use-after-free bugs were happening. A more subtle problem can happen when

s/a/some/

> Source/WebCore/ChangeLog:46
> +        Add new files for SVGValue and SVGListVlaues. Move definitions from

Typo: SVGListValues

> Source/WebCore/ChangeLog:81
> +        Don't pass SVGAnimatedType getter to resetFromBaseValues(). The PropertyTYpe

Typo: PropertyType

> Source/WebCore/svg/SVGAnimatedAngle.cpp:69
> +    const auto& fromAngle = static_cast<SVGAngleValue&>(*from->values[0]);
> +    const auto& fromEnumeration = static_cast<unsigned&>(*from->values[1]);

I don't like static_cast from SVGValue to underlying types of variants. It isn't really just a cast, since it is calling into a function that is checking the type and ASSERTing, etc.

I'd prefer .asSVGAngleValue() or something, or maybe even a new asVariantType<SVGAngleValue> helper.

> Source/WebCore/svg/SVGAnimatedAngle.cpp:86
> +    auto& toAngle = static_cast<SVGAngleValue&>(*to->values[0]);
> +    auto& toEnumeration = static_cast<unsigned&>(*to->values[1]);

Aren't these const too?

> Source/WebCore/svg/SVGAnimatedColor.cpp:48
> +    auto& fromColor = static_cast<Color&>(*from->values[0]);
> +    auto& toColor = static_cast<Color&>(*to->values[0]);

Yeah, I think we need a better way to convert from SVGValue to the underlying types.

> Source/WebCore/svg/SVGAnimatedNumberOptionalNumber.cpp:79
> +    const auto& formFirst = m_animationElement->animationMode() == ToAnimation ? static_cast<float&>(*animated->values[0]) : static_cast<float&>(*from->values[0]);
> +    const auto& formSecond = m_animationElement->animationMode() == ToAnimation ? static_cast<float&>(*animated->values[1]) : static_cast<float&>(*from->values[1]);

Typo: from

> Source/WebCore/svg/SVGAnimatedNumberOptionalNumber.cpp:88
> +    m_animationElement->animateAdditiveNumber(percentage, repeatCount, formFirst, toFirst, toAtEndOfDurationFirst, animatedFirst);
> +    m_animationElement->animateAdditiveNumber(percentage, repeatCount, formSecond, toSecond, toAtEndOfDurationSecond, animatedSecond);

Typo: from

> Source/WebCore/svg/SVGAnimatedType.h:57
> +    template<typename PropertyType>
> +    static std::unique_ptr<SVGAnimatedType> create(PropertyType&& property)

I don't understand why these are public but the constructor is also public. Which should people use? Can the actual constructor be private?

> Source/WebCore/svg/SVGAnimatedType.h:107
> +        ASSERT(values[0] && !values[1]);

Why is it an error to call toString on something with two values?

> Source/WebCore/svg/SVGAnimatedType.h:141
> +            [](const SVGAngleValue&)                { return AnimatedAngle; },
> +            [](const bool&)                         { return AnimatedBoolean; },
> +            [](const Color&)                        { return AnimatedColor; },
> +            [](const unsigned&)                     { return AnimatedEnumeration; },
> +            [](const int&)                          { return AnimatedInteger; },
> +            [](const SVGLengthValue&)               { return AnimatedLength; },
> +            [](const SVGLengthListValues&)          { return AnimatedLengthList; },
> +            [](const float)                         { return AnimatedNumber; },
> +            [](const SVGNumberListValues&)          { return AnimatedNumberList; },
> +            [](const SVGPathByteStream&)            { return AnimatedPath; },
> +            [](const SVGPreserveAspectRatioValue&)  { return AnimatedPreserveAspectRatio; },
> +            [](const SVGPointListValues&)           { return AnimatedPoints; },
> +            [](const FloatRect&)                    { return AnimatedRect; },
> +            [](const String&)                       { return AnimatedString; },
> +            [](const SVGTransformListValues&)       { return AnimatedTransformList; },
> +            [](const auto&)                         { return AnimatedUnknown; }

I don't think we usually indent things this way.

> Source/WebCore/svg/SVGAnimatedType.h:145
> +    RefPtr<SVGValue> values[2];

Rather than have an array, why not separate first and second members?

RefPtr<SVGValue> first;
RefPtr<SVGValue> second;

> Source/WebCore/svg/SVGFEConvolveMatrixElement.cpp:285
>          for (int i = 0; i < kernelMatrixSize; ++i)
> -            divisorValue += kernelMatrix.at(i);
> +            divisorValue += kernelMatrix.propertyAt(i);

Can you use a for : loop here?

[Said Abou-Hallawa]

Created attachment 334060 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 333814 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=333814&action=review

>> Source/WebCore/ChangeLog:16
>> +        use-after-free bugs were happening. A more subtle problem can happen when
> 
> s/a/some/

Fixed.

>> Source/WebCore/ChangeLog:46
>> +        Add new files for SVGValue and SVGListVlaues. Move definitions from
> 
> Typo: SVGListValues

Fixed.

>> Source/WebCore/ChangeLog:81
>> +        Don't pass SVGAnimatedType getter to resetFromBaseValues(). The PropertyTYpe
> 
> Typo: PropertyType

Fixed.

>> Source/WebCore/svg/SVGAnimatedAngle.cpp:69
>> +    const auto& fromEnumeration = static_cast<unsigned&>(*from->values[1]);
> 
> I don't like static_cast from SVGValue to underlying types of variants. It isn't really just a cast, since it is calling into a function that is checking the type and ASSERTing, etc.
> 
> I'd prefer .asSVGAngleValue() or something, or maybe even a new asVariantType<SVGAngleValue> helper.

I added a template function SVGValue::as() and I replaced all the casting by calls to as<PropertyType>().

>> Source/WebCore/svg/SVGAnimatedAngle.cpp:86
>> +    auto& toEnumeration = static_cast<unsigned&>(*to->values[1]);
> 
> Aren't these const too?

Fixed.

>> Source/WebCore/svg/SVGAnimatedColor.cpp:48
>> +    auto& toColor = static_cast<Color&>(*to->values[0]);
> 
> Yeah, I think we need a better way to convert from SVGValue to the underlying types.

Fixed.

>> Source/WebCore/svg/SVGAnimatedNumberOptionalNumber.cpp:79
>> +    const auto& formSecond = m_animationElement->animationMode() == ToAnimation ? static_cast<float&>(*animated->values[1]) : static_cast<float&>(*from->values[1]);
> 
> Typo: from

Fixed.

>> Source/WebCore/svg/SVGAnimatedNumberOptionalNumber.cpp:88
>> +    m_animationElement->animateAdditiveNumber(percentage, repeatCount, formSecond, toSecond, toAtEndOfDurationSecond, animatedSecond);
> 
> Typo: from

Fixed.

>> Source/WebCore/svg/SVGAnimatedType.h:57
>> +    static std::unique_ptr<SVGAnimatedType> create(PropertyType&& property)
> 
> I don't understand why these are public but the constructor is also public. Which should people use? Can the actual constructor be private?

The constructors were private :). But Sam wanted to use std::make_unique, so I had to make them public. See https://bugs.webkit.org/show_bug.cgi?id=168586#c2.

>> Source/WebCore/svg/SVGAnimatedType.h:107
>> +        ASSERT(values[0] && !values[1]);
> 
> Why is it an error to call toString on something with two values?

The deleted functions SVGAnimatedType::valueAsString() and SVGAnimatedType::setValueAsString() were expecting only six single value SVGAnimatedType. And we were asserting that only these six types are the ones that we allow calling these two functions. With this patch, we are asserting here that this two functions will not be called for double value SVGAnimatedType. And SVGValue::toString() and SVGValue::parse() will assert if the SVGPropertyTraits does not implement toString or parse() functions. Besides we do not have a SVGPropertyTraits for a double valued SVGAnimatedType.

>> Source/WebCore/svg/SVGAnimatedType.h:141
>> +            [](const auto&)                         { return AnimatedUnknown; }
> 
> I don't think we usually indent things this way.

Fixed.

>> Source/WebCore/svg/SVGAnimatedType.h:145
>> +    RefPtr<SVGValue> values[2];
> 
> Rather than have an array, why not separate first and second members?
> 
> RefPtr<SVGValue> first;
> RefPtr<SVGValue> second;

Fixed,

>> Source/WebCore/svg/SVGFEConvolveMatrixElement.cpp:285
>> +            divisorValue += kernelMatrix.propertyAt(i);
> 
> Can you use a for : loop here?

Fixed.

[Said Abou-Hallawa]

Created attachment 336505 [details]
Patch

[Said Abou-Hallawa]

Created attachment 336508 [details]
Patch

[Said Abou-Hallawa]

Created attachment 336521 [details]
Patch

[Said Abou-Hallawa]

Created attachment 337942 [details]
Patch

[EWS Watchlist]

Comment on attachment 337942 [details]
Patch

Attachment 337942 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7311818

New failing tests:
animations/needs-layout.html

[EWS Watchlist]

Created attachment 337952 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Radar WebKit Bug Importer]

<rdar://problem/41333684>

[Said Abou-Hallawa]

The approach of 191237 is better than the one taken by the attached patch.

*** This bug has been marked as a duplicate of bug 191237 ***