Bug 112880 - Wrong recovery when parsing invalid block after declaration
Summary: Wrong recovery when parsing invalid block after declaration
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergey Ryazanov
URL:
Keywords:
Depends on:
Blocks: 113401
  Show dependency treegraph
 
Reported: 2013-03-20 23:09 PDT by Sergey Ryazanov
Modified: 2013-05-25 05:38 PDT (History)
13 users (show)

See Also:


Attachments
Patch (2.65 KB, patch)
2013-03-20 23:13 PDT, Sergey Ryazanov
no flags Details | Formatted Diff | Diff
Patch (2.70 KB, patch)
2013-03-22 03:23 PDT, Sergey Ryazanov
no flags Details | Formatted Diff | Diff
Patch (6.11 KB, patch)
2013-03-25 12:14 PDT, Sergey Ryazanov
no flags Details | Formatted Diff | Diff
Patch (6.27 KB, patch)
2013-03-26 03:07 PDT, Sergey Ryazanov
no flags Details | Formatted Diff | Diff
Patch (6.24 KB, patch)
2013-03-27 07:39 PDT, Sergey Ryazanov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergey Ryazanov 2013-03-20 23:09:51 PDT
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.
Comment 1 Sergey Ryazanov 2013-03-20 23:13:30 PDT
Created attachment 194190 [details]
Patch
Comment 2 Sergey Ryazanov 2013-03-22 03:23:07 PDT
Created attachment 194496 [details]
Patch
Comment 3 Sergey Ryazanov 2013-03-25 12:14:44 PDT
Created attachment 194900 [details]
Patch
Comment 4 Build Bot 2013-03-25 14:39:56 PDT
Comment on attachment 194900 [details]
Patch

Attachment 194900 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17132913
Comment 5 Sergey Ryazanov 2013-03-26 03:07:05 PDT
Created attachment 195044 [details]
Patch
Comment 6 Alexander Pavlov (apavlov) 2013-03-27 06:43:23 PDT
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
Comment 7 Sergey Ryazanov 2013-03-27 07:39:39 PDT
Created attachment 195320 [details]
Patch
Comment 8 Tab Atkins 2013-03-28 11:34:08 PDT
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.
Comment 9 Sergey Ryazanov 2013-03-28 14:32:17 PDT
(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.
Comment 10 Tab Atkins 2013-03-28 15:15:26 PDT
(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.
Comment 11 Sergey Ryazanov 2013-03-29 12:02:34 PDT
> > 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.
Comment 12 Tab Atkins 2013-03-29 12:53:00 PDT
(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.)
Comment 13 Rune Lillesveen 2013-04-03 05:02:45 PDT
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.