[webkit-reviews] review denied: [Bug 134632] ASSERTION FAILED: name[0] == '@' && length >= 2 in WebCore::CSSParser::detectAtToken : [Attachment 234405] Proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Martin Hodovan
<mhodovan.u-szeged at partner.samsung.com>'s request for review:
Bug 134632: ASSERTION FAILED: name[0] == '@' && length >= 2 in
WebCore::CSSParser::detectAtToken
https://bugs.webkit.org/show_bug.cgi?id=134632

Attachment 234405: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=234405&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=234405&action=review


> Source/WebCore/css/CSSParser.cpp:11231
> +	       // The standard enables unicode escapes in at-rules. In this
case only the resultString will contain the
> +	       // correct identifier, hence we have to use it to determine its
length instead of the usual pointer arithmetic.

The bug is in parseIdentifier, which needs to bump the result pointer even in
the 16-bit slow case. Not doing that will cause many other problems, so just
changing this code path to quiet the assertion is wrong. The line of code that
should be added to parseIdentifer is:

    result += result16 - start16;

The reason it’s important to fix the bug in parseIdentifier is that multiple
call sites of parseIdentifier are affected by this, not just this one code
path.

But also, the 16-bit code path in parseIdentifier doesn’t really need to switch
to parsing 16-bit input. It’s only the output string that needs to be 16-bit,
and the way it currently does things is unnecessarily inefficient. This code is
made terribly confusing by using the name "result" for a pointer to the next
character to be parsed. We should call this current, not result, and we should
also investigate using a const pointer for it and eliminating code that writes
into it. But that’s beyond the scope of this.