This uses a separate JSObject stored in a reserved slot on the prototype object to hold the unforgeable properties.

Comment on attachment 729777 [details] [diff] [review] Add a JS_DefineOwnProperty API that takes a JSPropertyDescriptor Review of attachment 729777 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsapi.cpp @@ +3888,5 @@ > } > > +JS_PUBLIC_API(JSBool) > +JS_DefineOwnProperty(JSContext *cx, JSObject *objArg, jsid idArg, > + const JSPropertyDescriptor& descriptor, JSBool *bp) It's in the other patch, but your calls to this new method should probably throw if |!*bp| on success. The ObjectOps::defineProperty contract is that if definition can't happen, an error is thrown -- failure is never silent. So you probably want to throw in that case. We want to change ObjectOps::defineProperty to return whether the property was defined or not via outparam, like this method does, eventually. See bug 826587. Until I finish that, tho, defineProperty methods as used by ObjectOps should throw, if the property couldn't be defined. ::: js/src/jsapi.h @@ +3223,5 @@ > JS_DefineOwnProperty(JSContext *cx, JSObject *obj, jsid id, jsval descriptor, JSBool *bp); > > +extern JS_PUBLIC_API(JSBool) > +JS_DefineOwnProperty(JSContext *cx, JSObject *objArg, jsid idArg, > + const JSPropertyDescriptor& descriptor, JSBool *bp); Would you mind adding this to jsfriendapi.{cpp,h} instead of here? We want the ultimate public API to be PropDesc-based, and I'd rather not add an API with the better name here if it's not doing the better thing from the start.

Oh, and make it: extern JS_FRIEND_API(JSBool) js_DefineOwnProperty(arguments list) in jsfriendapi.h, please, so it's clear it's not-ideally public.

Comment on attachment 729790 [details] [diff] [review] Support Unforgeable on proxy-based DOM bindings >+++ b/dom/bindings/Codegen.py >+ return descriptor.concrete and descriptor.proxy and any(m for m in descriptor.interface.members if m.isAttr() and m.isUnforgeable()) I would really like a saner line length there. >@@ -266,21 +272,24 @@ class CGInterfaceObjectJSClass(CGThing): >+ slotCount += " + %i /* slots for the named constructors */" % len(self.descriptor.interface.namedConstructors) And here. >@@ -1609,16 +1611,31 @@ class CGCreateInterfaceObjectsMethod(CGA >+ if self.descriptor.concrete and UseHolderForUnforgeable(self.descriptor): UseHolderForUnforgeable already tests .concrete, so no need to test it again. >+ defineUnforgeables = "" This is duplicating a lot of InitUnforgeableProperties. The main differences seem to be: 1) Different object name (could be an argument). 2) Different failure return value (" nullptr" vs ""). 3) InitUnforgeableProperties has code at the end to indent things and whatnot. 4) This code is not doing an access check when defining the chrome-only properties (seems like a bug in this code to me). Could we perhaps factor out the non-trailing part of InitUnforgeableProperties into an InitUnforgeablePropertiesOnGivenObject or something and reuse it here? I'd really prefer that. >@@ -1681,18 +1698,27 @@ class CGCreateInterfaceObjectsMethod(CGA >+ if self.descriptor.concrete and UseHolderForUnforgeable(self.descriptor): Again, no need to check .concrete. >+ setUnforgeableHolder = CGGeneric("""JSObject* proto = protoAndIfaceArray[prototypes::id::%s]; >+if (proto) { This is really hard to read. I'd much prefer: setUnforgeableHolder = CGGeneric( "JSObject* proto = protoAndIfaceArray[prototypes::id::%s];\n" "if (proto) {\n" # etc ) > def InitUnforgeableProperties(descriptor, properties): >+ if descriptor.proxy: >+ return "" Maybe add a comment (either Python or C++, your call) that this is handled by the proxy handler and the thing on the proto? >@@ -6076,29 +6104,54 @@ class CGDOMJSProxyHandler_getOwnProperty >+ if UseHolderForUnforgeable(self.descriptor): >+ getUnforgeable = """{ Please add a comment about why the extra scope is needed. >+ mozilla::Maybe<JSAutoCompartment> ac; No need for the "mozilla::" bit. >+ if (!JS_GetPropertyDescriptorById(cx, &js::GetReservedSlot(protoAndIfaceArray[prototypes::id::%s], DOM_INTERFACE_PROTO_SLOTS_BASE).toObject(), id, flags, desc)) { More line-wrapping, please? > if indexedSetter or self.descriptor.operations['NamedSetter']: > setOrIndexedGet += "if (flags & JSRESOLVE_ASSIGNING) {\n" >+ setOrIndexedGet += getUnforgeable Shouldn't that get indented? But more importantly, if we hav a named setter this will only to getUnforgeable conditionally on JSRESOLVE_ASSIGNING. Why is that reasonable? At first glance it seems to me like the getUnforgeable bits should just always happen, so I don't understand the logic here. At the very least it could use some documentation explaining what it's doing... >@@ -6182,16 +6237,39 @@ class CGDOMJSProxyHandler_defineProperty >+ set += """{ Again, document the scope. This is duplicating a bunch of the code from getOwnPropertyDescriptor; worth factoring out into a shared function or not? Maybe it's not.... But at least a shared function on the Python side so we only have that blob of C++ once in our codebase? ;) >+ if (hasUnforgeable) { >+ return JS_DefineOwnProperty(cx, unforgeablePropertyHolder, id, *desc, &hasUnforgeable); So fundamentally this is just relying on the JS_DefineOwnProperty call to throw for us, right? Can it ever _not_ throw? If, as I think, it always throws, might be worth documenting why we're calling it anyway. But I suspect the intent here is to invoke the setter as needed; will JS_DefineOwnProperty do that? Or is the setter invoked in some other way on actual sets and this really does mean to always throw? >@@ -6283,41 +6361,63 @@ class CGDOMJSProxyHandler_getOwnPropertyNames >+{ Again, document. And again, the same code.... >+ if (!js::GetPropertyNames(cx, &js::GetReservedSlot(protoAndIfaceArray[prototypes::id::%s], DOM_INTERFACE_PROTO_SLOTS_BASE).toObject(), JSITER_OWNONLY | JSITER_HIDDEN, &props)) { Shorter lines, please. Why do we not need changes to _delete or _hasOwn? Seems fishy. >@@ -6368,17 +6468,31 @@ class CGDOMJSProxyHandler_get(ClassMetho >+ getUnforgeableOrExpando = """JSObject** protoAndIfaceArray = GetProtoAndIfaceArray(js::GetGlobalForObjectCrossCompartment(proxy)); Why do we not need to find the underlying global here in the Xray case? And if we don't, why did we need to do it for the other things above? > class CGDOMJSProxyHandler(CGClass): >+ if descriptor.operations['IndexedSetter'] or descriptor.operations['NamedSetter'] or UseHolderForUnforgeable(descriptor): Shorter lines, please. >+++ b/dom/bindings/DOMJSClass.h >+// Interface objects store a number of reserved slots equal to >+// DOM_INTERFACE_PROTO_SLOTS_BASE or DOM_INTERFACE_PROTO_SLOTS_BASE + 1 if a "Interface prototype objects" r- pending sorting out the questions about define, the compartment stuff in _get(), and the missing delete and hasOwn bits...

Oh, also, we'll need to make sure that our various ICs can deal with this stuff. In particular, if someone adds an unforgeable attribute to a non-overridebuiltins proxy bindings, will we deal?

(In reply to Boris Zbarsky (:bz) from comment #5) > It's in the other patch, but your calls to this new method should probably > throw if |!*bp| on success. The ObjectOps::defineProperty contract is that > if definition can't happen, an error is thrown -- failure is never silent. > So you probably want to throw in that case. But how could that happen? js_DefineOwnProperty passes |true| for throwError to DefinePropertyOnObject. If you want I can add an assert that |*bp| is always |true| on success.

(In reply to Peter Van der Beken [:peterv] from comment #9) > But how could that happen? js_DefineOwnProperty passes |true| for throwError > to DefinePropertyOnObject. If you want I can add an assert that |*bp| is > always |true| on success. Oh, right! Yes, an assertion to that effect would be helpful. But in that case, why have the |JSBool *bp| outparam at all? I know we want to get to the point of using that, only, everywhere, at some point. But I think I'd rather clean that up at use sites like this all in one go -- or at least not have misleading outparams like this, that make users *think* it's possible for the define to not happen and that case be not an error.

Comment on attachment 734836 [details] [diff] [review] Support Unforgeable on proxy-based DOM bindings v2 Actually I need to address one more comment.

OK. I'd love an interdiff vs what I already reviewed when you're done!

(In reply to Boris Zbarsky (:bz) from comment #5) > I would really like a saner line length there. Done. > And here. Done. > UseHolderForUnforgeable already tests .concrete, so no need to test it again. Done. > Could we perhaps factor out the non-trailing part of > InitUnforgeableProperties into an InitUnforgeablePropertiesOnGivenObject or > something and reuse it here? I'd really prefer that. Done. Note that I made the call to AccessCheck::isChrome take the object that we define the unforgeable properties on, it doesn't need a global since it uses the compartment anyway. > Again, no need to check .concrete. Done. > This is really hard to read. I'd much prefer: Done. > Maybe add a comment (either Python or C++, your call) that this is handled > by the proxy handler and the thing on the proto? Done. > Please add a comment about why the extra scope is needed. Done. > No need for the "mozilla::" bit. Done. > More line-wrapping, please? Done. > Shouldn't that get indented? Done. > But more importantly, if we hav a named setter this will only to > getUnforgeable conditionally on JSRESOLVE_ASSIGNING. Why is that > reasonable? At first glance it seems to me like the getUnforgeable bits > should just always happen, so I don't understand the logic here. At the > very least it could use some documentation explaining what it's doing... Our proxy handler code is a big mess, I started changing it to make it more logical but I'll move that to a separate bug. I've added the getUnforgeable after the indexed properties both for > This is duplicating a bunch of the code from getOwnPropertyDescriptor; worth > factoring out into a shared function or not? Maybe it's not.... But at > least a shared function on the Python side so we only have that blob of C++ > once in our codebase? ;) I refactored, not sure it was worth it after all. > > >+ if (hasUnforgeable) { > >+ return JS_DefineOwnProperty(cx, unforgeablePropertyHolder, id, *desc, &hasUnforgeable); > > So fundamentally this is just relying on the JS_DefineOwnProperty call to > throw for us, right? Can it ever _not_ throw? If, as I think, it always > throws, might be worth documenting why we're calling it anyway. But I > suspect the intent here is to invoke the setter as needed; will > JS_DefineOwnProperty do that? Or is the setter invoked in some other way on > actual sets and this really does mean to always throw? JS_DefineOwnProperty for an unforgeable property wouldn't throw if you call it with a descriptor that has the same values as the property on the object. This code isn't used for actual sets, the proxy code only calls the defineProperty handler for sets for existing properties if they are non-JSPROP_SHARED. > Again, document. And again, the same code.... Refactored. > Shorter lines, please. Done. > Why do we not need changes to _delete or _hasOwn? Seems fishy. > > Why do we not need to find the underlying global here in the Xray case? And > if we don't, why did we need to do it for the other things above? Xrays don't call this, see the very first line in the generated code for it (MOZ_ASSERT(!xpc::WrapperFactory::IsXrayWrapper(proxy), ...)). A lot of the other hooks are called for Xrays, so they need to deal. > > > class CGDOMJSProxyHandler(CGClass): > >+ if descriptor.operations['IndexedSetter'] or descriptor.operations['NamedSetter'] or UseHolderForUnforgeable(descriptor): > > Shorter lines, please. Done. > "Interface prototype objects" Done.

Comment on attachment 739542 [details] [diff] [review] Support Unforgeable on proxy-based DOM bindings v2.1 >+ Generate the code to execute the code in call on an unforgeable holder if ... the code in "code" ... right? >@@ -1702,18 +1755,29 @@ class CGCreateInterfaceObjectsMethod(CGA >+ "if (proto) {\n\n" Drop one of those \n? >+ CGIndenter(CGGeneric(get + getUnforgeable)).define() + Do we need a \n between the get and the getUnforgeable? Worth checking. r=me with those fixed.