Element::idForStyleResolution() is a foot-gun

[Chris Dumez]

Element::idForStyleResolution() is a foot-gun. It requires the caller to check Element::hasID() first or it may end up crashing when dereferencing elementData(). See Bug 156806.

[Chris Dumez]

Created attachment 276953 [details]
Patch

[Darin Adler]

Comment on attachment 276953 [details]
Patch

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

> Source/WebCore/css/ElementRuleCollector.cpp:158
> +        collectMatchingRulesForList(matchRequest.ruleSet->idRules(id.impl()), matchRequest, ruleRange);

Seems that idRules should take a reference not a pointer, since we are checking for null before calling it..

> Source/WebCore/css/SelectorChecker.cpp:658
> +    if (selector.match() == CSSSelector::Id) {
> +        auto& id = element.idForStyleResolution();
> +        return !id.isNull() && id == selector.value();
> +    }

Do we really need the isNull check here? Can selector.value() be null? I suggest proving it’s not needed and having the code be more streamlined without the isNull check.

[Chris Dumez]

Created attachment 276971 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 276971 [details]
Patch

Clearing flags on attachment: 276971

Committed r199844: <http://trac.webkit.org/changeset/199844>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.