Bug 156852

Summary: Element::idForStyleResolution() is a foot-gun
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: DOMAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, kling, simon.fraser
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=156806
Attachments:
Description Flags
Patch
none
Patch none

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

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.