Bug 1470081 part 1 - Stop using NativeDefineProperty for mapped ArgumentsObject properties. r?evilpie!,anba! draft
authorJan de Mooij <jdemooij@mozilla.com>
Mon, 22 Mar 2021 15:05:50 +0000
changeset 3619943 dd118d62498011984d08fe2b83cdd0a96b500432
parent 3619876 729eaf579f2827239182db122d59e42730ea540f
child 3619944 33a6a99e8ea4a81ab1ecc90241aa4bd70f422491
push id671374
push userreviewbot
push dateMon, 22 Mar 2021 15:06:24 +0000
treeherdertry@b40af46139a7 [default view] [failures only]
reviewersevilpie
bugs1470081
milestone88.0a1
Bug 1470081 part 1 - Stop using NativeDefineProperty for mapped ArgumentsObject properties. r?evilpie!,anba! Summary: This will let us remove GetterOp/SetterOp support from PropertyDescriptor in the next patch. Test Plan: Reviewers: evilpie!, anba! Subscribers: Bug #: 1470081 Differential Diff: PHID-DIFF-tpabzcmonnoaafugkhai
js/src/jit-test/tests/arguments/mapped-define.js
js/src/vm/ArgumentsObject.cpp
js/src/vm/NativeObject.cpp
js/src/vm/NativeObject.h
--- a/js/src/jit-test/tests/arguments/mapped-define.js
+++ b/js/src/jit-test/tests/arguments/mapped-define.js
@@ -1,9 +1,11 @@
-function f(a) {
+load(libdir + "asserts.js");
+
+function testMapped(a) {
     assertEq(arguments[0], 1);
 
     Object.defineProperty(arguments, 0, {value: 23, writable: true, configurable: true});
     assertEq(arguments[0], 23);
     assertEq(a, 23);
 
     a = 12;
     assertEq(a, 12);
@@ -12,9 +14,68 @@ function f(a) {
     Object.defineProperty(arguments, 0, {value: 9, writable: false, configurable: false});
     assertEq(arguments[0], 9);
     assertEq(a, 9);
 
     a = 4;
     assertEq(arguments[0], 9);
     assertEq(a, 4);
 }
-f(1);
+for (var i = 0; i < 5; i++) {
+    testMapped(1);
+}
+
+function testAttributes(x) {
+    Object.defineProperty(arguments, 0, {enumerable:true,
+                                         writable:true,
+                                         configurable:true,
+                                         value: 4});
+
+    // Should inherit enumerable/configurable attributes.
+    Object.defineProperty(arguments, 0, {writable:true,
+                                         value: 8});
+    assertEq(JSON.stringify(Object.getOwnPropertyDescriptor(arguments, 0)),
+             '{"value":8,"writable":true,"enumerable":true,"configurable":true}');
+    assertEq(x, 8);
+
+    // Property becomes non-configurable.
+    Object.defineProperty(arguments, 0, {writable:true,
+                                         configurable:false,
+                                         value: 6});
+    assertEq(JSON.stringify(Object.getOwnPropertyDescriptor(arguments, 0)),
+             '{"value":6,"writable":true,"enumerable":true,"configurable":false}');
+    assertEq(x, 6);
+
+    // Can no longer make it non-enumerable.
+    assertThrowsInstanceOf(() => Object.defineProperty(arguments, 0, {writable:true,
+                                                                      configurable:false,
+                                                                      enumerable:false,
+                                                                      value: 6}),
+                           TypeError);
+
+    // Can no longer make it configurable.
+    assertThrowsInstanceOf(() => Object.defineProperty(arguments, 0, {writable:true,
+                                                                      configurable:true,
+                                                                      value: 6}),
+                           TypeError);
+
+    // Can still make it non-writable.
+    Object.defineProperty(arguments, 0, {writable:false,
+                                         enumerable:true,
+                                         configurable:false,
+                                         value: 3});
+    assertEq(x, 3);
+
+    // No longer a mapped property.
+    x = 5;
+    assertEq(arguments[0], 3);
+
+    // Can no longer make it writable.
+    assertThrowsInstanceOf(() => Object.defineProperty(arguments, 0, {writable:true,
+                                                                      enumerable:true,
+                                                                      configurable:false,
+                                                                      value: 5}),
+                           TypeError);
+    assertEq(x, 5);
+}
+for (var i = 0; i < 5; i++) {
+    testAttributes(i);
+}
--- a/js/src/vm/ArgumentsObject.cpp
+++ b/js/src/vm/ArgumentsObject.cpp
@@ -633,16 +633,31 @@ bool ArgumentsObject::reifyIterator(JSCo
   if (!NativeDefineDataProperty(cx, obj, iteratorId, val, JSPROP_RESOLVING)) {
     return false;
   }
 
   obj->markIteratorOverridden();
   return true;
 }
 
+static bool ResolveArgumentsProperty(JSContext* cx,
+                                     Handle<ArgumentsObject*> obj, HandleId id,
+                                     GetterOp getter, SetterOp setter,
+                                     unsigned attrs, bool* resolvedp) {
+  // Note: we don't need to call ReshapeForShadowedProp here because we're just
+  // resolving an existing property instead of defining a new property.
+
+  if (!NativeObject::addAccessorProperty(cx, obj, id, getter, setter, attrs)) {
+    return false;
+  }
+
+  *resolvedp = true;
+  return true;
+}
+
 /* static */
 bool MappedArgumentsObject::obj_resolve(JSContext* cx, HandleObject obj,
                                         HandleId id, bool* resolvedp) {
   Rooted<MappedArgumentsObject*> argsobj(cx, &obj->as<MappedArgumentsObject>());
 
   if (JSID_IS_SYMBOL(id) &&
       JSID_TO_SYMBOL(id) == cx->wellKnownSymbols().iterator) {
     if (argsobj->hasOverriddenIterator()) {
@@ -673,23 +688,18 @@ bool MappedArgumentsObject::obj_resolve(
       return true;
     }
 
     if (argsobj->hasOverriddenCallee()) {
       return true;
     }
   }
 
-  if (!NativeDefineAccessorProperty(cx, argsobj, id, MappedArgGetter,
-                                    MappedArgSetter, attrs)) {
-    return false;
-  }
-
-  *resolvedp = true;
-  return true;
+  return ResolveArgumentsProperty(cx, argsobj, id, MappedArgGetter,
+                                  MappedArgSetter, attrs, resolvedp);
 }
 
 /* static */
 bool MappedArgumentsObject::obj_enumerate(JSContext* cx, HandleObject obj) {
   Rooted<MappedArgumentsObject*> argsobj(cx, &obj->as<MappedArgumentsObject>());
 
   RootedId id(cx);
   bool found;
@@ -715,16 +725,86 @@ bool MappedArgumentsObject::obj_enumerat
     if (!HasOwnProperty(cx, argsobj, id, &found)) {
       return false;
     }
   }
 
   return true;
 }
 
+static bool DefineMappedIndex(JSContext* cx, Handle<MappedArgumentsObject*> obj,
+                              HandleId id,
+                              MutableHandle<PropertyDescriptor> desc,
+                              ObjectOpResult& result) {
+  // The MappedArgGetter/MappedArgSetter property has to be defined manually
+  // because PropertyDescriptor and NativeDefineProperty don't support
+  // PropertyOp accessors.
+  //
+  // This exists in order to let code change the configurable/enumerable
+  // attributes for the mapped element properties.
+  //
+  // Note: because this preserves the default mapped-arguments behavior, we
+  // don't need to mark elements as overridden or deleted.
+
+  MOZ_ASSERT(id.isInt());
+  MOZ_ASSERT(!obj->isElementDeleted(id.toInt()));
+  MOZ_ASSERT(!obj->containsDenseElement(id.toInt()));
+
+  MOZ_ASSERT(!desc.isAccessorDescriptor());
+
+  // Mapped properties aren't used when defining a non-writable property.
+  MOZ_ASSERT(!desc.hasWritable() || desc.writable());
+
+  // First, resolve the property to simplify the code below.
+  Rooted<PropertyResult> prop(cx);
+  if (!NativeLookupOwnProperty<CanGC>(cx, obj, id, &prop)) {
+    return false;
+  }
+
+  MOZ_ASSERT(prop.isNativeProperty());
+
+  Shape* shape = prop.shape();
+  MOZ_ASSERT(shape);
+  MOZ_ASSERT(shape->writable());
+  MOZ_ASSERT(shape->getter() == MappedArgGetter);
+  MOZ_ASSERT(shape->setter() == MappedArgSetter);
+
+  // Determine whether the property should be configurable and/or enumerable.
+  bool configurable = shape->configurable();
+  bool enumerable = shape->enumerable();
+  if (configurable) {
+    if (desc.hasConfigurable()) {
+      configurable = desc.configurable();
+    }
+    if (desc.hasEnumerable()) {
+      enumerable = desc.enumerable();
+    }
+  } else {
+    // Property is not configurable so disallow any attribute changes.
+    if ((desc.hasConfigurable() && desc.configurable()) ||
+        (desc.hasEnumerable() && enumerable != desc.enumerable())) {
+      return result.fail(JSMSG_CANT_REDEFINE_PROP);
+    }
+  }
+
+  unsigned attrs = 0;
+  if (!configurable) {
+    attrs |= JSPROP_PERMANENT;
+  }
+  if (enumerable) {
+    attrs |= JSPROP_ENUMERATE;
+  }
+  if (!NativeObject::putAccessorProperty(cx, obj, id, MappedArgGetter,
+                                         MappedArgSetter, attrs)) {
+    return false;
+  }
+
+  return result.succeed();
+}
+
 // ES 2017 draft 9.4.4.2
 /* static */
 bool MappedArgumentsObject::obj_defineProperty(JSContext* cx, HandleObject obj,
                                                HandleId id,
                                                Handle<PropertyDescriptor> desc,
                                                ObjectOpResult& result) {
   // Step 1.
   Rooted<MappedArgumentsObject*> argsobj(cx, &obj->as<MappedArgumentsObject>());
@@ -736,40 +816,42 @@ bool MappedArgumentsObject::obj_definePr
     isMapped =
         arg < argsobj->initialLength() && !argsobj->isElementDeleted(arg);
   }
 
   // Step 4.
   Rooted<PropertyDescriptor> newArgDesc(cx, desc);
 
   // Step 5.
+  bool defineMapped = false;
   if (!desc.isAccessorDescriptor() && isMapped) {
     // Step 5.a.
     if (desc.hasWritable() && !desc.writable()) {
       if (!desc.hasValue()) {
         RootedValue v(cx, argsobj->element(JSID_TO_INT(id)));
         newArgDesc.setValue(v);
       }
       newArgDesc.setGetter(nullptr);
       newArgDesc.setSetter(nullptr);
     } else {
-      // In this case the live mapping is supposed to keep working,
-      // we have to pass along the Getter/Setter otherwise they are
-      // overwritten.
-      newArgDesc.setGetter(MappedArgGetter);
-      newArgDesc.setSetter(MappedArgSetter);
-      newArgDesc.value().setUndefined();
-      newArgDesc.attributesRef() |= JSPROP_IGNORE_VALUE;
+      // In this case the live mapping is supposed to keep working.
+      defineMapped = true;
     }
   }
 
   // Step 6. NativeDefineProperty will lookup [[Value]] for us.
-  if (!NativeDefineProperty(cx, obj.as<NativeObject>(), id, newArgDesc,
-                            result)) {
-    return false;
+  if (defineMapped) {
+    if (!DefineMappedIndex(cx, argsobj, id, &newArgDesc, result)) {
+      return false;
+    }
+  } else {
+    if (!NativeDefineProperty(cx, obj.as<NativeObject>(), id, newArgDesc,
+                              result)) {
+      return false;
+    }
   }
   // Step 7.
   if (!result.ok()) {
     return true;
   }
 
   // Step 8.
   if (isMapped) {
@@ -900,23 +982,18 @@ bool UnmappedArgumentsObject::obj_resolv
   } else if (JSID_IS_ATOM(id, cx->names().length)) {
     if (argsobj->hasOverriddenLength()) {
       return true;
     }
   } else {
     return true;
   }
 
-  if (!NativeDefineAccessorProperty(cx, argsobj, id, UnmappedArgGetter,
-                                    UnmappedArgSetter, attrs)) {
-    return false;
-  }
-
-  *resolvedp = true;
-  return true;
+  return ResolveArgumentsProperty(cx, argsobj, id, UnmappedArgGetter,
+                                  UnmappedArgSetter, attrs, resolvedp);
 }
 
 /* static */
 bool UnmappedArgumentsObject::obj_enumerate(JSContext* cx, HandleObject obj) {
   Rooted<UnmappedArgumentsObject*> argsobj(cx,
                                            &obj->as<UnmappedArgumentsObject>());
 
   RootedId id(cx);
--- a/js/src/vm/NativeObject.cpp
+++ b/js/src/vm/NativeObject.cpp
@@ -1758,39 +1758,16 @@ bool js::NativeDefineDataProperty(JSCont
                                   HandleId id, HandleValue value,
                                   unsigned attrs, ObjectOpResult& result) {
   Rooted<PropertyDescriptor> desc(cx);
   desc.initFields(nullptr, value, attrs, nullptr, nullptr);
   return NativeDefineProperty(cx, obj, id, desc, result);
 }
 
 bool js::NativeDefineAccessorProperty(JSContext* cx, HandleNativeObject obj,
-                                      HandleId id, GetterOp getter,
-                                      SetterOp setter, unsigned attrs) {
-  Rooted<PropertyDescriptor> desc(cx);
-  desc.initFields(nullptr, UndefinedHandleValue, attrs, getter, setter);
-
-  ObjectOpResult result;
-  if (!NativeDefineProperty(cx, obj, id, desc, result)) {
-    return false;
-  }
-
-  if (!result) {
-    // Off-thread callers should not get here: they must call this
-    // function only with known-valid arguments. Populating a new
-    // PlainObject with configurable properties is fine.
-    MOZ_ASSERT(!cx->isHelperThreadContext());
-    result.reportError(cx, obj, id);
-    return false;
-  }
-
-  return true;
-}
-
-bool js::NativeDefineAccessorProperty(JSContext* cx, HandleNativeObject obj,
                                       HandleId id, HandleObject getter,
                                       HandleObject setter, unsigned attrs) {
   Rooted<PropertyDescriptor> desc(cx);
   {
     GetterOp getterOp = JS_DATA_TO_FUNC_PTR(GetterOp, getter.get());
     SetterOp setterOp = JS_DATA_TO_FUNC_PTR(SetterOp, setter.get());
     desc.initFields(nullptr, UndefinedHandleValue, attrs, getterOp, setterOp);
   }
--- a/js/src/vm/NativeObject.h
+++ b/js/src/vm/NativeObject.h
@@ -1558,19 +1558,16 @@ extern bool NativeDefineProperty(JSConte
                                  Handle<JS::PropertyDescriptor> desc,
                                  ObjectOpResult& result);
 
 extern bool NativeDefineDataProperty(JSContext* cx, HandleNativeObject obj,
                                      HandleId id, HandleValue value,
                                      unsigned attrs, ObjectOpResult& result);
 
 /* If the result out-param is omitted, throw on failure. */
-extern bool NativeDefineAccessorProperty(JSContext* cx, HandleNativeObject obj,
-                                         HandleId id, GetterOp getter,
-                                         SetterOp setter, unsigned attrs);
 
 extern bool NativeDefineAccessorProperty(JSContext* cx, HandleNativeObject obj,
                                          HandleId id, HandleObject getter,
                                          HandleObject setter, unsigned attrs);
 
 extern bool NativeDefineDataProperty(JSContext* cx, HandleNativeObject obj,
                                      HandleId id, HandleValue value,
                                      unsigned attrs);