78521 2012-02-13 13:21:53 -0800 The comment about terminating nulls at the top of CSSParser::lex() is incorrect 2012-02-16 23:48:20 -0800 1 1 1 Unclassified WebKit CSS 528+ (Nightly build) All All RESOLVED FIXED P2 Trivial --- 1 cdn cdn davidbarr eric macpherson menard webkit.review.bot zherczeg oldest_to_newest 555502 0 cdn 2012-02-13 13:21:53 -0800 We should fix the comment // The input buffer is terminated by two \0, so // it is safe to read two characters ahead anytime at the top of CSSParser::lex(). It is wrong and suggests that there are 2 characters of nulls. This should probably say something like // The input buffer is terminated by a \0 character, so // it is safe to read one character ahead of a known non-null 556796 1 127087 cdn 2012-02-14 17:19:34 -0800 Created attachment 127087 Patch 558479 2 eric 2012-02-16 13:36:24 -0800 Difficult for me to review this, because I don't know enough about lex(). Who creates this buffer? Bison? Can we point to the bison docs where it describes the buffer it creates? 558520 3 cdn 2012-02-16 14:23:46 -0800 (In reply to comment #2) > Difficult for me to review this, because I don't know enough about lex(). Who creates this buffer? Bison? Can we point to the bison docs where it describes the buffer it creates? There is no real immediacy here as it is just a comment change. Zoltan can review this when he has time. 558657 4 davidbarr 2012-02-16 16:31:08 -0800 I think the comment is right but the code is wrong. CSSParser::lex depends on this invariant to detect the end of the buffer. 558664 5 cdn 2012-02-16 16:40:19 -0800 The one case where it depended on it was just changed. I'm not sure whether it was originally supposed to be one or 2 nulls but at this point only one is written and lex() only relies on there being one. 558694 6 davidbarr 2012-02-16 16:56:47 -0800 Thanks for the clarification, I see that dependence on double-null was removed recently: http://trac.webkit.org/changeset/107369 558940 7 127087 zherczeg 2012-02-16 22:55:32 -0800 Comment on attachment 127087 Patch The double NULL originated from flex. The last comment here explains it: http://stackoverflow.com/questions/1909166/how-to-parse-from-a-string-rather-than-a-file When I started working on the lexer, I kept this behaviour first. Later I realized I didn't need double NULL terminator, but the comment was remained and the comment parsing was wrong. 558965 8 127087 webkit.review.bot 2012-02-16 23:48:14 -0800 Comment on attachment 127087 Patch Clearing flags on attachment: 127087 Committed r108043: <http://trac.webkit.org/changeset/108043> 558967 9 webkit.review.bot 2012-02-16 23:48:20 -0800 All reviewed patches have been landed. Closing bug. 127087 2012-02-14 17:19:34 -0800 2012-02-16 23:48:14 -0800 Patch bug-78521-20120214171933.patch text/plain 1286 cdn SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEwNzc0NSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDEzIEBACisyMDEyLTAyLTE0ICBDcmlzIE5l Y2thciAgPGNkbkBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgQ29ycmVjdCBhIG1pc2xlYWRpbmcg Y29tbWVudCByZWdhcmRpbmcgc3RyaW5nIGRlbGltaXRlcnMgaW4gQ1NTIHBhcnNpbmcuCisgICAg ICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD03ODUyMQorCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogY3NzL0NTU1BhcnNl ci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpDU1NQYXJzZXI6OmxleCk6CisKIDIwMTItMDItMTQg IEFuZHJlYXMgS2xpbmcgIDxhd2Vzb21la2xpbmdAYXBwbGUuY29tPgogCiAgICAgICAgIEF2b2lk IGZ1bGwgc3R5bGUgcmVjYWxjIHdoZW4gcHJlc2VudGF0aW9uIGF0dHJpYnV0ZXMgY2hhbmdlLgpJ bmRleDogU291cmNlL1dlYkNvcmUvY3NzL0NTU1BhcnNlci5jcHAKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL1dlYkNvcmUvY3NzL0NTU1BhcnNlci5jcHAJKHJldmlzaW9uIDEwNzcyOSkKKysrIFNvdXJj ZS9XZWJDb3JlL2Nzcy9DU1NQYXJzZXIuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC04MTgyLDggKzgx ODIsOCBAQCBpbnQgQ1NTUGFyc2VyOjpsZXgodm9pZCogeXlsdmFsV2l0aG91dFR5CiAgICAgVUNo YXIqIHJlc3VsdDsKICAgICBib29sIGhhc0VzY2FwZTsKIAotICAgIC8vIFRoZSBpbnB1dCBidWZm ZXIgaXMgdGVybWluYXRlZCBieSB0d28gXDAsIHNvCi0gICAgLy8gaXQgaXMgc2FmZSB0byByZWFk IHR3byBjaGFyYWN0ZXJzIGFoZWFkIGFueXRpbWUuCisgICAgLy8gVGhlIGlucHV0IGJ1ZmZlciBp cyB0ZXJtaW5hdGVkIGJ5IGEgXDAgY2hhcmFjdGVyLCBzbworICAgIC8vIGl0IGlzIHNhZmUgdG8g cmVhZCBvbmUgY2hhcmFjdGVyIGFoZWFkIG9mIGEga25vd24gbm9uLW51bGwuCiAKICNpZm5kZWYg TkRFQlVHCiAgICAgLy8gSW4gZGVidWcgd2UgY2hlY2sgd2l0aCBhbiBBU1NFUlQgdGhhdCB0aGUg bGVuZ3RoIGlzID4gMCBmb3Igc3RyaW5nIHR5cGVzLgo=