Setting selectorText of PageRule accepts invalid selectors

third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/css/parser/CSSParserImpl.h"
 
 #include "core/css/CSSCustomPropertyDeclaration.h"
 #include "core/css/CSSKeyframesRule.h"
 #include "core/css/CSSStyleSheet.h"
 #include "core/css/StylePropertySet.h"
@@ skipping 155 matching lines @@
     });
     styleSheet->setHasSyntacticallyValidCSSHeader(firstRuleValid);
     TRACE_EVENT_END0("blink,blink_style", "CSSParserImpl::parseStyleSheet.parse");
 
     TRACE_EVENT_END2(
         "blink,blink_style", "CSSParserImpl::parseStyleSheet",
         "tokenCount", scope.tokenCount(),
         "length", string.length());
 }
 
+CSSSelectorList CSSParserImpl::parsePageSelector(CSSParserTokenRange prelude, StyleSheetContents* styleSheet)

[Timothy Loh, 2016/02/01 21:55:16]

prelude -> range? This isn't really a prelude when you set a selector via CSSOM.

[ramya.v, 2016/02/02 05:43:24]

Done in https://codereview.chromium.org/1653303002/

+{
+    prelude.consumeWhitespace();
+    AtomicString typeSelector;
+    if (prelude.peek().type() == IdentToken)
+        typeSelector = prelude.consume().value();
+
+    AtomicString pseudo;
+    if (prelude.peek().type() == ColonToken) {
+        prelude.consume();
+        if (prelude.peek().type() != IdentToken)
+            return CSSSelectorList();
+        pseudo = prelude.consume().value();
+    }
+
+    prelude.consumeWhitespace();
+    if (!prelude.atEnd())
+        return CSSSelectorList(); // Parse error; extra tokens in @page header

[Timothy Loh, 2016/02/01 21:55:15]

@page header -> @page selector (actually I don't think header was ever the
appropriate term)

[ramya.v, 2016/02/02 05:43:24]

Done in https://codereview.chromium.org/1653303002/

+
+    OwnPtr<CSSParserSelector> selector;
+    if (!typeSelector.isNull() && pseudo.isNull()) {
+        selector = CSSParserSelector::create(QualifiedName(nullAtom, typeSelector, styleSheet->defaultNamespace()));

[rune, 2016/02/01 14:09:34]

Namespaces for page names doesn't make sense, and also we never try to match
page names, but that can be handled in a different CL.

[ramya.v, 2016/02/01 15:50:05]

Sure will take care in subsequent patch.

+    } else {
+        selector = CSSParserSelector::create();
+        if (!pseudo.isNull()) {
+            selector->setMatch(CSSSelector::PagePseudoClass);
+            selector->updatePseudoType(pseudo.lower());
+            if (selector->pseudoType() == CSSSelector::PseudoUnknown)
+                return CSSSelectorList();
+        }
+        if (!typeSelector.isNull()) {
+            selector->prependTagSelector(QualifiedName(nullAtom, typeSelector, styleSheet->defaultNamespace()));
+        }
+    }
+
+    selector->setForPage();
+    Vector<OwnPtr<CSSParserSelector>> selectorVector;
+    selectorVector.append(selector.release());
+    CSSSelectorList selectorList = CSSSelectorList::adoptSelectorVector(selectorVector);
+    return selectorList;
+}
+
 PassOwnPtr<Vector<double>> CSSParserImpl::parseKeyframeKeyList(const String& keyList)
 {
     return consumeKeyframeKeyList(CSSTokenizer::Scope(keyList).tokenRange());
 }
 
 bool CSSParserImpl::supportsDeclaration(CSSParserTokenRange& range)
 {
     ASSERT(m_parsedProperties.isEmpty());
     consumeDeclaration(range, StyleRule::Style);
     bool result = !m_parsedProperties.isEmpty();
@@ skipping 343 matching lines @@
     consumeRuleList(block, KeyframesRuleList, [keyframeRule](PassRefPtrWillBeRawPtr<StyleRuleBase> keyframe) {
         keyframeRule->parserAppendKeyframe(toStyleRuleKeyframe(keyframe.get()));
     });
     keyframeRule->setName(name);
     keyframeRule->setVendorPrefixed(webkitPrefixed);
     return keyframeRule.release();
 }
 
 PassRefPtrWillBeRawPtr<StyleRulePage> CSSParserImpl::consumePageRule(CSSParserTokenRange prelude, CSSParserTokenRange block)
 {
     // We only support a small subset of the css-page spec.

[Timothy Loh, 2016/02/01 21:55:16]

This comment should be moved to parsePageSelector.

[ramya.v, 2016/02/02 05:43:24]

Done in https://codereview.chromium.org/1653303002/

-    prelude.consumeWhitespace();
-    AtomicString typeSelector;
-    if (prelude.peek().type() == IdentToken)
-        typeSelector = prelude.consume().value();
-
-    AtomicString pseudo;
-    if (prelude.peek().type() == ColonToken) {
-        prelude.consume();
-        if (prelude.peek().type() != IdentToken)
-            return nullptr; // Parse error; expected ident token following colon in @page header
-        pseudo = prelude.consume().value();
-    }
-
-    prelude.consumeWhitespace();
-    if (!prelude.atEnd())
-        return nullptr; // Parse error; extra tokens in @page header
-
-    OwnPtr<CSSParserSelector> selector;
-    if (!typeSelector.isNull() && pseudo.isNull()) {
-        selector = CSSParserSelector::create(QualifiedName(nullAtom, typeSelector, m_styleSheet->defaultNamespace()));
-    } else {
-        selector = CSSParserSelector::create();
-        if (!pseudo.isNull()) {
-            selector->setMatch(CSSSelector::PagePseudoClass);
-            selector->updatePseudoType(pseudo.lower());
-            if (selector->pseudoType() == CSSSelector::PseudoUnknown)
-                return nullptr; // Parse error; unknown page pseudo-class
-        }
-        if (!typeSelector.isNull())
-            selector->prependTagSelector(QualifiedName(nullAtom, typeSelector, m_styleSheet->defaultNamespace()));
-    }
+    CSSSelectorList selectorList = parsePageSelector(prelude, m_styleSheet);
+    if (!selectorList.isValid())
+        return nullptr;

[Timothy Loh, 2016/02/01 21:55:16]

FYI I was fairly deliberate with commenting on parse errors so that if we wanted
to add error logging in the future it'd be obvious what all the different parse
errors were. In this case it's pretty obvious from looking at the code, but it
wouldn't hurt to add a comment like "// Parse error, invalid @page selector"

[ramya.v, 2016/02/02 05:43:24]

Done in https://codereview.chromium.org/1653303002/

 
     if (m_observerWrapper) {
         unsigned endOffset = m_observerWrapper->endOffset(prelude);
         m_observerWrapper->observer().startRuleHeader(StyleRule::Page, m_observerWrapper->startOffset(prelude));
         m_observerWrapper->observer().endRuleHeader(endOffset);
     }
 
-    selector->setForPage();
-    Vector<OwnPtr<CSSParserSelector>> selectorVector;
-    selectorVector.append(selector.release());
-    CSSSelectorList selectorList = CSSSelectorList::adoptSelectorVector(selectorVector);
-
     consumeDeclarationList(block, StyleRule::Style);
 
     return StyleRulePage::create(std::move(selectorList), createStylePropertySet(m_parsedProperties, m_context.mode()));
 }
 
 PassRefPtrWillBeRawPtr<StyleRuleKeyframe> CSSParserImpl::consumeKeyframeStyleRule(CSSParserTokenRange prelude, CSSParserTokenRange block)
 {
     OwnPtr<Vector<double>> keyList = consumeKeyframeKeyList(prelude);
     if (!keyList)
         return nullptr;
@@ skipping 162 matching lines @@
         else
             return nullptr; // Parser error, invalid value in keyframe selector
         if (range.atEnd())
             return result.release();
         if (range.consume().type() != CommaToken)
             return nullptr; // Parser error
     }
 }
 
 } // namespace blink