RESOLVED FIXED 67612
Protect against incorrect Element::fast*Attribute() usage.
https://bugs.webkit.org/show_bug.cgi?id=67612
Summary Protect against incorrect Element::fast*Attribute() usage.
Andreas Kling
Reported 2011-09-05 11:47:10 PDT
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.
Attachments
Proposed patch (24.99 KB, patch)
2011-09-05 12:26 PDT, Andreas Kling
zimmermann: review-
Proposed patch v2 (27.28 KB, patch)
2011-09-06 04:50 PDT, Andreas Kling
no flags
Proposed patch v3 (22.28 KB, patch)
2011-09-08 04:13 PDT, Andreas Kling
no flags
Andreas Kling
Comment 1 2011-09-05 12:26:50 PDT
Created attachment 106352 [details] Proposed patch
Dirk Schulze
Comment 2 2011-09-05 14:33:24 PDT
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 3 2011-09-06 00:29:24 PDT
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
Comment 4 2011-09-06 00:39:53 PDT
(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
Comment 5 2011-09-06 00:40:24 PDT
(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
Comment 6 2011-09-06 01:42:54 PDT
(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
Comment 7 2011-09-06 04:50:32 PDT
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
Comment 8 2011-09-06 08:48:28 PDT
Is getClassAttribute actually faster than getAttribute(classAttr)? If not, maybe we shouldn’t add it.
Andreas Kling
Comment 9 2011-09-06 08:52:54 PDT
(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
Comment 10 2011-09-06 08:59:11 PDT
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
Comment 11 2011-09-08 04:13:00 PDT
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 12 2011-09-09 15:20:38 PDT
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 13 2011-10-17 12:30:49 PDT
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 14 2011-10-17 16:39:25 PDT
Comment on attachment 106721 [details] Proposed patch v3 Clearing flags on attachment: 106721 Committed r97670: <http://trac.webkit.org/changeset/97670>
WebKit Review Bot
Comment 15 2011-10-17 16:39:30 PDT
All reviewed patches have been landed. Closing bug.
Andreas Kling
Comment 16 2011-10-18 03:43:13 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.