Bug 112986 - Incorrect error handling for Media Queries
Summary: Incorrect error handling for Media Queries
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-21 17:26 PDT by Rune Lillesveen
Modified: 2014-02-05 11:22 PST (History)
12 users (show)

See Also:


Attachments
Invalid media query tests (1.63 KB, text/html)
2013-03-21 17:26 PDT, Rune Lillesveen
no flags Details
Patch (49.52 KB, patch)
2013-03-21 17:45 PDT, Rune Lillesveen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-02 (1.44 MB, application/zip)
2013-03-21 18:32 PDT, WebKit Review Bot
no flags Details
Patch (51.22 KB, patch)
2013-03-22 02:05 PDT, Rune Lillesveen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-08 (1.41 MB, application/zip)
2013-03-22 02:48 PDT, WebKit Review Bot
no flags Details
Patch (53.17 KB, patch)
2013-03-22 02:55 PDT, Rune Lillesveen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-14 (628.16 KB, application/zip)
2013-03-22 04:03 PDT, Build Bot
no flags Details
Archive of layout-test-results from gce-cr-linux-01 (1.46 MB, application/zip)
2013-03-22 04:07 PDT, WebKit Review Bot
no flags Details
Patch (53.54 KB, patch)
2013-03-22 04:53 PDT, Rune Lillesveen
no flags Details | Formatted Diff | Diff
Patch (54.19 KB, patch)
2013-04-12 05:47 PDT, Rune Lillesveen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rune Lillesveen 2013-03-21 17:26:39 PDT
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.
Comment 1 Rune Lillesveen 2013-03-21 17:45:33 PDT
Created attachment 194400 [details]
Patch
Comment 2 Build Bot 2013-03-21 18:22:11 PDT
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
Comment 3 WebKit Review Bot 2013-03-21 18:32:27 PDT
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
Comment 4 WebKit Review Bot 2013-03-21 18:32:31 PDT
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
Comment 5 Rune Lillesveen 2013-03-22 02:05:22 PDT
Created attachment 194474 [details]
Patch
Comment 6 WebKit Review Bot 2013-03-22 02:48:56 PDT
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
Comment 7 WebKit Review Bot 2013-03-22 02:48:59 PDT
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
Comment 8 Rune Lillesveen 2013-03-22 02:55:42 PDT
Created attachment 194491 [details]
Patch
Comment 9 Build Bot 2013-03-22 04:03:01 PDT
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
Comment 10 Build Bot 2013-03-22 04:03:03 PDT
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
Comment 11 WebKit Review Bot 2013-03-22 04:07:08 PDT
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
Comment 12 WebKit Review Bot 2013-03-22 04:07:13 PDT
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
Comment 13 Rune Lillesveen 2013-03-22 04:53:42 PDT
Created attachment 194516 [details]
Patch
Comment 14 Kenneth Rohde Christiansen 2013-03-28 09:00:10 PDT
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 != ?
Comment 15 Rune Lillesveen 2013-04-03 04:38:04 PDT
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.
Comment 16 Rune Lillesveen 2013-04-12 05:47:21 PDT
Created attachment 197751 [details]
Patch
Comment 17 Andreas Kling 2014-02-05 11:22:56 PST
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.