| Summary: | Incorrect error handling for Media Queries | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Rune Lillesveen <rune> | ||||||||||||||||||||||
| Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||
| Status: | RESOLVED CONFIGURATION CHANGED | ||||||||||||||||||||||||
| Severity: | Normal | CC: | ahmad.saleem792, annevk, buildbot, dglazkov, eric.carlson, esprehn+autocc, feature-media-reviews, jer.noble, karlcow, koivisto, macpherson, menard, ntim, ojan.autocc, rniwa, webkit.review.bot, zan | ||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||
Created attachment 194400 [details]
Patch
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 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 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
Created attachment 194474 [details]
Patch
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 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
Created attachment 194491 [details]
Patch
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 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 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 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
Created attachment 194516 [details]
Patch
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 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. Created attachment 197751 [details]
Patch
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.
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! > Do we need anything or spec changed or all browsers are getting this wrong? Thanks!
Do they all fail the same way? :)
(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. 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. |
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.