Bug 216121 – Array.prototype.push should always perform [[Set]] in strict mode

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

[Alexey Shvayka]

Array.prototype.push should always perform [[Set]] in strict mode

[Alexey Shvayka]

Created attachment 407877 [details]
Patch

[Alexey Shvayka]

(In reply to Alexey Shvayka from comment #1)
> Created attachment 407877 [details]
> Patch

Warmed-up runs, --outer 50:

                         r266507                     patch

new-array-push        6.5605+-0.4641            6.2634+-0.4522          might be 1.0474x faster
array-push-0        350.3329+-2.1429          348.3786+-1.2194        
array-push-1        733.3222+-13.1726    ?    739.4494+-13.0772       ? might be 1.0084x slower
array-push-2        822.0986+-4.4731     ?    822.2864+-3.1685        ?
array-push-3        836.1861+-24.9409         832.5322+-23.3867         might be 1.0044x faster

---

I've vetted all puts via PutPropertySlot in JSC: other usages set strict mode correctly.

[Darin Adler]

Do you understand why the JSC tests are failing on EWS?

[Alexey Shvayka]

(In reply to Darin Adler from comment #3)
> Do you understand why the JSC tests are failing on EWS?

Igalia's servers seem to be down. I don't think it is related.

[Alexey Shvayka]

EWS is all-green now. Thank you for review!

[EWS]

Committed r266581: <https://trac.webkit.org/changeset/266581>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407877 [details].

[Radar WebKit Bug Importer]

<rdar://problem/68330312>

[Darin Adler]

Comment on attachment 407877 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407877&action=review

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:919
> +        thisObj->putByIndexInline(globalObject, static_cast<uint64_t>(length + n), callFrame->uncheckedArgument(n), true);

Sorry I didn’t notice this when reviewing.

Can we remove this unnecessary static_cast? It caused me concern just now: I was worried that the addition was incorrectly performed in 32 bits with possible overflow before promoting the result to 64 bits. But it’s not, because length is already uint64_t, and for the same reason the static_cast is not needed or helpful.

[Alexey Shvayka]

Committed r266641: <https://trac.webkit.org/changeset/266641>

[Alexey Shvayka]

(In reply to Darin Adler from comment #8)
> Can we remove this unnecessary static_cast? It caused me concern just now: I
> was worried that the addition was incorrectly performed in 32 bits with
> possible overflow before promoting the result to 64 bits. But it’s not,
> because length is already uint64_t, and for the same reason the static_cast
> is not needed or helpful.

Thank you for the follow-up!
I've also checked another casts in ArrayPrototype.cpp, all seem necessary.