Comment on attachment 654739 [details] [diff] [review] v1 Review of attachment 654739 [details] [diff] [review]: ----------------------------------------------------------------- Please add a comment somewhere about why it's safe to resolve named properties on lists. If not, please file a followup bug on making sure that we don't let named properties shadow properties on the prototype chain. ::: js/xpconnect/wrappers/WrapperFactory.cpp @@ +294,5 @@ > > static XrayType > GetXrayType(JSObject *obj) > { > + if (mozilla::dom::IsDOMObject(obj)) { Nit: No braces on single-line if. ::: js/xpconnect/wrappers/XrayWrapper.cpp @@ +1058,5 @@ > JSObject *obj = getInnerObject(wrapper); > + const NativePropertyHooks *nativeHooks = GetDOMClass(obj)->mNativeHooks; > + > + if (nativeHooks->mResolveOwnProperty && > + nativeHooks->mResolveOwnProperty(cx, wrapper, id, set, desc) && If resolveOwnProperty returns false, we should treat that as if it threw an exception, and we'll need to propagate it here. @@ +1083,5 @@ > + JSObject *obj = getInnerObject(wrapper); > + const NativePropertyHooks *nativeHooks = GetDOMClass(obj)->mNativeHooks; > + > + if (nativeHooks->mResolveOwnProperty && > + nativeHooks->mResolveOwnProperty(cx, wrapper, id, set, desc) && As you pointed out, it might not make sense to do this here... Isn't it redundant with resolveNativeProperty? @@ +1120,5 @@ > + JSObject *obj = getInnerObject(wrapper); > + const NativePropertyHooks *nativeHooks = GetDOMClass(obj)->mNativeHooks; > + > + if (nativeHooks->mEnumerateOwnProperties && > + !nativeHooks->mEnumerateOwnProperties(cx, wrapper, props)) If enumerateOwnProperties returns false, we should treat that as if it threw an exception, and we'll need to propagate it here.