- Revision
- 204220
- Author
- [email protected]
- Date
- 2016-08-05 22:37:28 -0700 (Fri, 05 Aug 2016)
Log Message
NULL Reference Error in ElementRuleCollector
https://bugs.webkit.org/show_bug.cgi?id=160362
Patch by Jonathan Bedard <[email protected]> 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.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (204219 => 204220)
--- trunk/Source/WebCore/ChangeLog 2016-08-06 05:34:57 UTC (rev 204219)
+++ trunk/Source/WebCore/ChangeLog 2016-08-06 05:37:28 UTC (rev 204220)
@@ -1,3 +1,33 @@
+2016-08-05 Jonathan Bedard <[email protected]>
+
+ NULL Reference Error in ElementRuleCollector
+ https://bugs.webkit.org/show_bug.cgi?id=160362
+
+ 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.
+
2016-08-05 Chris Dumez <[email protected]>
DOMException should be constructible
Modified: trunk/Source/WebCore/css/DocumentRuleSets.cpp (204219 => 204220)
--- trunk/Source/WebCore/css/DocumentRuleSets.cpp 2016-08-06 05:34:57 UTC (rev 204219)
+++ trunk/Source/WebCore/css/DocumentRuleSets.cpp 2016-08-06 05:37:28 UTC (rev 204220)
@@ -39,6 +39,8 @@
DocumentRuleSets::DocumentRuleSets()
{
+ m_authorStyle = std::make_unique<RuleSet>();
+ m_authorStyle->disableAutoShrinkToFit();
}
DocumentRuleSets::~DocumentRuleSets()
@@ -78,6 +80,7 @@
void DocumentRuleSets::resetAuthorStyle()
{
+ m_isAuthorStyleDefined = true;
m_authorStyle = std::make_unique<RuleSet>();
m_authorStyle->disableAutoShrinkToFit();
}
Modified: trunk/Source/WebCore/css/DocumentRuleSets.h (204219 => 204220)
--- trunk/Source/WebCore/css/DocumentRuleSets.h 2016-08-06 05:34:57 UTC (rev 204219)
+++ trunk/Source/WebCore/css/DocumentRuleSets.h 2016-08-06 05:37:28 UTC (rev 204220)
@@ -44,7 +44,9 @@
public:
DocumentRuleSets();
~DocumentRuleSets();
- RuleSet* authorStyle() const { return m_authorStyle.get(); }
+
+ bool isAuthorStyleDefined() const { return m_isAuthorStyleDefined; }
+ RuleSet& authorStyle() const { return *m_authorStyle.get(); }
RuleSet* userStyle() const { return m_userStyle.get(); }
const RuleFeatureSet& features() const;
RuleSet* sibling() const { return m_siblingRuleSet.get(); }
@@ -69,6 +71,7 @@
void collectFeatures() const;
void collectRulesFromUserStyleSheets(const Vector<RefPtr<CSSStyleSheet>>&, RuleSet& userStyle, const MediaQueryEvaluator&, StyleResolver&);
+ bool m_isAuthorStyleDefined { false };
std::unique_ptr<RuleSet> m_authorStyle;
std::unique_ptr<RuleSet> m_userStyle;
Modified: trunk/Source/WebCore/css/ElementRuleCollector.cpp (204219 => 204220)
--- trunk/Source/WebCore/css/ElementRuleCollector.cpp 2016-08-06 05:34:57 UTC (rev 204219)
+++ trunk/Source/WebCore/css/ElementRuleCollector.cpp 2016-08-06 05:37:28 UTC (rev 204220)
@@ -82,7 +82,7 @@
ElementRuleCollector::ElementRuleCollector(const Element& element, const DocumentRuleSets& ruleSets, const SelectorFilter* selectorFilter)
: m_element(element)
- , m_authorStyle(*ruleSets.authorStyle())
+ , m_authorStyle(ruleSets.authorStyle())
, m_userStyle(ruleSets.userStyle())
, m_selectorFilter(selectorFilter)
{
@@ -234,7 +234,7 @@
matchRequest.treeContextOrdinal++;
- auto& shadowAuthorStyle = *m_element.shadowRoot()->styleResolver().ruleSets().authorStyle();
+ auto& shadowAuthorStyle = m_element.shadowRoot()->styleResolver().ruleSets().authorStyle();
auto& shadowHostRules = shadowAuthorStyle.hostPseudoClassRules();
if (shadowHostRules.isEmpty())
return;
@@ -265,12 +265,11 @@
// In nested case the slot may itself be assigned to a slot. Collect ::slotted rules from all the nested trees.
maybeSlotted = slot;
- auto* shadowAuthorStyle = hostShadowRoot->styleResolver().ruleSets().authorStyle();
- if (!shadowAuthorStyle)
+ if (!hostShadowRoot->styleResolver().ruleSets().isAuthorStyleDefined())
continue;
// Find out if there are any ::slotted rules in the shadow tree matching the current slot.
// FIXME: This is really part of the slot style and could be cached when resolving it.
- ElementRuleCollector collector(*slot, *shadowAuthorStyle, nullptr);
+ ElementRuleCollector collector(*slot, hostShadowRoot->styleResolver().ruleSets().authorStyle(), nullptr);
auto slottedPseudoElementRules = collector.collectSlottedPseudoElementRulesForSlot(matchRequest.includeEmptyRules);
if (!slottedPseudoElementRules)
continue;
Modified: trunk/Source/WebCore/css/PageRuleCollector.cpp (204219 => 204220)
--- trunk/Source/WebCore/css/PageRuleCollector.cpp 2016-08-06 05:34:57 UTC (rev 204219)
+++ trunk/Source/WebCore/css/PageRuleCollector.cpp 2016-08-06 05:37:28 UTC (rev 204220)
@@ -70,7 +70,8 @@
matchPageRules(CSSDefaultStyleSheets::defaultPrintStyle, isLeft, isFirst, page);
matchPageRules(m_ruleSets.userStyle(), isLeft, isFirst, page);
// Only consider the global author RuleSet for @page rules, as per the HTML5 spec.
- matchPageRules(m_ruleSets.authorStyle(), isLeft, isFirst, page);
+ if (m_ruleSets.isAuthorStyleDefined())
+ matchPageRules(&m_ruleSets.authorStyle(), isLeft, isFirst, page);
}
void PageRuleCollector::matchPageRules(RuleSet* rules, bool isLeftPage, bool isFirstPage, const String& pageName)
Modified: trunk/Source/WebCore/css/StyleResolver.cpp (204219 => 204220)
--- trunk/Source/WebCore/css/StyleResolver.cpp 2016-08-06 05:34:57 UTC (rev 204219)
+++ trunk/Source/WebCore/css/StyleResolver.cpp 2016-08-06 05:37:28 UTC (rev 204220)
@@ -1061,10 +1061,10 @@
bool StyleResolver::checkRegionStyle(const Element* regionElement)
{
- unsigned rulesSize = m_ruleSets.authorStyle()->regionSelectorsAndRuleSets().size();
+ unsigned rulesSize = m_ruleSets.authorStyle().regionSelectorsAndRuleSets().size();
for (unsigned i = 0; i < rulesSize; ++i) {
- ASSERT(m_ruleSets.authorStyle()->regionSelectorsAndRuleSets().at(i).ruleSet.get());
- if (checkRegionSelector(m_ruleSets.authorStyle()->regionSelectorsAndRuleSets().at(i).selector, regionElement))
+ ASSERT(m_ruleSets.authorStyle().regionSelectorsAndRuleSets().at(i).ruleSet.get());
+ if (checkRegionSelector(m_ruleSets.authorStyle().regionSelectorsAndRuleSets().at(i).selector, regionElement))
return true;
}
Modified: trunk/Source/WebCore/dom/AuthorStyleSheets.cpp (204219 => 204220)
--- trunk/Source/WebCore/dom/AuthorStyleSheets.cpp 2016-08-06 05:34:57 UTC (rev 204219)
+++ trunk/Source/WebCore/dom/AuthorStyleSheets.cpp 2016-08-06 05:37:28 UTC (rev 204220)
@@ -357,9 +357,9 @@
}
userAgentShadowTreeStyleResolver.ruleSets().resetAuthorStyle();
- auto& authorRuleSet = *styleResolver.ruleSets().authorStyle();
+ auto& authorRuleSet = styleResolver.ruleSets().authorStyle();
if (authorRuleSet.hasShadowPseudoElementRules())
- userAgentShadowTreeStyleResolver.ruleSets().authorStyle()->copyShadowPseudoElementRulesFrom(authorRuleSet);
+ userAgentShadowTreeStyleResolver.ruleSets().authorStyle().copyShadowPseudoElementRulesFrom(authorRuleSet);
}
const Vector<RefPtr<CSSStyleSheet>> AuthorStyleSheets::activeStyleSheetsForInspector() const
Modified: trunk/Source/WebCore/dom/Document.cpp (204219 => 204220)
--- trunk/Source/WebCore/dom/Document.cpp 2016-08-06 05:34:57 UTC (rev 204219)
+++ trunk/Source/WebCore/dom/Document.cpp 2016-08-06 05:37:28 UTC (rev 204220)
@@ -2205,9 +2205,9 @@
m_userAgentShadowTreeStyleResolver = std::make_unique<StyleResolver>(*this);
// FIXME: Filter out shadow pseudo elements we don't want to expose to authors.
- auto& documentAuthorStyle = *ensureStyleResolver().ruleSets().authorStyle();
+ auto& documentAuthorStyle = ensureStyleResolver().ruleSets().authorStyle();
if (documentAuthorStyle.hasShadowPseudoElementRules())
- m_userAgentShadowTreeStyleResolver->ruleSets().authorStyle()->copyShadowPseudoElementRulesFrom(documentAuthorStyle);
+ m_userAgentShadowTreeStyleResolver->ruleSets().authorStyle().copyShadowPseudoElementRulesFrom(documentAuthorStyle);
}
return *m_userAgentShadowTreeStyleResolver;
Modified: trunk/Source/WebCore/style/AttributeChangeInvalidation.cpp (204219 => 204220)
--- trunk/Source/WebCore/style/AttributeChangeInvalidation.cpp 2016-08-06 05:34:57 UTC (rev 204219)
+++ trunk/Source/WebCore/style/AttributeChangeInvalidation.cpp 2016-08-06 05:37:28 UTC (rev 204220)
@@ -38,7 +38,7 @@
static bool mayBeAffectedByHostStyle(ShadowRoot& shadowRoot, bool isHTML, const QualifiedName& attributeName)
{
auto& shadowRuleSets = shadowRoot.styleResolver().ruleSets();
- if (shadowRuleSets.authorStyle()->hostPseudoClassRules().isEmpty())
+ if (shadowRuleSets.authorStyle().hostPseudoClassRules().isEmpty())
return false;
auto& nameSet = isHTML ? shadowRuleSets.features().attributeCanonicalLocalNamesInRules : shadowRuleSets.features().attributeLocalNamesInRules;
@@ -68,7 +68,7 @@
return;
}
- if (m_element.shadowRoot() && ruleSets.authorStyle()->hasShadowPseudoElementRules()) {
+ if (m_element.shadowRoot() && ruleSets.authorStyle().hasShadowPseudoElementRules()) {
m_element.setNeedsStyleRecalc(FullStyleChange);
return;
}
Modified: trunk/Source/WebCore/style/ClassChangeInvalidation.cpp (204219 => 204220)
--- trunk/Source/WebCore/style/ClassChangeInvalidation.cpp 2016-08-06 05:34:57 UTC (rev 204219)
+++ trunk/Source/WebCore/style/ClassChangeInvalidation.cpp 2016-08-06 05:37:28 UTC (rev 204220)
@@ -88,7 +88,7 @@
static bool mayBeAffectedByHostStyle(ShadowRoot& shadowRoot, AtomicStringImpl* changedClass)
{
auto& shadowRuleSets = shadowRoot.styleResolver().ruleSets();
- if (shadowRuleSets.authorStyle()->hostPseudoClassRules().isEmpty())
+ if (shadowRuleSets.authorStyle().hostPseudoClassRules().isEmpty())
return false;
return shadowRuleSets.features().classesInRules.contains(changedClass);
}
@@ -114,7 +114,7 @@
if (changedClassesAffectingStyle.isEmpty())
return;
- if (shadowRoot && ruleSets.authorStyle()->hasShadowPseudoElementRules()) {
+ if (shadowRoot && ruleSets.authorStyle().hasShadowPseudoElementRules()) {
m_element.setNeedsStyleRecalc(FullStyleChange);
return;
}
Modified: trunk/Source/WebCore/style/IdChangeInvalidation.cpp (204219 => 204220)
--- trunk/Source/WebCore/style/IdChangeInvalidation.cpp 2016-08-06 05:34:57 UTC (rev 204219)
+++ trunk/Source/WebCore/style/IdChangeInvalidation.cpp 2016-08-06 05:37:28 UTC (rev 204220)
@@ -37,7 +37,7 @@
static bool mayBeAffectedByHostStyle(ShadowRoot& shadowRoot, const AtomicString& changedId)
{
auto& shadowRuleSets = shadowRoot.styleResolver().ruleSets();
- if (shadowRuleSets.authorStyle()->hostPseudoClassRules().isEmpty())
+ if (shadowRuleSets.authorStyle().hostPseudoClassRules().isEmpty())
return false;
return shadowRuleSets.features().idsInRules.contains(changedId.impl());
@@ -59,7 +59,7 @@
if (!mayAffectStyle)
return;
- if (m_element.shadowRoot() && ruleSets.authorStyle()->hasShadowPseudoElementRules()) {
+ if (m_element.shadowRoot() && ruleSets.authorStyle().hasShadowPseudoElementRules()) {
m_element.setNeedsStyleRecalc(FullStyleChange);
return;
}
Modified: trunk/Source/WebCore/style/StyleSharingResolver.cpp (204219 => 204220)
--- trunk/Source/WebCore/style/StyleSharingResolver.cpp 2016-08-06 05:34:57 UTC (rev 204219)
+++ trunk/Source/WebCore/style/StyleSharingResolver.cpp 2016-08-06 05:37:28 UTC (rev 204220)
@@ -97,7 +97,7 @@
return nullptr;
if (elementHasDirectionAuto(element))
return nullptr;
- if (element.shadowRoot() && !element.shadowRoot()->styleResolver().ruleSets().authorStyle()->hostPseudoClassRules().isEmpty())
+ if (element.shadowRoot() && !element.shadowRoot()->styleResolver().ruleSets().authorStyle().hostPseudoClassRules().isEmpty())
return nullptr;
Context context {
@@ -286,7 +286,7 @@
if (candidateElement.matchesDefaultPseudoClass() != element.matchesDefaultPseudoClass())
return false;
- if (element.shadowRoot() && !element.shadowRoot()->styleResolver().ruleSets().authorStyle()->hostPseudoClassRules().isEmpty())
+ if (element.shadowRoot() && !element.shadowRoot()->styleResolver().ruleSets().authorStyle().hostPseudoClassRules().isEmpty())
return false;
#if ENABLE(FULLSCREEN_API)
