Bug 528096: unexpected-token error recovery paths in the CSS parser need to UngetToken() before calling SkipUntil(), for correct behavior when the unexpected token is ( [ { or FUNCTION. r=dbaron
authorZack Weinberg <zweinberg@mozilla.com>
Wed, 27 Jan 2010 16:20:05 -0800
changeset 37596 9259fdc3570cba268fc4854f4687274d331e1011
parent 37595 d7081443284b6a7fae7c6dcedeb39e713450b6b0
child 37597 4c3d6950eb499a26569c6535f628b63e5e273ee7
push idunknown
push userunknown
push dateunknown
reviewersdbaron
bugs528096
milestone1.9.3a1pre
Bug 528096: unexpected-token error recovery paths in the CSS parser need to UngetToken() before calling SkipUntil(), for correct behavior when the unexpected token is ( [ { or FUNCTION. r=dbaron
layout/reftests/bugs/528096-1-ref.html
layout/reftests/bugs/528096-1.html
layout/reftests/bugs/checkmark.gif
layout/reftests/bugs/reftest.list
layout/style/nsCSSParser.cpp
layout/style/test/test_font_face_parser.html
layout/style/test/test_media_queries.html
layout/style/test/test_selectors.html
new file mode 100644
--- /dev/null
+++ b/layout/reftests/bugs/528096-1-ref.html
@@ -0,0 +1,34 @@
+<!DOCTYPE HTML>
+<!-- Test for some pieces of bug 528096; other pieces are tested in
+     the mochitests test_font_face_parser.html,
+     test_media_queries.html, and test_selectors.html -->
+<html><head>
+  <title>Test for bug 528096 (counters and image rects)</title>
+  <style>
+  div { display: inline-block; vertical-align: bottom;
+        margin-right: 6px; width: 16px; height: 16px;
+        background-color: lime; }
+  div::before { display: inline-block; height: 16px;
+                content: url("checkmark.gif"); }
+  </style>
+</head><body>
+
+<table><tr>
+  <td>-moz-image-rect()</td>
+  <td><div id="i1"></td>
+</tr><tr>
+  <td>counter()</td>
+  <td><div id="c1"></div><div id="c2"></div></td>
+</tr><tr>
+  <td>counters()</td>
+  <td><div id="cs1"></div><div id="cs2"></div><div id="cs3"></div></td>
+</tr></table>
+
+<p>
+<a target="_blank"
+   href="https://bugzilla.mozilla.org/show_bug.cgi?id=528096">Mozilla
+   bug 528096</a>. There should be no red or orange, and each square
+   should have a black checkmark in it.
+</p>
+
+</body></html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/bugs/528096-1.html
@@ -0,0 +1,70 @@
+<!DOCTYPE HTML>
+<!-- Test for some pieces of bug 528096; other pieces are tested in
+     the mochitests test_font_face_parser.html,
+     test_media_queries.html, and test_selectors.html -->
+<html><head>
+  <title>Test for bug 528096 (counters and image rects)</title>
+  <style>
+
+  div { display: inline-block; vertical-align: bottom;
+        margin-right: 6px; width: 16px; height: 16px;
+        background-color: orange; }
+  div::before { display: inline-block; height: 16px; }
+
+  /* -moz-image-rect() */
+  #i1::before {
+    background-color: lime;
+    background-image: -moz-image-rect((); background-color: red;);
+    content: url("checkmark.gif");
+  }
+
+  /* counter() */
+  #c1::before {
+    background-color: lime;
+    content: counter((); background-color: red;);
+    content: url("checkmark.gif");
+  }
+  #c2::before {
+    background-color: lime;
+    content: counter(foo,(); background-color: red;);
+    content: url("checkmark.gif");
+  }
+
+  /* counters() */
+  #cs1::before {
+    background-color: lime;
+    content: counters((); background-color: red;);
+    content: url("checkmark.gif");
+  }
+  #cs2::before {
+    background-color: lime;
+    content: counters(foo,(); background-color: red;);
+    content: url("checkmark.gif");
+  }
+  #cs3::before {
+    background-color: lime;
+    content: counters(foo,"s",(); background-color: red;);
+    content: url("checkmark.gif");
+  }
+  </style>
+</head><body>
+
+<table><tr>
+  <td>-moz-image-rect()</td>
+  <td><div id="i1"></td>
+</tr><tr>
+  <td>counter()</td>
+  <td><div id="c1"></div><div id="c2"></div></td>
+</tr><tr>
+  <td>counters()</td>
+  <td><div id="cs1"></div><div id="cs2"></div><div id="cs3"></div></td>
+</tr></table>
+
+<p>
+<a target="_blank"
+   href="https://bugzilla.mozilla.org/show_bug.cgi?id=528096">Mozilla
+   bug 528096</a>. There should be no red or orange, and each square
+   should have a black checkmark in it.
+</p>
+
+</body></html>
new file mode 100644
index 0000000000000000000000000000000000000000..b5a677d291fbc9dedfb51e4633df6cdcb090c884
GIT binary patch
literal 76
zc${<hbhEHb6krfwXkcUjg8%>jEB<6*<YHiC&;jv5G7L=0J^d@W_P)87>7bOwdoJ<*
bjmG$;oyi`0GnNOve7tXi<>@;?3=Gx)7fTs_
--- a/layout/reftests/bugs/reftest.list
+++ b/layout/reftests/bugs/reftest.list
@@ -1364,16 +1364,17 @@ fails HTTP(..) == 518172-2b.html 518172-
 == 527464-1.html 527464-ref.html
 == 528038-1a.html 528038-1-ref.html
 == 528038-1b.html 528038-1-ref.html
 == 528038-1c.html 528038-1-ref.html
 == 528038-1d.html 528038-1-ref.html
 == 528038-1e.html 528038-1-ref.html
 == 528038-1f.html 528038-1-ref.html
 == 528038-2.html 528038-2-ref.html
+== 528096-1.html 528096-1-ref.html
 == 530686-1.html 530686-1-ref.html
 == 531098-1.html 531098-1-ref.html
 == 531371-1.html 531371-1-ref.html
 == 534808-1.html 534808-1-ref.html
 == 534808-2.html 534808-2-ref.html
 == 534919-1.html 534919-1-ref.html
 == 537507-1.xul 537507-1-ref.xul
 == 537507-2.html 537507-2-ref.html
--- a/layout/style/nsCSSParser.cpp
+++ b/layout/style/nsCSSParser.cpp
@@ -288,17 +288,16 @@ protected:
   PRBool CheckEndProperty();
   nsSubstring* NextIdent();
   void SkipUntil(PRUnichar aStopSymbol);
   void SkipUntilStack(nsAutoTArray<PRUnichar, 16> &aStack);
   void SkipUntilOneOf(const PRUnichar* aStopSymbolChars);
   void SkipRuleSet(PRBool aInsideBraces);
   PRBool SkipAtRule();
   PRBool SkipDeclaration(PRBool aCheckForBraces);
-  PRBool GetNonCloseParenToken(PRBool aSkipWS);
 
   PRBool PushGroup(nsICSSGroupRule* aRule);
   void PopGroup(void);
 
   PRBool ParseRuleSet(RuleAppendFunc aAppendFunc, void* aProcessData,
                       PRBool aInsideBraces = PR_FALSE);
   PRBool ParseAtRule(RuleAppendFunc aAppendFunc, void* aProcessData);
   PRBool ParseCharsetRule(RuleAppendFunc aAppendFunc, void* aProcessData);
@@ -1732,16 +1731,17 @@ CSSParserImpl::ParseMediaQueryExpression
     return PR_FALSE;
   }
   if (! GetToken(PR_TRUE)) {
     REPORT_UNEXPECTED_EOF(PEMQExpressionEOF);
     return PR_FALSE;
   }
   if (eCSSToken_Ident != mToken.mType) {
     REPORT_UNEXPECTED_TOKEN(PEMQExpectedFeatureName);
+    UngetToken();
     SkipUntil(')');
     return PR_FALSE;
   }
 
   nsMediaExpression *expr = aQuery->NewExpression();
   if (!expr) {
     mScanner.SetLowLevelError(NS_ERROR_OUT_OF_MEMORY);
     SkipUntil(')');
@@ -1786,16 +1786,17 @@ CSSParserImpl::ParseMediaQueryExpression
       return PR_FALSE;
     }
     expr->mValue.Reset();
     return PR_TRUE;
   }
 
   if (!mToken.IsSymbol(':')) {
     REPORT_UNEXPECTED_TOKEN(PEMQExpectedFeatureNameEnd);
+    UngetToken();
     SkipUntil(')');
     return PR_FALSE;
   }
 
   PRBool rv;
   switch (feature->mValueType) {
     case nsMediaFeature::eLength:
       rv = ParseNonNegativeVariant(expr->mValue, VARIANT_LENGTH, nsnull);
@@ -1826,29 +1827,34 @@ CSSParserImpl::ParseMediaQueryExpression
         rv = ParseVariant(a->Item(0), VARIANT_INTEGER, nsnull) &&
              a->Item(0).GetIntValue() > 0 &&
              ExpectSymbol('/', PR_TRUE) &&
              ParseVariant(a->Item(1), VARIANT_INTEGER, nsnull) &&
              a->Item(1).GetIntValue() > 0;
       }
       break;
     case nsMediaFeature::eResolution:
-      rv = GetToken(PR_TRUE) && mToken.mType == eCSSToken_Dimension &&
+      rv = GetToken(PR_TRUE);
+      if (!rv)
+        break;
+      rv = mToken.mType == eCSSToken_Dimension &&
            mToken.mIntegerValid && mToken.mNumber > 0.0f;
-      if (rv) {
-        // No worries about whether unitless zero is allowed, since the
-        // value must be positive (and we checked that above).
-        NS_ASSERTION(!mToken.mIdent.IsEmpty(), "unit lied");
-        if (mToken.mIdent.LowerCaseEqualsLiteral("dpi")) {
-          expr->mValue.SetFloatValue(mToken.mNumber, eCSSUnit_Inch);
-        } else if (mToken.mIdent.LowerCaseEqualsLiteral("dpcm")) {
-          expr->mValue.SetFloatValue(mToken.mNumber, eCSSUnit_Centimeter);
-        } else {
-          rv = PR_FALSE;
-        }
+      if (!rv) {
+        UngetToken();
+        break;
+      }
+      // No worries about whether unitless zero is allowed, since the
+      // value must be positive (and we checked that above).
+      NS_ASSERTION(!mToken.mIdent.IsEmpty(), "unit lied");
+      if (mToken.mIdent.LowerCaseEqualsLiteral("dpi")) {
+        expr->mValue.SetFloatValue(mToken.mNumber, eCSSUnit_Inch);
+      } else if (mToken.mIdent.LowerCaseEqualsLiteral("dpcm")) {
+        expr->mValue.SetFloatValue(mToken.mNumber, eCSSUnit_Centimeter);
+      } else {
+        rv = PR_FALSE;
       }
       break;
     case nsMediaFeature::eEnumerated:
       rv = ParseVariant(expr->mValue, VARIANT_KEYWORD,
                         feature->mData.mKeywordTable);
       break;
   }
   if (!rv || !ExpectSymbol(')', PR_TRUE)) {
@@ -2298,28 +2304,16 @@ CSSParserImpl::SkipUntilOneOf(const PRUn
       }
     } else if (eCSSToken_Function == tk->mType) {
       SkipUntil(')');
     }
   }
 }
 
 PRBool
-CSSParserImpl::GetNonCloseParenToken(PRBool aSkipWS)
-{
-  if (!GetToken(aSkipWS))
-    return PR_FALSE;
-  if (mToken.mType == eCSSToken_Symbol && mToken.mSymbol == ')') {
-    UngetToken();
-    return PR_FALSE;
-  }
-  return PR_TRUE;
-}
-
-PRBool
 CSSParserImpl::SkipDeclaration(PRBool aCheckForBraces)
 {
   nsCSSToken* tk = &mToken;
   for (;;) {
     if (!GetToken(PR_TRUE)) {
       if (aCheckForBraces) {
         REPORT_UNEXPECTED_EOF(PESkipDeclBraceEOF);
       }
@@ -3921,16 +3915,17 @@ CSSParserImpl::ParseTreePseudoElement(ns
       return PR_FALSE;
     }
     if (eCSSToken_Ident == mToken.mType) {
       nsCOMPtr<nsIAtom> pseudo = do_GetAtom(mToken.mIdent);
       fakeSelector.AddPseudoClass(pseudo,
                                   nsCSSPseudoClasses::ePseudoClass_NotPseudoClass);
     }
     else if (!mToken.IsSymbol(',')) {
+      UngetToken();
       SkipUntil(')');
       return PR_FALSE;
     }
   }
   *aPseudoElementArgs = fakeSelector.mPseudoClassList;
   fakeSelector.mPseudoClassList = nsnull;
   return PR_TRUE;
 }
@@ -4637,69 +4632,77 @@ CSSParserImpl::ParseVariant(nsCSSValue& 
 
 
 PRBool
 CSSParserImpl::ParseCounter(nsCSSValue& aValue)
 {
   nsCSSUnit unit = (mToken.mIdent.LowerCaseEqualsLiteral("counter") ?
                     eCSSUnit_Counter : eCSSUnit_Counters);
 
-  if (!GetNonCloseParenToken(PR_TRUE) ||
-      eCSSToken_Ident != mToken.mType) {
-    SkipUntil(')');
-    return PR_FALSE;
-  }
-
-  nsRefPtr<nsCSSValue::Array> val =
-    nsCSSValue::Array::Create(unit == eCSSUnit_Counter ? 2 : 3);
-  if (!val) {
-    mScanner.SetLowLevelError(NS_ERROR_OUT_OF_MEMORY);
-    return PR_FALSE;
-  }
-
-  val->Item(0).SetStringValue(mToken.mIdent, eCSSUnit_Ident);
-
-  if (eCSSUnit_Counters == unit) {
-    // get mandatory separator string
-    if (!ExpectSymbol(',', PR_TRUE) ||
-        !(GetNonCloseParenToken(PR_TRUE) &&
-          eCSSToken_String == mToken.mType)) {
-      SkipUntil(')');
-      return PR_FALSE;
-    }
-    val->Item(1).SetStringValue(mToken.mIdent, eCSSUnit_String);
-  }
-
-  // get optional type
-  PRInt32 type = NS_STYLE_LIST_STYLE_DECIMAL;
-  if (ExpectSymbol(',', PR_TRUE)) {
-    nsCSSKeyword keyword;
-    PRBool success = GetNonCloseParenToken(PR_TRUE) &&
-                     eCSSToken_Ident == mToken.mType &&
-                     (keyword = nsCSSKeywords::LookupKeyword(mToken.mIdent)) !=
-                      eCSSKeyword_UNKNOWN;
-    if (success) {
-      success = nsCSSProps::FindKeyword(keyword,
-                                        nsCSSProps::kListStyleKTable, type);
-    }
-    if (!success) {
-      SkipUntil(')');
-      return PR_FALSE;
-    }
-  }
-  PRInt32 typeItem = eCSSUnit_Counters == unit ? 2 : 1;
-  val->Item(typeItem).SetIntValue(type, eCSSUnit_Enumerated);
-
-  if (!ExpectSymbol(')', PR_TRUE)) {
-    SkipUntil(')');
-    return PR_FALSE;
-  }
-
-  aValue.SetArrayValue(val, unit);
-  return PR_TRUE;
+  // A non-iterative for loop to break out when an error occurs.
+  for (;;) {
+    if (!GetToken(PR_TRUE)) {
+      break;
+    }
+    if (eCSSToken_Ident != mToken.mType) {
+      UngetToken();
+      break;
+    }
+
+    nsRefPtr<nsCSSValue::Array> val =
+      nsCSSValue::Array::Create(unit == eCSSUnit_Counter ? 2 : 3);
+    if (!val) {
+      mScanner.SetLowLevelError(NS_ERROR_OUT_OF_MEMORY);
+      break;
+    }
+
+    val->Item(0).SetStringValue(mToken.mIdent, eCSSUnit_Ident);
+
+    if (eCSSUnit_Counters == unit) {
+      // must have a comma and then a separator string
+      if (!ExpectSymbol(',', PR_TRUE) || !GetToken(PR_TRUE)) {
+        break;
+      }
+      if (eCSSToken_String != mToken.mType) {
+        UngetToken();
+        break;
+      }
+      val->Item(1).SetStringValue(mToken.mIdent, eCSSUnit_String);
+    }
+
+    // get optional type
+    PRInt32 type = NS_STYLE_LIST_STYLE_DECIMAL;
+    if (ExpectSymbol(',', PR_TRUE)) {
+      if (!GetToken(PR_TRUE)) {
+        break;
+      }
+      nsCSSKeyword keyword;
+      if (eCSSToken_Ident != mToken.mType ||
+          (keyword = nsCSSKeywords::LookupKeyword(mToken.mIdent)) ==
+            eCSSKeyword_UNKNOWN ||
+          !nsCSSProps::FindKeyword(keyword, nsCSSProps::kListStyleKTable,
+                                   type)) {
+        UngetToken();
+        break;
+      }
+    }
+
+    PRInt32 typeItem = eCSSUnit_Counters == unit ? 2 : 1;
+    val->Item(typeItem).SetIntValue(type, eCSSUnit_Enumerated);
+
+    if (!ExpectSymbol(')', PR_TRUE)) {
+      break;
+    }
+
+    aValue.SetArrayValue(val, unit);
+    return PR_TRUE;
+  }
+
+  SkipUntil(')');
+  return PR_FALSE;
 }
 
 PRBool
 CSSParserImpl::ParseAttr(nsCSSValue& aValue)
 {
   if (!GetToken(PR_TRUE)) {
     return PR_FALSE;
   }
@@ -4834,16 +4837,17 @@ CSSParserImpl::ParseImageRect(nsCSSValue
     if (mToken.mType == eCSSToken_String) {
       if (!SetValueToURL(url, mToken.mIdent))
         break;
     } else if (mToken.mType == eCSSToken_Function &&
                mToken.mIdent.LowerCaseEqualsLiteral("url")) {
       if (!ParseURL(url))
         break;
     } else {
+      UngetToken();
       break;
     }
     if (!ExpectSymbol(',', PR_TRUE))
       break;
 
     static const PRInt32 VARIANT_SIDE = VARIANT_NUMBER | VARIANT_PERCENT;
     if (!ParseNonNegativeVariant(top, VARIANT_SIDE, nsnull) ||
         !ExpectSymbol(',', PR_TRUE) ||
--- a/layout/style/test/test_font_face_parser.html
+++ b/layout/style/test/test_font_face_parser.html
@@ -158,16 +158,20 @@
     { rule: _("src: url(\"/fonts/Mouse\"); " +
 	      "src: url(\"/fonts/Cat\") format(Cat(); src: local(Rat); )"),
       d: { "src" : "url(\"/fonts/Mouse\")" },
       noncanonical: true },
     { rule: _("src: url(\"/fonts/Mouse\"); " +
 	      "src: url(\"/fonts/Cat\") format(\"Cat\"; src: local(Rat); )"),
       d: { "src" : "url(\"/fonts/Mouse\")" },
       noncanonical: true },
+    { rule: _("src: url(\"/fonts/Mouse\"); " +
+	      "src: url(\"/fonts/Cat\") format((); src: local(Rat); )"),
+      d: { "src" : "url(\"/fonts/Mouse\")" },
+      noncanonical: true },
 
     // Correct unicode-range:
     { rule: _("unicode-range: U+00A5;"), d: { "unicode-range" : "U+00A5" } },
     { rule: _("unicode-range: U+A5;"),
       d: { "unicode-range" : "U+00A5" }, noncanonical: true },
     { rule: _("unicode-range: U+00a5;"),
       d: { "unicode-range" : "U+00A5" }, noncanonical: true },
     { rule: _("unicode-range: u+00a5;"),
--- a/layout/style/test/test_media_queries.html
+++ b/layout/style/test/test_media_queries.html
@@ -585,16 +585,21 @@ function run() {
   should_apply("all,(badexpression)");
   should_apply("(badexpression),all");
   should_not_apply("(badexpression),badmedium");
   should_not_apply("badmedium,(badexpression)");
   should_apply("all,[badsyntax]");
   should_apply("[badsyntax],all");
   should_not_apply("badmedium,[badsyntax]");
   should_not_apply("[badsyntax],badmedium");
+  // bug 528096
+  should_not_apply_unbalanced("((resolution),all");
+  should_not_apply_unbalanced("(resolution(),all");
+  should_not_apply_unbalanced("(resolution (),all");
+  should_not_apply_unbalanced("(resolution:(),all");
 
   handle_posted_items();
 }
 
 /*
  * The cloning tests have to post tests that wait for onload.  However,
  * we also make a bunch of state changes during the tests above.  So we
  * always change state using the change_state call, with both makes the
--- a/layout/style/test/test_selectors.html
+++ b/layout/style/test/test_selectors.html
@@ -749,16 +749,26 @@ function run() {
                           bodychildset([5, 6, 7, 8, 10, 11]));
     test_selector_in_html("Span", setup_cased_spans,
                           bodychildset([0, 1, 2, 3, 4, 10]),
                           bodychildset([5, 6, 7, 8, 9, 11]));
     test_selector_in_html("SPAN", setup_cased_spans,
                           bodychildset([0, 1, 2, 3, 4, 11]),
                           bodychildset([5, 6, 7, 8, 9, 10]));
 
+    // bug 528096 (tree pseudos)
+    test_selector_in_html(":-moz-tree-column((){} a", single_a,
+                          empty_set, set_single, html_default_ns);
+    test_selector_in_html(":-moz-tree-column(x(){} a", single_a,
+                          empty_set, set_single, html_default_ns);
+    test_selector_in_html(":-moz-tree-column(a b (){} a", single_a,
+                          empty_set, set_single, html_default_ns);
+    test_selector_in_html(":-moz-tree-column(a, b (){} a", single_a,
+                          empty_set, set_single, html_default_ns);
+
     run_deferred_tests();
 }
 
 var deferred_tests = [];
 
 function defer_clonedoc_tests(docurl, onloadfunc)
 {
     deferred_tests.push( { docurl: docurl, onloadfunc: onloadfunc } );