Bug 156852 - Element::idForStyleResolution() is a foot-gun
Summary: Element::idForStyleResolution() is a foot-gun
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
Depends on:
Reported: 2016-04-21 09:15 PDT by Chris Dumez
Modified: 2016-04-21 16:28 PDT (History)
4 users (show)

See Also:

Patch (7.77 KB, patch)
2016-04-21 13:48 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (9.21 KB, patch)
2016-04-21 15:36 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2016-04-21 09:15:08 PDT
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.
Comment 1 Chris Dumez 2016-04-21 13:48:47 PDT
Created attachment 276953 [details]
Comment 2 Darin Adler 2016-04-21 13:52:58 PDT
Comment on attachment 276953 [details]

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.
Comment 3 Chris Dumez 2016-04-21 15:36:49 PDT
Created attachment 276971 [details]
Comment 4 WebKit Commit Bot 2016-04-21 16:28:29 PDT
Comment on attachment 276971 [details]

Clearing flags on attachment: 276971

Committed r199844: <http://trac.webkit.org/changeset/199844>
Comment 5 WebKit Commit Bot 2016-04-21 16:28:36 PDT
All reviewed patches have been landed.  Closing bug.