92056 2012-07-23 19:27:34 -0700 Clear the external characters pointer of an AtomicHTMLToken before the raw token is cleared. 2012-07-24 15:35:37 -0700 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 skyul skyul abarth eric oldest_to_newest 676328 0 skyul 2012-07-23 19:27:34 -0700 AtomicHTMLToken keeps a pointer to the HTMLToken's buffer instead of copying the characters for performance. Clear the external characters pointer before the raw token is cleared to make sure that we won't have a dangling pointer. 676329 1 153938 skyul 2012-07-23 19:30:33 -0700 Created attachment 153938 Patch 676330 2 skyul 2012-07-23 19:31:41 -0700 Adam, this is the follow-up patch you requested in Bug 91981. 676940 3 153938 abarth 2012-07-24 10:07:17 -0700 Comment on attachment 153938 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153938&action=review > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:459 > + if (token->type() == HTMLTokenTypes::Character) > + token->clearExternalCharacters(); I would just do this unconditionally. There isn't any harm in overwriting m_externalCharacters with 0 for other token types and it saves us this branch. 676947 4 abarth 2012-07-24 10:10:51 -0700 Thanks for writing this patch. :) 677226 5 skyul 2012-07-24 14:57:01 -0700 (In reply to comment #3) > > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:459 > > + if (token->type() == HTMLTokenTypes::Character) > > + token->clearExternalCharacters(); > > I would just do this unconditionally. There isn't any harm in overwriting m_externalCharacters with 0 for other token types and it saves us this branch. Thanks for the review! I will change it before I land the patch. 677239 6 skyul 2012-07-24 15:13:15 -0700 Committed r123536: <http://trac.webkit.org/changeset/123536> 677266 7 abarth 2012-07-24 15:27:38 -0700 This assert is wrong now that we're calling the function unconditionally. ASSERT(m_type == Token::Type::Character); 677268 8 skyul 2012-07-24 15:29:00 -0700 (In reply to comment #7) > This assert is wrong now that we're calling the function unconditionally. > > ASSERT(m_type == Token::Type::Character); Oops, sorry. I will fix it. 677283 9 skyul 2012-07-24 15:35:37 -0700 (In reply to comment #8) > (In reply to comment #7) > > This assert is wrong now that we're calling the function unconditionally. > > > > ASSERT(m_type == Token::Type::Character); > > Oops, sorry. I will fix it. Done. http://trac.webkit.org/changeset/123542 153938 2012-07-23 19:30:33 -0700 2012-07-24 10:07:16 -0700 Patch bug-92056-20120724113021.patch text/plain 2791 skyul U3VidmVyc2lvbiBSZXZpc2lvbjogMTIzNDE0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZDQ4NzNiOGRjNzNkODhk MDQ4NTNkMGMwYzBiN2RkNTMzZGJjMzNkOC4uMTEzMDRkODNkMDM1NmU4ZDIxNjU5NTg4MGNmZjEz ZDg2ZWY4MTg1MiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIyIEBACisyMDEyLTA3LTIzICBLd2Fu ZyBZdWwgU2VvICA8c2t5dWxAY29tcGFueTEwMC5uZXQ+CisKKyAgICAgICAgQ2xlYXIgdGhlIGV4 dGVybmFsIGNoYXJhY3RlcnMgcG9pbnRlciBvZiBhbiBBdG9taWNIVE1MVG9rZW4gYmVmb3JlIHRo ZSByYXcgdG9rZW4gaXMgY2xlYXJlZC4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcv c2hvd19idWcuY2dpP2lkPTkyMDU2CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BT ISkuCisKKyAgICAgICAgQXRvbWljSFRNTFRva2VuIGtlZXBzIGEgcG9pbnRlciB0byB0aGUgSFRN TFRva2VuJ3MgYnVmZmVyIGluc3RlYWQgb2YgY29weWluZyB0aGUKKyAgICAgICAgY2hhcmFjdGVy cyBmb3IgcGVyZm9ybWFuY2UuIENsZWFyIHRoZSBleHRlcm5hbCBjaGFyYWN0ZXJzIHBvaW50ZXIg YmVmb3JlIHRoZSByYXcgdG9rZW4KKyAgICAgICAgaXMgY2xlYXJlZCB0byBtYWtlIHN1cmUgdGhh dCB3ZSB3b24ndCBoYXZlIGEgZGFuZ2xpbmcgcG9pbnRlci4KKworICAgICAgICBObyBuZXcgdGVz dHMgLSBubyBmdW5jdGlvbmFsIGNoYW5nZXMuCisKKyAgICAgICAgKiBodG1sL3BhcnNlci9IVE1M VHJlZUJ1aWxkZXIuY3BwOgorICAgICAgICAoV2ViQ29yZTo6SFRNTFRyZWVCdWlsZGVyOjpjb25z dHJ1Y3RUcmVlRnJvbVRva2VuKToKKyAgICAgICAgKiB4bWwvcGFyc2VyL01hcmt1cFRva2VuQmFz ZS5oOgorICAgICAgICAoV2ViQ29yZTo6QXRvbWljTWFya3VwVG9rZW5CYXNlOjpjbGVhckV4dGVy bmFsQ2hhcmFjdGVycyk6CisgICAgICAgIChBdG9taWNNYXJrdXBUb2tlbkJhc2UpOgorCiAyMDEy LTA3LTIzICBIYW55ZWUgS2ltICA8Y2hvY29AY29tcGFueTEwMC5uZXQ+CiAKICAgICAgICAgV2hl biB1c2luZyBUZXh0dXJlTWFwcGVyLCBXZWJLaXQgZG9lcyBub3QgcmV0YWluIGZpbmFsIHZhbHVl IG9mIG9wYWNpdHkgaW4gYW5pbWF0aW9ucywgdW5sZXNzIGl0IGlzIHNwZWNpZmllZCBvbiB0aGUg bGFzdCBrZXkgZnJhbWUuCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9odG1sL3BhcnNlci9I VE1MVHJlZUJ1aWxkZXIuY3BwIGIvU291cmNlL1dlYkNvcmUvaHRtbC9wYXJzZXIvSFRNTFRyZWVC dWlsZGVyLmNwcAppbmRleCA3MjkzODhkNTg5MWUwYjIyMmI1OWMyMzVkZDUwNWE4NDc0MDdiM2E2 Li4xOWE5MWY4YzY0NTgzZTEzMzExYTlkY2UwZjU2MTk5NDU4NTU5MDI4IDEwMDY0NAotLS0gYS9T b3VyY2UvV2ViQ29yZS9odG1sL3BhcnNlci9IVE1MVHJlZUJ1aWxkZXIuY3BwCisrKyBiL1NvdXJj ZS9XZWJDb3JlL2h0bWwvcGFyc2VyL0hUTUxUcmVlQnVpbGRlci5jcHAKQEAgLTQ1MSw2ICs0NTEs MTMgQEAgdm9pZCBIVE1MVHJlZUJ1aWxkZXI6OmNvbnN0cnVjdFRyZWVGcm9tVG9rZW4oSFRNTFRv a2VuJiByYXdUb2tlbikKIAogICAgIGNvbnN0cnVjdFRyZWVGcm9tQXRvbWljVG9rZW4odG9rZW4u Z2V0KCkpOwogCisgICAgLy8gQXRvbWljSFRNTFRva2VuIGtlZXBzIGEgcG9pbnRlciB0byB0aGUg SFRNTFRva2VuJ3MgYnVmZmVyIGluc3RlYWQKKyAgICAvLyBvZiBjb3B5aW5nIHRoZSBjaGFyYWN0 ZXJzIGZvciBwZXJmb3JtYW5jZS4KKyAgICAvLyBDbGVhciB0aGUgZXh0ZXJuYWwgY2hhcmFjdGVy cyBwb2ludGVyIGJlZm9yZSB0aGUgcmF3IHRva2VuIGlzIGNsZWFyZWQKKyAgICAvLyB0byBtYWtl IHN1cmUgdGhhdCB3ZSB3b24ndCBoYXZlIGEgZGFuZ2xpbmcgcG9pbnRlci4KKyAgICBpZiAodG9r ZW4tPnR5cGUoKSA9PSBIVE1MVG9rZW5UeXBlczo6Q2hhcmFjdGVyKQorICAgICAgICB0b2tlbi0+ Y2xlYXJFeHRlcm5hbENoYXJhY3RlcnMoKTsKKwogICAgIGlmICghcmF3VG9rZW4uaXNVbmluaXRp YWxpemVkKCkpIHsKICAgICAgICAgQVNTRVJUKHJhd1Rva2VuLnR5cGUoKSA9PSBIVE1MVG9rZW5U eXBlczo6Q2hhcmFjdGVyKTsKICAgICAgICAgcmF3VG9rZW4uY2xlYXIoKTsKZGlmZiAtLWdpdCBh L1NvdXJjZS9XZWJDb3JlL3htbC9wYXJzZXIvTWFya3VwVG9rZW5CYXNlLmggYi9Tb3VyY2UvV2Vi Q29yZS94bWwvcGFyc2VyL01hcmt1cFRva2VuQmFzZS5oCmluZGV4IGFjYjVkNzNhMGNiNDc0OGU1 NzdlNDI0NjFmMzJmMjY0Zjg3NDQ5ZjQuLjA4MDgyMWRlZWNmMGZjMTI3NTA2OWVhMDMxZWRlMmEy N2JlMzQ0ZGEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3htbC9wYXJzZXIvTWFya3VwVG9r ZW5CYXNlLmgKKysrIGIvU291cmNlL1dlYkNvcmUveG1sL3BhcnNlci9NYXJrdXBUb2tlbkJhc2Uu aApAQCAtNDgyLDYgKzQ4MiwxMiBAQCBwdWJsaWM6CiAgICAgICAgIHJldHVybiBtX2RvY3R5cGVE YXRhLT5tX3N5c3RlbUlkZW50aWZpZXI7CiAgICAgfQogCisgICAgdm9pZCBjbGVhckV4dGVybmFs Q2hhcmFjdGVycygpCisgICAgeworICAgICAgICBBU1NFUlQobV90eXBlID09IFRva2VuOjpUeXBl OjpDaGFyYWN0ZXIpOworICAgICAgICBtX2V4dGVybmFsQ2hhcmFjdGVycyA9IDA7CisgICAgfQor CiBwcm90ZWN0ZWQ6CiAgICAgdHlwZW5hbWUgVG9rZW46OlR5cGU6OlR5cGUgbV90eXBlOwogCg==