Attachment 347051 Details for Bug 188539 – Patch

[patch] Patch

bug-188539-20180813154019.patch (text/plain), 17.73 KB, created by Alex Christensen on 2018-08-13 15:40:21 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Alex Christensen

Created: 2018-08-13 15:40:21 PDT

Size: 17.73 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 234821)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,46 @@
>+2018-08-13  Alex Christensen  <achristensen@webkit.org>
>+
>+        Make CSSSelectorList a little more sane
>+        https://bugs.webkit.org/show_bug.cgi?id=188539
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        This patch does four things:
>+        1. Use a UniqueArray<CSSSelector> instead of a raw pointer and manually calling destructors.
>+        2. Use move semantics a little bit better.
>+        3. Add a CSSSelectorList&& to the StyleRule and StyleRulePage because every time we create either
>+        one of those objects we call a setter to give it a CSSSelectorList.  That's what constructor arguments are for.
>+        4. Don't use CSSSelectorList.componentCount(), which iterates all components, to determine if it's empty.
>+        Use first() instead.
>+
>+        * css/CSSPageRule.cpp:
>+        (WebCore::CSSPageRule::setSelectorText):
>+        * css/CSSSelectorList.cpp:
>+        (WebCore::CSSSelectorList::CSSSelectorList):
>+        (WebCore::CSSSelectorList::componentCount const):
>+        (WebCore::CSSSelectorList::listSize const):
>+        (WebCore::CSSSelectorList::operator=):
>+        (WebCore::CSSSelectorList::deleteSelectors): Deleted.
>+        * css/CSSSelectorList.h:
>+        (WebCore::CSSSelectorList::CSSSelectorList):
>+        (WebCore::CSSSelectorList::first const):
>+        (WebCore::CSSSelectorList::indexOfNextSelectorAfter const):
>+        (WebCore::CSSSelectorList::~CSSSelectorList): Deleted.
>+        (WebCore::CSSSelectorList::adoptSelectorArray): Deleted.
>+        (WebCore::CSSSelectorList::hasOneSelector const): Deleted.
>+        * css/CSSStyleRule.cpp:
>+        (WebCore::CSSStyleRule::setSelectorText):
>+        * css/StyleRule.cpp:
>+        (WebCore::StyleRule::StyleRule):
>+        (WebCore::StyleRule::createForSplitting):
>+        (WebCore::StyleRulePage::StyleRulePage):
>+        * css/StyleRule.h:
>+        * css/parser/CSSParserImpl.cpp:
>+        (WebCore::CSSParserImpl::consumePageRule):
>+        (WebCore::CSSParserImpl::consumeStyleRule):
>+        * css/parser/CSSSelectorParser.cpp:
>+        (WebCore::CSSSelectorParser::consumePseudo):
>+
> 2018-08-13  Ali Juma  <ajuma@chromium.org>
> 
>         [IntersectionObserver] Validate threshold values
>Index: Source/WebCore/css/CSSPageRule.cpp
>===================================================================
>--- Source/WebCore/css/CSSPageRule.cpp	(revision 234813)
>+++ Source/WebCore/css/CSSPageRule.cpp	(working copy)
>@@ -77,7 +77,7 @@ void CSSPageRule::setSelectorText(const 
> 
>     CSSStyleSheet::RuleMutationScope mutationScope(this);
> 
>-    m_pageRule->wrapperAdoptSelectorList(selectorList);
>+    m_pageRule->wrapperAdoptSelectorList(WTFMove(selectorList));
> }
> 
> String CSSPageRule::cssText() const
>Index: Source/WebCore/css/CSSSelectorList.cpp
>===================================================================
>--- Source/WebCore/css/CSSSelectorList.cpp	(revision 234814)
>+++ Source/WebCore/css/CSSSelectorList.cpp	(working copy)
>@@ -37,15 +37,14 @@ CSSSelectorList::CSSSelectorList(const C
>     unsigned otherComponentCount = other.componentCount();
>     ASSERT_WITH_SECURITY_IMPLICATION(otherComponentCount);
> 
>-    m_selectorArray = reinterpret_cast<CSSSelector*>(fastMalloc(sizeof(CSSSelector) * otherComponentCount));
>+    m_selectorArray = makeUniqueArray<CSSSelector>(otherComponentCount);
>     for (unsigned i = 0; i < otherComponentCount; ++i)
>         new (NotNull, &m_selectorArray[i]) CSSSelector(other.m_selectorArray[i]);
> }
> 
> CSSSelectorList::CSSSelectorList(CSSSelectorList&& other)
>-    : m_selectorArray(other.m_selectorArray)
>+    : m_selectorArray(WTFMove(other.m_selectorArray))
> {
>-    other.m_selectorArray = nullptr;
> }
> 
> CSSSelectorList::CSSSelectorList(Vector<std::unique_ptr<CSSParserSelector>>&& selectorVector)
>@@ -58,7 +57,7 @@ CSSSelectorList::CSSSelectorList(Vector<
>             ++flattenedSize;
>     }
>     ASSERT(flattenedSize);
>-    m_selectorArray = reinterpret_cast<CSSSelector*>(fastMalloc(sizeof(CSSSelector) * flattenedSize));
>+    m_selectorArray = makeUniqueArray<CSSSelector>(flattenedSize);
>     size_t arrayIndex = 0;
>     for (size_t i = 0; i < selectorVector.size(); ++i) {
>         CSSParserSelector* current = selectorVector[i].get();
>@@ -87,10 +86,10 @@ unsigned CSSSelectorList::componentCount
> {
>     if (!m_selectorArray)
>         return 0;
>-    CSSSelector* current = m_selectorArray;
>+    CSSSelector* current = m_selectorArray.get();
>     while (!current->isLastInSelectorList())
>         ++current;
>-    return (current - m_selectorArray) + 1;
>+    return (current - m_selectorArray.get()) + 1;
> }
> 
> unsigned CSSSelectorList::listSize() const
>@@ -98,7 +97,7 @@ unsigned CSSSelectorList::listSize() con
>     if (!m_selectorArray)
>         return 0;
>     unsigned size = 1;
>-    CSSSelector* current = m_selectorArray;
>+    CSSSelector* current = m_selectorArray.get();
>     while (!current->isLastInSelectorList()) {
>         if (current->isLastInTagHistory())
>             ++size;
>@@ -109,29 +108,10 @@ unsigned CSSSelectorList::listSize() con
> 
> CSSSelectorList& CSSSelectorList::operator=(CSSSelectorList&& other)
> {
>-    deleteSelectors();
>-    m_selectorArray = other.m_selectorArray;
>-    other.m_selectorArray = nullptr;
>-
>+    m_selectorArray = WTFMove(other.m_selectorArray);
>     return *this;
> }
> 
>-void CSSSelectorList::deleteSelectors()
>-{
>-    if (!m_selectorArray)
>-        return;
>-
>-    CSSSelector* selectorArray = m_selectorArray;
>-    m_selectorArray = nullptr;
>-
>-    bool isLastSelector = false;
>-    for (CSSSelector* s = selectorArray; !isLastSelector; ++s) {
>-        isLastSelector = s->isLastInSelectorList();
>-        s->~CSSSelector();
>-    }
>-    fastFree(selectorArray);
>-}
>-
> String CSSSelectorList::selectorsText() const
> {
>     StringBuilder result;
>Index: Source/WebCore/css/CSSSelectorList.h
>===================================================================
>--- Source/WebCore/css/CSSSelectorList.h	(revision 234814)
>+++ Source/WebCore/css/CSSSelectorList.h	(working copy)
>@@ -27,6 +27,7 @@
> 
> #include "CSSSelector.h"
> #include <memory>
>+#include <wtf/UniqueArray.h>
> 
> namespace WebCore {
> 
>@@ -35,19 +36,16 @@ class CSSParserSelector;
> class CSSSelectorList {
>     WTF_MAKE_FAST_ALLOCATED;
> public:
>-    CSSSelectorList() : m_selectorArray(0) { }
>+    CSSSelectorList() = default;
>     CSSSelectorList(const CSSSelectorList&);
>     CSSSelectorList(CSSSelectorList&&);
>     CSSSelectorList(Vector<std::unique_ptr<CSSParserSelector>>&&);
>-
>-    ~CSSSelectorList() { deleteSelectors(); }
>-
>-    void adoptSelectorArray(CSSSelector* selectors) { ASSERT(!m_selectorArray); m_selectorArray = selectors; }
>+    CSSSelectorList(UniqueArray<CSSSelector>&& array)
>+        : m_selectorArray(WTFMove(array)) { }
> 
>     bool isValid() const { return !!m_selectorArray; }
>-    const CSSSelector* first() const { return m_selectorArray; }
>+    const CSSSelector* first() const { return m_selectorArray.get(); }
>     static const CSSSelector* next(const CSSSelector*);
>-    bool hasOneSelector() const { return m_selectorArray && !next(m_selectorArray); }
>     const CSSSelector* selectorAt(size_t index) const { return &m_selectorArray[index]; }
> 
>     size_t indexOfNextSelectorAfter(size_t index) const
>@@ -56,7 +54,7 @@ public:
>         current = next(current);
>         if (!current)
>             return notFound;
>-        return current - m_selectorArray;
>+        return current - m_selectorArray.get();
>     }
> 
>     bool selectorsNeedNamespaceResolution();
>@@ -75,7 +73,7 @@ private:
> 
>     // End of a multipart selector is indicated by m_isLastInTagHistory bit in the last item.
>     // End of the array is indicated by m_isLastInSelectorList bit in the last item.
>-    CSSSelector* m_selectorArray;
>+    UniqueArray<CSSSelector> m_selectorArray;
> };
> 
> inline const CSSSelector* CSSSelectorList::next(const CSSSelector* current)
>Index: Source/WebCore/css/CSSStyleRule.cpp
>===================================================================
>--- Source/WebCore/css/CSSStyleRule.cpp	(revision 234813)
>+++ Source/WebCore/css/CSSStyleRule.cpp	(working copy)
>@@ -102,7 +102,7 @@ void CSSStyleRule::setSelectorText(const
> 
>     CSSStyleSheet::RuleMutationScope mutationScope(this);
> 
>-    m_styleRule->wrapperAdoptSelectorList(selectorList);
>+    m_styleRule->wrapperAdoptSelectorList(WTFMove(selectorList));
> 
>     if (hasCachedSelectorText()) {
>         selectorTextCache().remove(this);
>Index: Source/WebCore/css/StyleRule.cpp
>===================================================================
>--- Source/WebCore/css/StyleRule.cpp	(revision 234813)
>+++ Source/WebCore/css/StyleRule.cpp	(working copy)
>@@ -181,9 +181,10 @@ unsigned StyleRule::averageSizeInBytes()
>     return sizeof(StyleRule) + sizeof(CSSSelector) + StyleProperties::averageSizeInBytes();
> }
> 
>-StyleRule::StyleRule(Ref<StylePropertiesBase>&& properties, bool hasDocumentSecurityOrigin)
>+StyleRule::StyleRule(Ref<StylePropertiesBase>&& properties, bool hasDocumentSecurityOrigin, CSSSelectorList&& selectors)
>     : StyleRuleBase(Style, hasDocumentSecurityOrigin)
>     , m_properties(WTFMove(properties))
>+    , m_selectorList(WTFMove(selectors))
> {
> }
> 
>@@ -213,13 +214,11 @@ MutableStyleProperties& StyleRule::mutab
> Ref<StyleRule> StyleRule::createForSplitting(const Vector<const CSSSelector*>& selectors, Ref<StyleProperties>&& properties, bool hasDocumentSecurityOrigin)
> {
>     ASSERT_WITH_SECURITY_IMPLICATION(!selectors.isEmpty());
>-    CSSSelector* selectorListArray = reinterpret_cast<CSSSelector*>(fastMalloc(sizeof(CSSSelector) * selectors.size()));
>+    auto selectorListArray = makeUniqueArray<CSSSelector>(selectors.size());
>     for (unsigned i = 0; i < selectors.size(); ++i)
>         new (NotNull, &selectorListArray[i]) CSSSelector(*selectors.at(i));
>     selectorListArray[selectors.size() - 1].setLastInSelectorList();
>-    auto rule = StyleRule::create(WTFMove(properties), hasDocumentSecurityOrigin);
>-    rule.get().parserAdoptSelectorArray(selectorListArray);
>-    return rule;
>+    return StyleRule::create(WTFMove(properties), hasDocumentSecurityOrigin, WTFMove(selectorListArray));
> }
> 
> Vector<RefPtr<StyleRule>> StyleRule::splitIntoMultipleRulesWithMaximumSelectorComponentCount(unsigned maxCount) const
>@@ -248,9 +247,10 @@ Vector<RefPtr<StyleRule>> StyleRule::spl
>     return rules;
> }
> 
>-StyleRulePage::StyleRulePage(Ref<StyleProperties>&& properties)
>+StyleRulePage::StyleRulePage(Ref<StyleProperties>&& properties, CSSSelectorList&& selectors)
>     : StyleRuleBase(Page)
>     , m_properties(WTFMove(properties))
>+    , m_selectorList(WTFMove(selectors))
> {
> }
> 
>Index: Source/WebCore/css/StyleRule.h
>===================================================================
>--- Source/WebCore/css/StyleRule.h	(revision 234814)
>+++ Source/WebCore/css/StyleRule.h	(working copy)
>@@ -118,9 +118,9 @@ private:
> class StyleRule final : public StyleRuleBase {
>     WTF_MAKE_FAST_ALLOCATED;
> public:
>-    static Ref<StyleRule> create(Ref<StylePropertiesBase>&& properties, bool hasDocumentSecurityOrigin)
>+    static Ref<StyleRule> create(Ref<StylePropertiesBase>&& properties, bool hasDocumentSecurityOrigin, CSSSelectorList&& selectors)
>     {
>-        return adoptRef(*new StyleRule(WTFMove(properties), hasDocumentSecurityOrigin));
>+        return adoptRef(*new StyleRule(WTFMove(properties), hasDocumentSecurityOrigin, WTFMove(selectors)));
>     }
>     
>     ~StyleRule();
>@@ -133,14 +133,13 @@ public:
> 
>     using StyleRuleBase::hasDocumentSecurityOrigin;
> 
>-    void wrapperAdoptSelectorList(CSSSelectorList& selectors)
>+    void wrapperAdoptSelectorList(CSSSelectorList&& selectors)
>     {
>         m_selectorList = WTFMove(selectors);
> #if ENABLE(CSS_SELECTOR_JIT)
>         m_compiledSelectors = nullptr;
> #endif
>     }
>-    void parserAdoptSelectorArray(CSSSelector* selectors) { m_selectorList.adoptSelectorArray(selectors); }
> 
>     Ref<StyleRule> copy() const { return adoptRef(*new StyleRule(*this)); }
> 
>@@ -162,7 +161,7 @@ public:
>     static unsigned averageSizeInBytes();
> 
> private:
>-    StyleRule(Ref<StylePropertiesBase>&&, bool hasDocumentSecurityOrigin);
>+    StyleRule(Ref<StylePropertiesBase>&&, bool hasDocumentSecurityOrigin, CSSSelectorList&&);
>     StyleRule(const StyleRule&);
> 
>     static Ref<StyleRule> createForSplitting(const Vector<const CSSSelector*>&, Ref<StyleProperties>&&, bool hasDocumentSecurityOrigin);
>@@ -200,7 +199,7 @@ private:
> 
> class StyleRulePage final : public StyleRuleBase {
> public:
>-    static Ref<StyleRulePage> create(Ref<StyleProperties>&& properties) { return adoptRef(*new StyleRulePage(WTFMove(properties))); }
>+    static Ref<StyleRulePage> create(Ref<StyleProperties>&& properties, CSSSelectorList&& selectors) { return adoptRef(*new StyleRulePage(WTFMove(properties), WTFMove(selectors))); }
> 
>     ~StyleRulePage();
> 
>@@ -208,12 +207,12 @@ public:
>     const StyleProperties& properties() const { return m_properties; }
>     MutableStyleProperties& mutableProperties();
> 
>-    void wrapperAdoptSelectorList(CSSSelectorList& selectors) { m_selectorList = WTFMove(selectors); }
>+    void wrapperAdoptSelectorList(CSSSelectorList&& selectors) { m_selectorList = WTFMove(selectors); }
> 
>     Ref<StyleRulePage> copy() const { return adoptRef(*new StyleRulePage(*this)); }
> 
> private:
>-    explicit StyleRulePage(Ref<StyleProperties>&&);
>+    explicit StyleRulePage(Ref<StyleProperties>&&, CSSSelectorList&&);
>     StyleRulePage(const StyleRulePage&);
>     
>     Ref<StyleProperties> m_properties;
>Index: Source/WebCore/css/parser/CSSParserImpl.cpp
>===================================================================
>--- Source/WebCore/css/parser/CSSParserImpl.cpp	(revision 234814)
>+++ Source/WebCore/css/parser/CSSParserImpl.cpp	(working copy)
>@@ -668,9 +668,7 @@ RefPtr<StyleRulePage> CSSParserImpl::con
> 
>     consumeDeclarationList(block, StyleRule::Style);
>     
>-    RefPtr<StyleRulePage> page = StyleRulePage::create(createStyleProperties(m_parsedProperties, m_context.mode));
>-    page->wrapperAdoptSelectorList(selectorList);
>-    return page;
>+    return StyleRulePage::create(createStyleProperties(m_parsedProperties, m_context.mode), WTFMove(selectorList));
> }
> 
> // FIXME-NEWPARSER: Support "apply"
>@@ -726,7 +724,6 @@ RefPtr<StyleRule> CSSParserImpl::consume
>     if (!selectorList.isValid())
>         return nullptr; // Parse error, invalid selector list
> 
>-    RefPtr<StyleRule> rule;
>     if (m_observerWrapper)
>         observeSelectors(*m_observerWrapper, prelude);
>     
>@@ -738,16 +735,12 @@ RefPtr<StyleRule> CSSParserImpl::consume
>         CSSParserTokenRange blockCopy = block;
>         blockCopy.consumeWhitespace();
>         if (!blockCopy.atEnd()) {
>-            rule = StyleRule::create(createDeferredStyleProperties(block), m_context.hasDocumentSecurityOrigin);
>-            rule->wrapperAdoptSelectorList(selectorList);
>-            return rule;
>+            return StyleRule::create(createDeferredStyleProperties(block), m_context.hasDocumentSecurityOrigin, WTFMove(selectorList));
>         }
>     }
> 
>     consumeDeclarationList(block, StyleRule::Style);
>-    rule = StyleRule::create(createStyleProperties(m_parsedProperties, m_context.mode), m_context.hasDocumentSecurityOrigin);
>-    rule->wrapperAdoptSelectorList(selectorList);
>-    return rule;
>+    return StyleRule::create(createStyleProperties(m_parsedProperties, m_context.mode), m_context.hasDocumentSecurityOrigin, WTFMove(selectorList));
> }
> 
> void CSSParserImpl::consumeDeclarationList(CSSParserTokenRange range, StyleRule::Type ruleType)
>Index: Source/WebCore/css/parser/CSSSelectorParser.cpp
>===================================================================
>--- Source/WebCore/css/parser/CSSSelectorParser.cpp	(revision 234814)
>+++ Source/WebCore/css/parser/CSSSelectorParser.cpp	(working copy)
>@@ -530,7 +530,7 @@ std::unique_ptr<CSSParserSelector> CSSSe
>             DisallowPseudoElementsScope scope(this);
>             std::unique_ptr<CSSSelectorList> selectorList = std::unique_ptr<CSSSelectorList>(new CSSSelectorList());
>             *selectorList = consumeComplexSelectorList(block);
>-            if (!selectorList->componentCount() || !block.atEnd())
>+            if (!selectorList->first() || !block.atEnd())
>                 return nullptr;
>             selector->setSelectorList(WTFMove(selectorList));
>             return selector;
>@@ -559,7 +559,7 @@ std::unique_ptr<CSSParserSelector> CSSSe
>                 block.consumeWhitespace();
>                 std::unique_ptr<CSSSelectorList> selectorList = std::unique_ptr<CSSSelectorList>(new CSSSelectorList());
>                 *selectorList = consumeComplexSelectorList(block);
>-                if (!selectorList->componentCount() || !block.atEnd())
>+                if (!selectorList->first() || !block.atEnd())
>                     return nullptr;
>                 selector->setSelectorList(WTFMove(selectorList));
>             }
>@@ -577,7 +577,7 @@ std::unique_ptr<CSSParserSelector> CSSSe
>         case CSSSelector::PseudoClassMatches: {
>             std::unique_ptr<CSSSelectorList> selectorList = std::unique_ptr<CSSSelectorList>(new CSSSelectorList());
>             *selectorList = consumeComplexSelectorList(block);
>-            if (!selectorList->componentCount() || !block.atEnd())
>+            if (!selectorList->first() || !block.atEnd())
>                 return nullptr;
>             selector->setSelectorList(WTFMove(selectorList));
>             return selector;
>@@ -586,7 +586,7 @@ std::unique_ptr<CSSParserSelector> CSSSe
>         case CSSSelector::PseudoClassHost: {
>             std::unique_ptr<CSSSelectorList> selectorList = std::unique_ptr<CSSSelectorList>(new CSSSelectorList());
>             *selectorList = consumeCompoundSelectorList(block);
>-            if (!selectorList->componentCount() || !block.atEnd())
>+            if (!selectorList->first() || !block.atEnd())
>                 return nullptr;
>             selector->setSelectorList(WTFMove(selectorList));
>             return selector;

Flags:

simon.fraser: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 188539: 347051