Skip to content

Fix Object.assign with array target losing length update (#2126)#2399

Merged
gbrail merged 2 commits into
mozilla:masterfrom
Finomosec:fix/object-assign-array-target
May 16, 2026
Merged

Fix Object.assign with array target losing length update (#2126)#2399
gbrail merged 2 commits into
mozilla:masterfrom
Finomosec:fix/object-assign-array-target

Conversation

@Finomosec

@Finomosec Finomosec commented May 11, 2026

Copy link
Copy Markdown
Contributor

Summary

Object.assign([], src) returned an empty array instead of a copy of src's
enumerable own properties. The integer-keyed property writes went through
AbstractEcmaObjectOperations.putScriptableObject.putOwnProperty
putImpl, which bypasses NativeArray.put(int) and so the array's length
was never updated. The slots got stored but stayed invisible to iteration
and serialization.

Reproducer (failing before this PR):

const arr = ['bla', 'blubb'];
const clone = Object.assign([], arr);
JSON.stringify(clone);      // → "[]"   (expected: ["bla","blubb"])
clone.length;               // →  0     (expected: 2)

Fix

Short-circuit to targetObj.put(...) in NativeObject.js_assign when the
target is a NativeArray, so the array's own put-path (which maintains
length) is used. Same special-case for the String-key path, since numeric
string keys like "0", "1" are also indexed properties for arrays.

The non-array code path stays exactly as before.

Tests

New ObjectAssignArrayTargetTest (32 cases) covering:

  • the original reproducer, length update, indexed access
  • pre-filled targets, sparse sources (holes skipped), multiple sources
  • mix with push / spread / forEach / map / reduce / splice
  • pre-sized arrays via new Array(n) (length must not shrink)
  • nested Object.assign chains, 100-entry volume test
  • object-target baseline (must remain unchanged)
  • spread, basic array/object destructuring (the supported subset)

All tests run in both interpreter and compiled mode via
Utils.assertWithAllModes_ES6.

Context

Refs #2126.

@Finomosec
Finomosec force-pushed the fix/object-assign-array-target branch from 86094e5 to a58aa1f Compare May 11, 2026 09:58
@Finomosec

Copy link
Copy Markdown
Contributor Author

For reviewer context: this PR implements exactly the approach @aardvark179 outlined in #2126 (comment):

We can't simply subclass putOwnProperty for NativeArray because that causes infinite recursion due to the way things are currently written. I think the only short term fix for this bug is to special case things for arrays.

Scope is intentionally narrow — only the NativeArray target case in js_assign. Other exotic object types that may share the same problem (raised earlier by @aardvark179 in the same issue) are left for the broader put/[[Set]] refactor that's planned alongside the scope split for 2.0.

Also pushed an update that adds the exact reproducer from the original issue (Object.assign([4,5,7], [1,2,4])[1,2,4]) as its own test case, so the regression is documented verbatim.

@rbri

rbri commented May 11, 2026

Copy link
Copy Markdown
Collaborator

@Finomosec pleasr run

./gradlew spotlessApply

to correct the formatting

Finomosec added 2 commits May 15, 2026 14:49
Object.assign([], src) returned an empty array instead of a copy of src's
enumerable own properties. Integer-keyed property writes went through
AbstractEcmaObjectOperations.put -> ScriptableObject.putOwnProperty ->
putImpl, which bypasses NativeArray.put(int) and so the array's length
was never updated. The slots got stored but stayed invisible to iteration
and serialization.

Short-circuit to the array's own put() when the target is a NativeArray so
length is maintained. Same special-case for the String-key code path, since
NativeArray treats numeric string keys ("0", "1", ...) as indexed properties
that affect length.

Tests cover the original report, length-maintenance, sparse-source skipping
holes, multi-source assigns, mix with push/spread/forEach/map/reduce/splice,
pre-sized arrays via the Array constructor, and the object-target baseline
(must remain unchanged). All tests run in both interpreter and compiled
mode via Utils.assertWithAllModes_ES6.

Fixes mozilla#2126
@Finomosec
Finomosec force-pushed the fix/object-assign-array-target branch from 5b5b388 to 77078c1 Compare May 15, 2026 12:50
@gbrail

gbrail commented May 16, 2026

Copy link
Copy Markdown
Collaborator

Looks great, going to give the others a minute to take a look, thanks!

@gbrail

gbrail commented May 16, 2026

Copy link
Copy Markdown
Collaborator

I rebased the tests and ran them locally, and no test262 tests failed, but sadly no new tests passed either. I still think we should merge this though!

@rbri

rbri commented May 16, 2026

Copy link
Copy Markdown
Collaborator

@gbrail did something similar, same result.

@gbrail

gbrail commented May 16, 2026

Copy link
Copy Markdown
Collaborator

Still a good fix though -- thanks!

@gbrail
gbrail merged commit f906693 into mozilla:master May 16, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants