189922 2018-09-24 11:48:27 -0700 Array.prototype.indexOf fast path needs to ensure the length is still valid after performing effects 2018-09-25 21:46:29 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 saam saam benjamin commit-queue fpizlo ggaren gskachkov keith_miller mark.lam msaboff rmorisset ticaiolima webkit-bug-importer ysuzuki oldest_to_newest 1462791 0 saam 2018-09-24 11:48:27 -0700 ... 1462794 1 saam 2018-09-24 11:52:17 -0700 Patch forthcoming 1462818 2 saam 2018-09-24 12:36:29 -0700 <rdar://problem/44651275> 1462823 3 350668 saam 2018-09-24 12:39:33 -0700 Created attachment 350668 patch 1462845 4 350668 mark.lam 2018-09-24 13:16:27 -0700 Comment on attachment 350668 patch View in context: https://bugs.webkit.org/attachment.cgi?id=350668&action=review r=me > Source/JavaScriptCore/ChangeLog:11 > + index may perform effects. E.g, it could change the length of the lower case "e.g." > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1173 > + bool canDoFastPath = array->canDoFastIndexedAccess(vm) > + && array->getArrayLength() == length; // The effects in getting `index` could have changed the length of this array. The "cannot do fast path" path below is still doing "for (; index < length; ++index)" using the cached length. I see that this is the correct behavior according to https://tc39.github.io/ecma262/#sec-array.prototype.indexof. Can you also add a test that modifies length in the other direction (increasing length) and verify that we don't iterate past the cached length (perhaps by using a Proxy override of hasProperty? 1462851 5 350668 saam 2018-09-24 13:22:21 -0700 Comment on attachment 350668 patch View in context: https://bugs.webkit.org/attachment.cgi?id=350668&action=review >> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1173 >> + && array->getArrayLength() == length; // The effects in getting `index` could have changed the length of this array. > > The "cannot do fast path" path below is still doing "for (; index < length; ++index)" using the cached length. I see that this is the correct behavior according to https://tc39.github.io/ecma262/#sec-array.prototype.indexof. Can you also add a test that modifies length in the other direction (increasing length) and verify that we don't iterate past the cached length (perhaps by using a Proxy override of hasProperty? Sure 1462898 6 350689 saam 2018-09-24 14:45:20 -0700 Created attachment 350689 patch for landing 1462972 7 350689 commit-queue 2018-09-24 16:06:01 -0700 Comment on attachment 350689 patch for landing Clearing flags on attachment: 350689 Committed r236437: <https://trac.webkit.org/changeset/236437> 1462973 8 commit-queue 2018-09-24 16:06:03 -0700 All reviewed patches have been landed. Closing bug. 1463509 9 ysuzuki 2018-09-25 21:46:29 -0700 Oops, nice! 350668 2018-09-24 12:39:33 -0700 2018-09-24 14:45:20 -0700 patch c-backup.diff text/plain 3111 saam SW5kZXg6IEpTVGVzdHMvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIEpTVGVzdHMvQ2hhbmdlTG9n CShyZXZpc2lvbiAyMzY0MTgpCisrKyBKU1Rlc3RzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpA QCAtMSwzICsxLDE1IEBACisyMDE4LTA5LTI0ICBTYWFtIEJhcmF0aSAgPHNiYXJhdGlAYXBwbGUu Y29tPgorCisgICAgICAgIEFycmF5LnByb3RvdHlwZS5pbmRleE9mIGZhc3QgcGF0aCBuZWVkcyB0 byBlbnN1cmUgdGhlIGxlbmd0aCBpcyBzdGlsbCB2YWxpZCBhZnRlciBwZXJmb3JtaW5nIGVmZmVj dHMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE4OTky MgorICAgICAgICA8cmRhcjovL3Byb2JsZW0vNDQ2NTEyNzU+CisKKyAgICAgICAgUmV2aWV3ZWQg YnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBzdHJlc3MvYXJyYXktaW5kZXhvZi1mYXN0 LXBhdGgtZWZmZWN0cy5qczogQWRkZWQuCisgICAgICAgIChhcnIuaW5kZXhPZi5uZXcuT2JqZWN0 LnZhbHVlT2YpOgorICAgICAgICAoYXJyLmluZGV4T2YubmV3Lk9iamVjdCk6CisKIDIwMTgtMDkt MjIgIFNhYW0gYmFyYXRpICA8c2JhcmF0aUBhcHBsZS5jb20+CiAKICAgICAgICAgVGhlIHNhbXBs aW5nIHNob3VsZCBub3QgdXNlIFN0cm9uZzxDb2RlQmxvY2s+IGluIGl0cyBtYWNoaW5lTG9jYXRp b24gZmllbGQKSW5kZXg6IEpTVGVzdHMvc3RyZXNzL2FycmF5LWluZGV4b2YtZmFzdC1wYXRoLWVm ZmVjdHMuanMKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PQotLS0gSlNUZXN0cy9zdHJlc3MvYXJyYXktaW5kZXhvZi1mYXN0 LXBhdGgtZWZmZWN0cy5qcwkobm9uZXhpc3RlbnQpCisrKyBKU1Rlc3RzL3N0cmVzcy9hcnJheS1p bmRleG9mLWZhc3QtcGF0aC1lZmZlY3RzLmpzCSh3b3JraW5nIGNvcHkpCkBAIC0wLDAgKzEsMTEg QEAKKy8vIFRoaXMgc2hvdWxkbid0IGNyYXNoIHdoZW4gcnVubmluZyB3aXRoIEFTQU4uCitsZXQg YXJyID0gW107Citmb3IgKHZhciBpID0gMDsgaSA8IDEwMDAwMDA7IGkrKykKKyAgICBhcnJbaV0g PSBbXTsKKworYXJyLmluZGV4T2YobmV3IE9iamVjdCgpLCB7CisgICAgdmFsdWVPZjogZnVuY3Rp b24gKCkgeworICAgICAgICBhcnIubGVuZ3RoID0gMDsKKyAgICAgICAgcmV0dXJuIDA7CisgICAg fQorfSk7CkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0K LS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDIzNjQxNikKKysr IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cJKHdvcmtpbmcgY29weSkKQEAgLTEsMyAr MSwxOSBAQAorMjAxOC0wOS0yNCAgU2FhbSBCYXJhdGkgIDxzYmFyYXRpQGFwcGxlLmNvbT4KKwor ICAgICAgICBBcnJheS5wcm90b3R5cGUuaW5kZXhPZiBmYXN0IHBhdGggbmVlZHMgdG8gZW5zdXJl IHRoZSBsZW5ndGggaXMgc3RpbGwgdmFsaWQgYWZ0ZXIgcGVyZm9ybWluZyBlZmZlY3RzCisgICAg ICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xODk5MjIKKyAgICAg ICAgPHJkYXI6Ly9wcm9ibGVtLzQ0NjUxMjc1PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgIFRoZSBpbXBsZW1lbnRhdGlvbiB3YXMgZmlyc3QgZ2V0dGlu ZyB0aGUgbGVuZ3RoIHRvIGl0ZXJhdGUgdXAgdG8sCisgICAgICAgIHRoZW4gZ2V0dGluZyB0aGUg c3RhcnRpbmcgaW5kZXguIEhvd2V2ZXIsIGdldHRpbmcgdGhlIHN0YXJ0aW5nCisgICAgICAgIGlu ZGV4IG1heSBwZXJmb3JtIGVmZmVjdHMuIEUuZywgaXQgY291bGQgY2hhbmdlIHRoZSBsZW5ndGgg b2YgdGhlCisgICAgICAgIGFycmF5LiBUaGlzIGNoYW5nZXMgaXQgc28gd2UgdmVyaWZ5IHRoZSBs ZW5ndGggaXMgc3RpbGwgdmFsaWQuCisKKyAgICAgICAgKiBydW50aW1lL0FycmF5UHJvdG90eXBl LmNwcDoKKyAgICAgICAgKEpTQzo6YXJyYXlQcm90b0Z1bmNJbmRleE9mKToKKwogMjAxOC0wOS0y MiAgU2FhbSBiYXJhdGkgIDxzYmFyYXRpQGFwcGxlLmNvbT4KIAogICAgICAgICBUaGUgc2FtcGxp bmcgc2hvdWxkIG5vdCB1c2UgU3Ryb25nPENvZGVCbG9jaz4gaW4gaXRzIG1hY2hpbmVMb2NhdGlv biBmaWVsZApJbmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvQXJyYXlQcm90b3R5 cGUuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0FycmF5 UHJvdG90eXBlLmNwcAkocmV2aXNpb24gMjM2NDE2KQorKysgU291cmNlL0phdmFTY3JpcHRDb3Jl L3J1bnRpbWUvQXJyYXlQcm90b3R5cGUuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xMTY5LDcgKzEx NjksOSBAQCBFbmNvZGVkSlNWYWx1ZSBKU0NfSE9TVF9DQUxMIGFycmF5UHJvdG9GCiAKICAgICBp ZiAoaXNKU0FycmF5KHRoaXNPYmplY3QpKSB7CiAgICAgICAgIEpTQXJyYXkqIGFycmF5ID0gYXNB cnJheSh0aGlzT2JqZWN0KTsKLSAgICAgICAgaWYgKGFycmF5LT5jYW5Eb0Zhc3RJbmRleGVkQWNj ZXNzKHZtKSkgeworICAgICAgICBib29sIGNhbkRvRmFzdFBhdGggPSBhcnJheS0+Y2FuRG9GYXN0 SW5kZXhlZEFjY2Vzcyh2bSkKKyAgICAgICAgICAgICYmIGFycmF5LT5nZXRBcnJheUxlbmd0aCgp ID09IGxlbmd0aDsgLy8gVGhlIGVmZmVjdHMgaW4gZ2V0dGluZyBgaW5kZXhgIGNvdWxkIGhhdmUg Y2hhbmdlZCB0aGUgbGVuZ3RoIG9mIHRoaXMgYXJyYXkuCisgICAgICAgIGlmIChjYW5Eb0Zhc3RQ YXRoKSB7CiAgICAgICAgICAgICBzd2l0Y2ggKGFycmF5LT5pbmRleGluZ1R5cGUoKSkgewogICAg ICAgICAgICAgY2FzZSBBTExfSU5UMzJfSU5ERVhJTkdfVFlQRVM6IHsKICAgICAgICAgICAgICAg ICBpZiAoIXNlYXJjaEVsZW1lbnQuaXNOdW1iZXIoKSkK 350689 2018-09-24 14:45:20 -0700 2018-09-24 16:06:01 -0700 patch for landing c-backup.diff text/plain 3830 saam SW5kZXg6IEpTVGVzdHMvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIEpTVGVzdHMvQ2hhbmdlTG9n CShyZXZpc2lvbiAyMzY0MTgpCisrKyBKU1Rlc3RzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpA QCAtMSwzICsxLDE0IEBACisyMDE4LTA5LTI0ICBTYWFtIEJhcmF0aSAgPHNiYXJhdGlAYXBwbGUu Y29tPgorCisgICAgICAgIEFycmF5LnByb3RvdHlwZS5pbmRleE9mIGZhc3QgcGF0aCBuZWVkcyB0 byBlbnN1cmUgdGhlIGxlbmd0aCBpcyBzdGlsbCB2YWxpZCBhZnRlciBwZXJmb3JtaW5nIGVmZmVj dHMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE4OTky MgorICAgICAgICA8cmRhcjovL3Byb2JsZW0vNDQ2NTEyNzU+CisKKyAgICAgICAgUmV2aWV3ZWQg YnkgTWFyayBMYW0uCisKKyAgICAgICAgKiBzdHJlc3MvYXJyYXktaW5kZXhvZi1mYXN0LXBhdGgt ZWZmZWN0cy5qczogQWRkZWQuCisgICAgICAgICogc3RyZXNzL2FycmF5LWluZGV4b2YtY2FjaGVk LWxlbmd0aC5qczogQWRkZWQuCisKIDIwMTgtMDktMjIgIFNhYW0gYmFyYXRpICA8c2JhcmF0aUBh cHBsZS5jb20+CiAKICAgICAgICAgVGhlIHNhbXBsaW5nIHNob3VsZCBub3QgdXNlIFN0cm9uZzxD b2RlQmxvY2s+IGluIGl0cyBtYWNoaW5lTG9jYXRpb24gZmllbGQKSW5kZXg6IEpTVGVzdHMvc3Ry ZXNzL2FycmF5LWluZGV4b2YtY2FjaGVkLWxlbmd0aC5qcwo9PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBKU1Rlc3Rz L3N0cmVzcy9hcnJheS1pbmRleG9mLWNhY2hlZC1sZW5ndGguanMJKG5vbmV4aXN0ZW50KQorKysg SlNUZXN0cy9zdHJlc3MvYXJyYXktaW5kZXhvZi1jYWNoZWQtbGVuZ3RoLmpzCSh3b3JraW5nIGNv cHkpCkBAIC0wLDAgKzEsMjQgQEAKK2Z1bmN0aW9uIGFzc2VydChiKSB7CisgICAgaWYgKCFiKQor ICAgICAgICB0aHJvdyBuZXcgRXJyb3I7CisKK30KKworY29uc3Qgb3JpZ2luYWxMZW5ndGggPSAx MDAwMDsKK2xldCBhcnIgPSBuZXcgUHJveHkoW10sIHsKKyAgICBoYXMoLi4uYXJncykgeworICAg ICAgICBhc3NlcnQocGFyc2VJbnQoYXJnc1sxXSkgPCBvcmlnaW5hbExlbmd0aCk7CisgICAgICAg IGFzc2VydChhcmdzWzBdLmxlbmd0aCAtIDEwID09PSBvcmlnaW5hbExlbmd0aCk7CisgICAgICAg IHJldHVybiBSZWZsZWN0LmhhcyguLi5hcmdzKTsKKyAgICB9Cit9KTsKKworZm9yICh2YXIgaSA9 IDA7IGkgPCBvcmlnaW5hbExlbmd0aDsgaSsrKQorICAgIGFycltpXSA9IFtdOworCithcnIuaW5k ZXhPZihuZXcgT2JqZWN0KCksIHsKKyAgICB2YWx1ZU9mOiBmdW5jdGlvbiAoKSB7CisgICAgICAg IGFyci5sZW5ndGggKz0gMTA7CisgICAgICAgIHJldHVybiAwOworICAgIH0KK30pOwpJbmRleDog SlNUZXN0cy9zdHJlc3MvYXJyYXktaW5kZXhvZi1mYXN0LXBhdGgtZWZmZWN0cy5qcwo9PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09Ci0tLSBKU1Rlc3RzL3N0cmVzcy9hcnJheS1pbmRleG9mLWZhc3QtcGF0aC1lZmZlY3RzLmpz CShub25leGlzdGVudCkKKysrIEpTVGVzdHMvc3RyZXNzL2FycmF5LWluZGV4b2YtZmFzdC1wYXRo LWVmZmVjdHMuanMJKHdvcmtpbmcgY29weSkKQEAgLTAsMCArMSwxMSBAQAorLy8gVGhpcyBzaG91 bGRuJ3QgY3Jhc2ggd2hlbiBydW5uaW5nIHdpdGggQVNBTi4KK2xldCBhcnIgPSBbXTsKK2ZvciAo dmFyIGkgPSAwOyBpIDwgMTAwMDAwMDsgaSsrKQorICAgIGFycltpXSA9IFtdOworCithcnIuaW5k ZXhPZihuZXcgT2JqZWN0KCksIHsKKyAgICB2YWx1ZU9mOiBmdW5jdGlvbiAoKSB7CisgICAgICAg IGFyci5sZW5ndGggPSAwOworICAgICAgICByZXR1cm4gMDsKKyAgICB9Cit9KTsKSW5kZXg6IFNv dXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL0phdmFT Y3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjM2NDE2KQorKysgU291cmNlL0phdmFTY3Jp cHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE5IEBACisyMDE4LTA5 LTI0ICBTYWFtIEJhcmF0aSAgPHNiYXJhdGlAYXBwbGUuY29tPgorCisgICAgICAgIEFycmF5LnBy b3RvdHlwZS5pbmRleE9mIGZhc3QgcGF0aCBuZWVkcyB0byBlbnN1cmUgdGhlIGxlbmd0aCBpcyBz dGlsbCB2YWxpZCBhZnRlciBwZXJmb3JtaW5nIGVmZmVjdHMKKyAgICAgICAgaHR0cHM6Ly9idWdz LndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE4OTkyMgorICAgICAgICA8cmRhcjovL3Byb2Js ZW0vNDQ2NTEyNzU+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTWFyayBMYW0uCisKKyAgICAgICAg VGhlIGltcGxlbWVudGF0aW9uIHdhcyBmaXJzdCBnZXR0aW5nIHRoZSBsZW5ndGggdG8gaXRlcmF0 ZSB1cCB0bywKKyAgICAgICAgdGhlbiBnZXR0aW5nIHRoZSBzdGFydGluZyBpbmRleC4gSG93ZXZl ciwgZ2V0dGluZyB0aGUgc3RhcnRpbmcKKyAgICAgICAgaW5kZXggbWF5IHBlcmZvcm0gZWZmZWN0 cy4gZS5nLCBpdCBjb3VsZCBjaGFuZ2UgdGhlIGxlbmd0aCBvZiB0aGUKKyAgICAgICAgYXJyYXku IFRoaXMgY2hhbmdlcyBpdCBzbyB3ZSB2ZXJpZnkgdGhlIGxlbmd0aCBpcyBzdGlsbCB2YWxpZC4K KworICAgICAgICAqIHJ1bnRpbWUvQXJyYXlQcm90b3R5cGUuY3BwOgorICAgICAgICAoSlNDOjph cnJheVByb3RvRnVuY0luZGV4T2YpOgorCiAyMDE4LTA5LTIyICBTYWFtIGJhcmF0aSAgPHNiYXJh dGlAYXBwbGUuY29tPgogCiAgICAgICAgIFRoZSBzYW1wbGluZyBzaG91bGQgbm90IHVzZSBTdHJv bmc8Q29kZUJsb2NrPiBpbiBpdHMgbWFjaGluZUxvY2F0aW9uIGZpZWxkCkluZGV4OiBTb3VyY2Uv SmF2YVNjcmlwdENvcmUvcnVudGltZS9BcnJheVByb3RvdHlwZS5jcHAKPT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0g U291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvQXJyYXlQcm90b3R5cGUuY3BwCShyZXZpc2lv biAyMzY0MTYpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9BcnJheVByb3RvdHlw ZS5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTExNjksNyArMTE2OSw5IEBAIEVuY29kZWRKU1ZhbHVl IEpTQ19IT1NUX0NBTEwgYXJyYXlQcm90b0YKIAogICAgIGlmIChpc0pTQXJyYXkodGhpc09iamVj dCkpIHsKICAgICAgICAgSlNBcnJheSogYXJyYXkgPSBhc0FycmF5KHRoaXNPYmplY3QpOwotICAg ICAgICBpZiAoYXJyYXktPmNhbkRvRmFzdEluZGV4ZWRBY2Nlc3Modm0pKSB7CisgICAgICAgIGJv b2wgY2FuRG9GYXN0UGF0aCA9IGFycmF5LT5jYW5Eb0Zhc3RJbmRleGVkQWNjZXNzKHZtKQorICAg ICAgICAgICAgJiYgYXJyYXktPmdldEFycmF5TGVuZ3RoKCkgPT0gbGVuZ3RoOyAvLyBUaGUgZWZm ZWN0cyBpbiBnZXR0aW5nIGBpbmRleGAgY291bGQgaGF2ZSBjaGFuZ2VkIHRoZSBsZW5ndGggb2Yg dGhpcyBhcnJheS4KKyAgICAgICAgaWYgKGNhbkRvRmFzdFBhdGgpIHsKICAgICAgICAgICAgIHN3 aXRjaCAoYXJyYXktPmluZGV4aW5nVHlwZSgpKSB7CiAgICAgICAgICAgICBjYXNlIEFMTF9JTlQz Ml9JTkRFWElOR19UWVBFUzogewogICAgICAgICAgICAgICAgIGlmICghc2VhcmNoRWxlbWVudC5p c051bWJlcigpKQo=