Bug 67612 – Protect against incorrect Element::fast*Attribute() usage.

• Component: DOM
• Status: RESOLVED FIXED

[Andreas Kling]

It's incorrect to call Element::fastGetAttribute() or Element::fastHasAttribute() on HTMLNames::styleAttr, and any of the SVG animatable attributes.

We should detect misuse of these functions in debug mode, at the very least.

[Andreas Kling]

Created attachment 106352 [details]
Proposed patch

[Dirk Schulze]

Comment on attachment 106352 [details]
Proposed patch

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

Some notes. But looks good in general.

> Source/WebCore/svg/SVGAnimationElement.cpp:256
> +    if (hasAttribute(SVGNames::valuesAttr))

All attributes of SMILElements are not animatable. Can you use fastHasAttribute for it?

> Source/WebCore/svg/animation/SVGSMILElement.cpp:543
> -    return fastGetAttribute(XLinkNames::hrefAttr);
> +    return getAttribute(XLinkNames::hrefAttr);

Ditto with fastGetAttribute.

[Nikolas Zimmermann]

Comment on attachment 106352 [details]
Proposed patch

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

> Source/WebCore/dom/Element.cpp:2008
> +bool Element::fastAttributeLookupAllowed(const QualifiedName& name) const
> +{
> +    if (name == HTMLNames::styleAttr)
> +        return false;

I'm revoking Dirks r+. This needs to be a static hashset, this is way too inefficient.

[Dirk Schulze]

(In reply to comment #3)
> (From update of attachment 106352 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106352&action=review
> 
> > Source/WebCore/dom/Element.cpp:2008
> > +bool Element::fastAttributeLookupAllowed(const QualifiedName& name) const
> > +{
> > +    if (name == HTMLNames::styleAttr)
> > +        return false;
> 
> I'm revoking Dirks r+. This needs to be a static hashset, this is way too inefficient.

I talked about that with kling. This is just for the assertion and does influence a release build!

[Dirk Schulze]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 106352 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=106352&action=review
> > 
> > > Source/WebCore/dom/Element.cpp:2008
> > > +bool Element::fastAttributeLookupAllowed(const QualifiedName& name) const
> > > +{
> > > +    if (name == HTMLNames::styleAttr)
> > > +        return false;
> > 
> > I'm revoking Dirks r+. This needs to be a static hashset, this is way too inefficient.
> 
> I talked about that with kling. This is just for the assertion and does influence a release build!

missed "'t" does NOT influence a release build!

[Nikolas Zimmermann]

(In reply to comment #5)
> > > 
> > > I'm revoking Dirks r+. This needs to be a static hashset, this is way too inefficient.
> > 
> > I talked about that with kling. This is just for the assertion and does influence a release build!
> 
> missed "'t" does NOT influence a release build!

Still no reason to put this burden on every fastAttribute call in debug builds. A static HashSet is also much easier to read.

[Andreas Kling]

Created attachment 106406 [details]
Proposed patch v2

Second attack!

- Put the relevant things in !NDEBUG blocks.
- Moved animatable property check to "static bool SVGElement::isAnimatableAttribute(const QualifiedName&)" where we use a static hashset as suggested by Nikolas.
- Added comment in Element.h about getClassAttribute().

[Darin Adler]

Is getClassAttribute actually faster than getAttribute(classAttr)? If not, maybe we shouldn’t add it.

[Andreas Kling]

(In reply to comment #8)
> Is getClassAttribute actually faster than getAttribute(classAttr)? If not, maybe we shouldn’t add it.

They are essentially equivalent with this patch, bar 2 hinted-against branches.

If kept, getClassAttribute() should probably be inlined, I was hesitant to do so because it necessitates including HTMLNames.h from Element.h.

I'm leaning towards moving the checks to the few call sites we have.

[Darin Adler]

Here’s my take on the class part: The fastGetAttribute function is not such a superb optimization that we need to strain ourselves to use it. If the class attribute can sometimes be animatable I suggest just calling getAttribute instead of fastGetAttribute for classAttr for now. We can optimize in a separate patch if there’s some measurable improvement to be made.

[Andreas Kling]

Created attachment 106721 [details]
Proposed patch v3

As Darin pointed out, adding a special getter for the class attribute is probably not worth the added complexity just to get this optimization.
This new patch gets rid of Element::getClassAttribute() and switches fastGetAttribute(classAttr) to getAttribute(classAttr) instead.
Also added a missing export of fastAttributeLookupAllowed() to fix the mac/debug build.

[Dirk Schulze]

Comment on attachment 106721 [details]
Proposed patch v3

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

> Source/WebCore/ChangeLog:9
> +        Add debug-only checks in Element's fastHasAttribute() and fastGetAttribute() to verify
> +        that we aren't looking up the "style" attribute or any of the SVG animatable attributes.

You should make it more clear why you change the fast* functions to get* functions.

> Source/WebCore/ChangeLog:11
> +        No new tests, erroneous behavior is covered by assertions.

And that the some tests fail once you add the assertion because of wrong use of fast*. Also I'd like to see a test that shows that fast* calls were used in the wrong context.

> Source/WebCore/svg/SVGElement.h:105
> +    static bool isAnimatableAttribute(const QualifiedName&);

can you call it isAnimateableSVGAttribute to make it more clear?

[Darin Adler]

Comment on attachment 106721 [details]
Proposed patch v3

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

> Source/WebCore/css/CSSStyleSelector.cpp:926
> +    if (element->getAttribute(typeAttr) != m_element->getAttribute(typeAttr))

I would like more explanation of why typeAttr is animatable. Is this the same as SVGName::typeAttr?

>> Source/WebCore/svg/SVGElement.h:105
>> +    static bool isAnimatableAttribute(const QualifiedName&);
> 
> can you call it isAnimateableSVGAttribute to make it more clear?

I think the current name is pretty clear given the context as a member of SVGElement.

[WebKit Review Bot]

Comment on attachment 106721 [details]
Proposed patch v3

Clearing flags on attachment: 106721

Committed r97670: <http://trac.webkit.org/changeset/97670>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Andreas Kling]

(In reply to comment #13)
> (From update of attachment 106721 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106721&action=review
> 
> > Source/WebCore/css/CSSStyleSelector.cpp:926
> > +    if (element->getAttribute(typeAttr) != m_element->getAttribute(typeAttr))
> 
> I would like more explanation of why typeAttr is animatable. Is this the same as SVGName::typeAttr?

Correct, there is no way to distinguish between SVGNames::typeAttr and HTMLNames::typeAttr at runtime.