Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(286)

Issue 241053002: ASSERTION FAILED: name[0] == '@' && length >= 2 in WebCore::CSSTokenizer::detectAtToken (Closed)

Created:
6 years, 8 months ago by reni
Modified:
6 years, 7 months ago
CC:
blink-reviews, ed+blinkwatch_opera.com, dglazkov+blink, apavlov+blink_chromium.org, darktears, rune+blink, rwlbuis
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

ASSERTION FAILED: name[0] == '@' && length >= 2 in WebCore::CSSTokenizer::detectAtToken At-rules have to contain 2 or more characters: one '@' character and at least one more identifier character. This condition is checked in the failing assertion. Currently, the length of an at-rule is determined by doing pointer arithmetic on the 'result' pointer, which is expected to be set to the end of the at-rule identifier by the WebCore::*CSSTokenizer::parseIdentifier method. If the at-rule token is a sequence of 8-bit-only characters then 'result' will point correctly at the end of the identifier. However, if the at-rule contains a 16-bit Unicode escape then 'result' will not be updated correctly anymore, hence it cannot be used for length calculation. On the other hand, 'resultString' always contains the whole at-rule identifier (without the '@'' character), so taking its length + 1 will give the correct length for any at-rule. R=jchaffraix@chromium.org BUG=364492 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174853

Patch Set 1 #

Total comments: 5

Patch Set 2 : Updated patch #

Unified diffs Side-by-side diffs Delta from patch set Stats (+26 lines, -1 line) Patch
A LayoutTests/fast/css/atrule-with-escape-character-crash.html View 1 1 chunk +22 lines, -0 lines 0 comments Download
A LayoutTests/fast/css/atrule-with-escape-character-crash-expected.txt View 1 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/css/CSSTokenizer-in.cpp View 1 1 chunk +3 lines, -1 line 0 comments Download

Messages

Total messages: 12 (0 generated)
reni
Could someone enable the try-bots for this patch set, pls?
6 years, 8 months ago (2014-04-17 14:40:09 UTC) #1
Julien - ping for review
Wouldn't it make sense to fix |result| to point to the right location instead of ...
6 years, 8 months ago (2014-04-18 16:56:42 UTC) #2
reni
On 2014/04/18 16:56:42, Julien Chaffraix - PST wrote: > Wouldn't it make sense to fix ...
6 years, 8 months ago (2014-04-23 23:32:14 UTC) #3
reni
How does it look?
6 years, 7 months ago (2014-04-29 07:00:55 UTC) #4
reni
On 2014/04/29 07:00:55, renata.hodovan wrote: > How does it look? Could somebody take a look ...
6 years, 7 months ago (2014-05-07 06:54:22 UTC) #5
Julien - ping for review
Sorry for the delay! https://codereview.chromium.org/241053002/diff/1/LayoutTests/fast/css/atrule-with-escape-character-crash.html File LayoutTests/fast/css/atrule-with-escape-character-crash.html (right): https://codereview.chromium.org/241053002/diff/1/LayoutTests/fast/css/atrule-with-escape-character-crash.html#newcode1 LayoutTests/fast/css/atrule-with-escape-character-crash.html:1: <html> The test is missing ...
6 years, 7 months ago (2014-05-08 15:26:28 UTC) #6
reni
https://codereview.chromium.org/241053002/diff/1/Source/core/css/CSSTokenizer-in.cpp File Source/core/css/CSSTokenizer-in.cpp (right): https://codereview.chromium.org/241053002/diff/1/Source/core/css/CSSTokenizer-in.cpp#newcode1484 Source/core/css/CSSTokenizer-in.cpp:1484: detectAtToken<SrcCharacterType>(resultString.length() + 1, hasEscape); This is (was) the only ...
6 years, 7 months ago (2014-05-22 22:04:38 UTC) #7
Julien - ping for review
lgtm. We prefer descriptions that are below 80 characters (ideally below 72 characters, see https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/rqGhY7PfkYY) ...
6 years, 7 months ago (2014-05-23 10:42:09 UTC) #8
reni
Updated patch based on the comments.
6 years, 7 months ago (2014-05-26 14:00:11 UTC) #9
Julien - ping for review
The CQ bit was checked by jchaffraix@chromium.org
6 years, 7 months ago (2014-05-27 09:23:58 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rhodovan.u-szeged@partner.samsung.com/241053002/20001
6 years, 7 months ago (2014-05-27 09:24:04 UTC) #11
commit-bot: I haz the power
6 years, 7 months ago (2014-05-27 10:21:45 UTC) #12
Message was sent while issue was closed.
Change committed as 174853

Powered by Google App Engine
This is Rietveld 408576698