528096 - missing UngetToken calls in nsCSSParser

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

While reviewing bug 399495 I noticed a bunch of missing UngetToken calls in nsCSSParser (immediately prior to calls to SkipUntil) that lead to incorrect error handling when the unexpected token that we hit is (, {, or [.

These are (from bug 399495 comment 6 and bug 399495 comment 9):

CSSParserImpl::ParseMediaQueryExpression needs an UngetToken before its first
SkipUntil call, the SkipUntil call after !mToken.IsSymbol(':')

CSSParserImpl::ParseTreePseudoElement needs the same before its SkipUntil
(which was introduced since this patch was written)

CSSParserImpl::ParseCounter also needs an UngetToken before its first three
SkipUntil calls.

CSSParserImpl::ParseImageRect needs an UngetToken before a number of its break
statements.

Oh, I missed a missing UngetToken in:
    case nsMediaFeature::eResolution:
in CSSParserImpl::ParseMediaQueryExpression.


We should:
 * fix these
 * add tests for them
 * make sure there's also a test for the one that patch already fixes that's in CSSParserImpl::ParseFontSrcFormat

Comment 1

[Zack Weinberg (:zwol)]

Attachment: patch

Finally got around to writing the patch for this.

Comment 2

[Zack Weinberg (:zwol)]

N.B. The patch here applies on top of the patch for bug 399495.  I'd like to land that patch (which has been r+ for some time now) and this patch together, very soon.

Comment 3

[Zack Weinberg (:zwol)]

Further things I should have said:  The patch passes local reftests and layout/style mochitests.  I was only able to find one break statement in ParseImageRect that required an UngetToken - all the others have either just called functions that can be trusted not to consume tokens on failure, or have consumed a token that does not change grouping behavior.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 423051 [details] [diff] [review]
patch

In ParseCounter, you have:

>+    if (!GetNonCloseParenToken(PR_TRUE) ||
>+        eCSSToken_Ident != mToken.mType) {
>+      UngetToken();
>+      break;
>+    }

But this needs to be split into two conditions since you should only
call UngetToken if the second condition is false.

>+      if (!ExpectSymbol(',', PR_TRUE) ||
>+          !(GetNonCloseParenToken(PR_TRUE) &&
>+            eCSSToken_String == mToken.mType)) {
>+        UngetToken();
>+        break;
>+      }

Same here (except only for the third part, counted after turning !(a&&b)
into !a||!b).

>+      nsCSSKeyword keyword;
>+      if (!GetNonCloseParenToken(PR_TRUE) ||
>+          eCSSToken_Ident != mToken.mType) {
>+        UngetToken();
>+        break;
>+      }
>+      keyword = nsCSSKeywords::LookupKeyword(mToken.mIdent);
>+      if (keyword == eCSSKeyword_UNKNOWN ||
>+          !nsCSSProps::FindKeyword(keyword, nsCSSProps::kListStyleKTable,
>+                                   type)) {
>+        break;
>+      }

Same here, except you also (in addition to not wanting the UngetToken
for the GetNonCloseParenToken failure in the first condition) *do* want
the UngetToken to apply to the second break.

I think you should also remove GetNonCloseParenToken entirely (and just
use GetToken); it was an incorrect workaround for the UngetToken calls
being missing.


I presume you checked that the tests fail pre-patch?

r=dbaron with that

Comment 5

[Zack Weinberg (:zwol)]

(In reply to comment #4)
> (From update of attachment 423051 [details] [diff] [review])
> In ParseCounter, you have:
> 
> >+    if (!GetNonCloseParenToken(PR_TRUE) ||
> >+        eCSSToken_Ident != mToken.mType) {
> >+      UngetToken();
> >+      break;
> >+    }
> 
> But this needs to be split into two conditions since you should only
> call UngetToken if the second condition is false.

Lemme make sure I understand why.  GetNonCloseParenToken returns PR_FALSE if it encounters EOF, or if it encounters a close paren.

In the former case, we'll process the token immediately before the EOF (presumably "counter(" or "counters(") twice, because EOF is not a token, so whatever was in mToken before mScanner.Next() failed will still be there.  This cannot be observed, because the duplicated token will be absorbed by SkipUntil() which will then close all nesting levels when it sees EOF.

In the latter case, we'll push back the close paren twice and hit the assertion in UngetToken().  This is also unobservable in terms of parser behavior, but still clearly wrong.

Yes?

[...]
> >+      keyword = nsCSSKeywords::LookupKeyword(mToken.mIdent);
> >+      if (keyword == eCSSKeyword_UNKNOWN ||
> >+          !nsCSSProps::FindKeyword(keyword, nsCSSProps::kListStyleKTable,
> >+                                   type)) {
> >+        break;
> >+      }
> 
> [...] you [...] *do* want the UngetToken to apply to [this] break.

Why is that?  We know the token is an Ident at this point, so whether it has been pushed back or not does not affect where SkipUntil stops.

> I think you should also remove GetNonCloseParenToken entirely (and just
> use GetToken); it was an incorrect workaround for the UngetToken calls
> being missing.

Will do.

> I presume you checked that the tests fail pre-patch?

Yes, in fact I wrote them by messing with the syntax until I got something that was an observable failure, for each problem case.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I didn't work through the observable effects in all cases, but I think we should just UngetToken on the things that we didn't actually consume; failing to do this was the original problem in this bug.

(In reply to comment #5)
> Why is that?  We know the token is an Ident at this point, so whether it has
> been pushed back or not does not affect where SkipUntil stops.

Why not (at least relative to the way the code was written before your patch, which you can easily restore)?  It's one *fewer* branch in the code.

Comment 7

[Zack Weinberg (:zwol)]

(In reply to comment #6)
> I didn't work through the observable effects in all cases, but I think we
> should just UngetToken on the things that we didn't actually consume; failing
> to do this was the original problem in this bug.

Ok.

> > Why is that?  We know the token is an Ident at this point, so whether it has
> > been pushed back or not does not affect where SkipUntil stops.
> 
> Why not (at least relative to the way the code was written before your patch,
> which you can easily restore)?  It's one *fewer* branch in the code.

I wanted to make sure there wasn't some reason why it *had* to be done that I was missing.  This is what that part looks like now:

+    // get optional type
+    PRInt32 type = NS_STYLE_LIST_STYLE_DECIMAL;
+    if (ExpectSymbol(',', PR_TRUE)) {
+      nsCSSKeyword keyword;
+      if (!GetToken(PR_TRUE)) {
+        break;
+      }
+      if (eCSSToken_Ident != mToken.mType ||
+          (keyword = nsCSSKeywords::LookupKeyword(mToken.mIdent)) ==
+          eCSSKeyword_Unknown ||
+          !nsCSSProps::FindKeyword(keyword, nsCSSProps::kListStyleKTable,
+                                   type)) {
+        UngetToken();
+        break;
+      }
+    }

Good?

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Good, but may as well move the declaration of keyword down three lines, and also indent eCSSKeyword_Unknown two more spaces.

Comment 9

[Zack Weinberg (:zwol)]

doh.  eCSSKeyword_UNKNOWN, of course.

Comment 10

[Zack Weinberg (:zwol)]

Attachment: patch as committed

Here's what I just pushed, along with bug 399495.

Comment 11

[Zack Weinberg (:zwol)]

http://hg.mozilla.org/mozilla-central/rev/9259fdc3570c