Bug 1712701 - Assert hasValue in PropertyDescriptor before allowing access to the value. r=jandem

Differential Revision: https://phabricator.services.mozilla.com/D116161

dom/indexedDB/KeyPath.cpp

@@ -104,8 +104,7 @@ nsresult GetJSValFromKeyPathString(
       JS::Rooted<JS::Value> intermediate(aCx);
       bool hasProp = false;
 
-      // Bug 1710041: This should check for isDataDescriptor!
-      if (desc.isSome()) {
+      if (desc.isSome() && desc->isDataDescriptor()) {
         intermediate = desc->value();
         hasProp = true;
       } else {

js/public/PropertyDescriptor.h

@@ -260,7 +260,7 @@ class JS_PUBLIC_API PropertyDescriptor {
 
   bool hasValue() const { return hasValue_; }
   Value value() const {
-    // TODO: Assert hasValue()
+    MOZ_ASSERT(hasValue());
     return value_;
   }
   void setValue(const Value& v) {
@@ -373,6 +373,7 @@ class WrappedPtrOperations<JS::PropertyDescriptor, Wrapper> {
 
   bool hasValue() const { return desc().hasValue(); }
   JS::Handle<JS::Value> value() const {
+    MOZ_ASSERT(hasValue());
     return JS::Handle<JS::Value>::fromMarkedLocation(desc().valueDoNotUse());
   }
 
@@ -403,6 +404,7 @@ class MutableWrappedPtrOperations<JS::PropertyDescriptor, Wrapper>
 
  public:
   JS::MutableHandle<JS::Value> value() {
+    MOZ_ASSERT(desc().hasValue());
     return JS::MutableHandle<JS::Value>::fromMarkedLocation(
         desc().valueDoNotUse());
   }

js/src/debugger/Object.cpp

@@ -2163,11 +2163,12 @@ bool DebuggerObject::getOwnPropertyDescriptor(
   if (desc_.isSome()) {
     Rooted<PropertyDescriptor> desc(cx, *desc_);
 
-    // Rewrap the debuggee values in desc for the debugger.
-    if (!dbg->wrapDebuggeeValue(cx, desc.value())) {
-      return false;
+    if (desc.hasValue()) {
+      // Rewrap the debuggee values in desc for the debugger.
+      if (!dbg->wrapDebuggeeValue(cx, desc.value())) {
+        return false;
+      }
     }
-
     if (desc.hasGetterObject()) {
       RootedValue get(cx, ObjectOrNullValue(desc.getterObject()));
       if (!dbg->wrapDebuggeeValue(cx, &get)) {

js/src/vm/Compartment.cpp

@@ -417,8 +417,12 @@ bool Compartment::wrap(JSContext* cx,
       return false;
     }
   }
-
-  return wrap(cx, desc.value());
+  if (desc.hasValue()) {
+    if (!wrap(cx, desc.value())) {
+      return false;
+    }
+  }
+  return true;
 }
 
 bool Compartment::wrap(JSContext* cx,

js/src/vm/JSContext-inl.h

@@ -187,7 +187,9 @@ class ContextChecks {
     if (desc.hasSetterObject()) {
       check(desc.setterObject(), argIndex);
     }
-    check(desc.value(), argIndex);
+    if (desc.hasValue()) {
+      check(desc.value(), argIndex);
+    }
   }
 
   void check(Handle<mozilla::Maybe<Value>> maybe, int argIndex) {

js/src/vm/NativeObject.cpp

@@ -1399,7 +1399,11 @@ static MOZ_ALWAYS_INLINE bool AddOrChangeProperty(
     }
   }
 
-  return CallAddPropertyHook(cx, obj, id, desc.value());
+  if (desc.isDataDescriptor()) {
+    return CallAddPropertyHook(cx, obj, id, desc.value());
+  }
+
+  return CallAddPropertyHook(cx, obj, id, UndefinedHandleValue);
 }
 
 // Versions of AddOrChangeProperty optimized for adding a plain data property.

js/xpconnect/src/Sandbox.cpp

@@ -730,7 +730,7 @@ bool SandboxProxyHandler::getPropertyDescriptorImpl(
       !WrapAccessorFunction(cx, desc.setterObject(), proxy)) {
     return false;
   }
-  if (desc.value().isObject()) {
+  if (desc.hasValue() && desc.value().isObject()) {
     RootedObject val(cx, &desc.value().toObject());
     if (JS::IsCallable(val) &&
         // Don't wrap DOM constructors: they don't care about the "this"

js/xpconnect/wrappers/ChromeObjectWrapper.cpp

@@ -22,7 +22,8 @@ bool ChromeObjectWrapper::defineProperty(JSContext* cx, HandleObject wrapper,
                                          HandleId id,
                                          Handle<PropertyDescriptor> desc,
                                          ObjectOpResult& result) const {
-  if (!AccessCheck::checkPassToPrivilegedCode(cx, wrapper, desc.value())) {
+  if (desc.hasValue() &&
+      !AccessCheck::checkPassToPrivilegedCode(cx, wrapper, desc.value())) {
     return false;
   }
   return ChromeObjectWrapperBase::defineProperty(cx, wrapper, id, desc, result);

js/xpconnect/wrappers/WaiveXrayWrapper.cpp

@@ -12,47 +12,40 @@ using namespace JS;
 
 namespace xpc {
 
-static bool WaiveAccessors(JSContext* cx,
-                           MutableHandle<PropertyDescriptor> desc) {
-  if (desc.hasGetterObject() && desc.getterObject()) {
-    RootedValue v(cx, JS::ObjectValue(*desc.getterObject()));
-    if (!WrapperFactory::WaiveXrayAndWrap(cx, &v)) {
-      return false;
-    }
-    desc.setGetterObject(&v.toObject());
-  }
-
-  if (desc.hasSetterObject() && desc.setterObject()) {
-    RootedValue v(cx, JS::ObjectValue(*desc.setterObject()));
-    if (!WrapperFactory::WaiveXrayAndWrap(cx, &v)) {
-      return false;
-    }
-    desc.setSetterObject(&v.toObject());
-  }
-  return true;
-}
-
 bool WaiveXrayWrapper::getOwnPropertyDescriptor(
     JSContext* cx, HandleObject wrapper, HandleId id,
     MutableHandle<mozilla::Maybe<PropertyDescriptor>> desc) const {
-  Rooted<mozilla::Maybe<PropertyDescriptor>> desc_(cx);
   if (!CrossCompartmentWrapper::getOwnPropertyDescriptor(cx, wrapper, id,
-                                                         &desc_)) {
+                                                         desc)) {
     return false;
   }
 
-  if (desc_.isNothing()) {
-    desc.reset();
+  if (desc.isNothing()) {
     return true;
   }
 
-  Rooted<PropertyDescriptor> pd(cx, *desc_);
-  if (!WrapperFactory::WaiveXrayAndWrap(cx, pd.value()) ||
-      !WaiveAccessors(cx, &pd)) {
-    return false;
+  Rooted<PropertyDescriptor> desc_(cx, *desc);
+  if (desc_.hasValue()) {
+    if (!WrapperFactory::WaiveXrayAndWrap(cx, desc_.value())) {
+      return false;
+    }
+  }
+  if (desc_.hasGetterObject() && desc_.getterObject()) {
+    RootedValue v(cx, JS::ObjectValue(*desc_.getterObject()));
+    if (!WrapperFactory::WaiveXrayAndWrap(cx, &v)) {
+      return false;
+    }
+    desc_.setGetterObject(&v.toObject());
+  }
+  if (desc_.hasSetterObject() && desc_.setterObject()) {
+    RootedValue v(cx, JS::ObjectValue(*desc_.setterObject()));
+    if (!WrapperFactory::WaiveXrayAndWrap(cx, &v)) {
+      return false;
+    }
+    desc_.setSetterObject(&v.toObject());
   }
 
-  desc.set(mozilla::Some(pd.get()));
+  desc.set(mozilla::Some(desc_.get()));
   return true;
 }
 

js/xpconnect/wrappers/XrayWrapper.cpp

@@ -1879,7 +1879,7 @@ static bool RecreateLostWaivers(JSContext* cx, const PropertyDescriptor* orig,
   // Compute whether the original objects were waived, and implicitly, whether
   // they were objects at all.
   bool valueWasWaived =
-      orig->value().isObject() &&
+      orig->hasValue() && orig->value().isObject() &&
       WrapperFactory::HasWaiveXrayFlag(&orig->value().toObject());
   bool getterWasWaived = orig->hasGetterObject() && orig->getterObject() &&
                          WrapperFactory::HasWaiveXrayFlag(orig->getterObject());