Bug 1160757. Make it clear that XrayWrapper::getPropertyDescriptor is unused. r=bholley
authorBoris Zbarsky <bzbarsky@mit.edu>
Mon, 21 Jan 2019 03:34:29 +0000
changeset 514725 565a04cfb0e443328f4b2c74e2ce439fc12eaa9f
parent 514724 a9ff8ccd4e3ddb1c53267f98a75a64a3a74f87bb
child 514726 ba9f9c0ce2561b72c6dedd0bf5817ba452bb4638
push id1953
push userffxbld-merge
push dateMon, 11 Mar 2019 12:10:20 +0000
treeherdermozilla-release@9c35dcbaa899 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbholley
bugs1160757
milestone66.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1160757. Make it clear that XrayWrapper::getPropertyDescriptor is unused. r=bholley Differential Revision: https://phabricator.services.mozilla.com/D15669
js/xpconnect/wrappers/XrayWrapper.cpp
--- a/js/xpconnect/wrappers/XrayWrapper.cpp
+++ b/js/xpconnect/wrappers/XrayWrapper.cpp
@@ -1869,55 +1869,18 @@ bool XrayWrapper<Base, Traits>::isExtens
   *extensible = true;
   return true;
 }
 
 template <typename Base, typename Traits>
 bool XrayWrapper<Base, Traits>::getPropertyDescriptor(
     JSContext* cx, HandleObject wrapper, HandleId id,
     JS::MutableHandle<PropertyDescriptor> desc) const {
-  // FIXME: This method is unused.  Will get sorted out in bug 1160757.
-  assertEnteredPolicy(cx, wrapper, id,
-                      BaseProxyHandler::GET | BaseProxyHandler::SET |
-                          BaseProxyHandler::GET_PROPERTY_DESCRIPTOR);
-  RootedObject target(cx, Traits::getTargetObject(wrapper));
-  RootedObject holder(cx, Traits::singleton.ensureHolder(cx, wrapper));
-
-  if (!holder) {
-    return false;
-  }
-
-  // Ordering is important here.
-  //
-  // We first need to call resolveOwnProperty, even before checking the holder,
-  // because there might be a new dynamic |own| property that appears and
-  // shadows a previously-resolved non-own property that we cached on the
-  // holder. This can happen with indexed properties on NodeLists, for example,
-  // which are |own| value props.
-  //
-  // resolveOwnProperty may or may not cache what it finds on the holder,
-  // depending on how ephemeral it decides the property is. This means that we
-  // have to first check the result of resolveOwnProperty, and _then_, if that
-  // comes up blank, check the holder for any cached native properties.
-
-  // Check resolveOwnProperty.
-  if (!Traits::singleton.resolveOwnProperty(cx, wrapper, target, holder, id,
-                                            desc)) {
-    return false;
-  }
-
-  // Check the holder.
-  if (!desc.object() &&
-      !JS_GetOwnPropertyDescriptorById(cx, holder, id, desc)) {
-    return false;
-  }
-  if (desc.object()) {
-    desc.object().set(wrapper);
-  }
-  return true;
+  MOZ_CRASH("Shouldn't be called: we return true for hasPrototype()");
+  return false;
 }
 
 template <typename Base, typename Traits>
 bool XrayWrapper<Base, Traits>::getOwnPropertyDescriptor(
     JSContext* cx, HandleObject wrapper, HandleId id,
     JS::MutableHandle<PropertyDescriptor> desc) const {
   assertEnteredPolicy(cx, wrapper, id,
                       BaseProxyHandler::GET | BaseProxyHandler::SET |
@@ -2102,30 +2065,29 @@ bool XrayWrapper<Base, Traits>::delete_(
 
   return Traits::singleton.delete_(cx, wrapper, id, result);
 }
 
 template <typename Base, typename Traits>
 bool XrayWrapper<Base, Traits>::get(JSContext* cx, HandleObject wrapper,
                                     HandleValue receiver, HandleId id,
                                     MutableHandleValue vp) const {
-  // Skip our Base if it isn't already ProxyHandler.
-
-  // This uses getPropertyDescriptor for backward compatibility with
-  // the old BaseProxyHandler::get implementation.
+  // This is called by Proxy::get, but since we return true for hasPrototype()
+  // it's only called for properties that hasOwn() claims we have as own
+  // properties.  Since we only need to worry about own properties, we can use
+  // getOwnPropertyDescriptor here.
   Rooted<PropertyDescriptor> desc(cx);
-  if (!getPropertyDescriptor(cx, wrapper, id, &desc)) {
+  if (!getOwnPropertyDescriptor(cx, wrapper, id, &desc)) {
     return false;
   }
   desc.assertCompleteIfFound();
 
-  if (!desc.object()) {
-    vp.setUndefined();
-    return true;
-  }
+  MOZ_ASSERT(desc.object(),
+             "hasOwn() claimed we have this property, so why would we not get "
+             "a descriptor here?");
 
   // Everything after here follows [[Get]] for ordinary objects.
   if (desc.isDataDescriptor()) {
     vp.set(desc.value());
     return true;
   }
 
   MOZ_ASSERT(desc.isAccessorDescriptor());
@@ -2139,32 +2101,25 @@ bool XrayWrapper<Base, Traits>::get(JSCo
   return Call(cx, receiver, getter, HandleValueArray::empty(), vp);
 }
 
 template <typename Base, typename Traits>
 bool XrayWrapper<Base, Traits>::set(JSContext* cx, HandleObject wrapper,
                                     HandleId id, HandleValue v,
                                     HandleValue receiver,
                                     ObjectOpResult& result) const {
-  MOZ_CRASH("Shouldn't be called");
+  MOZ_CRASH("Shouldn't be called: we return true for hasPrototype()");
   return false;
 }
 
 template <typename Base, typename Traits>
 bool XrayWrapper<Base, Traits>::has(JSContext* cx, HandleObject wrapper,
                                     HandleId id, bool* bp) const {
-  // This uses getPropertyDescriptor for backward compatibility with
-  // the old BaseProxyHandler::has implementation.
-  Rooted<PropertyDescriptor> desc(cx);
-  if (!getPropertyDescriptor(cx, wrapper, id, &desc)) {
-    return false;
-  }
-
-  *bp = !!desc.object();
-  return true;
+  MOZ_CRASH("Shouldn't be called: we return true for hasPrototype()");
+  return false;
 }
 
 template <typename Base, typename Traits>
 bool XrayWrapper<Base, Traits>::hasOwn(JSContext* cx, HandleObject wrapper,
                                        HandleId id, bool* bp) const {
   // Skip our Base if it isn't already ProxyHandler.
   return js::BaseProxyHandler::hasOwn(cx, wrapper, id, bp);
 }
@@ -2174,17 +2129,17 @@ bool XrayWrapper<Base, Traits>::getOwnEn
     JSContext* cx, HandleObject wrapper, AutoIdVector& props) const {
   // Skip our Base if it isn't already ProxyHandler.
   return js::BaseProxyHandler::getOwnEnumerablePropertyKeys(cx, wrapper, props);
 }
 
 template <typename Base, typename Traits>
 JSObject* XrayWrapper<Base, Traits>::enumerate(JSContext* cx,
                                                HandleObject wrapper) const {
-  MOZ_CRASH("Shouldn't be called");
+  MOZ_CRASH("Shouldn't be called: we return true for hasPrototype()");
   return nullptr;
 }
 
 template <typename Base, typename Traits>
 bool XrayWrapper<Base, Traits>::call(JSContext* cx, HandleObject wrapper,
                                      const JS::CallArgs& args) const {
   assertEnteredPolicy(cx, wrapper, JSID_VOID, BaseProxyHandler::CALL);
   // Hard cast the singleton since SecurityWrapper doesn't have one.