Last Comment Bug 883987 - @supports conditions should not accept BAD_STRING and BAD_URL tokens
: @supports conditions should not accept BAD_STRING and BAD_URL tokens
Status: RESOLVED FIXED
: css3
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla25
Assigned To: Corey Ford [:coyotebush]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-06-17 12:17 PDT by David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
Modified: 2013-06-28 18:34 PDT (History)
5 users (show)
dholbert: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
test of BAD_STRING in property value (134 bytes, text/html)
2013-06-17 12:18 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details
test of BAD_URL in property value (145 bytes, text/html)
2013-06-17 12:20 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details
test of BAD_URL in skipped region (136 bytes, text/html)
2013-06-17 12:21 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details
test of BAD_STRING in skipped region (125 bytes, text/html)
2013-06-17 12:21 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details
Patch (6.62 KB, patch)
2013-06-25 18:51 PDT, Corey Ford [:coyotebush]
dbaron: review-
Details | Diff | Review
Patch v2 (6.96 KB, patch)
2013-06-27 17:06 PDT, Corey Ford [:coyotebush]
dbaron: review+
Details | Diff | Review

Description David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-06-17 12:17:25 PDT
As we discussed during the CSS WG meeting in Tokyo two weeks ago, @supports rules aren't supposed to allow BAD_STRING and BAD_URL tokens inside of the conditions.  This is what the current spec says, but we don't implement it correctly.

http://dev.w3.org/csswg/css-conditional/#at-supports

This applies to both inside the values of properties being tested and inside unknown supports grammar (a la bug 814566).  (I've only verified that we have the bug for the first case, though.)
Comment 1 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-06-17 12:18:45 PDT
Created attachment 763723 [details]
test of BAD_STRING in property value
Comment 2 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-06-17 12:20:10 PDT
Created attachment 763724 [details]
test of BAD_URL in property value
Comment 3 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-06-17 12:21:04 PDT
Created attachment 763725 [details]
test of BAD_URL in skipped region
Comment 4 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-06-17 12:21:52 PDT
Created attachment 763726 [details]
test of BAD_STRING in skipped region
Comment 5 Corey Ford [:coyotebush] 2013-06-20 17:03:15 PDT
Wouldn't mind having a bit more on my plate. I can work on this.
Comment 6 Corey Ford [:coyotebush] 2013-06-21 15:26:05 PDT
It looks like I'll need some pointers on

 * what part of the spec deals with this
 * what part(s) of the parser will likely need to be modified
Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-06-24 21:08:03 PDT
So the part of the spec that deals with this does so somewhat implicitly.  In particular, http://dev.w3.org/csswg/css-conditional/#at-supports has:

supports_declaration_condition
  : '(' S* declaration ')'
  ;

general_enclosed
  : ( FUNCTION | '(' ) ( any | unused )* ')'
  ;

general_enclosed is intended to accept "almost anything in CSS".  Likewise declaration is:

declaration : property S* ':' S* value;

and value is something that again, accepts "almost anything in CSS".


The key point here is that "almost anything" is not the same as "anything".  There are likely differences other than the one mentioned here (BAD_STRING and BAD_URL tokens) -- but those other differences are more-than-likely mistakes in the spec rather than mistakes in the implementation.  (We probably need to do some work to look for and fix them, though.)

However, we explicitly discussed BAD_STRING and BAD_URL, and determined that it is intended that they *not* be included, since they're tokens that should always be errors.

(I'd also note that the new css-syntax spec is redefining how BAD_URL works, such that it will no longer match our implementation.  But we'll eventually want to change to match it.)


In terms of code, the parsing code lives in CSSParserImpl::ParseSupportsCondition and the functions it calls.  Note that there are some SkipUntil() calls inside those functions that are in success paths rather than failure paths (probably the only such SkipUntil() calls in the parser).  See also the eCSSToken_Bad_String and eCSSToken_Bad_URL token types generated in nsCSSScanner.
Comment 8 Corey Ford [:coyotebush] 2013-06-25 13:36:05 PDT
Okay. My sense so far is that, to satisfy both this and bug 814556, we might need to divide "failed to parse" return values in some places into those where the condition just evaluates to false and those where we throw out the entire @supports rule. Perhaps a sort of tri-state logic: success/failure/BAD_*.
Comment 9 Corey Ford [:coyotebush] 2013-06-25 13:47:30 PDT
Bug 814566, that is, of course.
Comment 10 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-06-25 14:13:25 PDT
I don't think we need to use return values for this sort of thing -- we can use a member variable on the scanner for whether we encountered a certain token.  (i.e., a similar approach to bug 773296 comment 51)
Comment 11 Corey Ford [:coyotebush] 2013-06-25 18:51:23 PDT
Created attachment 767550 [details] [diff] [review]
Patch

That works out well.

Slightly outdated try push: https://tbpl.mozilla.org/?tree=Try&rev=ee8c26bc9cbc
(Changes since: removed an unnecessary update of mSeenBadToken, added a test to confirm such.)
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-06-26 15:12:57 PDT
Comment on attachment 767550 [details] [diff] [review]
Patch

The scanner changes look fine, but the parser changes seem wrong:  the parser still needs to obey the same rules for matching parentheses (and it's probably worth testing that) -- it's just the resulting rule isn't a valid @supports rule.
Comment 13 Corey Ford [:coyotebush] 2013-06-27 17:06:54 PDT
Created attachment 768660 [details] [diff] [review]
Patch v2

Per our discussion, moved the check into ParseSupportsCondition, and added a sixth test case.
Comment 14 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-06-27 17:21:57 PDT
Comment on attachment 768660 [details] [diff] [review]
Patch v2

r=dbaron
Comment 15 Corey Ford [:coyotebush] 2013-06-27 19:55:13 PDT
Looks good on try. https://tbpl.mozilla.org/?tree=Try&rev=778583987c93
Comment 16 Daniel Holbert [:dholbert] 2013-06-28 00:25:47 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/57041efa3c2b
Comment 17 Phil Ringnalda (:philor) 2013-06-28 18:34:10 PDT
https://hg.mozilla.org/mozilla-central/rev/57041efa3c2b

Note You need to log in before you can comment on or make changes to this bug.