Changeset 199844 in webkit

Timestamp:

Apr 21, 2016 4:28:48 PM (8 years ago)

Author:

Chris Dumez

Message:

Element::idForStyleResolution() is a foot-gun
​https://bugs.webkit.org/show_bug.cgi?id=156852

Reviewed by Darin Adler.

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

This patch updates Element::idForStyleResolution() to return nullAtom is the
Element does not have an ID. I did not see a performance impact on Speedometer,
Dromaeo DOM Core, Dromaeo CSS Selectors and our local performanceTests/.

• css/ElementRuleCollector.cpp:

(WebCore::ElementRuleCollector::collectMatchingRules):

• css/SelectorChecker.cpp:

(WebCore::SelectorChecker::checkOne):

• css/SelectorFilter.cpp:

(WebCore::collectElementIdentifierHashes):

• dom/Element.h:

(WebCore::Element::idForStyleResolution):

• rendering/RenderBlockFlow.cpp:

(WebCore::needsAppleMailPaginationQuirk):

• rendering/RenderTreeAsText.cpp:

(WebCore::writeRenderRegionList):

• style/StyleSharingResolver.cpp:

(WebCore::Style::SharingResolver::canShareStyleWithElement):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• css/ElementRuleCollector.cpp (modified) (1 diff)
• css/RuleSet.h (modified) (1 diff)
• css/SelectorChecker.cpp (modified) (1 diff)
• css/SelectorFilter.cpp (modified) (1 diff)
• dom/Element.h (modified) (1 diff)
• rendering/RenderBlockFlow.cpp (modified) (1 diff)
• rendering/RenderTreeAsText.cpp (modified) (1 diff)
• style/StyleSharingResolver.cpp (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2016-04-21  Chris Dumez  <cdumez@apple.com>
+
+        Element::idForStyleResolution() is a foot-gun
+        https://bugs.webkit.org/show_bug.cgi?id=156852
+
+        Reviewed by Darin Adler.
+
+        Element::idForStyleResolution() is a foot-gun. It requires the caller to check
+        Element::hasID() first or it may end up crashing when dereferencing elementData()
+        (e.g. see Bug 156806).
+
+        This patch updates Element::idForStyleResolution() to return nullAtom is the
+        Element does not have an ID. I did not see a performance impact on Speedometer,
+        Dromaeo DOM Core, Dromaeo CSS Selectors and our local performanceTests/.
+
+        * css/ElementRuleCollector.cpp:
+        (WebCore::ElementRuleCollector::collectMatchingRules):
+        * css/SelectorChecker.cpp:
+        (WebCore::SelectorChecker::checkOne):
+        * css/SelectorFilter.cpp:
+        (WebCore::collectElementIdentifierHashes):
+        * dom/Element.h:
+        (WebCore::Element::idForStyleResolution):
+        * rendering/RenderBlockFlow.cpp:
+        (WebCore::needsAppleMailPaginationQuirk):
+        * rendering/RenderTreeAsText.cpp:
+        (WebCore::writeRenderRegionList):
+        * style/StyleSharingResolver.cpp:
+        (WebCore::Style::SharingResolver::canShareStyleWithElement):
+
 2016-04-21  Brady Eidson  <beidson@apple.com>
 

trunk/Source/WebCore/css/ElementRuleCollector.cpp

@@ -154 +154 @@
     // We need to collect the rules for id, class, tag, and everything else into a buffer and
     // then sort the buffer.
-    if (m_element.hasID())
-        collectMatchingRulesForList(matchRequest.ruleSet->idRules(m_element.idForStyleResolution().impl()), matchRequest, ruleRange);
+    auto& id = m_element.idForStyleResolution();
+    if (!id.isNull())
+        collectMatchingRulesForList(matchRequest.ruleSet->idRules(*id.impl()), matchRequest, ruleRange);
     if (m_element.hasClass()) {
         for (size_t i = 0; i < m_element.classNames().size(); ++i)

trunk/Source/WebCore/css/RuleSet.h

@@ -175 +175 @@
     const RuleFeatureSet& features() const { return m_features; }
 
-    const RuleDataVector* idRules(AtomicStringImpl* key) const { return m_idRules.get(key); }
+    const RuleDataVector* idRules(AtomicStringImpl& key) const { return m_idRules.get(&key); }
     const RuleDataVector* classRules(AtomicStringImpl* key) const { return m_classRules.get(key); }
     const RuleDataVector* tagRules(AtomicStringImpl* key, bool isHTMLName) const;

trunk/Source/WebCore/css/SelectorChecker.cpp

@@ -653 +653 @@
         return element.hasClass() && element.classNames().contains(selector.value());
 
-    if (selector.match() == CSSSelector::Id)
-        return element.hasID() && element.idForStyleResolution() == selector.value();
+    if (selector.match() == CSSSelector::Id) {
+        ASSERT(!selector.value().isNull());
+        return element.idForStyleResolution() == selector.value();
+    }
 
     if (selector.isAttributeSelector()) {

trunk/Source/WebCore/css/SelectorFilter.cpp

@@ -44 +44 @@
     identifierHashes.append(tagLowercaseLocalName.impl()->existingHash() * TagNameSalt);
 
-    if (element->hasID())
-        identifierHashes.append(element->idForStyleResolution().impl()->existingHash() * IdAttributeSalt);
+    auto& id = element->idForStyleResolution();
+    if (!id.isNull())
+        identifierHashes.append(id.impl()->existingHash() * IdAttributeSalt);
     const StyledElement* styledElement = element->isStyledElement() ? static_cast<const StyledElement*>(element) : 0;
     if (styledElement && styledElement->hasClass()) {

trunk/Source/WebCore/dom/Element.h

@@ -665 +665 @@
 inline const AtomicString& Element::idForStyleResolution() const
 {
-    ASSERT(hasID());
-    return elementData()->idForStyleResolution();
+    return hasID() ? elementData()->idForStyleResolution() : nullAtom;
 }
 

trunk/Source/WebCore/rendering/RenderBlockFlow.cpp

@@ -1644 +1644 @@
         return false;
 
-    if (renderer.element() && renderer.element()->hasID() && renderer.element()->idForStyleResolution() == "messageContentContainer")
+    if (renderer.element() && renderer.element()->idForStyleResolution() == "messageContentContainer")
         return true;
 

trunk/Source/WebCore/rendering/RenderTreeAsText.cpp

@@ -691 +691 @@
             ts << " {" << tagName.toString() << "}";
 
-            if (generatingElement->hasID())
-                ts << " #" << generatingElement->idForStyleResolution();
+            auto& generatingElementId = generatingElement->idForStyleResolution();
+            if (!generatingElementId.isNull())
+                ts << " #" << generatingElementId;
 
             if (isRenderNamedFlowFragment)

trunk/Source/WebCore/style/StyleSharingResolver.cpp

@@ -89 +89 @@
         return nullptr;
     // Ids stop style sharing if they show up in the stylesheets.
-    if (element.hasID() && m_ruleSets.features().idsInRules.contains(element.idForStyleResolution().impl()))
+    auto& id = element.idForStyleResolution();
+    if (!id.isNull() && m_ruleSets.features().idsInRules.contains(id.impl()))
         return nullptr;
     if (parentElementPreventsSharing(parentElement))
@@ -238 +239 @@
         return false;
 
-    if (candidateElement.hasID() && m_ruleSets.features().idsInRules.contains(candidateElement.idForStyleResolution().impl()))
+    auto& candidateElementId = candidateElement.idForStyleResolution();
+    if (!candidateElementId.isNull() && m_ruleSets.features().idsInRules.contains(candidateElementId.impl()))
         return false;