Bug 1762575 - Part 1: Use slow-path when any arguments element is overridden. r=iain

`UnmappedArgumentsObject` doesn't mark an element as deleted when it is
redefined through `Object.defineProperty()`, that means the current element
value can only be read through `ArgumentsObject::element()` when the
`ELEMENT_OVERRIDDEN_BIT` isn't set.

Alternatively to this patch, we could also add a `ObjectOps::defineProperty`
hook for `UnmappedArgumentsObject` to properly update the element value instead
of redefining the property. But reassigning argument elements should be
relatively uncommon, so it should be fine to just always take the slow-path
when `ELEMENT_OVERRIDDEN_BIT` is set.

The tests in "bug1762575-1.js" work on mapped arguments objects and already
pass without this patch. "bug1762575-2.js" uses mapped arguments objects and
only passes with this patch. "bug1762575-3.js" tests some implementation
details when arguments object keep a strong reference to their elements.

Differential Revision: https://phabricator.services.mozilla.com/D142666

js/src/builtin/Array.cpp

@@ -3475,7 +3475,7 @@ static bool SliceSparse(JSContext* cx, HandleObject obj, uint64_t begin,
 static JSObject* SliceArguments(JSContext* cx, Handle<ArgumentsObject*> argsobj,
                                 uint32_t begin, uint32_t count) {
   MOZ_ASSERT(!argsobj->hasOverriddenLength() &&
-             !argsobj->isAnyElementDeleted());
+             !argsobj->hasOverriddenElement());
   MOZ_ASSERT(begin + count <= argsobj->initialLength());
 
   ArrayObject* result = NewDenseFullyAllocatedArray(cx, count);
@@ -3532,7 +3532,7 @@ static bool ArraySliceOrdinary(JSContext* cx, HandleObject obj, uint64_t begin,
 
   if (obj->is<ArgumentsObject>()) {
     Handle<ArgumentsObject*> argsobj = obj.as<ArgumentsObject>();
-    if (!argsobj->hasOverriddenLength() && !argsobj->isAnyElementDeleted()) {
+    if (!argsobj->hasOverriddenLength() && !argsobj->hasOverriddenElement()) {
       MOZ_ASSERT(begin <= UINT32_MAX, "begin is limited by |argsobj|'s length");
       JSObject* narr = SliceArguments(cx, argsobj, uint32_t(begin), count);
       if (!narr) {

js/src/jit-test/tests/arguments/bug1762575-1.js

@@ -0,0 +1,44 @@
+// Tests on mapped arguments objects.
+
+function ArraySlice() {
+  Object.defineProperty(arguments, 0, {value: 1});
+  var result = Array.prototype.slice.call(arguments);
+  assertEq(result[0], 1);
+}
+ArraySlice(0);
+
+function ArrayShift() {
+  Object.defineProperty(arguments, 0, {value: 1});
+  var result = Array.prototype.shift.call(arguments);
+  assertEq(result, 1);
+}
+ArrayShift(0);
+
+function ArrayPop() {
+  Object.defineProperty(arguments, 0, {value: 1});
+  var result = Array.prototype.pop.call(arguments);
+  assertEq(result, 1);
+}
+ArrayPop(0);
+
+function ArrayJoin() {
+  Object.defineProperty(arguments, 0, {value: 1});
+  var result = Array.prototype.join.call(arguments);
+  assertEq(result, "1");
+}
+ArrayJoin(0);
+
+function ArrayIncludes() {
+  Object.defineProperty(arguments, 0, {value: 1});
+  var result = Array.prototype.includes.call(arguments, 1);
+  assertEq(result, true);
+}
+ArrayIncludes(0);
+
+function FunctionApply() {
+  Object.defineProperty(arguments, 0, {value: 1});
+  var id = x => x;
+  var result = id.apply(null, arguments);
+  assertEq(result, 1);
+}
+FunctionApply(0);

js/src/jit-test/tests/arguments/bug1762575-2.js

@@ -0,0 +1,50 @@
+// Tests on unmapped arguments objects.
+
+function ArraySlice() {
+  "use strict";
+  Object.defineProperty(arguments, 0, {value: 1});
+  var result = Array.prototype.slice.call(arguments);
+  assertEq(result[0], 1);
+}
+ArraySlice(0);
+
+function ArrayShift() {
+  "use strict";
+  Object.defineProperty(arguments, 0, {value: 1});
+  var result = Array.prototype.shift.call(arguments);
+  assertEq(result, 1);
+}
+ArrayShift(0);
+
+function ArrayPop() {
+  "use strict";
+  Object.defineProperty(arguments, 0, {value: 1});
+  var result = Array.prototype.pop.call(arguments);
+  assertEq(result, 1);
+}
+ArrayPop(0);
+
+function ArrayJoin() {
+  "use strict";
+  Object.defineProperty(arguments, 0, {value: 1});
+  var result = Array.prototype.join.call(arguments);
+  assertEq(result, "1");
+}
+ArrayJoin(0);
+
+function ArrayIncludes() {
+  "use strict";
+  Object.defineProperty(arguments, 0, {value: 1});
+  var result = Array.prototype.includes.call(arguments, 1);
+  assertEq(result, true);
+}
+ArrayIncludes(0);
+
+function FunctionApply() {
+  "use strict";
+  Object.defineProperty(arguments, 0, {value: 1});
+  var id = x => x;
+  var result = id.apply(null, arguments);
+  assertEq(result, 1);
+}
+FunctionApply(0);

js/src/jit-test/tests/arguments/bug1762575-3.js

@@ -0,0 +1,89 @@
+// Test implementation details when arguments object keep strong references
+// to their elements.
+
+function checkWeakRef(f, isCleared = true) {
+  let [wr, args] = f({});
+
+  assertEq(wr.deref() !== undefined, true);
+
+  clearKeptObjects();
+  gc();
+
+  // In an ideal world, the reference is always cleared. But in specific
+  // circumstances we're currently keeping the reference alive. Should this
+  // ever change, feel free to update this test case accordingly. IOW having
+  // |isCleared == false| is okay for spec correctness, but it isn't optimal.
+  assertEq(wr.deref() === undefined, isCleared);
+}
+
+checkWeakRef(function() {
+  // Create a weak-ref for the first argument.
+  let wr = new WeakRef(arguments[0]);
+
+  // Clear the reference from the arguments object.
+  arguments[0] = null;
+
+  // Let the arguments object escape.
+  return [wr, arguments];
+});
+
+checkWeakRef(function() {
+  // Create a weak-ref for the first argument.
+  let wr = new WeakRef(arguments[0]);
+
+  // Clear the reference from the arguments object.
+  Object.defineProperty(arguments, 0, {value: null});
+
+  // Let the arguments object escape.
+  return [wr, arguments];
+});
+
+checkWeakRef(function self() {
+  // Create a weak-ref for the first argument.
+  let wr = new WeakRef(arguments[0]);
+
+  // Delete operation doesn't clear the reference!
+  delete arguments[0];
+
+  // Let the arguments object escape.
+  return [wr, arguments];
+}, /*isCleared=*/ false);
+
+checkWeakRef(function() {
+  "use strict";
+
+  // Create a weak-ref for the first argument.
+  let wr = new WeakRef(arguments[0]);
+
+  // Clear the reference from the arguments object.
+  arguments[0] = null;
+
+  // Let the arguments object escape.
+  return [wr, arguments];
+});
+
+checkWeakRef(function() {
+  "use strict";
+
+  // Create a weak-ref for the first argument.
+  let wr = new WeakRef(arguments[0]);
+
+  // This define operation doesn't clear the reference!
+  Object.defineProperty(arguments, 0, {value: null});
+
+  // Let the arguments object escape.
+  return [wr, arguments];
+}, /*isCleared=*/ false);
+
+checkWeakRef(function() {
+  "use strict";
+
+  // Create a weak-ref for the first argument.
+  let wr = new WeakRef(arguments[0]);
+
+  // Delete operation doesn't clear the reference!
+  delete arguments[0];
+
+  // Let the arguments object escape.
+  return [wr, arguments];
+}, /*isCleared=*/ false);

js/src/vm/ArgumentsObject-inl.h

@@ -50,7 +50,7 @@ inline bool ArgumentsObject::maybeGetElements(uint32_t start, uint32_t count,
   MOZ_ASSERT(start + count >= start);
 
   uint32_t length = initialLength();
-  if (start > length || start + count > length || isAnyElementDeleted()) {
+  if (start > length || start + count > length || hasOverriddenElement()) {
     return false;
   }
 

js/src/vm/ArgumentsObject.h

@@ -40,9 +40,6 @@ class RareArgumentsData {
   static RareArgumentsData* create(JSContext* cx, ArgumentsObject* obj);
   static size_t bytesRequired(size_t numActuals);
 
-  bool isAnyElementDeleted(size_t len) const {
-    return IsAnyBitArrayElementSet(deletedBits_, len);
-  }
   bool isElementDeleted(size_t len, size_t i) const {
     MOZ_ASSERT(i < len);
     return IsBitArrayElementSet(deletedBits_, len, i);
@@ -338,13 +335,6 @@ class ArgumentsObject : public NativeObject {
     return result;
   }
 
-  bool isAnyElementDeleted() const {
-    bool result = maybeRareData() &&
-                  maybeRareData()->isAnyElementDeleted(initialLength());
-    MOZ_ASSERT_IF(result, hasOverriddenElement());
-    return result;
-  }
-
   bool markElementDeleted(JSContext* cx, uint32_t i);
 
   /*
@@ -412,7 +402,7 @@ class ArgumentsObject : public NativeObject {
    * NB: Returning false does not indicate error!
    */
   bool maybeGetElement(uint32_t i, MutableHandleValue vp) {
-    if (i >= initialLength() || isElementDeleted(i)) {
+    if (i >= initialLength() || hasOverriddenElement()) {
       return false;
     }
     vp.set(element(i));