From 16727033d9db70a418512b3d350c005817abb79d Mon Sep 17 00:00:00 2001 From: Tom Schuster Date: Thu, 29 Apr 2021 17:36:49 +0000 Subject: [PATCH] Bug 1706404 - Use Maybe more in XPConnect. r=smaug The current defineProperty code sadly uses existing_desc.object() to check if the property is being defined would shadow an own property. All of this code is not really something that we should be doing, but I am not even sure how we would replace GetPropertyDescriptor with GetOwnPropertyDescriptor here. Depends on D113661 Differential Revision: https://phabricator.services.mozilla.com/D113662 --- js/xpconnect/src/Sandbox.cpp | 14 +++--- js/xpconnect/wrappers/XrayWrapper.cpp | 69 +++++++++++++++------------ js/xpconnect/wrappers/XrayWrapper.h | 30 +++++++----- 3 files changed, 63 insertions(+), 50 deletions(-) diff --git a/js/xpconnect/src/Sandbox.cpp b/js/xpconnect/src/Sandbox.cpp index 004fb4751d89..3cc1bf604258 100644 --- a/js/xpconnect/src/Sandbox.cpp +++ b/js/xpconnect/src/Sandbox.cpp @@ -704,24 +704,24 @@ bool SandboxProxyHandler::getPropertyDescriptorImpl( MOZ_ASSERT(JS::GetCompartment(obj) == JS::GetCompartment(proxy)); - Rooted desc(cx); if (getOwn) { - if (!JS_GetOwnPropertyDescriptorById(cx, obj, id, &desc)) { + if (!JS_GetOwnPropertyDescriptorById(cx, obj, id, desc_)) { return false; } } else { - if (!JS_GetPropertyDescriptorById(cx, obj, id, &desc)) { + Rooted holder(cx); + if (!JS_GetPropertyDescriptorById(cx, obj, id, desc_, &holder)) { return false; } } - if (!desc.object()) { - // No property, nothing to do - desc_.reset(); + if (desc_.isNothing()) { return true; } - // Now fix up the getter/setter/value as needed to be bound to desc->obj. + Rooted desc(cx, *desc_); + + // Now fix up the getter/setter/value as needed. if (desc.hasGetterObject() && !WrapAccessorFunction(cx, desc.getterObject(), proxy)) { return false; diff --git a/js/xpconnect/wrappers/XrayWrapper.cpp b/js/xpconnect/wrappers/XrayWrapper.cpp index ddac9ff02d07..b62d7d2fa349 100644 --- a/js/xpconnect/wrappers/XrayWrapper.cpp +++ b/js/xpconnect/wrappers/XrayWrapper.cpp @@ -758,10 +758,11 @@ bool JSXrayTraits::delete_(JSContext* cx, HandleObject wrapper, HandleId id, return result.succeed(); } -bool JSXrayTraits::defineProperty(JSContext* cx, HandleObject wrapper, - HandleId id, Handle desc, - Handle existingDesc, - ObjectOpResult& result, bool* defined) { +bool JSXrayTraits::defineProperty( + JSContext* cx, HandleObject wrapper, HandleId id, + Handle desc, + Handle> existingDesc, + Handle existingHolder, ObjectOpResult& result, bool* defined) { *defined = false; RootedObject holder(cx, ensureHolder(cx, wrapper)); if (!holder) { @@ -798,17 +799,19 @@ bool JSXrayTraits::defineProperty(JSContext* cx, HandleObject wrapper, "property on [Object] or [Array] XrayWrapper"); return false; } - if (existingDesc.hasGetterOrSetter()) { - JS_ReportErrorASCII(cx, - "Not allowed to overwrite accessor property on " - "[Object] or [Array] XrayWrapper"); - return false; - } - if (existingDesc.object() && existingDesc.object() != wrapper) { - JS_ReportErrorASCII(cx, - "Not allowed to shadow non-own Xray-resolved " - "property on [Object] or [Array] XrayWrapper"); - return false; + if (existingDesc.isSome()) { + if (existingDesc->hasGetterOrSetter()) { + JS_ReportErrorASCII(cx, + "Not allowed to overwrite accessor property on " + "[Object] or [Array] XrayWrapper"); + return false; + } + if (existingHolder != wrapper) { + JS_ReportErrorASCII(cx, + "Not allowed to shadow non-own Xray-resolved " + "property on [Object] or [Array] XrayWrapper"); + return false; + } } Rooted wrappedDesc(cx, desc); @@ -1716,12 +1719,13 @@ bool DOMXrayTraits::delete_(JSContext* cx, JS::HandleObject wrapper, return XrayDeleteNamedProperty(cx, wrapper, target, id, result); } -bool DOMXrayTraits::defineProperty(JSContext* cx, HandleObject wrapper, - HandleId id, Handle desc, - Handle existingDesc, - JS::ObjectOpResult& result, bool* done) { +bool DOMXrayTraits::defineProperty( + JSContext* cx, HandleObject wrapper, HandleId id, + Handle desc, + Handle> existingDesc, + Handle existingHolder, JS::ObjectOpResult& result, bool* done) { // Check for an indexed property on a Window. If that's happening, do - // nothing but set done to tru so it won't get added as an expando. + // nothing but set done to true so it won't get added as an expando. if (IsWindow(cx, wrapper)) { if (IsArrayIndex(GetArrayIndexFromId(id))) { *done = true; @@ -1955,8 +1959,10 @@ bool XrayWrapper::defineProperty(JSContext* cx, ObjectOpResult& result) const { assertEnteredPolicy(cx, wrapper, id, BaseProxyHandler::SET); - Rooted existing_desc(cx); - if (!JS_GetPropertyDescriptorById(cx, wrapper, id, &existing_desc)) { + Rooted> existingDesc(cx); + Rooted existingHolder(cx); + if (!JS_GetPropertyDescriptorById(cx, wrapper, id, &existingDesc, + &existingHolder)) { return false; } @@ -1964,28 +1970,29 @@ bool XrayWrapper::defineProperty(JSContext* cx, // non-own properties, since the above lookup is not limited to own // properties. At present, this may not always do the right thing because // we often lie (sloppily) about where we found properties and set - // desc.object() to |wrapper|. Once we fully fix our Xray prototype semantics, - // this should work as intended. - if (existing_desc.object() == wrapper && !existing_desc.configurable()) { + // existingHolder to |wrapper|. Once we fully fix our Xray prototype + // semantics, this should work as intended. + if (existingDesc.isSome() && existingHolder == wrapper && + !existingDesc->configurable()) { // We have a non-configurable property. See if the caller is trying to // re-configure it in any way other than making it non-writable. - if (existing_desc.isAccessorDescriptor() || desc.isAccessorDescriptor() || + if (existingDesc->isAccessorDescriptor() || desc.isAccessorDescriptor() || (desc.hasEnumerable() && - existing_desc.enumerable() != desc.enumerable()) || - (desc.hasWritable() && !existing_desc.writable() && desc.writable())) { + existingDesc->enumerable() != desc.enumerable()) || + (desc.hasWritable() && !existingDesc->writable() && desc.writable())) { // We should technically report non-configurability in strict mode, but // doing that via JSAPI used to be a lot of trouble. See bug 1135997. return result.succeed(); } - if (!existing_desc.writable()) { + if (!existingDesc->writable()) { // Same as the above for non-writability. return result.succeed(); } } bool done = false; - if (!Traits::singleton.defineProperty(cx, wrapper, id, desc, existing_desc, - result, &done)) { + if (!Traits::singleton.defineProperty(cx, wrapper, id, desc, existingDesc, + existingHolder, result, &done)) { return false; } if (done) { diff --git a/js/xpconnect/wrappers/XrayWrapper.h b/js/xpconnect/wrappers/XrayWrapper.h index a724ccfc0e16..6c9fed784c6c 100644 --- a/js/xpconnect/wrappers/XrayWrapper.h +++ b/js/xpconnect/wrappers/XrayWrapper.h @@ -154,10 +154,12 @@ class DOMXrayTraits : public XrayTraits { bool delete_(JSContext* cx, JS::HandleObject wrapper, JS::HandleId id, JS::ObjectOpResult& result); - bool defineProperty(JSContext* cx, JS::HandleObject wrapper, JS::HandleId id, - JS::Handle desc, - JS::Handle existingDesc, - JS::ObjectOpResult& result, bool* done); + bool defineProperty( + JSContext* cx, JS::HandleObject wrapper, JS::HandleId id, + JS::Handle desc, + JS::Handle> existingDesc, + JS::Handle existingHolder, JS::ObjectOpResult& result, + bool* done); virtual bool enumerateNames(JSContext* cx, JS::HandleObject wrapper, unsigned flags, JS::MutableHandleIdVector props); static bool call(JSContext* cx, JS::HandleObject wrapper, @@ -193,10 +195,12 @@ class JSXrayTraits : public XrayTraits { bool delete_(JSContext* cx, JS::HandleObject wrapper, JS::HandleId id, JS::ObjectOpResult& result); - bool defineProperty(JSContext* cx, JS::HandleObject wrapper, JS::HandleId id, - JS::Handle desc, - JS::Handle existingDesc, - JS::ObjectOpResult& result, bool* defined); + bool defineProperty( + JSContext* cx, JS::HandleObject wrapper, JS::HandleId id, + JS::Handle desc, + JS::Handle> existingDesc, + JS::Handle existingHolder, JS::ObjectOpResult& result, + bool* defined); virtual bool enumerateNames(JSContext* cx, JS::HandleObject wrapper, unsigned flags, JS::MutableHandleIdVector props); @@ -297,10 +301,12 @@ class OpaqueXrayTraits : public XrayTraits { JS::HandleObject holder, JS::HandleId id, JS::MutableHandle desc) override; - bool defineProperty(JSContext* cx, JS::HandleObject wrapper, JS::HandleId id, - JS::Handle desc, - JS::Handle existingDesc, - JS::ObjectOpResult& result, bool* defined) { + bool defineProperty( + JSContext* cx, JS::HandleObject wrapper, JS::HandleId id, + JS::Handle desc, + JS::Handle> existingDesc, + JS::Handle existingHolder, JS::ObjectOpResult& result, + bool* defined) { *defined = false; return true; }