Bugzilla

Oh, this is a good catch. It's a regression from bug 1749460, although reading through the pre-existing arguments object code while figuring this out also makes me want to blame the general design of arguments object property lookup.

```
function foo() {
  "use strict"; // Ensure that the arguments object is unmapped.
  Object.defineProperty(arguments, 0, {value: 2});
  arguments[0] = 3;
  return Object(...arguments);
}

print(foo(1));
```
The problem is that [OptimizeArgumentsSpreadCall](https://searchfox.org/mozilla-central/rev/5ce782034b736293836a2850ab698e149ca04657/js/src/vm/Interpreter.cpp#5189) checks whether the length or iterator has been overridden for an arguments object, or whether any element has been deleted, and assumes that this is sufficient to justify calling `ArrayFromArgumentsObject` and copying values from the ArgumentsData array.

This is true if all updates to the elements of the arguments object use SetProperty (eg `arguments[0] = 3`): the resolve hook for UnmappedArgumentsObject and the [custom data property code in NativeSetExistingDataProperty](https://searchfox.org/mozilla-central/rev/5ce782034b736293836a2850ab698e149ca04657/js/src/vm/NativeObject.cpp#2289-2292) conspire to update the ArgumentsData array directly. 

However, if we use `defineProperty` to redefine an element of an unmapped arguments object, the `hasOverriddenElement` flag is [set](https://searchfox.org/mozilla-central/rev/5ce782034b736293836a2850ab698e149ca04657/js/src/vm/NativeObject.cpp#1463), but we don't have any equivalent to the special casing in NativeSetExistingDataProperty. Instead, the updated value is stored in a slot of the arguments object (as if it were a sparse element). If we don't check the overridden element flag in OptimizeArgumentsSpreadCall, then we will read the stale value out of the ArgumentsData, instead of the correct value in the slot. (After the slot has been created, mutation using SetProperty will also update the existing slot instead of using the resolve hook.)

The immediate fix is to change OptimizeArgumentsSpreadCall to check `args->hasOverriddenElement()` instead of `args->isAnyElementDeleted()`. In the long run we might also want to clean up some of the arguments object code. For example, it's not clear to me that there's any value in storing the arguments for an unmapped arguments object in a separate array, instead of storing them as regular elements. I also have to believe that there's a cleaner way to make this all work than our current mess of hooks, flags, and special cases.

Oh, this is a good catch. It's a regression from bug 1740737, although reading through the pre-existing arguments object code while figuring this out also makes me want to blame the general design of arguments object property lookup.

```
function foo() {
  "use strict"; // Ensure that the arguments object is unmapped.
  Object.defineProperty(arguments, 0, {value: 2});
  arguments[0] = 3;
  return Object(...arguments);
}

print(foo(1));
```
The problem is that [OptimizeArgumentsSpreadCall](https://searchfox.org/mozilla-central/rev/5ce782034b736293836a2850ab698e149ca04657/js/src/vm/Interpreter.cpp#5189) checks whether the length or iterator has been overridden for an arguments object, or whether any element has been deleted, and assumes that this is sufficient to justify calling `ArrayFromArgumentsObject` and copying values from the ArgumentsData array.

This is true if all updates to the elements of the arguments object use SetProperty (eg `arguments[0] = 3`): the resolve hook for UnmappedArgumentsObject and the [custom data property code in NativeSetExistingDataProperty](https://searchfox.org/mozilla-central/rev/5ce782034b736293836a2850ab698e149ca04657/js/src/vm/NativeObject.cpp#2289-2292) conspire to update the ArgumentsData array directly. 

However, if we use `defineProperty` to redefine an element of an unmapped arguments object, the `hasOverriddenElement` flag is [set](https://searchfox.org/mozilla-central/rev/5ce782034b736293836a2850ab698e149ca04657/js/src/vm/NativeObject.cpp#1463), but we don't have any equivalent to the special casing in NativeSetExistingDataProperty. Instead, the updated value is stored in a slot of the arguments object (as if it were a sparse element). If we don't check the overridden element flag in OptimizeArgumentsSpreadCall, then we will read the stale value out of the ArgumentsData, instead of the correct value in the slot. (After the slot has been created, mutation using SetProperty will also update the existing slot instead of using the resolve hook.)

The immediate fix is to change OptimizeArgumentsSpreadCall to check `args->hasOverriddenElement()` instead of `args->isAnyElementDeleted()`. In the long run we might also want to clean up some of the arguments object code. For example, it's not clear to me that there's any value in storing the arguments for an unmapped arguments object in a separate array, instead of storing them as regular elements. I also have to believe that there's a cleaner way to make this all work than our current mess of hooks, flags, and special cases.