Changeset 204220 in webkit


Ignore:
Timestamp:
Aug 5, 2016 10:37:28 PM (8 years ago)
Author:
commit-queue@webkit.org
Message:

NULL Reference Error in ElementRuleCollector
https://bugs.webkit.org/show_bug.cgi?id=160362

Patch by Jonathan Bedard <Jonathan Bedard> on 2016-08-05
Reviewed by Darin Adler.

No new tests, existing CSS tests cover this change.

Undefined behavior sanitizer found a reference bound to a NULL pointer.
The root cause of this issue was a discrepancy between whether an author style needed to be defined. In some logic, an undefined author style was considered acceptable, but in other logic, author style was always assumed to be defined. To fix this, a variable was added so that while author style is always defined, there is a flag indicating if this definition occurred in the constructor for use by functions which allow an undefined author style.

  • css/DocumentRuleSets.cpp:

(WebCore::DocumentRuleSets::DocumentRuleSets): Define author style by default.
(WebCore::DocumentRuleSets::resetAuthorStyle): Switch author style flag.

  • css/DocumentRuleSets.h: Added author style flag, changed authorStyle accessor to reference from pointer.
  • css/ElementRuleCollector.cpp:

(WebCore::ElementRuleCollector::ElementRuleCollector): Original location of undefined behavior.
(WebCore::ElementRuleCollector::matchHostPseudoClassRules): Changed pointer to reference.
(WebCore::ElementRuleCollector::matchSlottedPseudoElementRules): Changed pointer to reference.

  • css/PageRuleCollector.cpp:

(WebCore::PageRuleCollector::matchAllPageRules): Check new flag, changed pointer to reference.

  • css/StyleResolver.cpp: Changed pointer to reference.
  • dom/Document.cpp: Dito.
  • style/AttributeChangeInvalidation.cpp: Dito.
  • style/ClassChangeInvalidation.cpp: Dito.
  • style/IdChangeInvalidation.cpp: Dito.
  • style/StyleSharingResolver.cpp: Dito.
Location:
trunk/Source/WebCore
Files:
12 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebCore/ChangeLog

    r204219 r204220  
     12016-08-05  Jonathan Bedard  <jbedard@apple.com>
     2
     3        NULL Reference Error in ElementRuleCollector
     4        https://bugs.webkit.org/show_bug.cgi?id=160362
     5
     6        Reviewed by Darin Adler.
     7
     8        No new tests, existing CSS tests cover this change.
     9
     10        Undefined behavior sanitizer found a reference bound to a NULL pointer.
     11        The root cause of this issue was a discrepancy between whether an author style needed to be defined.  In some logic, an undefined author style was considered acceptable, but in other logic, author style was always assumed to be defined.  To fix this, a variable was added so that while author style is always defined, there is a flag indicating if this definition occurred in the constructor for use by functions which allow an undefined author style.
     12
     13        * css/DocumentRuleSets.cpp:
     14        (WebCore::DocumentRuleSets::DocumentRuleSets): Define author style by default.
     15        (WebCore::DocumentRuleSets::resetAuthorStyle): Switch author style flag.
     16        * css/DocumentRuleSets.h: Added author style flag, changed authorStyle accessor to reference from pointer.
     17        * css/ElementRuleCollector.cpp:
     18        (WebCore::ElementRuleCollector::ElementRuleCollector): Original location of undefined behavior.
     19        (WebCore::ElementRuleCollector::matchHostPseudoClassRules): Changed pointer to reference.
     20        (WebCore::ElementRuleCollector::matchSlottedPseudoElementRules): Changed pointer to reference.
     21        * css/PageRuleCollector.cpp:
     22        (WebCore::PageRuleCollector::matchAllPageRules): Check new flag, changed pointer to reference.
     23
     24        * css/StyleResolver.cpp: Changed pointer to reference.
     25        * dom/Document.cpp: Dito.
     26        * style/AttributeChangeInvalidation.cpp: Dito.
     27        * style/ClassChangeInvalidation.cpp: Dito.
     28        * style/IdChangeInvalidation.cpp: Dito.
     29        * style/StyleSharingResolver.cpp: Dito.
     30
    1312016-08-05  Chris Dumez  <cdumez@apple.com>
    232
  • trunk/Source/WebCore/css/DocumentRuleSets.cpp

    r202104 r204220  
    4040DocumentRuleSets::DocumentRuleSets()
    4141{
     42    m_authorStyle = std::make_unique<RuleSet>();
     43    m_authorStyle->disableAutoShrinkToFit();
    4244}
    4345
     
    7981void DocumentRuleSets::resetAuthorStyle()
    8082{
     83    m_isAuthorStyleDefined = true;
    8184    m_authorStyle = std::make_unique<RuleSet>();
    8285    m_authorStyle->disableAutoShrinkToFit();
  • trunk/Source/WebCore/css/DocumentRuleSets.h

    r199735 r204220  
    4545    DocumentRuleSets();
    4646    ~DocumentRuleSets();
    47     RuleSet* authorStyle() const { return m_authorStyle.get(); }
     47   
     48    bool isAuthorStyleDefined() const { return m_isAuthorStyleDefined; }
     49    RuleSet& authorStyle() const { return *m_authorStyle.get(); }
    4850    RuleSet* userStyle() const { return m_userStyle.get(); }
    4951    const RuleFeatureSet& features() const;
     
    7072    void collectRulesFromUserStyleSheets(const Vector<RefPtr<CSSStyleSheet>>&, RuleSet& userStyle, const MediaQueryEvaluator&, StyleResolver&);
    7173
     74    bool m_isAuthorStyleDefined { false };
    7275    std::unique_ptr<RuleSet> m_authorStyle;
    7376    std::unique_ptr<RuleSet> m_userStyle;
  • trunk/Source/WebCore/css/ElementRuleCollector.cpp

    r202091 r204220  
    8383ElementRuleCollector::ElementRuleCollector(const Element& element, const DocumentRuleSets& ruleSets, const SelectorFilter* selectorFilter)
    8484    : m_element(element)
    85     , m_authorStyle(*ruleSets.authorStyle())
     85    , m_authorStyle(ruleSets.authorStyle())
    8686    , m_userStyle(ruleSets.userStyle())
    8787    , m_selectorFilter(selectorFilter)
     
    235235    matchRequest.treeContextOrdinal++;
    236236
    237     auto& shadowAuthorStyle = *m_element.shadowRoot()->styleResolver().ruleSets().authorStyle();
     237    auto& shadowAuthorStyle = m_element.shadowRoot()->styleResolver().ruleSets().authorStyle();
    238238    auto& shadowHostRules = shadowAuthorStyle.hostPseudoClassRules();
    239239    if (shadowHostRules.isEmpty())
     
    266266        // In nested case the slot may itself be assigned to a slot. Collect ::slotted rules from all the nested trees.
    267267        maybeSlotted = slot;
    268         auto* shadowAuthorStyle = hostShadowRoot->styleResolver().ruleSets().authorStyle();
    269         if (!shadowAuthorStyle)
     268        if (!hostShadowRoot->styleResolver().ruleSets().isAuthorStyleDefined())
    270269            continue;
    271270        // Find out if there are any ::slotted rules in the shadow tree matching the current slot.
    272271        // FIXME: This is really part of the slot style and could be cached when resolving it.
    273         ElementRuleCollector collector(*slot, *shadowAuthorStyle, nullptr);
     272        ElementRuleCollector collector(*slot, hostShadowRoot->styleResolver().ruleSets().authorStyle(), nullptr);
    274273        auto slottedPseudoElementRules = collector.collectSlottedPseudoElementRulesForSlot(matchRequest.includeEmptyRules);
    275274        if (!slottedPseudoElementRules)
  • trunk/Source/WebCore/css/PageRuleCollector.cpp

    r203322 r204220  
    7171    matchPageRules(m_ruleSets.userStyle(), isLeft, isFirst, page);
    7272    // Only consider the global author RuleSet for @page rules, as per the HTML5 spec.
    73     matchPageRules(m_ruleSets.authorStyle(), isLeft, isFirst, page);
     73    if (m_ruleSets.isAuthorStyleDefined())
     74        matchPageRules(&m_ruleSets.authorStyle(), isLeft, isFirst, page);
    7475}
    7576
  • trunk/Source/WebCore/css/StyleResolver.cpp

    r202738 r204220  
    10621062bool StyleResolver::checkRegionStyle(const Element* regionElement)
    10631063{
    1064     unsigned rulesSize = m_ruleSets.authorStyle()->regionSelectorsAndRuleSets().size();
     1064    unsigned rulesSize = m_ruleSets.authorStyle().regionSelectorsAndRuleSets().size();
    10651065    for (unsigned i = 0; i < rulesSize; ++i) {
    1066         ASSERT(m_ruleSets.authorStyle()->regionSelectorsAndRuleSets().at(i).ruleSet.get());
    1067         if (checkRegionSelector(m_ruleSets.authorStyle()->regionSelectorsAndRuleSets().at(i).selector, regionElement))
     1066        ASSERT(m_ruleSets.authorStyle().regionSelectorsAndRuleSets().at(i).ruleSet.get());
     1067        if (checkRegionSelector(m_ruleSets.authorStyle().regionSelectorsAndRuleSets().at(i).selector, regionElement))
    10681068            return true;
    10691069    }
  • trunk/Source/WebCore/dom/AuthorStyleSheets.cpp

    r203601 r204220  
    358358
    359359    userAgentShadowTreeStyleResolver.ruleSets().resetAuthorStyle();
    360     auto& authorRuleSet = *styleResolver.ruleSets().authorStyle();
     360    auto& authorRuleSet = styleResolver.ruleSets().authorStyle();
    361361    if (authorRuleSet.hasShadowPseudoElementRules())
    362         userAgentShadowTreeStyleResolver.ruleSets().authorStyle()->copyShadowPseudoElementRulesFrom(authorRuleSet);
     362        userAgentShadowTreeStyleResolver.ruleSets().authorStyle().copyShadowPseudoElementRulesFrom(authorRuleSet);
    363363}
    364364
  • trunk/Source/WebCore/dom/Document.cpp

    r204196 r204220  
    22062206
    22072207        // FIXME: Filter out shadow pseudo elements we don't want to expose to authors.
    2208         auto& documentAuthorStyle = *ensureStyleResolver().ruleSets().authorStyle();
     2208        auto& documentAuthorStyle = ensureStyleResolver().ruleSets().authorStyle();
    22092209        if (documentAuthorStyle.hasShadowPseudoElementRules())
    2210             m_userAgentShadowTreeStyleResolver->ruleSets().authorStyle()->copyShadowPseudoElementRulesFrom(documentAuthorStyle);
     2210            m_userAgentShadowTreeStyleResolver->ruleSets().authorStyle().copyShadowPseudoElementRulesFrom(documentAuthorStyle);
    22112211    }
    22122212
  • trunk/Source/WebCore/style/AttributeChangeInvalidation.cpp

    r202227 r204220  
    3939{
    4040    auto& shadowRuleSets = shadowRoot.styleResolver().ruleSets();
    41     if (shadowRuleSets.authorStyle()->hostPseudoClassRules().isEmpty())
     41    if (shadowRuleSets.authorStyle().hostPseudoClassRules().isEmpty())
    4242        return false;
    4343
     
    6969    }
    7070
    71     if (m_element.shadowRoot() && ruleSets.authorStyle()->hasShadowPseudoElementRules()) {
     71    if (m_element.shadowRoot() && ruleSets.authorStyle().hasShadowPseudoElementRules()) {
    7272        m_element.setNeedsStyleRecalc(FullStyleChange);
    7373        return;
  • trunk/Source/WebCore/style/ClassChangeInvalidation.cpp

    r202227 r204220  
    8989{
    9090    auto& shadowRuleSets = shadowRoot.styleResolver().ruleSets();
    91     if (shadowRuleSets.authorStyle()->hostPseudoClassRules().isEmpty())
     91    if (shadowRuleSets.authorStyle().hostPseudoClassRules().isEmpty())
    9292        return false;
    9393    return shadowRuleSets.features().classesInRules.contains(changedClass);
     
    115115        return;
    116116
    117     if (shadowRoot && ruleSets.authorStyle()->hasShadowPseudoElementRules()) {
     117    if (shadowRoot && ruleSets.authorStyle().hasShadowPseudoElementRules()) {
    118118        m_element.setNeedsStyleRecalc(FullStyleChange);
    119119        return;
  • trunk/Source/WebCore/style/IdChangeInvalidation.cpp

    r202227 r204220  
    3838{
    3939    auto& shadowRuleSets = shadowRoot.styleResolver().ruleSets();
    40     if (shadowRuleSets.authorStyle()->hostPseudoClassRules().isEmpty())
     40    if (shadowRuleSets.authorStyle().hostPseudoClassRules().isEmpty())
    4141        return false;
    4242
     
    6060        return;
    6161
    62     if (m_element.shadowRoot() && ruleSets.authorStyle()->hasShadowPseudoElementRules()) {
     62    if (m_element.shadowRoot() && ruleSets.authorStyle().hasShadowPseudoElementRules()) {
    6363        m_element.setNeedsStyleRecalc(FullStyleChange);
    6464        return;
  • trunk/Source/WebCore/style/StyleSharingResolver.cpp

    r203324 r204220  
    9898    if (elementHasDirectionAuto(element))
    9999        return nullptr;
    100     if (element.shadowRoot() && !element.shadowRoot()->styleResolver().ruleSets().authorStyle()->hostPseudoClassRules().isEmpty())
     100    if (element.shadowRoot() && !element.shadowRoot()->styleResolver().ruleSets().authorStyle().hostPseudoClassRules().isEmpty())
    101101        return nullptr;
    102102
     
    287287        return false;
    288288
    289     if (element.shadowRoot() && !element.shadowRoot()->styleResolver().ruleSets().authorStyle()->hostPseudoClassRules().isEmpty())
     289    if (element.shadowRoot() && !element.shadowRoot()->styleResolver().ruleSets().authorStyle().hostPseudoClassRules().isEmpty())
    290290        return false;
    291291
Note: See TracChangeset for help on using the changeset viewer.