Avoid passing GCHermesValue& around in JSObject

neildhar · facebook-github-bot · commit cfceeda4e2e1 · 2021-03-12T20:20:26.000-08:00
Summary:
Instead of passing around references to GCHermesValue, accept and
return HermesValues from get/set functions in JSObject. This reduces
the possibility of accidentally holding dangling references into a
relocated JSObject, which is going to be more of a risk with HV32
(since many more places will be able to allocate).

This change also lets us remove references from the API for
ArrayStorage.

Reviewed By: avp

Differential Revision: D26998350

fbshipit-source-id: defa243c3784c8013eaa3109e84eb5219b06c981
diff --git a/include/hermes/VM/JSArray.h b/include/hermes/VM/JSArray.h
@@ -304,7 +304,7 @@ class JSArray final : public ArrayImpl {
   }
 
   static uint32_t getLength(const JSArray *self) {
-    return directSlotRef(self, lengthPropIndex()).getNumber();
+    return getDirectSlotValue(self, lengthPropIndex()).getNumber();
   }
 
   /// Create an instance of Array, with [[Prototype]] initialized with
@@ -391,9 +391,11 @@ class JSArray final : public ArrayImpl {
  private:
   /// A helper to update the named '.length' property.
   static void putLength(JSArray *self, Runtime *runtime, uint32_t newLength) {
-    directSlotRef(self, lengthPropIndex())
-        .setNonPtr(
-            HermesValue::encodeNumberValue(newLength), &runtime->getHeap());
+    setDirectSlotValueNonPtr(
+        self,
+        lengthPropIndex(),
+        HermesValue::encodeNumberValue(newLength),
+        &runtime->getHeap());
   }
 
   /// Update the JavaScript '.length' property, which also resizes the array.
diff --git a/include/hermes/VM/JSObject.h b/include/hermes/VM/JSObject.h
@@ -512,21 +512,21 @@ class JSObject : public GCCell {
       JSObject *parent,
       PropOpFlags opFlags = PropOpFlags());
 
-  /// Return a reference to an internal property slot.
-  static GCHermesValue &
-  internalPropertyRef(JSObject *self, PointerBase *base, SlotIndex index) {
+  /// Return the value of an internal property slot.
+  static HermesValue
+  internalProperty(JSObject *self, PointerBase *base, SlotIndex index) {
     assert(
         HiddenClass::debugIsPropertyDefined(
             self->clazz_.get(base),
             base,
             InternalProperty::getSymbolID(index)) &&
         "internal slot must be reserved");
-    return namedSlotRef<PropStorage::Inline::Yes>(self, base, index);
+    return getNamedSlotValue<PropStorage::Inline::Yes>(self, base, index);
   }
 
   static HermesValue
   getInternalProperty(JSObject *self, PointerBase *base, SlotIndex index) {
-    return internalPropertyRef(self, base, index);
+    return internalProperty(self, base, index);
   }
 
   static void setInternalProperty(
@@ -595,29 +595,31 @@ class JSObject : public GCCell {
         OwnKeysFlags().plusIncludeSymbols().plusIncludeNonEnumerable());
   }
 
-  /// Return a reference to a direct property storage space by \p index.
+  /// Load a value from the direct property storage space by \p index.
   /// \pre index < DIRECT_PROPERTY_SLOTS.
-  static GCHermesValue &directSlotRef(JSObject *self, SlotIndex index);
-  static const GCHermesValue &directSlotRef(
+  inline static HermesValue getDirectSlotValue(
       const JSObject *self,
       SlotIndex index);
 
-  /// Return a reference to a slot in the "named value" storage space by
-  /// \p index.
-  /// \pre inl == PropStorage::Inline::Yes -> index <
-  ///   PropStorage::kValueToSegmentThreshold.
-  template <PropStorage::Inline inl = PropStorage::Inline::No>
-  static GCHermesValue &
-  namedSlotRef(JSObject *self, PointerBase *runtime, SlotIndex index);
+  /// Store a value to the direct property storage space by \p index.
+  /// \pre index < DIRECT_PROPERTY_SLOTS.
+  inline static void setDirectSlotValue(
+      JSObject *self,
+      SlotIndex index,
+      HermesValue value,
+      GC *gc);
+  inline static void setDirectSlotValueNonPtr(
+      JSObject *self,
+      SlotIndex index,
+      HermesValue value,
+      GC *gc);
 
   /// Load a value from the "named value" storage space by \p index.
   /// \pre inl == PropStorage::Inline::Yes -> index <
   /// PropStorage::kValueToSegmentThreshold.
   template <PropStorage::Inline inl = PropStorage::Inline::No>
-  static HermesValue
-  getNamedSlotValue(JSObject *self, PointerBase *runtime, SlotIndex index) {
-    return namedSlotRef<inl>(self, runtime, index);
-  }
+  inline static HermesValue
+  getNamedSlotValue(JSObject *self, PointerBase *runtime, SlotIndex index);
 
   /// Load a value from the "named value" storage space by the slot described by
   /// the property descriptor \p desc.
@@ -1591,21 +1593,38 @@ inline T *JSObject::initDirectPropStorage(Runtime *runtime, T *self) {
   return self;
 }
 
-inline GCHermesValue &JSObject::directSlotRef(JSObject *self, SlotIndex index) {
+inline HermesValue JSObject::getDirectSlotValue(
+    const JSObject *self,
+    SlotIndex index) {
   assert(index < DIRECT_PROPERTY_SLOTS && "Must be a direct property");
   return self->directProps()[index];
 }
 
-inline const GCHermesValue &JSObject::directSlotRef(
-    const JSObject *self,
-    SlotIndex index) {
+inline void JSObject::setDirectSlotValue(
+    JSObject *self,
+    SlotIndex index,
+    HermesValue value,
+    GC *gc) {
   assert(index < DIRECT_PROPERTY_SLOTS && "Must be a direct property");
-  return self->directProps()[index];
+  self->directProps()[index].set(value, gc);
+}
+
+inline void JSObject::setDirectSlotValueNonPtr(
+    JSObject *self,
+    SlotIndex index,
+    HermesValue value,
+    GC *gc) {
+  assert(index < DIRECT_PROPERTY_SLOTS && "Must be a direct property");
+  self->directProps()[index].setNonPtr(value, gc);
 }
 
 template <PropStorage::Inline inl>
-inline GCHermesValue &
-JSObject::namedSlotRef(JSObject *self, PointerBase *runtime, SlotIndex index) {
+inline HermesValue JSObject::getNamedSlotValue(
+    JSObject *self,
+    PointerBase *runtime,
+    SlotIndex index) {
+  assert(!self->flags_.proxyObject && "getNamedSlotValue called on a Proxy");
+
   if (LLVM_LIKELY(index < DIRECT_PROPERTY_SLOTS))
     return self->directProps()[index];
 
diff --git a/lib/VM/JSObject.cpp b/lib/VM/JSObject.cpp
@@ -2410,8 +2410,8 @@ void JSObject::_snapshotAddEdgesImpl(GCCell *cell, GC *gc, HeapSnapshot &snap) {
           return;
         }
         // Else, it's a user-visible property.
-        GCHermesValue &prop =
-            namedSlotRef(self, gc->getPointerBase(), desc.slot);
+        HermesValue prop =
+            getNamedSlotValue(self, gc->getPointerBase(), desc.slot);
         const llvh::Optional<HeapSnapshot::NodeID> idForProp =
             gc->getSnapshotID(prop);
         if (!idForProp) {
