Setting selectorText of PageRule accepts invalid selectors

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

Index: third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp
diff --git a/third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp b/third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp
index 47300c6a897999dfec368d8391fce2fce1ef1305..1d61ee53cd61039cb6a6c4c370dc8c9e31077df1 100644
--- a/third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp
+++ b/third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp
@@ -173,6 +173,48 @@ void CSSParserImpl::parseStyleSheet(const String& string, const CSSParserContext
         "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());
@@ -537,37 +579,9 @@ PassRefPtrWillBeRawPtr<StyleRuleKeyframes> CSSParserImpl::consumeKeyframesRule(b
 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);
@@ -575,11 +589,6 @@ PassRefPtrWillBeRawPtr<StyleRulePage> CSSParserImpl::consumePageRule(CSSParserTo
         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()));