Bug 112986 – Incorrect error handling for Media Queries

• Component: CSS
• Status: RESOLVED CONFIGURATION CHANGED

[Rune Lillesveen]

Created attachment 194395 [details]
Invalid media query tests

WebKit fails to do correct error recovery for media queries a lot of cases. Not able to balance parentheses and brackets, not able to see valid media queries after an invalid one, and does not return 'not all' for invalid queries. See attached test-case for a demonstration.

[Rune Lillesveen]

Created attachment 194400 [details]
Patch

[Build Bot]

Comment on attachment 194400 [details]
Patch

Attachment 194400 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17125890

[WebKit Review Bot]

Comment on attachment 194400 [details]
Patch

Attachment 194400 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17213838

New failing tests:
css3/filters/custom-with-at-rule-syntax/parsing-at-rule-invalid.html

[WebKit Review Bot]

Created attachment 194413 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

[Rune Lillesveen]

Created attachment 194474 [details]
Patch

[WebKit Review Bot]

Comment on attachment 194474 [details]
Patch

Attachment 194474 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17221549

New failing tests:
css3/filters/custom-with-at-rule-syntax/parsing-at-rule-invalid.html
css3/filters/custom-with-at-rule-syntax/parsing-at-rule-valid.html

[WebKit Review Bot]

Created attachment 194488 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

[Rune Lillesveen]

Created attachment 194491 [details]
Patch

[Build Bot]

Comment on attachment 194491 [details]
Patch

Attachment 194491 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17191731

New failing tests:
css3/filters/custom-with-at-rule-syntax/parsing-at-rule-invalid.html

[Build Bot]

Created attachment 194504 [details]
Archive of layout-test-results from webkit-ews-14

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: <class 'webkitpy.common.config.ports.MacWK2Port'>  Platform: Mac OS X 10.8.2

[WebKit Review Bot]

Comment on attachment 194491 [details]
Patch

Attachment 194491 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17147672

New failing tests:
css3/filters/custom-with-at-rule-syntax/parsing-at-rule-invalid.html

[WebKit Review Bot]

Created attachment 194507 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

[Rune Lillesveen]

Created attachment 194516 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 194516 [details]
Patch

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

> Source/WebCore/ChangeLog:52
> +        * DerivedSources.make:
> +        * GNUmakefile.am:
> +        * css/CSSGrammar.y.in:
> +        * css/CSSParser.cpp:
> +        (WebCore::CSSParser::CSSParser):
> +        (WebCore::CSSParser::parseMediaQuery):
> +        (WebCore::isCSSLetter):
> +        (WebCore::CSSParser::realLex):
> +        (WebCore::CSSParser::rewriteSpecifiers):
> +        * css/CSSParser.h:
> +        (WebCore):
> +        (BracketStack):
> +        (WebCore::BracketStack::push):
> +        (WebCore::BracketStack::pop):
> +        (WebCore::BracketStack::top):
> +        (WebCore::BracketStack::level):
> +        (CSSParser):
> +        (WebCore::CSSParser::reEmitLastToken):
> +        * css/MediaList.cpp:
> +        (WebCore::MediaQuerySet::replaceLastMediaQuery):
> +        (WebCore):
> +        * css/MediaList.h:

You could add some comments here

> Source/WebCore/css/CSSParser.cpp:1490
> -    setupParser("@-webkit-mediaquery ", string, "} ");
> +    // can't use { because tokenizer state switches from mediaquery to initial

Now you are fixing it, please use uppercase C, as comments should start with uppercase.

> Source/WebCore/css/CSSParser.h:507
> +    // Error token reached while parsing a media_query, synchronized at current
> +    // token.

just keep on one line

> Source/WebCore/css/CSSParser.h:670
> +    enum ReEmitState {
> +        NoEmit,
> +        ReEmit,
> +        ReEmitted
> +    };

Why not ReEmissionState { DontEmit, ReEmit, DidReEmit }?

> Source/WebCore/css/CSSParser.h:821
> +inline void CSSParser::reEmitLastToken()
> +{

This method makes it sounds as it is going to re-emit, but that is not really what it is doing

> Source/WebCore/css/CSSParser.h:825
> +    ASSERT(m_reEmitToken == NoEmit);

shouldnt this be != ?

[Rune Lillesveen]

Comment on attachment 194516 [details]
Patch

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

>> Source/WebCore/ChangeLog:52
>> +        * css/MediaList.h:
> 
> You could add some comments here

Do you mean inside css/MediaList.h? Or anything missing in the Changelog?

>> Source/WebCore/css/CSSParser.cpp:1490
>> +    // can't use { because tokenizer state switches from mediaquery to initial
> 
> Now you are fixing it, please use uppercase C, as comments should start with uppercase.

Will fix.

>> Source/WebCore/css/CSSParser.h:507
>> +    // token.
> 
> just keep on one line

Will fix.

>> Source/WebCore/css/CSSParser.h:670
>> +    };
> 
> Why not ReEmissionState { DontEmit, ReEmit, DidReEmit }?

Will fix.

>> Source/WebCore/css/CSSParser.h:821
>> +{
> 
> This method makes it sounds as it is going to re-emit, but that is not really what it is doing

How about "markCurrentTokenForReEmission()"?

>> Source/WebCore/css/CSSParser.h:825
>> +    ASSERT(m_reEmitToken == NoEmit);
> 
> shouldnt this be != ?

No, this assert triggers if the a token has just been re-emitted. It would mean that one of these situations occurred, given a production of e.g. "error ','" that caused the first re-emission:

1. The re-emitted ',' matches a ',' token in a parent rule which also contains an error token.
2. The re-emitted ',' matches the same production containing the error token causing an infinite re-emission.

The assert is there to catch such problematic constructs in the grammar. The if-test below the assert is there to prevent infinite re-emissions, should it occur.

[Rune Lillesveen]

Created attachment 197751 [details]
Patch

[Andreas Kling]

Comment on attachment 197751 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.

[Ahmad Saleem]

I took the test and linked to proper testharness and changed it to JSFiddle:

Link - https://jsfiddle.net/3xce219m/show

All browsers (Safari 16.1, Safari Technology Preview 156, Chrome Canary 109 and Firefox Nightly 108) fail all tests from it.

Do we need anything or spec changed or all browsers are getting this wrong? Thanks!

[Karl Dubost]

> Do we need anything or spec changed or all browsers are getting this wrong? Thanks!

Do they all fail the same way? :)

[Ahmad Saleem]

(In reply to Karl Dubost from comment #19)
> > Do we need anything or spec changed or all browsers are getting this wrong? Thanks!
> 
> Do they all fail the same way? :)

Yes - 'got "MEDIA RULE DROPPED!"'... So they are failing in same way as well.

[Anne van Kesteren]

Yeah this is the kind of thing that would have been addressed in the intervening years due to web-platform-tests and general code maintenance.