WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
Proposed patch v2
(27.28 KB, patch)
2011-09-06 04:50 PDT
,
Andreas Kling
no flags
Details
Formatted Diff
Diff
Proposed patch v3
(22.28 KB, patch)
2011-09-08 04:13 PDT
,
Andreas Kling
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug