145352 2015-05-23 14:02:55 -0700 String.prototype.charAt() should use StringView. 2015-12-15 19:46:26 -0800 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED Performance P2 Normal --- 1 kling kling commit-queue darin kling oldest_to_newest 1097122 0 kling 2015-05-23 14:02:55 -0700 String.prototype.charAt() currently always reifies JSString. This is wasteful if the JSString is a substring. It also uses jsSingleCharacterSubstring() which is a counter-productive optimization, so let's remove that. 1097123 1 253651 kling 2015-05-23 14:11:09 -0700 Created attachment 253651 Patch 1097360 2 253651 commit-queue 2015-05-26 11:54:17 -0700 Comment on attachment 253651 Patch Clearing flags on attachment: 253651 Committed r184865: <http://trac.webkit.org/changeset/184865> 1097361 3 commit-queue 2015-05-26 11:54:20 -0700 All reviewed patches have been landed. Closing bug. 1149432 4 253651 ggaren 2015-12-15 16:17:56 -0800 Comment on attachment 253651 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253651&action=review > Source/JavaScriptCore/runtime/StringPrototype.cpp:788 > + StringView string = thisValue.toString(exec)->view(exec); This code is wrong. It takes a temporary reference to a string's backing store, but nothing prevents that backing store from being deleted or garbage collected. This code is only possible because JSString::SafeView will automatically convert to StringView, due to its operator StringView(). It is very weird that the safe type automatically converts to the unsafe type. That makes the safe type unsafe too. 1149438 5 253651 darin 2015-12-15 16:35:37 -0800 Comment on attachment 253651 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253651&action=review >> Source/JavaScriptCore/runtime/StringPrototype.cpp:788 >> + StringView string = thisValue.toString(exec)->view(exec); > > This code is wrong. It takes a temporary reference to a string's backing store, but nothing prevents that backing store from being deleted or garbage collected. > > This code is only possible because JSString::SafeView will automatically convert to StringView, due to its operator StringView(). It is very weird that the safe type automatically converts to the unsafe type. That makes the safe type unsafe too. I spotted the same mistake in another patch recently. 1149443 6 kling 2015-12-15 16:40:22 -0800 crab That operator StringView() is kind of a footgun eh. 1149482 7 darin 2015-12-15 19:45:06 -0800 Oh, all we need to do is to make the return type be SafeView instead of StringView. 1149483 8 darin 2015-12-15 19:46:03 -0800 (In reply to comment #4) > It is very weird > that the safe type automatically converts to the unsafe type. That makes the > safe type unsafe too. The job of SafeView is to make code like this OK: function(value->view(exec)); It doesn’t make code with local variables safe. Perhaps we can find an even better solution. 1149484 9 darin 2015-12-15 19:46:26 -0800 (In reply to comment #7) > Oh, all we need to do is to make the return type be SafeView instead of > StringView. local variable type, not return type Never occurred to me to actually use SafeView as a local variable. 253651 2015-05-23 14:11:09 -0700 2015-05-26 11:54:17 -0700 Patch bug-145352.diff text/plain 5641 kling ZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvQ2hhbmdlTG9nCmluZGV4IDViZGQ3YTQuLjZkMjE2ZTUgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDMyIEBACiAyMDE1LTA1LTIzICBBbmRyZWFzIEtsaW5n ICA8YWtsaW5nQGFwcGxlLmNvbT4KIAorICAgICAgICBTdHJpbmcucHJvdG90eXBlLmNoYXJBdCgp IHNob3VsZCB1c2UgU3RyaW5nVmlldy4KKyAgICAgICAgPGh0dHBzOi8vd2Via2l0Lm9yZy9iLzE0 NTM1Mj4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBS ZW1vdmUgdGhlIGpzU2luZ2xlQ2hhcmFjdGVyU3Vic3RyaW5nKCkgZnVuY3Rpb24gc2luY2UgaXQn cyBhY3R1YWxseSBjb21wbGV0ZWx5CisgICAgICAgIGNvdW50ZXItcHJvZHVjdGl2ZTogaXQgY291 bGQgY3JlYXRlIGEgc2luZ2xlLWNoYXJhY3RlciBzdHJpbmcgdGhhdCB3b3VsZCByZXRhaW4KKyAg ICAgICAgYSBtdWNoIGxhcmdlciBzdHJpbmcgZm9yIHRoZSBkdXJhdGlvbiBvZiBpdHMgbGlmZXRp bWUuCisKKyAgICAgICAgVGhpcyBtYWRlIHNlbnNlIGJlZm9yZSBTdHJpbmdJbXBsIGxlYXJuZWQg dG8gcHV0IGl0cyBjaGFyYWN0ZXJzIGF0IHRoZSB0YWlsIGVuZAorICAgICAgICBvZiBpdHMgb3du IGFsbG9jYXRpb24uIE5vdyB0aGF0IGl0IGRvZXMsIGl0J3MgZmFyIGJldHRlciB0byBqdXN0IGNy ZWF0ZSBhIG5ldworICAgICAgICBzaW5nbGUtY2hhcmFjdGVyIFN0cmluZ0ltcGwuCisKKyAgICAg ICAgV2l0aCB0aGF0IG91dCBvZiB0aGUgd2F5LCB3ZSBjYW4gbWFrZSBTdHJpbmcucHJvdG90eXBl LmNoYXJBdCgpIHVzZSBTdHJpbmdWaWV3CisgICAgICAgIHRvIGF2b2lkIHJlaWZ5aW5nIHN1YnN0 cmluZyBKU1N0cmluZ3MgKGFuZCBhdm9pZCBzb21lIHJlZiBjaHVybiB0b28uKQorCisgICAgICAg ICogcnVudGltZS9KU1N0cmluZy5jcHA6CisgICAgICAgIChKU0M6OkpTUm9wZVN0cmluZzo6Z2V0 SW5kZXhTbG93Q2FzZSk6CisgICAgICAgICogcnVudGltZS9KU1N0cmluZy5oOgorICAgICAgICAo SlNDOjpKU1N0cmluZzo6Z2V0SW5kZXgpOgorICAgICAgICAoSlNDOjpqc1NpbmdsZUNoYXJhY3Rl clN1YnN0cmluZyk6IERlbGV0ZWQuCisgICAgICAgICogcnVudGltZS9TdHJpbmdQcm90b3R5cGUu Y3BwOgorICAgICAgICAoSlNDOjpzdHJpbmdQcm90b0Z1bmNDaGFyQXQpOgorICAgICAgICAoSlND OjpzdHJpbmdQcm90b0Z1bmNTcGxpdCk6CisKKzIwMTUtMDUtMjMgIEFuZHJlYXMgS2xpbmcgIDxh a2xpbmdAYXBwbGUuY29tPgorCiAgICAgICAgIFN0cmluZy5wcm90b3R5cGUuaW5kZXhPZigpIHNo b3VsZCB1c2UgU3RyaW5nVmlldy4KICAgICAgICAgPGh0dHBzOi8vd2Via2l0Lm9yZy9iLzE0NTM1 MT4KIApkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNTdHJpbmcu Y3BwIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNTdHJpbmcuY3BwCmluZGV4IDYz MjI4ZTAuLmYxNGZhMTEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1l L0pTU3RyaW5nLmNwcAorKysgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU1N0cmlu Zy5jcHAKQEAgLTM3NCw3ICszNzQsNyBAQCBKU1N0cmluZyogSlNSb3BlU3RyaW5nOjpnZXRJbmRl eFNsb3dDYXNlKEV4ZWNTdGF0ZSogZXhlYywgdW5zaWduZWQgaSkKICAgICAgICAgcmV0dXJuIGpz RW1wdHlTdHJpbmcoZXhlYyk7CiAgICAgQVNTRVJUKCFpc1JvcGUoKSk7CiAgICAgUkVMRUFTRV9B U1NFUlQoaSA8IG1fdmFsdWUubGVuZ3RoKCkpOwotICAgIHJldHVybiBqc1NpbmdsZUNoYXJhY3Rl clN1YnN0cmluZyhleGVjLCBtX3ZhbHVlLCBpKTsKKyAgICByZXR1cm4ganNTaW5nbGVDaGFyYWN0 ZXJTdHJpbmcoZXhlYywgbV92YWx1ZVtpXSk7CiB9CiAKIEpTVmFsdWUgSlNTdHJpbmc6OnRvUHJp bWl0aXZlKEV4ZWNTdGF0ZSosIFByZWZlcnJlZFByaW1pdGl2ZVR5cGUpIGNvbnN0CmRpZmYgLS1n aXQgYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU1N0cmluZy5oIGIvU291cmNlL0ph dmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNTdHJpbmcuaAppbmRleCAwZWI3NjA4Li4xZmJlNzQ2IDEw MDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU1N0cmluZy5oCisrKyBi L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0pTU3RyaW5nLmgKQEAgLTQ1LDcgKzQ1LDYg QEAgSlNTdHJpbmcqIGpzU3RyaW5nKEV4ZWNTdGF0ZSosIGNvbnN0IFN0cmluZyYpOyAvLyByZXR1 cm5zIGVtcHR5IHN0cmluZyBpZiBwYXNzZWQKIAogSlNTdHJpbmcqIGpzU2luZ2xlQ2hhcmFjdGVy U3RyaW5nKFZNKiwgVUNoYXIpOwogSlNTdHJpbmcqIGpzU2luZ2xlQ2hhcmFjdGVyU3RyaW5nKEV4 ZWNTdGF0ZSosIFVDaGFyKTsKLUpTU3RyaW5nKiBqc1NpbmdsZUNoYXJhY3RlclN1YnN0cmluZyhF eGVjU3RhdGUqLCBjb25zdCBTdHJpbmcmLCB1bnNpZ25lZCBvZmZzZXQpOwogSlNTdHJpbmcqIGpz U3Vic3RyaW5nKFZNKiwgY29uc3QgU3RyaW5nJiwgdW5zaWduZWQgb2Zmc2V0LCB1bnNpZ25lZCBs ZW5ndGgpOwogSlNTdHJpbmcqIGpzU3Vic3RyaW5nKEV4ZWNTdGF0ZSosIGNvbnN0IFN0cmluZyYs IHVuc2lnbmVkIG9mZnNldCwgdW5zaWduZWQgbGVuZ3RoKTsKIApAQCAtNDQ0LDE2ICs0NDMsNiBA QCBBTFdBWVNfSU5MSU5FIEpTU3RyaW5nKiBqc1NpbmdsZUNoYXJhY3RlclN0cmluZyhWTSogdm0s IFVDaGFyIGMpCiAgICAgcmV0dXJuIEpTU3RyaW5nOjpjcmVhdGUoKnZtLCBTdHJpbmcoJmMsIDEp LmltcGwoKSk7CiB9CiAKLUFMV0FZU19JTkxJTkUgSlNTdHJpbmcqIGpzU2luZ2xlQ2hhcmFjdGVy U3Vic3RyaW5nKEV4ZWNTdGF0ZSogZXhlYywgY29uc3QgU3RyaW5nJiBzLCB1bnNpZ25lZCBvZmZz ZXQpCi17Ci0gICAgVk0qIHZtID0gJmV4ZWMtPnZtKCk7Ci0gICAgQVNTRVJUKG9mZnNldCA8IHN0 YXRpY19jYXN0PHVuc2lnbmVkPihzLmxlbmd0aCgpKSk7Ci0gICAgVUNoYXIgYyA9IHMuY2hhcmFj dGVyQXQob2Zmc2V0KTsKLSAgICBpZiAoYyA8PSBtYXhTaW5nbGVDaGFyYWN0ZXJTdHJpbmcpCi0g ICAgICAgIHJldHVybiB2bS0+c21hbGxTdHJpbmdzLnNpbmdsZUNoYXJhY3RlclN0cmluZyhjKTsK LSAgICByZXR1cm4gSlNTdHJpbmc6OmNyZWF0ZSgqdm0sIFN0cmluZ0ltcGw6OmNyZWF0ZVN1YnN0 cmluZ1NoYXJpbmdJbXBsKHMuaW1wbCgpLCBvZmZzZXQsIDEpKTsKLX0KLQogaW5saW5lIEpTU3Ry aW5nKiBqc05vbnRyaXZpYWxTdHJpbmcoVk0qIHZtLCBjb25zdCBTdHJpbmcmIHMpCiB7CiAgICAg QVNTRVJUKHMubGVuZ3RoKCkgPiAxKTsKQEAgLTUwNyw3ICs0OTYsNyBAQCBpbmxpbmUgSlNTdHJp bmcqIEpTU3RyaW5nOjpnZXRJbmRleChFeGVjU3RhdGUqIGV4ZWMsIHVuc2lnbmVkIGkpCiAgICAg aWYgKGlzUm9wZSgpKQogICAgICAgICByZXR1cm4gc3RhdGljX2Nhc3Q8SlNSb3BlU3RyaW5nKj4o dGhpcyktPmdldEluZGV4U2xvd0Nhc2UoZXhlYywgaSk7CiAgICAgQVNTRVJUKGkgPCBtX3ZhbHVl Lmxlbmd0aCgpKTsKLSAgICByZXR1cm4ganNTaW5nbGVDaGFyYWN0ZXJTdWJzdHJpbmcoZXhlYywg bV92YWx1ZSwgaSk7CisgICAgcmV0dXJuIGpzU2luZ2xlQ2hhcmFjdGVyU3RyaW5nKGV4ZWMsIG1f dmFsdWVbaV0pOwogfQogCiBpbmxpbmUgSlNTdHJpbmcqIGpzU3RyaW5nKFZNKiB2bSwgY29uc3Qg U3RyaW5nJiBzKQpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvU3Ry aW5nUHJvdG90eXBlLmNwcCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL1N0cmluZ1By b3RvdHlwZS5jcHAKaW5kZXggYTViYjY4Yy4uM2EyMTBkZCAxMDA2NDQKLS0tIGEvU291cmNlL0ph dmFTY3JpcHRDb3JlL3J1bnRpbWUvU3RyaW5nUHJvdG90eXBlLmNwcAorKysgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvcnVudGltZS9TdHJpbmdQcm90b3R5cGUuY3BwCkBAIC03ODUsMTggKzc4NSwx NyBAQCBFbmNvZGVkSlNWYWx1ZSBKU0NfSE9TVF9DQUxMIHN0cmluZ1Byb3RvRnVuY0NoYXJBdChF eGVjU3RhdGUqIGV4ZWMpCiAgICAgSlNWYWx1ZSB0aGlzVmFsdWUgPSBleGVjLT50aGlzVmFsdWUo KTsKICAgICBpZiAoIWNoZWNrT2JqZWN0Q29lcmNpYmxlKHRoaXNWYWx1ZSkpCiAgICAgICAgIHJl dHVybiB0aHJvd1ZNVHlwZUVycm9yKGV4ZWMpOwotICAgIFN0cmluZyBzID0gdGhpc1ZhbHVlLnRv U3RyaW5nKGV4ZWMpLT52YWx1ZShleGVjKTsKLSAgICB1bnNpZ25lZCBsZW4gPSBzLmxlbmd0aCgp OworICAgIFN0cmluZ1ZpZXcgc3RyaW5nID0gdGhpc1ZhbHVlLnRvU3RyaW5nKGV4ZWMpLT52aWV3 KGV4ZWMpOwogICAgIEpTVmFsdWUgYTAgPSBleGVjLT5hcmd1bWVudCgwKTsKICAgICBpZiAoYTAu aXNVSW50MzIoKSkgewogICAgICAgICB1aW50MzJfdCBpID0gYTAuYXNVSW50MzIoKTsKLSAgICAg ICAgaWYgKGkgPCBsZW4pCi0gICAgICAgICAgICByZXR1cm4gSlNWYWx1ZTo6ZW5jb2RlKGpzU2lu Z2xlQ2hhcmFjdGVyU3Vic3RyaW5nKGV4ZWMsIHMsIGkpKTsKKyAgICAgICAgaWYgKGkgPCBzdHJp bmcubGVuZ3RoKCkpCisgICAgICAgICAgICByZXR1cm4gSlNWYWx1ZTo6ZW5jb2RlKGpzU2luZ2xl Q2hhcmFjdGVyU3RyaW5nKGV4ZWMsIHN0cmluZ1tpXSkpOwogICAgICAgICByZXR1cm4gSlNWYWx1 ZTo6ZW5jb2RlKGpzRW1wdHlTdHJpbmcoZXhlYykpOwogICAgIH0KICAgICBkb3VibGUgZHBvcyA9 IGEwLnRvSW50ZWdlcihleGVjKTsKLSAgICBpZiAoZHBvcyA+PSAwICYmIGRwb3MgPCBsZW4pCi0g ICAgICAgIHJldHVybiBKU1ZhbHVlOjplbmNvZGUoanNTaW5nbGVDaGFyYWN0ZXJTdWJzdHJpbmco ZXhlYywgcywgc3RhdGljX2Nhc3Q8dW5zaWduZWQ+KGRwb3MpKSk7CisgICAgaWYgKGRwb3MgPj0g MCAmJiBkcG9zIDwgc3RyaW5nLmxlbmd0aCgpKQorICAgICAgICByZXR1cm4gSlNWYWx1ZTo6ZW5j b2RlKGpzU2luZ2xlQ2hhcmFjdGVyU3RyaW5nKGV4ZWMsIHN0cmluZ1tzdGF0aWNfY2FzdDx1bnNp Z25lZD4oZHBvcyldKSk7CiAgICAgcmV0dXJuIEpTVmFsdWU6OmVuY29kZShqc0VtcHR5U3RyaW5n KGV4ZWMpKTsKIH0KIApAQCAtMTI0Niw3ICsxMjQ1LDcgQEAgRW5jb2RlZEpTVmFsdWUgSlNDX0hP U1RfQ0FMTCBzdHJpbmdQcm90b0Z1bmNTcGxpdChFeGVjU3RhdGUqIGV4ZWMpCiAgICAgICAgICAg ICBBU1NFUlQobGltaXQpOwogCiAgICAgICAgICAgICBkbyB7Ci0gICAgICAgICAgICAgICAgcmVz dWx0LT5wdXREaXJlY3RJbmRleChleGVjLCBwb3NpdGlvbiwganNTaW5nbGVDaGFyYWN0ZXJTdWJz dHJpbmcoZXhlYywgaW5wdXQsIHBvc2l0aW9uKSk7CisgICAgICAgICAgICAgICAgcmVzdWx0LT5w dXREaXJlY3RJbmRleChleGVjLCBwb3NpdGlvbiwganNTaW5nbGVDaGFyYWN0ZXJTdHJpbmcoZXhl YywgaW5wdXRbcG9zaXRpb25dKSk7CiAgICAgICAgICAgICB9IHdoaWxlICgrK3Bvc2l0aW9uIDwg bGltaXQpOwogCiAgICAgICAgICAgICByZXR1cm4gSlNWYWx1ZTo6ZW5jb2RlKHJlc3VsdCk7Cg==