Bug 189922 – Array.prototype.indexOf fast path needs to ensure the length is still valid after performing effects

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Saam Barati]

Patch forthcoming

[Saam Barati]

<rdar://problem/44651275>

[Saam Barati]

Created attachment 350668 [details]
patch

[Mark Lam]

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 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]

Created attachment 350689 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 350689 [details]
patch for landing

Clearing flags on attachment: 350689

Committed r236437: <https://trac.webkit.org/changeset/236437>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yusuke Suzuki]

Oops, nice!