Fix for CSS Variables tokenisation not checking CSS identifier length

Fix for CSS Variables tokenisation not checking CSS identifier length

This bug was failing the fast/css/parsing-expr-error-recovery.html test
in debug when CSS Variables was turned on for all tests.

BUG=242182
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=150955

[alancutter (OOO until 2018), 2013-05-21 01:18:48]

The try bots seem to be unreliable at this time.
I have tested this patch against ToT on Linux debug and encounter no new
regressions.

[eseidel, 2013-05-21 01:33:11]

https://codereview.chromium.org/15317009/diff/1/Source/core/css/CSSParser.cpp
File Source/core/css/CSSParser.cpp (right):

https://codereview.chromium.org/15317009/diff/1/Source/core/css/CSSParser.cpp...
Source/core/css/CSSParser.cpp:10313: if (length >= 12) {
Why 12?  webkit-var is 10 chars, no?  Can this by sizeof("webkit-var")?  Should
it be?

[alancutter (OOO until 2018), 2013-05-21 03:56:05]

Thanks for the review Eric.

Made minor tweaks and addressed your comments.
Have retested locally on Linux debug and saw no new regressions for this patch.

https://codereview.chromium.org/15317009/diff/1/Source/core/css/CSSParser.cpp
File Source/core/css/CSSParser.cpp (right):

https://codereview.chromium.org/15317009/diff/1/Source/core/css/CSSParser.cpp...
Source/core/css/CSSParser.cpp:10313: if (length >= 12) {
On 2013/05/21 01:33:11, eseidel wrote:
> Why 12?  webkit-var is 10 chars, no?  Can this by sizeof("webkit-var")? 
Should
> it be?

We are trying to match on "-webkit-var-" as prefixed variable property names are
of the form "-webkit-var-name".
This function is called inside a switch that has already checked the first
character of the token and determined it to be '-'. The first part of the if
checks the 12th character (name[11]) before the "webkit-var" part is checked.

I should add an assert regarding the first character being '-', this might make
it more clear what is going on.

Looking at this again it should be (length >= 13) as at least one character is
required for the variable name.

[Steve Block, 2013-05-21 04:47:53]

https://codereview.chromium.org/15317009/diff/9001/Source/core/css/CSSParser.cpp
File Source/core/css/CSSParser.cpp (right):

https://codereview.chromium.org/15317009/diff/9001/Source/core/css/CSSParser....
Source/core/css/CSSParser.cpp:10294: if (length >= 13) {
if (length < 13)
    return false;

https://codereview.chromium.org/15317009/diff/9001/Source/core/css/CSSParser....
Source/core/css/CSSParser.cpp:10296: ASSERT(name[0] == '-');
Can this assert be moved to the top of the function?

https://codereview.chromium.org/15317009/diff/9001/Source/core/css/CSSParser....
Source/core/css/CSSParser.cpp:10297: if (name[11] == '-' &&
isIdentifierStartAfterDash(name + 12) && isEqualToCSSIdentifier(name + 1,
"webkit-var"))
using sizeof() here as eric suggests would make this more readable

[Mike Lawther (Google), 2013-05-21 04:52:14]

On 2013/05/21 03:56:05, alancutter wrote:
https://codereview.chromium.org/15317009/diff/1/Source/core/css/CSSParser.cpp...
> Source/core/css/CSSParser.cpp:10313: if (length >= 12) {
> On 2013/05/21 01:33:11, eseidel wrote:
> > Why 12?  webkit-var is 10 chars, no?  Can this by sizeof("webkit-var")? 
> Should
> > it be?
> 
> We are trying to match on "-webkit-var-" as prefixed variable property names
are
> of the form "-webkit-var-name".

My reaction was the same as Eric's - could this instead be
sizeof("-webkit-var-")? Note that this will come out as 13 due to the trailing
'\0'. Having the string here makes it clearer, plus makes it obvious it needs to
be changed when unprefixing.

[alancutter (OOO until 2018), 2013-05-21 05:05:15]

On 2013/05/21 04:52:14, Mike Lawther (Google) wrote:
> My reaction was the same as Eric's - could this instead be
> sizeof("-webkit-var-")? Note that this will come out as 13 due to the trailing
> '\0'. Having the string here makes it clearer, plus makes it obvious it needs
to
> be changed when unprefixing.

Done.

https://codereview.chromium.org/15317009/diff/9001/Source/core/css/CSSParser.cpp
File Source/core/css/CSSParser.cpp (right):

https://codereview.chromium.org/15317009/diff/9001/Source/core/css/CSSParser....
Source/core/css/CSSParser.cpp:10294: if (length >= 13) {
On 2013/05/21 04:47:54, Steve Block wrote:
> if (length < 13)
>     return false;

Done.

https://codereview.chromium.org/15317009/diff/9001/Source/core/css/CSSParser....
Source/core/css/CSSParser.cpp:10296: ASSERT(name[0] == '-');
On 2013/05/21 04:47:54, Steve Block wrote:
> Can this assert be moved to the top of the function?

Done.

[Steve Block, 2013-05-22 06:48:07]

looks good, but eseidel should probably sign-off

[eseidel, 2013-05-22 07:11:43]

lgtm

It's really hard to tell how many off-by-one errors this code has. :)  I've lost
count. :(

But I'm also not going to hold this change up.  We need our parsers to be less
fragile, not more.

https://codereview.chromium.org/15317009/diff/16001/Source/core/css/CSSParser...
File Source/core/css/CSSParser.cpp (right):

https://codereview.chromium.org/15317009/diff/16001/Source/core/css/CSSParser...
Source/core/css/CSSParser.cpp:10298: return name[11] == '-' &&
isIdentifierStartAfterDash(name + 12) && isEqualToCSSIdentifier(name + 1,
"webkit-var"));
Soooo many magic numbers!

[commit-bot: I haz the power, 2013-05-22 07:31:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alancutter@chromium.org/15317009/16001

[commit-bot: I haz the power, 2013-05-22 07:48:39]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_layout.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-23 03:41:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alancutter@chromium.org/15317009/28001

[commit-bot: I haz the power, 2013-05-23 04:26:37]

Change committed as 150955