Improved parse error handling for CSSMQ.

Improved parse error handling for CSSMQ.
    
Corrected the grammar for media queries and media query lists.
    
Do recognize "and(" as function tokens. The existing code erroneously split
it into "and" and "(" inside media queries.
    
Keep returning 0 instead of "not all" for @-webkit-mediaquery parse errors
to avoid that things like deleteMedium("#?:/") delete "not all" queries.

BUG=236302
NOTRY=true

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151842

[SeRya, 2013-05-09 16:47:42]

Diff tool doesn't work for me for some reason so I'm answering in comments.

1. Using "closing_brace" in internal rules (like  internal_rule, internal_decls)
hardly make seance. As far as I understand there is no way to use that grammar
rules except "setupParser" in CSSParser. And it always has implicit closing
brace. As consequence of that CSS in a style attribute may forgive enclosed
brace but may not forgive unclosed parenthesis or square brace.

2. Adding new error messages is potentially harmful operation. In some cases
invalid CSS is used intentionally as compatibility hacks. It could flood
inspector console. So I'd prefer to add it separately (even though it unlikely
with media queries).

3. I don't understand why there is so many rules for error recovery. Could you
explain how it is better than just using "error_recovery: /*empty*/|error|'('
error_recovery closing_parenthesis|...", please?

4. Please explain why do you need rule "media_query: '(' media_query_recovery
closing_parenthesis". Rule "media_query: media_query_recovery" should handle
this case fine.

5. Rule "media_list: mq_list error_location;" looks suspicious for me. It may
reduce "media_query,media_query," but may not "media_query,media_query,{}".

[kenneth.r.christiansen, 2013-05-30 17:30:47]

https://codereview.chromium.org/14620012/diff/1/LayoutTests/fast/media/media-...
File LayoutTests/fast/media/media-descriptor-syntax-01.html (right):

https://codereview.chromium.org/14620012/diff/1/LayoutTests/fast/media/media-...
LayoutTests/fast/media/media-descriptor-syntax-01.html:13: <p> This text should
not be red. </p>
Maybe remove the space after red.

[kenneth.r.christiansen, 2013-05-30 17:33:10]

For some reason I cannot see the changes in Source/core/css/CSSGrammar.y.in

[rune, 2013-05-31 08:51:54]

New patch on top of Serya's error recovery fixes. I've done less piggy-backing
this time and limited it to the error recovery of media query parsing inside
stylesheets and the "and(" tokenization issue.

[rune, 2013-05-31 08:53:37]

On 2013/05/30 17:30:47, kenneth.r.christiansen wrote:
>
https://codereview.chromium.org/14620012/diff/1/LayoutTests/fast/media/media-...
> File LayoutTests/fast/media/media-descriptor-syntax-01.html (right):
> 
>
https://codereview.chromium.org/14620012/diff/1/LayoutTests/fast/media/media-...
> LayoutTests/fast/media/media-descriptor-syntax-01.html:13: <p> This text
should
> not be red. </p>
> Maybe remove the space after red.

I've dropped this change in the second patch set.

[rune, 2013-05-31 09:24:01]

On 2013/05/09 16:47:42, SeRya wrote:
> Diff tool doesn't work for me for some reason so I'm answering in comments.
> 
> 1. Using "closing_brace" in internal rules (like  internal_rule,
internal_decls)
> hardly make seance. As far as I understand there is no way to use that grammar
> rules except "setupParser" in CSSParser. And it always has implicit closing
> brace. As consequence of that CSS in a style attribute may forgive enclosed
> brace but may not forgive unclosed parenthesis or square brace.

I've dropped these changes in the second patch set. A way of fixing it, is to
change the setupParser calls to not adding braces, just an @-symbol, to handle
unexpected EOF properly. That's for another patch.

> 2. Adding new error messages is potentially harmful operation. In some cases
> invalid CSS is used intentionally as compatibility hacks. It could flood
> inspector console. So I'd prefer to add it separately (even though it unlikely
> with media queries).

Dropped in second patch.

> 3. I don't understand why there is so many rules for error recovery. Could you
> explain how it is better than just using "error_recovery: /*empty*/|error|'('
> error_recovery closing_parenthesis|...", please?

I've used your recovery rules in the second patch set. Have another look.

> 4. Please explain why do you need rule "media_query: '(' media_query_recovery
> closing_parenthesis". Rule "media_query: media_query_recovery" should handle
> this case fine.

No. There is a general problem with input that is shifted and reduced up to the
point where you have reach a synchronization token for your error recovery
rules.

Example:

"@media all and (()) "

Here tokens are fine up to "@media all and (". When you encounter the second '('
you still haven't seen an error and the second '(' will be shifted as part of
the error token. But, your normal error recovery requires that you have an error
token before it starts balancing using invalid_parentheses_block.

You'll also see I've added a production to "media":

"before_media_rule MEDIA_SYM maybe_space media_list ';'"

in order to catch cases like:

"@media all; div { color: green }"

Without it, the semi-colon would be the first shifted token for the error token
and the succeeding div rule would not have been recognized.

I don't know if there's a better way in bison.

> 5. Rule "media_list: mq_list error_location;" looks suspicious for me. It may
> reduce "media_query,media_query," but may not "media_query,media_query,{}".

I don't think I understand. Btw, error_location can now be removed since no
error is reported anymore - I'll update the patch.

[kenneth.r.christiansen, 2013-05-31 09:39:14]

> to avoid that things like deleteMedium("#?:/") delete "not all" queries. 

There is a test for deleteMedium and appendMedium now. You should add this test
case there as well

[rune, 2013-05-31 10:12:22]

On 2013/05/31 09:39:14, kenneth.r.christiansen wrote:
> > to avoid that things like deleteMedium("#?:/") delete "not all" queries. 
> 
> There is a test for deleteMedium and appendMedium now. You should add this
test
> case there as well

Done.

[rune, 2013-05-31 22:53:46]

On 2013/05/31 09:24:01, rune wrote:
> On 2013/05/09 16:47:42, SeRya wrote:

> > 4. Please explain why do you need rule "media_query: '('
media_query_recovery
> > closing_parenthesis". Rule "media_query: media_query_recovery" should handle
> > this case fine.
> 
> No. There is a general problem with input that is shifted and reduced up to
the
> point where you have reach a synchronization token for your error recovery
> rules.
> 
> Example:
> 
> "@media all and (()) "
> 
> Here tokens are fine up to "@media all and (". When you encounter the second
'('
> you still haven't seen an error and the second '(' will be shifted as part of
> the error token. But, your normal error recovery requires that you have an
error
> token before it starts balancing using invalid_parentheses_block.

I've moved the error recovery for this case to media_query_exp instead, to cover
the case for multiple media_query_exps. New patch set coming up.

[SeRya, 2013-06-01 06:28:03]

https://codereview.chromium.org/14620012/diff/20001/Source/core/css/CSSGramma...
File Source/core/css/CSSGrammar.y.in (right):

https://codereview.chromium.org/14620012/diff/20001/Source/core/css/CSSGramma...
Source/core/css/CSSGrammar.y.in:578: media_feature:
May be use just IDENT rather than media_feature?

https://codereview.chromium.org/14620012/diff/20001/Source/core/css/CSSGramma...
Source/core/css/CSSGrammar.y.in:598: | '(' error error_recovery
closing_parenthesis {
If you remove YYERROR you need to add maybe_space at the end.

However I'd suggest not remove YYERROR but handle errors on the hier levels. It
seems that media_list/mq_list know that to do with errors. I think it is better
place.

https://codereview.chromium.org/14620012/diff/20001/Source/core/css/CSSGramma...
Source/core/css/CSSGrammar.y.in:612: if ($1 && $4) {
If you handle errors on higher level you likely can relay $1 & $4 are never 0
(unless createFloatingMediaQueryExpList can return it).

https://codereview.chromium.org/14620012/diff/20001/Source/core/css/CSSGramma...
Source/core/css/CSSGrammar.y.in:734: | before_media_rule MEDIA_SYM maybe_space
media_list ';' {
It shouldn't be needed. "before_media_rule MEDIA_SYM at_rule_recovery" handles
this case.

grammar should push "error" on ";". Then drop media_list, then drop maybe_space,
then "error ';'" should be reduced to at_rule_recovery. Have you checked if
grammar works without this rule?

https://codereview.chromium.org/14620012/diff/20001/Source/core/css/CSSParser...
File Source/core/css/CSSParser.cpp (right):

https://codereview.chromium.org/14620012/diff/20001/Source/core/css/CSSParser...
Source/core/css/CSSParser.cpp:10312: if (!hasEscape)
It seems MediaQueryMode isn't used any longer and should be removed.

[kenneth.r.christiansen, 2013-06-01 10:12:24]

How does this patch affect our pass rate of
http://www.w3.org/Style/CSS/Test/MediaQueries/20120229/test_media_queries.html ?

[rune, 2013-06-03 09:08:11]

https://codereview.chromium.org/14620012/diff/20001/Source/core/css/CSSGramma...
File Source/core/css/CSSGrammar.y.in (right):

https://codereview.chromium.org/14620012/diff/20001/Source/core/css/CSSGramma...
Source/core/css/CSSGrammar.y.in:578: media_feature:
On 2013/06/01 06:28:03, SeRya wrote:
> May be use just IDENT rather than media_feature?

Fixed in next patch set.

https://codereview.chromium.org/14620012/diff/20001/Source/core/css/CSSGramma...
Source/core/css/CSSGrammar.y.in:598: | '(' error error_recovery
closing_parenthesis {
On 2013/06/01 06:28:03, SeRya wrote:
> If you remove YYERROR you need to add maybe_space at the end.
> 
> However I'd suggest not remove YYERROR but handle errors on the hier levels.
It
> seems that media_list/mq_list know that to do with errors. I think it is
better
> place.

Fixed in next patch set.

https://codereview.chromium.org/14620012/diff/20001/Source/core/css/CSSGramma...
Source/core/css/CSSGrammar.y.in:612: if ($1 && $4) {
On 2013/06/01 06:28:03, SeRya wrote:
> If you handle errors on higher level you likely can relay $1 & $4 are never 0
> (unless createFloatingMediaQueryExpList can return it).

Fixed in next patch set.

https://codereview.chromium.org/14620012/diff/20001/Source/core/css/CSSGramma...
Source/core/css/CSSGrammar.y.in:734: | before_media_rule MEDIA_SYM maybe_space
media_list ';' {
On 2013/06/01 06:28:03, SeRya wrote:
> It shouldn't be needed. "before_media_rule MEDIA_SYM at_rule_recovery" handles
> this case.
> 
> grammar should push "error" on ";". Then drop media_list, then drop
maybe_space,
> then "error ';'" should be reduced to at_rule_recovery. Have you checked if
> grammar works without this rule?

Media query error #4 (parsing of "@media all; @media tv {}") in
fast/media/media-query-list-syntax.html fails without this rule. I guess that's
because without it, the error recovery in media_query will look for either a ','
or a '{'.

https://codereview.chromium.org/14620012/diff/20001/Source/core/css/CSSParser...
File Source/core/css/CSSParser.cpp (right):

https://codereview.chromium.org/14620012/diff/20001/Source/core/css/CSSParser...
Source/core/css/CSSParser.cpp:10312: if (!hasEscape)
On 2013/06/01 06:28:03, SeRya wrote:
> It seems MediaQueryMode isn't used any longer and should be removed.

It's still used for detectMediaQueryToken below.

[rune, 2013-06-03 09:36:53]

On 2013/06/01 10:12:24, kenneth.r.christiansen wrote:
> How does this patch affect our pass rate of
> http://www.w3.org/Style/CSS/Test/MediaQueries/20120229/test_media_queries.html
?

Down two from 11 to 9 (subtest_355 and subtest 356 started to pass) if I'm
comparing correct builds. I think the remaining issues are in
MediaQuerySet::parseMediaQueryList/CSSParser::parseMediaQuery. I thought it
would be better handled with a separate patch once this patch is good.

[apavlov, 2013-06-03 11:03:44]

https://codereview.chromium.org/14620012/diff/29001/LayoutTests/fast/css/medi...
File LayoutTests/fast/css/media-rule-no-whitespace.html (right):

https://codereview.chromium.org/14620012/diff/29001/LayoutTests/fast/css/medi...
LayoutTests/fast/css/media-rule-no-whitespace.html:5: @media all and(min-width:
2px) { #styled { color: red; } }
This patch forbids zero whitespace between the 'and' and '(' tokens. I should
say that this had worked this way until
https://bugs.webkit.org/show_bug.cgi?id=106458 (upstreaming
http://crbug.com/168339, a real-world issue) was fixed.

http://www.w3.org/TR/css3-mediaqueries/#syntax allows zero or more whitespace
characters between 'and' and '(':

media_query
 : [ONLY | NOT]? S* media_type S* [ AND S* expression ]*
 | expression [ AND S* expression ]*
 ;

However, example 20 in http://www.w3.org/TR/css3-mediaqueries/#error-handling
contradicts this: "The following is an malformed media query because having no
space between ‘and’ and the expression is not allowed. (That is reserved for the
functional notation syntax.)"

I'd like knowledgeable people to shed some light on this issue.

[rune, 2013-06-03 11:17:26]

On 2013/06/03 11:03:44, apavlov wrote:
>
https://codereview.chromium.org/14620012/diff/29001/LayoutTests/fast/css/medi...
> File LayoutTests/fast/css/media-rule-no-whitespace.html (right):
> 
>
https://codereview.chromium.org/14620012/diff/29001/LayoutTests/fast/css/medi...
> LayoutTests/fast/css/media-rule-no-whitespace.html:5: @media all
and(min-width:
> 2px) { #styled { color: red; } }
> This patch forbids zero whitespace between the 'and' and '(' tokens. I should
> say that this had worked this way until
> https://bugs.webkit.org/show_bug.cgi?id=106458 (upstreaming
> http://crbug.com/168339, a real-world issue) was fixed.
> 
> http://www.w3.org/TR/css3-mediaqueries/#syntax allows zero or more whitespace
> characters between 'and' and '(':
> 
> media_query
>  : [ONLY | NOT]? S* media_type S* [ AND S* expression ]*
>  | expression [ AND S* expression ]*
>  ;
> 
> However, example 20 in http://www.w3.org/TR/css3-mediaqueries/#error-handling
> contradicts this: "The following is an malformed media query because having no
> space between ‘and’ and the expression is not allowed. (That is reserved for
the
> functional notation syntax.)"

I don't see a contradiction since no space would give you a FUNCTION token for
"and(" and therefore not match the grammar. A peculiarity of the S*, is that
"and/**/(" works. This was actually mentioned yesterday on www-style:
http://lists.w3.org/Archives/Public/www-style/2013Jun/0010.html

So, I think the dilemma is:

- Choose interoperability with spec and other engines.
- Choose interoperability with older webkit/blink.

[SeRya, 2013-06-03 11:31:26]

Looks almost OK.

https://codereview.chromium.org/14620012/diff/20001/Source/core/css/CSSGramma...
File Source/core/css/CSSGrammar.y.in (right):

https://codereview.chromium.org/14620012/diff/20001/Source/core/css/CSSGramma...
Source/core/css/CSSGrammar.y.in:734: | before_media_rule MEDIA_SYM maybe_space
media_list ';' {
On 2013/06/03 09:08:11, rune wrote:
> On 2013/06/01 06:28:03, SeRya wrote:
> > It shouldn't be needed. "before_media_rule MEDIA_SYM at_rule_recovery"
handles
> > this case.
> > 
> > grammar should push "error" on ";". Then drop media_list, then drop
> maybe_space,
> > then "error ';'" should be reduced to at_rule_recovery. Have you checked if
> > grammar works without this rule?
> 
> Media query error #4 (parsing of "@media all; @media tv {}") in
> fast/media/media-query-list-syntax.html fails without this rule. I guess
that's
> because without it, the error recovery in media_query will look for either a
','
> or a '{'.

I see why it needed. Because "all;" reduces to media_query (in error recovery
part). So it looks inevitable.

https://codereview.chromium.org/14620012/diff/29001/Source/core/css/CSSGramma...
File Source/core/css/CSSGrammar.y.in (right):

https://codereview.chromium.org/14620012/diff/29001/Source/core/css/CSSGramma...
Source/core/css/CSSGrammar.y.in:629: media_query_or_null:
My suggestion is:

valid_media_query:
    ...;

media_query:
    valid_media_query
    | error rule_error_recovery { $$ = parser->createFloatingNotAllQuery() };

webkit_mediaquery: WEBKIT_MEDIAQUERY_SYM maybe_space valid_media_query '}';

I think it is better because reuse an existing pattern. webkit_mediaquery
doesn't need error recovery.

[rune, 2013-06-03 11:40:36]

On 2013/06/03 11:31:26, SeRya wrote:

> My suggestion is:
> 
> valid_media_query:
>     ...;
> 
> media_query:
>     valid_media_query
>     | error rule_error_recovery { $$ = parser->createFloatingNotAllQuery() };
> 
> webkit_mediaquery: WEBKIT_MEDIAQUERY_SYM maybe_space valid_media_query '}';
> 
> I think it is better because reuse an existing pattern. webkit_mediaquery
> doesn't need error recovery.

Yeah. That looks better.

[rune, 2013-06-03 11:47:31]

On 2013/06/03 11:40:36, rune wrote:
> On 2013/06/03 11:31:26, SeRya wrote:

> > I think it is better because reuse an existing pattern. webkit_mediaquery
> > doesn't need error recovery.
> 
> Yeah. That looks better.

New patch.

[SeRya, 2013-06-03 12:58:07]

LGTM with nits, but would be good to get LGTM from anyone of the reviewers else
due to complexity of the change.

https://codereview.chromium.org/14620012/diff/36001/Source/core/css/CSSGramma...
File Source/core/css/CSSGrammar.y.in (right):

https://codereview.chromium.org/14620012/diff/36001/Source/core/css/CSSGramma...
Source/core/css/CSSGrammar.y.in:641: | error rule_error_recovery { $$ =
parser->createFloatingNotAllQuery(); }
The action should be placed on a separate line.

[kenneth.r.christiansen, 2013-06-03 16:59:00]

lgtm

[rune, 2013-06-05 11:02:03]

I did modify the original code for media query tokenization to skip parenthesis
for AND only. The reason is that the original code would tokenize "f()" as:

FUNCTION '(' ')'
    
which broke balancing of parentheses as FUNCTION implies an opening parenthesis.

[apavlov, 2013-06-05 11:34:45]

lgtm

[commit-bot: I haz the power, 2013-06-05 11:34:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/14620012/49001

[commit-bot: I haz the power, 2013-06-05 13:04:51]

Retried try job too often on win_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[commit-bot: I haz the power, 2013-06-05 14:25:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/14620012/49001

[commit-bot: I haz the power, 2013-06-05 14:25:54]

Change committed as 151842