Bug 125637

Summary:

Broken error recovery in @media at-rule

Product:

WebKit

Reporter:

Martin Hodovan <mhodovan.u-szeged>

Component:

CSS

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, darin, eric.carlson, esprehn+autocc, glenn, gyuyoung.kim, jer.noble, kling, koivisto, macpherson, menard, mhodovan.u-szeged, ossy

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

URL:

www.wordpress.com

Attachments:

Description	Flags
diff	none
Proposed patch	none
Proposed patch with test case	darin: review-, darin: commit-queue-
Bad error recovery log	none
Correct error recovery log	none
Proposed patch	none

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	Details Formatted Diff Diff
Proposed patch with test case (4.98 KB, patch) 2013-12-12 06:41 PST, Martin Hodovan	darin: review- darin: commit-queue-	Details Formatted Diff Diff
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	Details Formatted Diff Diff
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.