Bug 112880 – Wrong recovery when parsing invalid block after declaration

• Component: CSS
• Status: RESOLVED CONFIGURATION CHANGED

[Sergey Ryazanov]

The following example is parsed incorrectly:

body {
   background-color: red;
   {invalid block}#
}

body {
   background-color: greed;
}

Parser ignoring the opening brace of the invalid block and considers its closing brace as a closing for the declaration list. Only happens if 1) invalid block follows another declaration and 2) followed by any character (including whitespace).

In this example second rule is ignored.

[Sergey Ryazanov]

Created attachment 194190 [details]
Patch

[Sergey Ryazanov]

Created attachment 194496 [details]
Patch

[Sergey Ryazanov]

Created attachment 194900 [details]
Patch

[Build Bot]

Comment on attachment 194900 [details]
Patch

Attachment 194900 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17132913

[Sergey Ryazanov]

Created attachment 195044 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 195044 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195044&action=review

> Source/WebCore/ChangeLog:10
> +        The rgammar rule changed to handle invalid tokens before and after invalid block.

rgammar -> grammar

[Sergey Ryazanov]

Created attachment 195320 [details]
Patch

[Tab Atkins]

I haven't looked at your patch to see what code changes have been made, but the correct behavior here (assuming "greed" is a typo for "green") is described here: http://dev.w3.org/csswg/css3-syntax/#consume-a-list-of-declarations

In summary, while you're inside of a style block, and thus in the "consume a list of declarations" mode, encountering the {invalid block} hits the "anything else" clause, which is a syntax error.  You then seek forward, throwing away everything, until you hit a ; or the end of the style block.

So, the page should be green, obviously.

[Sergey Ryazanov]

(In reply to comment #8)
> I haven't looked at your patch to see what code changes have been made, but the correct behavior here (assuming "greed" is a typo for "green") is described here: http://dev.w3.org/csswg/css3-syntax/#consume-a-list-of-declarations

There also is a section that clarifies error recovery mechanism: http://www.w3.org/TR/css3-syntax/#error-handling. It states that the parser must observe the rule "matching pairs of (), [], {}, "", and '', and correctly handling escapes".

> In summary, while you're inside of a style block, and thus in the "consume a list of declarations" mode, encountering the {invalid block} hits the "anything else" clause, which is a syntax error.  You then seek forward, throwing away everything, until you hit a ; or the end of the style block.
> 
> So, the page should be green, obviously.

The test from the description doesn't fail any longer (because of https://bugs.webkit.org/show_bug.cgi?id=113142) but other tests still do. There is a result of running the test on TOT:
FAIL: Test 6
FAIL: Test 8
FAIL: Test 10
FAIL: Test 12
FAIL: Test 13
FAIL: Test 15

For instance:
        .malformed6 {color: red; #{}}
        #test6 {
            display:none;
        }

Parser log shows it considers "#{" as a syntax error and drops it. First closing brace is considered as closing brace for the whole rule. Next closing brace is prevents the second selector from being parsed correctly.

[Tab Atkins]

(In reply to comment #9)
> (In reply to comment #8)
> > I haven't looked at your patch to see what code changes have been made, but the correct behavior here (assuming "greed" is a typo for "green") is described here: http://dev.w3.org/csswg/css3-syntax/#consume-a-list-of-declarations
> 
> There also is a section that clarifies error recovery mechanism: http://www.w3.org/TR/css3-syntax/#error-handling. It states that the parser must observe the rule "matching pairs of (), [], {}, "", and '', and correctly handling escapes".

Don't look at that spec - it's from 2003 and very obsolete and wrong.  Multiple syntax changes that have been made to CSS 2.1 never got applied to that spec.  Only look at the Editor's draft at the link I gave you instead, at least until I actually get my draft published as a Working Draft.

> > In summary, while you're inside of a style block, and thus in the "consume a list of declarations" mode, encountering the {invalid block} hits the "anything else" clause, which is a syntax error.  You then seek forward, throwing away everything, until you hit a ; or the end of the style block.
> > 
> > So, the page should be green, obviously.
> 
> The test from the description doesn't fail any longer (because of https://bugs.webkit.org/show_bug.cgi?id=113142) but other tests still do. There is a result of running the test on TOT:
> FAIL: Test 6
> FAIL: Test 8
> FAIL: Test 10
> FAIL: Test 12
> FAIL: Test 13
> FAIL: Test 15
> 
> For instance:
>         .malformed6 {color: red; #{}}
>         #test6 {
>             display:none;
>         }
> 
> Parser log shows it considers "#{" as a syntax error and drops it. First closing brace is considered as closing brace for the whole rule. Next closing brace is prevents the second selector from being parsed correctly.

Yes, those are definitely wrong and should be processed as you think - the "#" should parse as a DELIM character, and the {} following that is a simple block.

[Sergey Ryazanov]

> > For instance:
> >         .malformed6 {color: red; #{}}
> >         #test6 {
> >             display:none;
> >         }
> > 
> > Parser log shows it considers "#{" as a syntax error and drops it. First closing brace is considered as closing brace for the whole rule. Next closing brace is prevents the second selector from being parsed correctly.
> 
> Yes, those are definitely wrong and should be processed as you think - the "#" should parse as a DELIM character, and the {} following that is a simple block.

This example is parsed wrongly from common sense (as far as I understand invalid_blocks are ignored for future syntax compatibility) and consistency (because the current grammar throws away such blocks in some contexts and doesn't in others) and the old spec's points of view. But the new draft doesn't say that. In this example the parser works exactly as you said:
1. Sees #. It is not a valid declaration, not EOF or a block end. So it's a syntax error.
2. Reads the next token '{'. It is not an end of the block, not EOF, not a semicolon. So it moves forward.
3. It sees the '}' token. This is the first closing brace it met so it closes the declaration list.

[Tab Atkins]

(In reply to comment #11)
> > > For instance:
> > >         .malformed6 {color: red; #{}}
> > >         #test6 {
> > >             display:none;
> > >         }
> > > 
> > > Parser log shows it considers "#{" as a syntax error and drops it. First closing brace is considered as closing brace for the whole rule. Next closing brace is prevents the second selector from being parsed correctly.
> > 
> > Yes, those are definitely wrong and should be processed as you think - the "#" should parse as a DELIM character, and the {} following that is a simple block.
> 
> This example is parsed wrongly from common sense (as far as I understand invalid_blocks are ignored for future syntax compatibility) and consistency (because the current grammar throws away such blocks in some contexts and doesn't in others) and the old spec's points of view. But the new draft doesn't say that. In this example the parser works exactly as you said:
> 1. Sees #. It is not a valid declaration, not EOF or a block end. So it's a syntax error.
> 2. Reads the next token '{'. It is not an end of the block, not EOF, not a semicolon. So it moves forward.
> 3. It sees the '}' token. This is the first closing brace it met so it closes the declaration list.

Are you trying to say that those steps are what the current Syntax spec (at http://dev.w3.org/csswg/css-syntax/) says?  Because it doesn't - it does what I said, instead, where the # is a syntax error, it consumes the {} whole as a simple block, and then recovers when it sees the } closing the style rule.  (It knows that, because the style rule has already found its closing brace, and is just parsing its contents, so there's no chance of misparsing on that.)

[Rune Lillesveen]

Bug 112986 (currently being reviewed) introduces a bracket stack keeping track of matching parentheses. That patch is fixing error handling for media queries specifically, but error recovery for other parts of the grammar needs to take balancing of parentheses into account too. I just didn't want to make that patch too large.

[Ahmad Saleem]

Webkit CSS Parser was updated with Blink one, this patch will not apply and the test pass any way in Safari 16:

Link - https://jsfiddle.net/t6repdLk/

Marking this as "RESOLVED CONFIGURATION CHANGED". Thanks!