RESOLVED FIXED 125637
Broken error recovery in @media at-rule 
https://bugs.webkit.org/show_bug.cgi?id=125637
Summary Broken error recovery in @media at-rule
Martin Hodovan
Reported 2013-12-12 06:12:57 PST
The @media rule is dropped if an opening curly bracket '{' is missing from any of its inner rules. E.g. the background image of www.wordpress.com does not display, because of a malformed rule inside of the @media rule. The minimized problem is: @media { .pic-d{background-image:url(http://wordpress.com/home.logged-out/images/pic-d.jpg?1)} span background-image:url(http://wordpress.com/home.logged-out/images/automattic-white.png);} } The first subrule sets the background picture correctly, but since the opening '{' is missing from the second subrule, the whole line is considered to be a selector and the last two '}' brackets will be swallowed as well. And since the @media rule loses its closing '}', it will be dropped and ignored completely. This error recovery got broken 3 months ago (r155536). The changeset contains a seemingly consistent restructuring by removing the selector_with_trailing_whitespace production and using its definition instead. However, it ruins the error recovery: instead of removing the whole selector_with_trailing_whitespace, it removes the last WHITESPACE only and keeps trying to find a selector.
Attachments
diff (1.37 KB, text/plain)
2013-12-12 06:13 PST, Martin Hodovan 		no flags
Details
Proposed patch (3.46 KB, patch)
2013-12-12 06:34 PST, Martin Hodovan 		no flags
DetailsFormatted DiffDiff
Proposed patch with test case (4.98 KB, patch)
2013-12-12 06:41 PST, Martin Hodovan 		darin: review-
darin: commit-queue-
DetailsFormatted DiffDiff
Bad error recovery log (4.21 KB, text/plain)
2013-12-12 06:53 PST, Martin Hodovan 		no flags
Details
Correct error recovery log (4.50 KB, text/plain)
2013-12-12 06:54 PST, Martin Hodovan 		no flags
Details
Proposed patch (5.41 KB, patch)
2013-12-12 08:38 PST, Martin Hodovan 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Martin Hodovan
Comment 1 2013-12-12 06:13:53 PST
Created attachment 219079 [details] diff
Martin Hodovan
Comment 2 2013-12-12 06:34:41 PST
Created attachment 219080 [details] Proposed patch
Martin Hodovan
Comment 3 2013-12-12 06:41:57 PST
Created attachment 219081 [details] Proposed patch with test case
Martin Hodovan
Comment 4 2013-12-12 06:53:18 PST
Created attachment 219082 [details] Bad error recovery log The debug output of the parser around the error recovery without the patch.
Martin Hodovan
Comment 5 2013-12-12 06:54:52 PST
Created attachment 219083 [details] Correct error recovery log The debug output of the parser around the error recovery with the patch.
Darin Adler
Comment 6 2013-12-12 07:40:56 PST
Comment on attachment 219081 [details] Proposed patch with test case View in context: https://bugs.webkit.org/attachment.cgi?id=219081&action=review Fix looks fine. Test is incorrect, though. Easy to fix. > LayoutTests/fast/css/media-error-recovery.html:6 > + if (window.testRunner) > + testRunner.dumpAsText(); This is a mistake. The test won’t work if you only dump the text. The test writes out the same text when it fails as when it succeeds. To make a text-only test, something in the test has to actually show whether the test has succeeded or failed. For an example of one way to do this, please look at the media-rule-no-whitespace.html test.
Peter Gal
Comment 7 2013-12-12 07:43:25 PST
Comment on attachment 219081 [details] Proposed patch with test case View in context: https://bugs.webkit.org/attachment.cgi?id=219081&action=review > LayoutTests/ChangeLog:1 > +2013-12-12 Martin Hodovan <mhodovan@webkit.org> This email address conflicts with the other ChangeLog's email address. Which one is the correct one? > LayoutTests/fast/css/media-error-recovery.html:18 > + if (window.testRunner) > + testRunner.dumpAsText(); > +</script> > + > +<style> > + @media { > + body { background-color: green; } > + .foo background-color: red; } > + } > +</style> > +</head> > +<body> > + Background should be green. > +</body> I'm not an expert in LayoutTests but this looks as it will always dump the text: 'Background should be green.' which is not good. You won't be able to detect the difference with the test scripts (run-webkit-tests). A better way would be adding js script which checks the color of the background after it is loaded.
Martin Hodovan
Comment 8 2013-12-12 08:38:54 PST
Created attachment 219086 [details] Proposed patch
Martin Hodovan
Comment 9 2013-12-18 10:12:41 PST
Is it OK like this?
Darin Adler
Comment 10 2013-12-18 10:26:20 PST
Comment on attachment 219086 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=219086&action=review > LayoutTests/fast/css/media-error-recovery.html:6 > + if (window.testRunner) Should not be using tabs here.
WebKit Commit Bot
Comment 11 2013-12-18 10:55:49 PST
Comment on attachment 219086 [details] Proposed patch Clearing flags on attachment: 219086 Committed r160779: <http://trac.webkit.org/changeset/160779>
WebKit Commit Bot
Comment 12 2013-12-18 10:55:52 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.