Fix issues with parsing of media query lists that have bad queries in them. (Bug 454226) r+sr=bzbarsky

layout/style/nsCSSParser.cpp

@@ -286,9 +286,10 @@ protected:
   PRBool ParseCharsetRule(RuleAppendFunc aAppendFunc, void* aProcessData);
   PRBool ParseImportRule(RuleAppendFunc aAppendFunc, void* aProcessData);
   PRBool GatherURL(nsString& aURL);
-  // Callers must clear or throw out aMedia if GatherMedia returns false.
   PRBool GatherMedia(nsMediaList* aMedia,
                      PRUnichar aStopSymbol);
+  PRBool ParseMediaQuery(PRUnichar aStopSymbol, nsMediaQuery **aQuery,
+                         PRBool *aParsedSomething, PRBool *aHitStop);
   PRBool ParseMediaQueryExpression(nsMediaQuery* aQuery);
   PRBool ProcessImport(const nsString& aURLSpec,
                        nsMediaList* aMedia,
@@ -1464,131 +1465,152 @@ CSSParserImpl::GatherURL(nsString& aURL)
   return PR_FALSE;
 }
 
-// Callers must clear or throw out aMedia if GatherMedia returns false.
 PRBool
-CSSParserImpl::GatherMedia(nsMediaList* aMedia,
-                           PRUnichar aStopSymbol)
+CSSParserImpl::ParseMediaQuery(PRUnichar aStopSymbol,
+                               nsMediaQuery **aQuery,
+                               PRBool *aParsedSomething,
+                               PRBool *aHitStop)
 {
+  *aQuery = nsnull;
+  *aParsedSomething = PR_FALSE;
+  *aHitStop = PR_FALSE;
+
   // "If the comma-separated list is the empty list it is assumed to
   // specify the media query 'all'."  (css3-mediaqueries, section
   // "Media Queries")
   if (!GetToken(PR_TRUE)) {
+    *aHitStop = PR_TRUE;
     // expected termination by EOF
     if (aStopSymbol == PRUnichar(0))
       return PR_TRUE;
 
-    // unexpected termination by EOF; if we were looking for a
-    // semicolon, return true anyway, for the same reason this is
-    // done by ExpectSymbol().
+    // unexpected termination by EOF
     REPORT_UNEXPECTED_EOF(PEGatherMediaEOF);
-    return aStopSymbol == PRUnichar(';');
+    return PR_TRUE;
   }
 
   if (eCSSToken_Symbol == mToken.mType &&
       mToken.mSymbol == aStopSymbol) {
+    *aHitStop = PR_TRUE;
     UngetToken();
     return PR_TRUE;
   }
   UngetToken();
-  aMedia->SetNonEmpty();
 
-  for (;;) {
-    // We want to still have |query| after we transfer ownership from
-    // |queryHolder| to |aMedia|.
-    nsMediaQuery *query;
-    {
-      nsAutoPtr<nsMediaQuery> queryHolder(new nsMediaQuery);
-      if (!queryHolder) {
-        mScanner.SetLowLevelError(NS_ERROR_OUT_OF_MEMORY);
+  *aParsedSomething = PR_TRUE;
+
+  nsAutoPtr<nsMediaQuery> query(new nsMediaQuery);
+  if (!query) {
+    mScanner.SetLowLevelError(NS_ERROR_OUT_OF_MEMORY);
+    return PR_FALSE;
+  }
+
+  if (ExpectSymbol('(', PR_TRUE)) {
+    // we got an expression without a media type
+    UngetToken(); // so ParseMediaQueryExpression can handle it
+    query->SetType(nsGkAtoms::all);
+    query->SetTypeOmitted();
+    // Just parse the first expression here.
+    if (!ParseMediaQueryExpression(query)) {
+      OUTPUT_ERROR();
+      query->SetHadUnknownExpression();
+    }
+  } else {
+    nsCOMPtr<nsIAtom> mediaType;
+    PRBool gotNotOrOnly = PR_FALSE;
+    for (;;) {
+      if (!GetToken(PR_TRUE)) {
+        REPORT_UNEXPECTED_EOF(PEGatherMediaEOF);
         return PR_FALSE;
       }
-      query = queryHolder;
+      if (eCSSToken_Ident != mToken.mType) {
+        REPORT_UNEXPECTED_TOKEN(PEGatherMediaNotIdent);
+        UngetToken();
+        return PR_FALSE;
+      }
+      // case insensitive from CSS - must be lower cased
+      ToLowerCase(mToken.mIdent);
+      mediaType = do_GetAtom(mToken.mIdent);
+      if (gotNotOrOnly ||
+          (mediaType != nsGkAtoms::_not && mediaType != nsGkAtoms::only))
+        break;
+      gotNotOrOnly = PR_TRUE;
+      if (mediaType == nsGkAtoms::_not)
+        query->SetNegated();
+      else
+        query->SetHasOnly();
+    }
+    query->SetType(mediaType);
+  }
 
-      // In terms of error handling, it doesn't really matter when we
-      // append this, since aMedia's contents get dropped entirely
-      // whenever there is an error.
-      nsresult rv = aMedia->AppendQuery(queryHolder);
+  for (;;) {
+    if (!GetToken(PR_TRUE)) {
+      *aHitStop = PR_TRUE;
+      // expected termination by EOF
+      if (aStopSymbol == PRUnichar(0))
+        break;
+
+      // unexpected termination by EOF
+      REPORT_UNEXPECTED_EOF(PEGatherMediaEOF);
+      break;
+    }
+
+    if (eCSSToken_Symbol == mToken.mType &&
+        mToken.mSymbol == aStopSymbol) {
+      *aHitStop = PR_TRUE;
+      UngetToken();
+      break;
+    }
+    if (eCSSToken_Symbol == mToken.mType && mToken.mSymbol == ',') {
+      // Done with the expressions for this query
+      break;
+    }
+    if (eCSSToken_Ident != mToken.mType ||
+        !mToken.mIdent.LowerCaseEqualsLiteral("and")) {
+      REPORT_UNEXPECTED_TOKEN(PEGatherMediaNotComma);
+      UngetToken();
+      return PR_FALSE;
+    }
+    if (!ParseMediaQueryExpression(query)) {
+      OUTPUT_ERROR();
+      query->SetHadUnknownExpression();
+    }
+  }
+  *aQuery = query.forget();
+  return PR_TRUE;
+}
+
+// Returns false only when there is a low-level error in the scanner
+// (out-of-memory).
+PRBool
+CSSParserImpl::GatherMedia(nsMediaList* aMedia,
+                           PRUnichar aStopSymbol)
+{
+  for (;;) {
+    nsAutoPtr<nsMediaQuery> query;
+    PRBool parsedSomething, hitStop;
+    if (!ParseMediaQuery(aStopSymbol, getter_Transfers(query),
+                         &parsedSomething, &hitStop)) {
+      if (NS_FAILED(mScanner.GetLowLevelError())) {
+        return PR_FALSE;
+      }
+      SkipUntil(',');
+    }
+    if (parsedSomething) {
+      aMedia->SetNonEmpty();
+    }
+    if (query) {
+      nsresult rv = aMedia->AppendQuery(query);
       if (NS_FAILED(rv)) {
         mScanner.SetLowLevelError(rv);
         return PR_FALSE;
       }
-      NS_ASSERTION(!queryHolder, "ownership should have been transferred");
     }
-
-    if (ExpectSymbol('(', PR_TRUE)) {
-      // we got an expression without a media type
-      UngetToken(); // so ParseMediaQueryExpression can handle it
-      query->SetType(nsGkAtoms::all);
-      query->SetTypeOmitted();
-      // Just parse the first expression here.
-      if (!ParseMediaQueryExpression(query)) {
-        OUTPUT_ERROR();
-        query->SetHadUnknownExpression();
-      }
-    } else {
-      nsCOMPtr<nsIAtom> mediaType;
-      PRBool gotNotOrOnly = PR_FALSE;
-      for (;;) {
-        if (!GetToken(PR_TRUE)) {
-          REPORT_UNEXPECTED_EOF(PEGatherMediaEOF);
-          return PR_FALSE;
-        }
-        if (eCSSToken_Ident != mToken.mType) {
-          REPORT_UNEXPECTED_TOKEN(PEGatherMediaNotIdent);
-          UngetToken();
-          return PR_FALSE;
-        }
-        // case insensitive from CSS - must be lower cased
-        ToLowerCase(mToken.mIdent);
-        mediaType = do_GetAtom(mToken.mIdent);
-        if (gotNotOrOnly ||
-            (mediaType != nsGkAtoms::_not && mediaType != nsGkAtoms::only))
-          break;
-        gotNotOrOnly = PR_TRUE;
-        if (mediaType == nsGkAtoms::_not)
-          query->SetNegated();
-        else
-          query->SetHasOnly();
-      }
-      query->SetType(mediaType);
-    }
-
-    for (;;) {
-      if (!GetToken(PR_TRUE)) {
-        // expected termination by EOF
-        if (aStopSymbol == PRUnichar(0))
-          return PR_TRUE;
-
-        // unexpected termination by EOF; if we were looking for a
-        // semicolon, return true anyway, for the same reason this is
-        // done by ExpectSymbol().
-        REPORT_UNEXPECTED_EOF(PEGatherMediaEOF);
-        return aStopSymbol == PRUnichar(';');
-      }
-
-      if (eCSSToken_Symbol == mToken.mType &&
-          mToken.mSymbol == aStopSymbol) {
-        UngetToken();
-        return PR_TRUE;
-      }
-      if (eCSSToken_Symbol == mToken.mType && mToken.mSymbol == ',') {
-        // Done with the expressions for this query
-        break;
-      }
-      if (eCSSToken_Ident != mToken.mType ||
-          !mToken.mIdent.LowerCaseEqualsLiteral("and")) {
-        REPORT_UNEXPECTED_TOKEN(PEGatherMediaNotComma);
-        UngetToken();
-        return PR_FALSE;
-      }
-      if (!ParseMediaQueryExpression(query)) {
-        OUTPUT_ERROR();
-        query->SetHadUnknownExpression();
-      }
+    if (hitStop) {
+      break;
     }
   }
-  NS_NOTREACHED("unreachable code");
-  return PR_FALSE; // keep the compiler happy
+  return PR_TRUE;
 }
 
 PRBool

layout/style/test/test_acid3_test46.html

@@ -109,7 +109,7 @@ extracted from the test framework there and put into Mochitest.
   check('q', false);
   check('r', false);
   check('s', false);
-  check('t', false); // 20
+  check('t', true); // 20 - false in old spec
   check('u', false);
   check('v', true);
   check('w', true);

layout/style/test/test_media_queries.html

@@ -421,6 +421,21 @@ function run() {
   should_not_apply("(grid");
   should_apply("all,(grid");
   should_not_apply("(grid,all");
+  // bug 454226
+  should_apply(",all");
+  should_apply("all,");
+  should_apply(",all,");
+  should_apply("all,badmedium");
+  should_apply("badmedium,all");
+  should_not_apply(",badmedium,");
+  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");
 
   SimpleTest.finish();
 }