184952 2018-04-24 19:52:18 -0700 fromCharCode is missing some exception checks 2018-04-27 11:33:51 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 keith_miller keith_miller darin ews-watchlist mark.lam msaboff saam webkit-bug-importer oldest_to_newest 1417398 0 keith_miller 2018-04-24 19:52:18 -0700 fromCharCode is missing some exception checks 1417400 1 338699 keith_miller 2018-04-24 19:55:36 -0700 Created attachment 338699 Patch 1417403 2 338699 saam 2018-04-24 20:09:13 -0700 Comment on attachment 338699 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338699&action=review > JSTests/stress/fromCharCode-exception-check.js:5 > +} catch (e) { } probably worth checking the exception type here 1417406 3 keith_miller 2018-04-24 20:12:43 -0700 <rdar://problem/39001185> 1417407 4 338699 keith_miller 2018-04-24 20:13:51 -0700 Comment on attachment 338699 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338699&action=review >> JSTests/stress/fromCharCode-exception-check.js:5 >> +} catch (e) { } > > probably worth checking the exception type here Done. 1417408 5 keith_miller 2018-04-24 20:14:29 -0700 Committed r230980: <https://trac.webkit.org/changeset/230980> 1418396 6 338699 darin 2018-04-26 21:07:41 -0700 Comment on attachment 338699 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338699&action=review > Source/JavaScriptCore/runtime/StringConstructor.cpp:82 > + RETURN_IF_EXCEPTION(scope, encodedJSValue()); > + scope.release(); Sad to have to do this branch just to optimize the exception case. Can’t this have the optimization that the old code did, where it just did a little wasteful work to make the single character string that would then be ignored, and let the exception propagate without checking for it explicitly? Or is that impractical now? 1418525 7 338699 keith_miller 2018-04-27 10:10:31 -0700 Comment on attachment 338699 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338699&action=review >> Source/JavaScriptCore/runtime/StringConstructor.cpp:82 >> + scope.release(); > > Sad to have to do this branch just to optimize the exception case. Can’t this have the optimization that the old code did, where it just did a little wasteful work to make the single character string that would then be ignored, and let the exception propagate without checking for it explicitly? Or is that impractical now? We could do that. You'd just need to move the scope.release() above the current line. In practice though, I'd guess that the branch doesn't actually matter much, especially if it's never taken. By the time we get to the DFG/FTL, we already have an intrinsic for fromCharCode so this code doesn't really matter. In the LLInt/Baseline there are so many other branches this one probably doesn't matter. Either way, since it's safe to just ignore the exception status I'll remove the RETURN_IF_EXCEPTION line. 1418570 8 keith_miller 2018-04-27 11:33:51 -0700 (In reply to Keith Miller from comment #7) > Comment on attachment 338699 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=338699&action=review > > >> Source/JavaScriptCore/runtime/StringConstructor.cpp:82 > >> + scope.release(); > > > > Sad to have to do this branch just to optimize the exception case. Can’t this have the optimization that the old code did, where it just did a little wasteful work to make the single character string that would then be ignored, and let the exception propagate without checking for it explicitly? Or is that impractical now? > > We could do that. You'd just need to move the scope.release() above the > current line. > > In practice though, I'd guess that the branch doesn't actually matter much, > especially if it's never taken. By the time we get to the DFG/FTL, we > already have an intrinsic for fromCharCode so this code doesn't really > matter. In the LLInt/Baseline there are so many other branches this one > probably doesn't matter. > > Either way, since it's safe to just ignore the exception status I'll remove > the RETURN_IF_EXCEPTION line. Uploaded: https://bugs.webkit.org/show_bug.cgi?id=185083 338699 2018-04-24 19:55:36 -0700 2018-04-24 20:09:13 -0700 Patch bug-184952-20180424195534.patch text/plain 3757 keith_miller U3VidmVyc2lvbiBSZXZpc2lvbjogMjMwOTc5CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCA2 MDAxMzc3MGQxMDI2ZGIzZDM5Y2RlYTliNTI5MDA3YzYzNjk0ZThlLi40MTI3OWVjYjUxOWRhYzNj YzNhM2QwZGFkOTRlNGZhN2E5NmQxZDk1IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNyBAQAorMjAxOC0wNC0yNCAgS2VpdGggTWlsbGVyICA8a2VpdGhfbWlsbGVyQGFwcGxl LmNvbT4KKworICAgICAgICBmcm9tQ2hhckNvZGUgaXMgbWlzc2luZyBzb21lIGV4Y2VwdGlvbiBj aGVja3MKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE4 NDk1MgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEkg YWxzbyByZW1vdmVkIHRoZSBwb2ludGxlc3Mgc2xvdyBwYXRoIGZ1bmN0aW9uIGFuZCBtb3ZlZCBp dCBpbnRvIHRoZQorICAgICAgICBtYWluIGZ1bmN0aW9uLgorCisgICAgICAgICogcnVudGltZS9T dHJpbmdDb25zdHJ1Y3Rvci5jcHA6CisgICAgICAgIChKU0M6OnN0cmluZ0Zyb21DaGFyQ29kZSk6 CisgICAgICAgIChKU0M6OnN0cmluZ0Zyb21DaGFyQ29kZVNsb3dDYXNlKTogRGVsZXRlZC4KKwog MjAxOC0wNC0yNCAgRmlsaXAgUGl6bG8gIDxmcGl6bG9AYXBwbGUuY29tPgogCiAgICAgICAgIE11 bHRpQnlPZmZzZXQgc2hvdWxkIGVtaXQgb25lIGZld2VyIGJyYW5jaGVzIGluIHRoZSBjYXNlIHRo YXQgdGhlIHNldCBvZiBzdHJ1Y3R1cmVzIGlzIHByb3ZlZCBhbHJlYWR5CmRpZmYgLS1naXQgYS9T b3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9TdHJpbmdDb25zdHJ1Y3Rvci5jcHAgYi9Tb3Vy Y2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9TdHJpbmdDb25zdHJ1Y3Rvci5jcHAKaW5kZXggNTZi YWNlZjY4ODAwMjgyOGQ2MWY5Mjg1NjNmOTUwYzk1NmE0ZDRlMi4uMTBhYmM3NjEwNWQyYjBkMGVl YTk4MjU5ZjcwYzJkNDkzYWRkOTBhYSAxMDA2NDQKLS0tIGEvU291cmNlL0phdmFTY3JpcHRDb3Jl L3J1bnRpbWUvU3RyaW5nQ29uc3RydWN0b3IuY3BwCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29y ZS9ydW50aW1lL1N0cmluZ0NvbnN0cnVjdG9yLmNwcApAQCAtNzAsMjEgKzcwLDI3IEBAIHZvaWQg U3RyaW5nQ29uc3RydWN0b3I6OmZpbmlzaENyZWF0aW9uKFZNJiB2bSwgU3RyaW5nUHJvdG90eXBl KiBzdHJpbmdQcm90b3R5cGUpCiAKIC8vIC0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLSBG dW5jdGlvbnMgLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0KIAotc3RhdGljIE5FVkVS X0lOTElORSBKU1ZhbHVlIHN0cmluZ0Zyb21DaGFyQ29kZVNsb3dDYXNlKEV4ZWNTdGF0ZSogZXhl YykKK3N0YXRpYyBFbmNvZGVkSlNWYWx1ZSBKU0NfSE9TVF9DQUxMIHN0cmluZ0Zyb21DaGFyQ29k ZShFeGVjU3RhdGUqIGV4ZWMpCiB7CisgICAgVk0mIHZtID0gZXhlYy0+dm0oKTsKKyAgICBhdXRv IHNjb3BlID0gREVDTEFSRV9USFJPV19TQ09QRSh2bSk7CisKICAgICB1bnNpZ25lZCBsZW5ndGgg PSBleGVjLT5hcmd1bWVudENvdW50KCk7CisgICAgaWYgKExJS0VMWShsZW5ndGggPT0gMSkpIHsK KyAgICAgICAgdW5zaWduZWQgY29kZSA9IGV4ZWMtPnVuY2hlY2tlZEFyZ3VtZW50KDApLnRvVUlu dDMyKGV4ZWMpOworICAgICAgICBSRVRVUk5fSUZfRVhDRVBUSU9OKHNjb3BlLCBlbmNvZGVkSlNW YWx1ZSgpKTsKKyAgICAgICAgc2NvcGUucmVsZWFzZSgpOworICAgICAgICByZXR1cm4gSlNWYWx1 ZTo6ZW5jb2RlKGpzU2luZ2xlQ2hhcmFjdGVyU3RyaW5nKGV4ZWMsIGNvZGUpKTsKKyAgICB9CisK ICAgICBVQ2hhciogYnVmOwogICAgIGF1dG8gaW1wbCA9IFN0cmluZ0ltcGw6OmNyZWF0ZVVuaW5p dGlhbGl6ZWQobGVuZ3RoLCBidWYpOwotICAgIGZvciAodW5zaWduZWQgaSA9IDA7IGkgPCBsZW5n dGg7ICsraSkKKyAgICBmb3IgKHVuc2lnbmVkIGkgPSAwOyBpIDwgbGVuZ3RoOyArK2kpIHsKICAg ICAgICAgYnVmW2ldID0gc3RhdGljX2Nhc3Q8VUNoYXI+KGV4ZWMtPnVuY2hlY2tlZEFyZ3VtZW50 KGkpLnRvVUludDMyKGV4ZWMpKTsKLSAgICByZXR1cm4ganNTdHJpbmcoZXhlYywgV1RGTW92ZShp bXBsKSk7Ci19Ci0KLXN0YXRpYyBFbmNvZGVkSlNWYWx1ZSBKU0NfSE9TVF9DQUxMIHN0cmluZ0Zy b21DaGFyQ29kZShFeGVjU3RhdGUqIGV4ZWMpCi17Ci0gICAgaWYgKExJS0VMWShleGVjLT5hcmd1 bWVudENvdW50KCkgPT0gMSkpCi0gICAgICAgIHJldHVybiBKU1ZhbHVlOjplbmNvZGUoanNTaW5n bGVDaGFyYWN0ZXJTdHJpbmcoZXhlYywgZXhlYy0+dW5jaGVja2VkQXJndW1lbnQoMCkudG9VSW50 MzIoZXhlYykpKTsKLSAgICByZXR1cm4gSlNWYWx1ZTo6ZW5jb2RlKHN0cmluZ0Zyb21DaGFyQ29k ZVNsb3dDYXNlKGV4ZWMpKTsKKyAgICAgICAgUkVUVVJOX0lGX0VYQ0VQVElPTihzY29wZSwgZW5j b2RlZEpTVmFsdWUoKSk7CisgICAgfQorICAgIHNjb3BlLnJlbGVhc2UoKTsKKyAgICByZXR1cm4g SlNWYWx1ZTo6ZW5jb2RlKGpzU3RyaW5nKGV4ZWMsIFdURk1vdmUoaW1wbCkpKTsKIH0KIAogSlND ZWxsKiBKU0NfSE9TVF9DQUxMIHN0cmluZ0Zyb21DaGFyQ29kZShFeGVjU3RhdGUqIGV4ZWMsIGlu dDMyX3QgYXJnKQpkaWZmIC0tZ2l0IGEvSlNUZXN0cy9DaGFuZ2VMb2cgYi9KU1Rlc3RzL0NoYW5n ZUxvZwppbmRleCAwYWUwODk0MDM1MWRlOTY5MjNlYmM2NzQ0N2FiZTc4Y2JhOGI0ZjczLi4zNzQx MzRkZDllMDY1M2RjY2ViYjY3YjRhMmJiMmE4OTMzNGZhMzk5IDEwMDY0NAotLS0gYS9KU1Rlc3Rz L0NoYW5nZUxvZworKysgYi9KU1Rlc3RzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEzIEBACisyMDE4 LTA0LTI0ICBLZWl0aCBNaWxsZXIgIDxrZWl0aF9taWxsZXJAYXBwbGUuY29tPgorCisgICAgICAg IGZyb21DaGFyQ29kZSBpcyBtaXNzaW5nIHNvbWUgZXhjZXB0aW9uIGNoZWNrcworICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTg0OTUyCisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBzdHJlc3MvZnJvbUNoYXJD b2RlLWV4Y2VwdGlvbi1jaGVjay5qczogQWRkZWQuCisgICAgICAgIChnZXQgY2F0Y2gpOgorCiAy MDE4LTA0LTI0ICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KIAogICAgICAgICBHYXJk ZW5pbmc6IHRlc3QgZml4IGFmdGVyIHIyMzA4NjMuCmRpZmYgLS1naXQgYS9KU1Rlc3RzL3N0cmVz cy9mcm9tQ2hhckNvZGUtZXhjZXB0aW9uLWNoZWNrLmpzIGIvSlNUZXN0cy9zdHJlc3MvZnJvbUNo YXJDb2RlLWV4Y2VwdGlvbi1jaGVjay5qcwpuZXcgZmlsZSBtb2RlIDEwMDY0NAppbmRleCAwMDAw MDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwLi5iMTA3MzI4ZDA4MzVhZDk2Nzk5 ODQxZWFmMWJhNjNmNGM1MjkwZDJjCi0tLSAvZGV2L251bGwKKysrIGIvSlNUZXN0cy9zdHJlc3Mv ZnJvbUNoYXJDb2RlLWV4Y2VwdGlvbi1jaGVjay5qcwpAQCAtMCwwICsxLDUgQEAKKy8vIFRoaXMg c2hvdWxkbid0IGNyYXNoLgorCit0cnkgeworICAgIFN0cmluZy5mcm9tQ2hhckNvZGUoU3ltYm9s KCksIG5ldyBQcm94eSh7fSwgeyBnZXQoKSB7IH0gfSkpOworfSBjYXRjaCAoZSkgeyB9Cg==