Bug 189922

Summary: Array.prototype.indexOf fast path needs to ensure the length is still valid after performing effects
Product: WebKit Reporter: Saam Barati <saam>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, fpizlo, ggaren, gskachkov, keith_miller, mark.lam, msaboff, rmorisset, ticaiolima, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
mark.lam: review+
patch for landing none

Saam Barati
Reported 2018-09-24 11:48:27 PDT
...
Attachments
patch (3.04 KB, patch)
2018-09-24 12:39 PDT, Saam Barati 		mark.lam: review+
DetailsFormatted DiffDiff
patch for landing (3.74 KB, patch)
2018-09-24 14:45 PDT, Saam Barati 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Saam Barati
Comment 1 2018-09-24 11:52:17 PDT
Patch forthcoming
Saam Barati
Comment 2 2018-09-24 12:36:29 PDT
<rdar://problem/44651275>
Saam Barati
Comment 3 2018-09-24 12:39:33 PDT
Created attachment 350668 [details] patch
Mark Lam
Comment 4 2018-09-24 13:16:27 PDT
Comment on attachment 350668 [details] 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?
Saam Barati
Comment 5 2018-09-24 13:22:21 PDT
Comment on attachment 350668 [details] 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
Saam Barati
Comment 6 2018-09-24 14:45:20 PDT
Created attachment 350689 [details] patch for landing
WebKit Commit Bot
Comment 7 2018-09-24 16:06:01 PDT
Comment on attachment 350689 [details] patch for landing Clearing flags on attachment: 350689 Committed r236437: <https://trac.webkit.org/changeset/236437>
WebKit Commit Bot
Comment 8 2018-09-24 16:06:03 PDT
All reviewed patches have been landed. Closing bug.
Yusuke Suzuki
Comment 9 2018-09-25 21:46:29 PDT
Oops, nice!
Note You need to log in before you can comment on or make changes to this bug.