[webkit-reviews] review granted: [Bug 189922] Array.prototype.indexOf fast path needs to ensure the length is still valid after performing effects : [Attachment 350668] patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Saam Barati <sbarati at apple.com>'s
request for review:
Bug 189922: Array.prototype.indexOf fast path needs to ensure the length is
still valid after performing effects
https://bugs.webkit.org/show_bug.cgi?id=189922

Attachment 350668: patch

https://bugs.webkit.org/attachment.cgi?id=350668&action=review




--- Comment #4 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 350668
  --> https://bugs.webkit.org/attachment.cgi?id=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?