I think we can do this on top of bug 1469217. Then we no longer have to expose JSGetterOp/JSSetterOp and GetterOp/SetterOp will be JS engine implementation details for ArrayObject/ArgumentsObject.

This is a bit subtle so I'll request review from both of you. The goal of this is to isolate the craziness into ArgumentsObject.cpp

I tried to keep the changes as simple as possible, but there's probably more PropertyDescriptor cleanup we can do after this.

Comment on attachment 8986721 [details] [diff] [review] Part 1 - Stop using NativeDefineProperty for mapped ArgumentsObject properties Review of attachment 8986721 [details] [diff] [review]: ----------------------------------------------------------------- I think this is okay. ::: js/src/vm/ArgumentsObject.cpp @@ +646,5 @@ > + MOZ_ASSERT(!obj->isElementDeleted(uint32_t(JSID_TO_INT(id)))); > + MOZ_ASSERT(!obj->containsDenseElement(uint32_t(JSID_TO_INT(id)))); > + > + MOZ_ASSERT(!desc.hasWritable() || desc.writable()); > + MOZ_ASSERT(!desc.isAccessorDescriptor()) @@ +653,5 @@ > + if (!HasOwnProperty(cx, obj, id, &hasOwn)) > + return false; > + MOZ_ASSERT(hasOwn); > + > + Shape* shape = obj->lookup(cx, id); I think you could have used NativeLookupOwnProperty instead of HasOwnProperty + lookup. This also matches NativeDefineProperty.

Comment on attachment 8986725 [details] [diff] [review] Part 2 - Remove JSGetterOp/JSSetterOp from PropertyDescriptor and JSAPI Review of attachment 8986725 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsapi.h @@ +2057,5 @@ > JS::HandleObject object() const { > return JS::HandleObject::fromMarkedLocation(&desc().obj); > } > unsigned attributes() const { return desc().attrs; } > + JSObject* maybeGetterObject() const { return desc().getter; } Can we assert MOZ_ASSERT_IF(!has(JSPROP_GETTER), !desc().getter); here?

(In reply to Tom Schuster [:evilpie] from comment #3) > I think you could have used NativeLookupOwnProperty instead of > HasOwnProperty + lookup. This also matches NativeDefineProperty. Yes, good idea. (In reply to Tom Schuster [:evilpie] from comment #4) > Can we assert MOZ_ASSERT_IF(!has(JSPROP_GETTER), !desc().getter); here? That should work.

Comment on attachment 8986725 [details] [diff] [review] Part 2 - Remove JSGetterOp/JSSetterOp from PropertyDescriptor and JSAPI Review of attachment 8986725 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/ipc/JavaScriptShared.cpp @@ +573,5 @@ > if (!toObjectVariant(cx, getter, &objVar)) > return false; > out->getter() = objVar; > } else { > out->getter() = UnknownPropertyOp; Isn't this branch dead now? ::: js/src/jsapi.h @@ +2188,5 @@ > (writable ? 0 : JSPROP_READONLY)); > } > void setAttributes(unsigned attrs) { desc().attrs = attrs; } > > + void setGetter(JSObject* obj) { I kinda wish these wouldn't even exist anymore.

(In reply to Tom Schuster [:evilpie] from comment #6) > Isn't this branch dead now? Oh good point. I fixed the IPC => getter/setter code but there's more we can clean up for the other direction. I'll post a follow-up patch later.

Comment on attachment 8986721 [details] [diff] [review] Part 1 - Stop using NativeDefineProperty for mapped ArgumentsObject properties Review of attachment 8986721 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the suggestions from evilpie applied and my review questions answered. ::: js/src/vm/ArgumentsObject.cpp @@ +593,5 @@ > if (argsobj->hasOverriddenCallee()) > return true; > } > > + if (!NativeObject::addAccessorProperty(cx, argsobj, id, MappedArgGetter, MappedArgSetter, This will skip ReshapeForShadowedProp [1] and UpdateShapeTypeAndValue [2], correct? Should we still try to call these functions or if not, document why it's okay to skip them? (I'm just wondering if there are any issues lurking, for example for array objects we sometimes don't notify TI which leads to marking the "length" property differently depending on the executed code paths [3]. And not repeating the same issues for arguments objects may make sense.) [1] https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/js/src/vm/NativeObject.cpp#1382 [2] https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/js/src/vm/NativeObject.cpp#1427 [3] Initially "length" is not marked at all, by going through SetArrayLength -> NativeObject::changeProperty it can be marked as [non-data], by going through SetIntegrityLevel it can be marked as [non-writable]. @@ +682,5 @@ > + if (!NativeObject::putAccessorProperty(cx, obj, id, MappedArgGetter, MappedArgSetter, > + attrs)) > + { > + return false; > + } Arguments objects' elements will no longer be marked as overriden [1], is this an issue here? Here we'll also skip calling UpdateShapeTypeAndValue. And I wonder if this'll the first call to putAccessorProperty which actually uses the check at [2], because NativeObject::changeProperty(...) [3] and AddOrChangeProperty (via NativeDefineProperty) [4] have their own checks to detect redundant putAccessorProperty calls. :-) [1] https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/js/src/vm/NativeObject.cpp#1654 [2] https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/js/src/vm/Shape.cpp#1017 [3] https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/js/src/vm/Shape.cpp#1093 [4] https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/js/src/vm/NativeObject.cpp#1698

Comment on attachment 8986725 [details] [diff] [review] Part 2 - Remove JSGetterOp/JSSetterOp from PropertyDescriptor and JSAPI Review of attachment 8986725 [details] [diff] [review]: ----------------------------------------------------------------- Follow-up bug: Looks like CallJSGetterOp, CallJSSetterOp, CallJSAddPropertyOp, and CallJSDeletePropertyOp can now be moved from JSContext-inl.h into NativeObject.cpp. ::: dom/base/ChromeUtils.cpp @@ +228,5 @@ > id = idVal; > if (!JS_GetOwnPropertyDescriptorById(cx, obj, id, &desc)) { > continue; > } > + if (desc.hasGetterOrSetter()) { Pre-existing: I guess this should actually be |if (desc.isAccessorDescriptor()|, at least from a more spec-oriented POV. ::: js/src/builtin/Array.cpp @@ -635,5 @@ > - // This array .length property was found on the prototype > - // chain. Ideally the setter should not have been called, but since > - // we're here, do an impression of SetPropertyByDefining. > - return DefineDataProperty(cx, obj, id, v, JSPROP_ENUMERATE, result); > - } That means we can perform the similar clean-up in [1] and [2], right? [1] https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/js/src/vm/ArgumentsObject.cpp#482-483 [2] https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/js/src/vm/ArgumentsObject.cpp#732-733 ::: js/src/jsapi.h @@ +2058,5 @@ > return JS::HandleObject::fromMarkedLocation(&desc().obj); > } > unsigned attributes() const { return desc().attrs; } > + JSObject* maybeGetterObject() const { return desc().getter; } > + JSObject* maybeSetterObject() const { return desc().setter; } We may even be able to avoid adding these two methods after a bit more clean-up for property descriptors. ::: js/src/proxy/ScriptedProxyHandler.cpp @@ +53,5 @@ > // Step 4. > if ((!desc.hasWritable() || > (current.hasWritable() && desc.writable() == current.writable())) && > + (!desc.hasGetterObject() || desc.getterObject() == current.maybeGetterObject()) && > + (!desc.hasSetterObject() || desc.setterObject() == current.maybeSetterObject()) && Can you file a follow-up bug to update this function to match the latest spec? Spec change: https://github.com/tc39/ecma262/pull/353 @@ +135,5 @@ > MOZ_ASSERT(desc.isAccessorDescriptor()); // by step 7 > > if (current.configurable()) > return true; > + if (desc.hasSetterObject() && desc.setterObject() != current.maybeSetterObject()) { Hmm, I wonder if this could be simplified to |desc.setterObject() != current.setterObject()|, because |current| is always a complete accessor property descriptor, which means JSPROP_SETTER (and JSPROP_GETTER) are always set, cf. [1]. [1] https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/js/src/jsapi.h#2134 ::: js/src/proxy/SecurityWrapper.cpp @@ +108,5 @@ > SecurityWrapper<Base>::defineProperty(JSContext* cx, HandleObject wrapper, HandleId id, > Handle<PropertyDescriptor> desc, > ObjectOpResult& result) const > { > + if (desc.hasGetterOrSetter()) { Nit: |if (desc.isAccessorDescriptor())| ::: js/src/vm/NativeObject.cpp @@ +1387,5 @@ > desc.attributes() == JSPROP_ENUMERATE && > (!obj->isIndexed() || !obj->containsPure(id)) && > !obj->is<TypedArrayObject>()) > { > + MOZ_ASSERT(!desc.hasGetterObject() && !desc.hasSetterObject()); Shorter: |MOZ_ASSERT(!desc.isAccessorDescriptor())| @@ +1409,2 @@ > if (AddOrChange == IsAddOrChange::Add) { > + if (Shape::isDataProperty(desc.attributes(), maybeGetter, maybeSetter)) { I think using |desc.isAccessorDescriptor()| here is now the better choice. @@ +1415,4 @@ > desc.attributes()); > } > } else { > + if (Shape::isDataProperty(desc.attributes(), maybeGetter, maybeSetter)) { Ditto. @@ +1439,5 @@ > NativeObject::maybeDensifySparseElements(cx, obj); > if (edResult == DenseElementResult::Failure) > return false; > if (edResult == DenseElementResult::Success) { > + MOZ_ASSERT(!desc.hasSetterObject()); I think we can also use |MOZ_ASSERT(!desc.isAccessorDescriptor())| here, can't we? @@ +1567,5 @@ > } > > + // Check for GetterOp/SetterOp. > + if (!prop.isDenseOrTypedArrayElement() && > + (prop.shape()->getter() || prop.shape()->setter())) Can we use |prop.shape()->isAccessorShape()| here? ::: js/src/vm/UnboxedObject.cpp @@ +929,2 @@ > // This define is equivalent to setting an existing property. > + MOZ_ASSERT(!desc.hasGetterObject() && !desc.hasSetterObject()); Shorter: |MOZ_ASSERT(!desc.isAccessorDescriptor())| ::: js/xpconnect/src/Sandbox.cpp @@ +676,5 @@ > > if (!(desc->attrs & attrFlag)) { > XPCThrower::Throw(NS_ERROR_UNEXPECTED, cx); > return false; > } This should now be dead, too. It looks like this handled the case when |desc.[gs]etter()| was is non-null, but JSPROP_[GS]ETTER isn't set. So the whole code here could probably simplified to: --- static bool WrapAccessorFunction(JSContext* cx, MutableHandleObject obj, HandleObject sandboxProtoProxy) { if (!obj) { return true; } obj.set(WrapCallable(cx, obj, sandboxProtoProxy)); return !!obj; } --- And the caller can then be changed to: --- // Now fix up the getter/setter/value as needed to be bound to desc->obj. if (desc.hasGetterObject() && !WrapAccessorFunction(cx, desc.getterObject(), proxy)) return false; if (desc.hasSetterObject() && !WrapAccessorFunction(cx, desc.setterObject(), proxy)) return false; --- This also matches how property descriptors are wrapped here: https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/js/src/vm/Compartment.cpp#371-378 ::: js/xpconnect/wrappers/FilteringWrapper.cpp @@ +236,5 @@ > if (!JSID_IS_INT(id)) { > desc.attributesRef() &= ~JSPROP_ENUMERATE; > } > desc.attributesRef() &= ~JSPROP_PERMANENT; > + if (!desc.hasGetterOrSetter()) Nit: |if (desc.isDataDescriptor())|

Pushed by jdemooij@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3d94e8d65c71 part 1 - Stop using NativeDefineProperty for mapped ArgumentsObject properties. r=evilpie,anba https://hg.mozilla.org/integration/autoland/rev/2d857a9fa2ca part 2 - Remove JSGetterOp/JSSetterOp from PropertyDescriptor. r=evilpie,anba https://hg.mozilla.org/integration/autoland/rev/0365c93db1c0 part 3 - Remove unnecessary Class checks in SetterOps. r=anba https://hg.mozilla.org/integration/autoland/rev/250d9004c967 part 4 - Remove JSGetterOp and JSSetterOp. r=evilpie https://hg.mozilla.org/integration/autoland/rev/d7e2a6ed23ff part 5 - Move CallJS{Getter,Setter,AddProperty,DeleteProperty}Op into NativeObject.cpp. r=anba