Bug 110926 – Can't use fastGetAttribute with WebCore::HTMLNames::classAttr when element is a SVGStyledElement

• Component: SVG
• Status: RESOLVED CONFIGURATION CHANGED

[Tim 'mithro' Ansell]

Created attachment 190393 [details]
Only use fast path when you don't have a SVG element.

The problem is that HTMLNames::classAttr for SVG elements is animatable, so we need to use getAttribute rather than fastGetAttribute.

Please see http://code.google.com/p/chromium/issues/detail?id=178030 for more complete details.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/bindings/scripts/CodeGenerator.pm', u'Source/WebCore/dom/Element.cpp', u'Source/WebCore/dom/Element.h']" exit_code: 1
Source/WebCore/dom/Element.h:794:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/dom/Element.h:792:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/dom/Element.h:796:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 3 in 2 files


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

[Early Warning System Bot]

Comment on attachment 190393 [details]
Only use fast path when you don't have a SVG element.

Attachment 190393 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16815011

[kov's GTK+ EWS bot]

Comment on attachment 190393 [details]
Only use fast path when you don't have a SVG element.

Attachment 190393 [details] did not pass gtk-ews (gtk):
Output: http://webkit-commit-queue.appspot.com/results/16823001

[Tim 'mithro' Ansell]

Created attachment 190398 [details]
Patch

[Early Warning System Bot]

Comment on attachment 190398 [details]
Patch

Attachment 190398 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16825001

[Early Warning System Bot]

Comment on attachment 190398 [details]
Patch

Attachment 190398 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16827002

[Build Bot]

Comment on attachment 190398 [details]
Patch

Attachment 190398 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16770546

[Tim 'mithro' Ansell]

Created attachment 190407 [details]
Patch

[Early Warning System Bot]

Comment on attachment 190407 [details]
Patch

Attachment 190407 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16790055

[Early Warning System Bot]

Comment on attachment 190407 [details]
Patch

Attachment 190407 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16825022

[Tim 'mithro' Ansell]

Created attachment 190417 [details]
Patch

[Philip Rogers]

Comment on attachment 190417 [details]
Patch

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

I'm not a reviewer nor am I very familiar with this code, so please take my suggestions with a grain of salt!

Do you mind adding a test to this patch? Just a simple test that crashes without your patch and succeeds with it should be enough.

Do you mind testing the performance of this patch? Could dromaeo be affected, for instance (http://dromaeo.com)?

Is JSC affected by this bug as well?

> Source/WebCore/ChangeLog:4
> +        use the fastGetAttribute. 

Please put the bug url below the short summary but above the full ChangeLog entry. http://www.webkit.org/coding/contributing.html#changelogs has an example of this.

Just a minor suggestion: you may want to rephrase your summary with just what the patch does. For example: "Use fastGetAttribute for className on HTML nodes but not SVG nodes"

> Source/WebCore/bindings/scripts/CodeGenerator.pm:597
> +{

See comment below

> Source/WebCore/bindings/scripts/CodeGenerator.pm:643
> +            $functionName = "tryFastGetAttribute";

Can you split this into a separate patch?

> Source/WebCore/dom/Element.cpp:-2562
> -#ifndef NDEBUG

I'm not sure this is a good idea: previously this was guarded with NDEBUG but now it will always get compiled in. Is removing this required?

> Source/WebCore/dom/Element.h:794
> +inline const AtomicString& Element::tryFastGetAttribute(const QualifiedName& name) const

I think this can be removed as well.

[Tim 'mithro' Ansell]

Created attachment 190422 [details]
Patch

[Tim 'mithro' Ansell]

Minimal patch uploaded with fixes as recommended by pdr.

Yet to create a test case.

[Tim 'mithro' Ansell]

Created attachment 190427 [details]
Patch

[Tim 'mithro' Ansell]

Added a test case.

[Tim 'mithro' Ansell]

Created attachment 190429 [details]
Patch

[Philip Rogers]

Comment on attachment 190429 [details]
Patch

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

I added a couple minor nits about the test.

I'm going to cc Kentaro Hara who may be able to review the performance side of this patch or suggest a specific benchmark.

> LayoutTests/dom/svg/svg-className-lookup-crash.htm:1
> +<body>

Please add:
<!DOCTYPE html>
<html>

Proper html makes the parser panda happy :)

> LayoutTests/dom/svg/svg-className-lookup-crash.htm:4
> +var svgElement1 = document.createElementNS("http://www.w3.org/2000/svg", "view");

Is this necessary since we already have a <view> above?

> LayoutTests/dom/svg/svg-className-lookup-crash.htm:13
> +  window.testRunner.dumpAsText();

This is a great test but we can actually make it even better by printing the class name result:
if (window.testRunner) {
  document.write("Accessing a SVGStyledElement's class attribute should not crash. The expected className is myClass, the actual class was " + result);
  testRunner.dumpAsText();
}

[Andreas Kling]

Comment on attachment 190429 [details]
Patch

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

> Source/WebCore/dom/Element.h:831
> +#if ENABLE(SVG)
> +    return isSVGElement() ? getAttribute(WebCore::HTMLNames::classAttr) : fastGetAttribute(WebCore::HTMLNames::classAttr);
> +#else
> +    return fastGetAttribute(WebCore::HTMLNames::classAttr);
> +#endif

You could check Element::hasClass() before calling fastGetAttribute() in the non-SVGElement case(s).
This would avoid iterating over the attribute storage when there is no class attribute to be found.

[Andreas Kling]

(In reply to comment #19)
> (From update of attachment 190429 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190429&action=review
> 
> > Source/WebCore/dom/Element.h:831
> > +#if ENABLE(SVG)
> > +    return isSVGElement() ? getAttribute(WebCore::HTMLNames::classAttr) : fastGetAttribute(WebCore::HTMLNames::classAttr);
> > +#else
> > +    return fastGetAttribute(WebCore::HTMLNames::classAttr);
> > +#endif
> 
> You could check Element::hasClass() before calling fastGetAttribute() in the non-SVGElement case(s).
> This would avoid iterating over the attribute storage when there is no class attribute to be found.

Also, the code would look slightly nicer with an early return instead of the ternary operator:

inline const AtomicString& Element::getClassAttribute()
{
#if ENABLE(SVG)
    if (isSVGElement())
        return getAttribute(HTMLNames::classAttr);
#endif
    if (!hasClass())
        return nullAtom;
    return fastGetAttribute(HTMLNames::classAttr);
}

[Erik Arvidsson]

Comment on attachment 190429 [details]
Patch

I like this approach.

Andreas: I'm not sure that the hasClass test is worth it? It all depends on how often we expect people to read className of an element that does not have a class attribute. In the old days with Sizzle and all I would say it was really common but now that most people have transitioned to querySelector it is clearly less common.

[Erik Arvidsson]

Comment on attachment 190429 [details]
Patch

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

>> LayoutTests/dom/svg/svg-className-lookup-crash.htm:13
>> +  window.testRunner.dumpAsText();
> 
> This is a great test but we can actually make it even better by printing the class name result:
> if (window.testRunner) {
>   document.write("Accessing a SVGStyledElement's class attribute should not crash. The expected className is myClass, the actual class was " + result);
>   testRunner.dumpAsText();
> }

Or just switch to js-test-pre/post and use shouldBe

[Andreas Kling]

(In reply to comment #21)
> (From update of attachment 190429 [details])
> I like this approach.
> 
> Andreas: I'm not sure that the hasClass test is worth it? It all depends on how often we expect people to read className of an element that does not have a class attribute. In the old days with Sizzle and all I would say it was really common but now that most people have transitioned to querySelector it is clearly less common.

I don't feel strongly about the hasClass() check. It's probably not worth it.

[Darin Adler]

Comment on attachment 190429 [details]
Patch

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

> Source/WebCore/dom/Element.h:828
> +    return isSVGElement() ? getAttribute(WebCore::HTMLNames::classAttr) : fastGetAttribute(WebCore::HTMLNames::classAttr);

Is this faster than just calling getAttribute?

[Tim 'mithro' Ansell]

So I'm unsure what you want me to do to get this bug committed?

[Erik Arvidsson]

(In reply to comment #25)
> So I'm unsure what you want me to do to get this bug committed?

There are some comments in the code review that you have not yet resolved.

[Tim 'mithro' Ansell]

Created attachment 191566 [details]
Patch

[Darin Adler]

Comment on attachment 191566 [details]
Patch

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

review- because of the hasClass thing -- I’m also pretty sure we don’t need a new getClassAttribute function

> Source/WebCore/dom/Element.h:830
> +inline const AtomicString& Element::getClassAttribute() const
> +{
> +#if ENABLE(SVG)
> +    if (isSVGElement())
> +        return getAttribute(HTMLNames::classAttr);
> +#endif
> +    if (!hasClass())
> +        return nullAtom;
> +    return fastGetAttribute(HTMLNames::classAttr);
> +}

The consensus in the comments seems to be that the hasClass check is not a worthwhile optimization.

If we do want it, then I believe the hasClass check can go before the SVG element check.

Is this measurably more efficient than calling getAttribute(classAttr)? I suspect it is not. And if not, then we should have the code generator call getAttribute and not add a new getClassAttribute function.

If this is, then we should consider doing this for every attribute other than styleAttr. The only thing that is specific to the class attribute here is the hasClass function.

Otherwise this looks pretty good to me.

[Tim 'mithro' Ansell]

It is my understanding that using just getAttribute rather than fastGetAttribute is a significant speed reduction. In the history of this file the code was changed to just do a getAttribute and was reverted for performance reasons.

On SVG we *can't* use fastGetAttribute as class is animatable. I believe getAttribute is slow because it has to synchronize with animatable values (I don't know what that actually means however).

[Eric Seidel (no email)]

Comment on attachment 191566 [details]
Patch

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

>> Source/WebCore/dom/Element.h:830
>> +}
> 
> The consensus in the comments seems to be that the hasClass check is not a worthwhile optimization.
> 
> If we do want it, then I believe the hasClass check can go before the SVG element check.
> 
> Is this measurably more efficient than calling getAttribute(classAttr)? I suspect it is not. And if not, then we should have the code generator call getAttribute and not add a new getClassAttribute function.
> 
> If this is, then we should consider doing this for every attribute other than styleAttr. The only thing that is specific to the class attribute here is the hasClass function.
> 
> Otherwise this looks pretty good to me.

I believe this bug is fallout from performance optimizations we made for className in the bindings generator.

getAttribute, as you know, is very slow.  At least compared to fastGetAttribute.

Normally, we can use fastGetAttribute for everything.  Unfortunately the SVG spec says that "class" is an animatable attribute.  meaning, we can't use fastGetAttribute for it class on an SVGStyledElement.

Arv thought he fixed this back in bug 105565, but not quite.  We can still end up calling fastGetAttribute(className) on an SVGElement with animated class and hit an ASSERT in debug. :(

We stumbled upon this while investigating an unrelated crasher from the fuzzer.

[Eric Seidel (no email)]

(In reply to comment #28)
> (From update of attachment 191566 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191566&action=review
>
> Is this measurably more efficient than calling getAttribute(classAttr)? I suspect it is not. And if not, then we should have the code generator call getAttribute and not add a new getClassAttribute function.

Calling fastGetAttribute(classAttr) is measurably more efficient than calling getAttribute(classAttr).  We go to great lengths to avoid getAttribute in many places:
http://trac.webkit.org/browser/trunk/Source/WebCore/css/StyleResolver.cpp#L985

I guess that call could move to mitho's new function since it's basically the same.
 
> If this is, then we should consider doing this for every attribute other than styleAttr. The only thing that is specific to the class attribute here is the hasClass function.

I'm not sure I follow.  The generator (and most code) already uses fastGetAttr for everything.  This bug is about us using fastGetAttribute in one case where it was not OK and hitting an ASSERT in debug builds.

> Otherwise this looks pretty good to me.

LGTM too.  Please move the hasClass to earlier, and consider changing the StyleResolver line.  Be aware that that style resolver code is *extremely* performance sensitive.

[Tim 'mithro' Ansell]

Created attachment 196057 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 196057 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Use fastGetAttribute for className on HTML nodes but not SVG nodes.

This line should match the but title.

> Source/WebCore/ChangeLog:12
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the long description but after the bug URL.

> LayoutTests/dom/svg/svg-className-lookup-crash.htm:1
> +<body>

Missing DOCTYPE.

> LayoutTests/dom/svg/svg-className-lookup-crash.htm:13
> +  window.testRunner.dumpAsText();

Nit: This should be indented by 4 spaces, not 2.

[Ryosuke Niwa]

Comment on attachment 196057 [details]
Patch

Oh, oops, I didn't meant to r+ it.

[Tim 'mithro' Ansell]

Created attachment 196066 [details]
Patch

[Tim 'mithro' Ansell]

Sorry guys, it seems in the confusion I uploaded an old version from my desktop computer rather than the most current version from my laptop. (The two machines are now in sync.) Dangers of coming back from two weeks of holidays :P

I'll now get to Ryosuke Niwa's points.

[Tim 'mithro' Ansell]

Created attachment 196067 [details]
Patch

[Tim 'mithro' Ansell]

Comment on attachment 196057 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        Use fastGetAttribute for className on HTML nodes but not SVG nodes.
> 
> This line should match the but title.

Done.

>> Source/WebCore/ChangeLog:12
>> +        Reviewed by NOBODY (OOPS!).
> 
> This line should appear before the long description but after the bug URL.

Done.

>> LayoutTests/dom/svg/svg-className-lookup-crash.htm:1
>> +<body>
> 
> Missing DOCTYPE.

Done.

>> LayoutTests/dom/svg/svg-className-lookup-crash.htm:13
>> +  window.testRunner.dumpAsText();
> 
> Nit: This should be indented by 4 spaces, not 2.

Done.

[Darin Adler]

Comment on attachment 196067 [details]
Patch

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

> Source/WebCore/dom/Element.h:862
> +    return fastGetAttribute(HTMLNames::classAttr);

This is a great idea, but I believe the execution is still a little bit incorrect.

It’s a programming error to call fastGetAttribute on classAttr from any call site other than this function. Thus the fastAttributeLookupAllowed function should return false for classAttr. Thus, this line of code is wrong, and the fastAttributeLookupAllowed function also is wrong to return "true" for classAttr since it’s not allowed.

There are a number of ways to fix this. The simplest is to make a separate private inline function for the “guts” of fastGetAttribute, without the fastAttributeLookupAllowed assertion, and call that here. There are various other suitable refactoring. Even for SVG elements, there is no reason to do the styleAttr check that getAttribute does when fetching a classAttr, so that’s a case we could optimize further if desired.

Have we done performance tests on the difference in performance from calling getAttribute vs. "if (isSVG) getAttribute() else fastGetAttribute()"? If it’s not a measurable difference then we should consider that implementation.

[Darin Adler]

Comment on attachment 196067 [details]
Patch

review- because of my last comment; still a good idea, but we need to do a little better

[Andreas Kling]

(In reply to comment #40)
> (From update of attachment 196067 [details])
> review- because of my last comment; still a good idea, but we need to do a little better

IIUC this bug is all about generated bindings. We know at compile time if fastGetAttribute(classAttr) is okay or not for a given interface, right? So why not do the branch then instead of at runtime?

[Eric Seidel (no email)]

Comment on attachment 196067 [details]
Patch

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

>> Source/WebCore/dom/Element.h:862
>> +    return fastGetAttribute(HTMLNames::classAttr);
> 
> This is a great idea, but I believe the execution is still a little bit incorrect.
> 
> It’s a programming error to call fastGetAttribute on classAttr from any call site other than this function. Thus the fastAttributeLookupAllowed function should return false for classAttr. Thus, this line of code is wrong, and the fastAttributeLookupAllowed function also is wrong to return "true" for classAttr since it’s not allowed.
> 
> There are a number of ways to fix this. The simplest is to make a separate private inline function for the “guts” of fastGetAttribute, without the fastAttributeLookupAllowed assertion, and call that here. There are various other suitable refactoring. Even for SVG elements, there is no reason to do the styleAttr check that getAttribute does when fetching a classAttr, so that’s a case we could optimize further if desired.
> 
> Have we done performance tests on the difference in performance from calling getAttribute vs. "if (isSVG) getAttribute() else fastGetAttribute()"? If it’s not a measurable difference then we should consider that implementation.

It appears that fastAttributeLookupAllowed already returns "false" for "class" in the SVG case:
http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Element.cpp#L2609

So I'm not sure that the refactoring you suggest is necessary?  fastAttributeLookupAllowed seems to have enough context to tell if class is safe or not.

[Eric Seidel (no email)]

(In reply to comment #42)
> (From update of attachment 196067 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=196067&action=review
> > Have we done performance tests on the difference in performance from calling getAttribute vs. "if (isSVG) getAttribute() else fastGetAttribute()"? If it’s not a measurable difference then we should consider that implementation.

getAttribute() is definitely much slower than fastGetAttribute, but you already know that.  :)  I'm not sure exactly what you're suggesting as an alternative here.  Are you suggesting that we send all SVG attribute accesses down the getAttribute path?  That seems overly conservative, if true?

[Tim 'mithro' Ansell]

Comment on attachment 196067 [details]
Patch

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

>> Source/WebCore/dom/Element.h:862
>> +    return fastGetAttribute(HTMLNames::classAttr);
> 
> This is a great idea, but I believe the execution is still a little bit incorrect.
> 
> It’s a programming error to call fastGetAttribute on classAttr from any call site other than this function. Thus the fastAttributeLookupAllowed function should return false for classAttr. Thus, this line of code is wrong, and the fastAttributeLookupAllowed function also is wrong to return "true" for classAttr since it’s not allowed.
> 
> There are a number of ways to fix this. The simplest is to make a separate private inline function for the “guts” of fastGetAttribute, without the fastAttributeLookupAllowed assertion, and call that here. There are various other suitable refactoring. Even for SVG elements, there is no reason to do the styleAttr check that getAttribute does when fetching a classAttr, so that’s a case we could optimize further if desired.
> 
> Have we done performance tests on the difference in performance from calling getAttribute vs. "if (isSVG) getAttribute() else fastGetAttribute()"? If it’s not a measurable difference then we should consider that implementation.

This patch doesn't affect fastAttributeLookupAllowed. The fastAttributeLookupAllowed function already returns *false* for classAttr when the object is an SVG object, which is why this patch even started.

This patch does precisely;
if (isSVG) getAttribute() else fastGetAttribute()

As Eric mentioned
>Calling fastGetAttribute(classAttr) is measurably more efficient than calling getAttribute(classAttr).  We go to great lengths to avoid getAttribute in many places: http://trac.webkit.org/browser/trunk/Source/WebCore/css/StyleResolver.cpp#L985

[Brent Fulgham]

This code has been significantly refactored since this patch was proposed. There doesn't seem to be any action we can take here.

[Radar WebKit Bug Importer]

<rdar://problem/97099368>