106919 2013-01-15 10:06:07 -0800 Make AtomicMarkupTokenBase use a bare UChar* for external characters 2013-01-16 14:22:24 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 106127 1 tonyg tonyg abarth benjamin msaboff ojan.autocc webkit.review.bot oldest_to_newest 807409 0 tonyg 2013-01-15 10:06:07 -0800 Make AtomicMarkupTokenBase use a bare UChar* for external characters 807410 1 182800 tonyg 2013-01-15 10:07:26 -0800 Created attachment 182800 Patch 807444 2 182800 eric 2013-01-15 10:35:36 -0800 Comment on attachment 182800 Patch ok. 807475 3 182800 webkit.review.bot 2013-01-15 11:07:51 -0800 Comment on attachment 182800 Patch Clearing flags on attachment: 182800 Committed r139760: <http://trac.webkit.org/changeset/139760> 807476 4 webkit.review.bot 2013-01-15 11:07:54 -0800 All reviewed patches have been landed. Closing bug. 808083 5 182800 benjamin 2013-01-15 23:33:12 -0800 Comment on attachment 182800 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182800&action=review > Source/WebCore/xml/parser/MarkupTokenBase.h:529 > + size_t charactersLength() const > + { > + ASSERT(m_type == Token::Type::Character); > + return m_externalCharactersLength; > } While you are at it: this would be better as unsigned instead of size_t. Character buffers are usually unsigned (WTFString included). > Source/WebCore/xml/parser/MarkupTokenBase.h:588 > + size_t m_externalCharactersLength; ditto. 808513 6 182800 tonyg 2013-01-16 10:37:44 -0800 Comment on attachment 182800 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182800&action=review >> Source/WebCore/xml/parser/MarkupTokenBase.h:529 >> } > > While you are at it: this would be better as unsigned instead of size_t. > Character buffers are usually unsigned (WTFString included). abarth tells me that on a 64bit system, unsigned will still be 32 bits but size_t will be 64. Since Vectors are arbitrary length, using size_t is appropriate here. 808532 7 abarth 2013-01-16 11:02:15 -0800 I wonder if we should crash if we get a character token that's larger than 32 bits. I bet we do later anyway due to overflow checks in StringImpl. 808675 8 benjamin 2013-01-16 13:44:21 -0800 > abarth tells me that on a 64bit system, unsigned will still be 32 bits but size_t will be 64. That is correct. > Since Vectors are arbitrary length, using size_t is appropriate here. Well, that's all nice when creating the Token, but the method you call will get you un trouble. E.g.: String characters = String(token->characters(), token->charactersLength()); Your patch changed from using Vector types to Character types, which is why I suggest we move to string conventions. > I wonder if we should crash if we get a character token that's larger than 32 bits. I bet we do later anyway due to overflow checks in StringImpl. Yep, I think you are right. Seeing what is here, I think that's the way to make this correct. 808714 9 abarth 2013-01-16 14:22:24 -0800 Yeah. Would you be willing to file a bug about this issue? The issue pre-dates this patch since the old code used Vector::size as well. 182800 2013-01-15 10:07:26 -0800 2013-01-16 10:37:44 -0800 Patch bug-106919-20130115100428.patch text/plain 5444 tonyg U3VidmVyc2lvbiBSZXZpc2lvbjogMTM5NzQ4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggOGM3YTcwZDkyOTlhODc0 MGRhN2Q1NGQzMDMyMmU4MWYzMjRmODBmYy4uMDQ5ZTE0Y2I0MTA2ZGE5NDEwY2E3MzkzNTUyZTdh MzYyMTMwNzI0NyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI2IEBACisyMDEzLTAxLTE1ICBUb255 IEdlbnRpbGNvcmUgIDx0b255Z0BjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgTWFrZSBBdG9taWNN YXJrdXBUb2tlbkJhc2UgdXNlIGEgYmFyZSBVQ2hhciogZm9yIGV4dGVybmFsIGNoYXJhY3RlcnMK KyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEwNjkxOQor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoaXMgYWxs b3dzIGFuIGFyYml0cmFyeSBiYWNraW5nIGZvciBleHRlcm5hbCBjaGFyYWN0ZXJzIHdoaWNoIGlz IG5lY2Vzc2FyeSBmb3IgdGhlIHRocmVhZGVkIEhUTUwgcGFyc2VyLgorCisgICAgICAgIE5vIG5l dyB0ZXN0cyBiZWNhdXNlIG5vIG5ldyBmdW5jdGlvbmFsaXR5LgorCisgICAgICAgICogaHRtbC9w YXJzZXIvSFRNTFRyZWVCdWlsZGVyLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkhUTUxUcmVlQnVp bGRlcjo6RXh0ZXJuYWxDaGFyYWN0ZXJUb2tlbkJ1ZmZlcjo6RXh0ZXJuYWxDaGFyYWN0ZXJUb2tl bkJ1ZmZlcik6CisgICAgICAgIChXZWJDb3JlOjpIVE1MVHJlZUJ1aWxkZXI6OnByb2Nlc3NUb2tl bkluRm9yZWlnbkNvbnRlbnQpOgorICAgICAgICAqIHhtbC9wYXJzZXIvTWFya3VwVG9rZW5CYXNl Lmg6CisgICAgICAgIChXZWJDb3JlOjpBdG9taWNNYXJrdXBUb2tlbkJhc2U6OkF0b21pY01hcmt1 cFRva2VuQmFzZSk6CisgICAgICAgIChXZWJDb3JlOjpBdG9taWNNYXJrdXBUb2tlbkJhc2U6OmNo YXJhY3RlcnMpOgorICAgICAgICAoQXRvbWljTWFya3VwVG9rZW5CYXNlKToKKyAgICAgICAgKFdl YkNvcmU6OkF0b21pY01hcmt1cFRva2VuQmFzZTo6Y2hhcmFjdGVyc0xlbmd0aCk6CisgICAgICAg IChXZWJDb3JlOjpBdG9taWNNYXJrdXBUb2tlbkJhc2U6OmNsZWFyRXh0ZXJuYWxDaGFyYWN0ZXJz KToKKyAgICAgICAgKiB4bWwvcGFyc2VyL1hNTFRyZWVCdWlsZGVyLmNwcDoKKyAgICAgICAgKFdl YkNvcmU6OlhNTFRyZWVCdWlsZGVyOjpwcm9jZXNzQ2hhcmFjdGVyKToKKwogMjAxMy0wMS0xNSAg VnNldm9sb2QgVmxhc292ICA8dnNldmlrQGNocm9taXVtLm9yZz4KIAogICAgICAgICBVbnJldmll d2VkLCByZW1vdmUgZmlsZSB0aGF0IHdhcyBhY2NpZGVudGFsbHkgYWRkZWQgdG8gdGhlIHdyb25n IHBhdGNoLgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvaHRtbC9wYXJzZXIvSFRNTFRyZWVC dWlsZGVyLmNwcCBiL1NvdXJjZS9XZWJDb3JlL2h0bWwvcGFyc2VyL0hUTUxUcmVlQnVpbGRlci5j cHAKaW5kZXggMWU1M2M3NDhmMmZjODM2YjEwMzRiMjQzNTEyM2Q5YmYzMmY5MTFkMy4uZGYyN2Y4 ZDIxMWMwMzYzNjJiYTk1ZjAyZGQ4ZjlkNTEyNmI4ZmVhYSAxMDA2NDQKLS0tIGEvU291cmNlL1dl YkNvcmUvaHRtbC9wYXJzZXIvSFRNTFRyZWVCdWlsZGVyLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29y ZS9odG1sL3BhcnNlci9IVE1MVHJlZUJ1aWxkZXIuY3BwCkBAIC0xNDksOCArMTQ5LDggQEAgY2xh c3MgSFRNTFRyZWVCdWlsZGVyOjpFeHRlcm5hbENoYXJhY3RlclRva2VuQnVmZmVyIHsKICAgICBX VEZfTUFLRV9OT05DT1BZQUJMRShFeHRlcm5hbENoYXJhY3RlclRva2VuQnVmZmVyKTsKIHB1Ymxp YzoKICAgICBleHBsaWNpdCBFeHRlcm5hbENoYXJhY3RlclRva2VuQnVmZmVyKEF0b21pY0hUTUxU b2tlbiogdG9rZW4pCi0gICAgICAgIDogbV9jdXJyZW50KHRva2VuLT5jaGFyYWN0ZXJzKCkuZGF0 YSgpKQotICAgICAgICAsIG1fZW5kKG1fY3VycmVudCArIHRva2VuLT5jaGFyYWN0ZXJzKCkuc2l6 ZSgpKQorICAgICAgICA6IG1fY3VycmVudCh0b2tlbi0+Y2hhcmFjdGVycygpKQorICAgICAgICAs IG1fZW5kKG1fY3VycmVudCArIHRva2VuLT5jaGFyYWN0ZXJzTGVuZ3RoKCkpCiAgICAgICAgICwg bV9pc0FsbDhCaXREYXRhKHRva2VuLT5pc0FsbDhCaXREYXRhKCkpCiAgICAgewogICAgICAgICBB U1NFUlQoIWlzRW1wdHkoKSk7CkBAIC0yODU0LDcgKzI4NTQsNyBAQCB2b2lkIEhUTUxUcmVlQnVp bGRlcjo6cHJvY2Vzc1Rva2VuSW5Gb3JlaWduQ29udGVudChBdG9taWNIVE1MVG9rZW4qIHRva2Vu KQogICAgICAgICBtX3RyZWUuaW5zZXJ0Q29tbWVudCh0b2tlbik7CiAgICAgICAgIHJldHVybjsK ICAgICBjYXNlIEhUTUxUb2tlblR5cGVzOjpDaGFyYWN0ZXI6IHsKLSAgICAgICAgU3RyaW5nIGNo YXJhY3RlcnMgPSBTdHJpbmcodG9rZW4tPmNoYXJhY3RlcnMoKS5kYXRhKCksIHRva2VuLT5jaGFy YWN0ZXJzKCkuc2l6ZSgpKTsKKyAgICAgICAgU3RyaW5nIGNoYXJhY3RlcnMgPSBTdHJpbmcodG9r ZW4tPmNoYXJhY3RlcnMoKSwgdG9rZW4tPmNoYXJhY3RlcnNMZW5ndGgoKSk7CiAgICAgICAgIG1f dHJlZS5pbnNlcnRUZXh0Tm9kZShjaGFyYWN0ZXJzKTsKICAgICAgICAgaWYgKG1fZnJhbWVzZXRP ayAmJiAhaXNBbGxXaGl0ZXNwYWNlT3JSZXBsYWNlbWVudENoYXJhY3RlcnMoY2hhcmFjdGVycykp CiAgICAgICAgICAgICBtX2ZyYW1lc2V0T2sgPSBmYWxzZTsKZGlmZiAtLWdpdCBhL1NvdXJjZS9X ZWJDb3JlL3htbC9wYXJzZXIvTWFya3VwVG9rZW5CYXNlLmggYi9Tb3VyY2UvV2ViQ29yZS94bWwv cGFyc2VyL01hcmt1cFRva2VuQmFzZS5oCmluZGV4IDIwZWI5ZTk3MzQ3ZWNhNzc4NzNiNjZjZmU1 ZWI2MjIxOGViNDg5NDEuLjQzNmRhZGQ4Mzk5NTJhODY1MTJmZmFiYzlmZDE2NmZkN2I1MGNjNTcg MTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3htbC9wYXJzZXIvTWFya3VwVG9rZW5CYXNlLmgK KysrIGIvU291cmNlL1dlYkNvcmUveG1sL3BhcnNlci9NYXJrdXBUb2tlbkJhc2UuaApAQCAtNDU4 LDcgKzQ1OCw4IEBAIHB1YmxpYzoKICAgICAgICAgICAgICAgICBtX2RhdGEgPSBTdHJpbmcodG9r ZW4tPmNvbW1lbnQoKS5kYXRhKCksIHRva2VuLT5jb21tZW50KCkuc2l6ZSgpKTsKICAgICAgICAg ICAgIGJyZWFrOwogICAgICAgICBjYXNlIFRva2VuOjpUeXBlOjpDaGFyYWN0ZXI6Ci0gICAgICAg ICAgICBtX2V4dGVybmFsQ2hhcmFjdGVycyA9ICZ0b2tlbi0+Y2hhcmFjdGVycygpOworICAgICAg ICAgICAgbV9leHRlcm5hbENoYXJhY3RlcnMgPSB0b2tlbi0+Y2hhcmFjdGVycygpLmRhdGEoKTsK KyAgICAgICAgICAgIG1fZXh0ZXJuYWxDaGFyYWN0ZXJzTGVuZ3RoID0gdG9rZW4tPmNoYXJhY3Rl cnMoKS5zaXplKCk7CiAgICAgICAgICAgICBtX2lzQWxsOEJpdERhdGEgPSB0b2tlbi0+aXNBbGw4 Qml0RGF0YSgpOwogICAgICAgICAgICAgYnJlYWs7CiAgICAgICAgIGRlZmF1bHQ6CkBAIC00NzAs NiArNDcxLDcgQEAgcHVibGljOgogICAgICAgICA6IG1fdHlwZSh0eXBlKQogICAgICAgICAsIG1f bmFtZShuYW1lKQogICAgICAgICAsIG1fZXh0ZXJuYWxDaGFyYWN0ZXJzKDApCisgICAgICAgICwg bV9leHRlcm5hbENoYXJhY3RlcnNMZW5ndGgoMCkKICAgICAgICAgLCBtX2lzQWxsOEJpdERhdGEo ZmFsc2UpCiAgICAgICAgICwgbV9hdHRyaWJ1dGVzKGF0dHJpYnV0ZXMpCiAgICAgewpAQCAtNTE0 LDEwICs1MTYsMTYgQEAgcHVibGljOgogICAgICAgICByZXR1cm4gbV9hdHRyaWJ1dGVzOwogICAg IH0KIAotICAgIGNvbnN0IHR5cGVuYW1lIFRva2VuOjpEYXRhVmVjdG9yJiBjaGFyYWN0ZXJzKCkg Y29uc3QKKyAgICBjb25zdCBVQ2hhciogY2hhcmFjdGVycygpIGNvbnN0CiAgICAgewogICAgICAg ICBBU1NFUlQobV90eXBlID09IFRva2VuOjpUeXBlOjpDaGFyYWN0ZXIpOwotICAgICAgICByZXR1 cm4gKm1fZXh0ZXJuYWxDaGFyYWN0ZXJzOworICAgICAgICByZXR1cm4gbV9leHRlcm5hbENoYXJh Y3RlcnM7CisgICAgfQorCisgICAgc2l6ZV90IGNoYXJhY3RlcnNMZW5ndGgoKSBjb25zdAorICAg IHsKKyAgICAgICAgQVNTRVJUKG1fdHlwZSA9PSBUb2tlbjo6VHlwZTo6Q2hhcmFjdGVyKTsKKyAg ICAgICAgcmV0dXJuIG1fZXh0ZXJuYWxDaGFyYWN0ZXJzTGVuZ3RoOwogICAgIH0KIAogICAgIGJv b2wgaXNBbGw4Qml0RGF0YSgpIGNvbnN0CkBAIC01NDgsNiArNTU2LDcgQEAgcHVibGljOgogICAg IHZvaWQgY2xlYXJFeHRlcm5hbENoYXJhY3RlcnMoKQogICAgIHsKICAgICAgICAgbV9leHRlcm5h bENoYXJhY3RlcnMgPSAwOworICAgICAgICBtX2V4dGVybmFsQ2hhcmFjdGVyc0xlbmd0aCA9IDA7 CiAgICAgICAgIG1faXNBbGw4Qml0RGF0YSA9IGZhbHNlOwogICAgIH0KIApAQCAtNTc1LDcgKzU4 NCw4IEBAIHByb3RlY3RlZDoKICAgICAvLwogICAgIC8vIEZJWE1FOiBBZGQgYSBtZWNoYW5pc20g Zm9yICJpbnRlcm5hbGl6aW5nIiB0aGUgY2hhcmFjdGVycyB3aGVuIHRoZQogICAgIC8vICAgICAg ICBIVE1MVG9rZW4gaXMgZGVzdHJ1Y3RlZC4KLSAgICBjb25zdCB0eXBlbmFtZSBUb2tlbjo6RGF0 YVZlY3RvciogbV9leHRlcm5hbENoYXJhY3RlcnM7CisgICAgY29uc3QgVUNoYXIqIG1fZXh0ZXJu YWxDaGFyYWN0ZXJzOworICAgIHNpemVfdCBtX2V4dGVybmFsQ2hhcmFjdGVyc0xlbmd0aDsKICAg ICBib29sIG1faXNBbGw4Qml0RGF0YTsKIAogICAgIC8vIEZvciBET0NUWVBFCmRpZmYgLS1naXQg YS9Tb3VyY2UvV2ViQ29yZS94bWwvcGFyc2VyL1hNTFRyZWVCdWlsZGVyLmNwcCBiL1NvdXJjZS9X ZWJDb3JlL3htbC9wYXJzZXIvWE1MVHJlZUJ1aWxkZXIuY3BwCmluZGV4IDdmYWFkZjU4MjNkOGEx NTExZTVjNGQ2OGEwYTdkZTQ2NDdlMGRiMjEuLmQzOTZlMDIxYTI3MTVhMzAyMThhZWU1ZWZiMjAy ZDNhYzVkYWRhNTQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3htbC9wYXJzZXIvWE1MVHJl ZUJ1aWxkZXIuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3htbC9wYXJzZXIvWE1MVHJlZUJ1aWxk ZXIuY3BwCkBAIC0yNzMsNyArMjczLDcgQEAgdm9pZCBYTUxUcmVlQnVpbGRlcjo6cHJvY2Vzc0Vu ZFRhZyhjb25zdCBBdG9taWNYTUxUb2tlbiYgdG9rZW4pCiAKIHZvaWQgWE1MVHJlZUJ1aWxkZXI6 OnByb2Nlc3NDaGFyYWN0ZXIoY29uc3QgQXRvbWljWE1MVG9rZW4mIHRva2VuKQogewotICAgIGFw cGVuZFRvVGV4dCh0b2tlbi5jaGFyYWN0ZXJzKCkuZGF0YSgpLCB0b2tlbi5jaGFyYWN0ZXJzKCku c2l6ZSgpKTsKKyAgICBhcHBlbmRUb1RleHQodG9rZW4uY2hhcmFjdGVycygpLCB0b2tlbi5jaGFy YWN0ZXJzTGVuZ3RoKCkpOwogfQogCiB2b2lkIFhNTFRyZWVCdWlsZGVyOjpwcm9jZXNzQ0RBVEEo Y29uc3QgQXRvbWljWE1MVG9rZW4mIHRva2VuKQo=