Bug 216121 - Array.prototype.push should always perform [[Set]] in strict mode
Summary: Array.prototype.push should always perform [[Set]] in strict mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Trivial
Assignee: Alexey Shvayka
URL:
Keywords: InRadar, WebExposed
Depends on:
Blocks:
 
Reported: 2020-09-03 04:58 PDT by Alexey Shvayka
Modified: 2020-09-04 15:37 PDT (History)
9 users (show)

See Also:

Attachments
Patch (4.02 KB, patch)
2020-09-03 06:05 PDT, Alexey Shvayka 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Shvayka 2020-09-03 04:58:55 PDT 
Array.prototype.push should always perform [[Set]] in strict mode
Comment 1 Alexey Shvayka 2020-09-03 06:05:41 PDT 
Created attachment 407877 [details]
Patch
Comment 2 Alexey Shvayka 2020-09-03 06:10:41 PDT 
(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.
Comment 3 Darin Adler 2020-09-03 09:51:15 PDT 
Do you understand why the JSC tests are failing on EWS?
Comment 4 Alexey Shvayka 2020-09-03 09:53:05 PDT 
(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.
Comment 5 Alexey Shvayka 2020-09-04 00:45:52 PDT 
EWS is all-green now. Thank you for review!
Comment 6 EWS 2020-09-04 01:05:40 PDT 
Committed r266581: <https://trac.webkit.org/changeset/266581>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407877 [details].
Comment 7 Radar WebKit Bug Importer 2020-09-04 01:06:16 PDT 
<rdar://problem/68330312>
Comment 8 Darin Adler 2020-09-04 12:30:18 PDT 
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.
Comment 9 Alexey Shvayka 2020-09-04 15:34:00 PDT 
Committed r266641: <https://trac.webkit.org/changeset/266641>
Comment 10 Alexey Shvayka 2020-09-04 15:37:18 PDT 
(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.