Avoid an assert when defining a prop on a Proxy array target

Summary: JSProxy::defineOwnProperty was implemented to call JSObject::defineOwnProperty. Recently, this would start to assert if a prop which looks like an array index was used. In order to avoid this, change JSProxy::defineOwnProperty to take a Handle<> as the name, and call JSObject::defineOwnComputedProperty, which knows how to handle index-ish properties. This has the downside of converting a named property from SymbolID to Handle to SymbolID in some proxy cases, but the perf loss is preferable to writing a SymbolID overload of JSProxy::defineOwnProperty. closes #371 Reviewed By: tmikov Differential Revision: D23914310 fbshipit-source-id: 633ad089bccf1aef559ffb95e05deaf6a22c52af
facebook · Sep 26, 2020 · 2201966 · 2201966
1 parent 5a0f9ea
commit 2201966
Show file tree

Hide file tree

Showing 4 changed files with 27 additions and 21 deletions.
diff --git a/include/hermes/VM/JSProxy.h b/include/hermes/VM/JSProxy.h
@@ -113,7 +113,7 @@ class JSProxy : public JSObject {
   static CallResult<bool> defineOwnProperty(
       Handle<JSObject> selfHandle,
       Runtime *runtime,
-      SymbolID name,
+      Handle<> nameValHandle,
       DefinePropertyFlags dpFlags,
       Handle<> valueOrAccessor,
       PropOpFlags opFlags);

diff --git a/lib/VM/JSObject.cpp b/lib/VM/JSObject.cpp
@@ -1447,14 +1447,9 @@ CallResult<bool> JSObject::putNamedWithReceiver_RJS(
             ->set(name, *valueHandle);
       }
       ComputedPropertyDescriptor desc;
+      Handle<> nameValHandle = runtime->makeHandle(name);
       CallResult<bool> descDefinedRes = getOwnComputedPrimitiveDescriptor(
-          receiverHandle,
-          runtime,
-          name.isUniqued() ? runtime->makeHandle(HermesValue::encodeStringValue(
-                                 runtime->getStringPrimFromSymbolID(name)))
-                           : runtime->makeHandle(name),
-          IgnoreProxy::No,
-          desc);
+          receiverHandle, runtime, nameValHandle, IgnoreProxy::No, desc);
       if (LLVM_UNLIKELY(descDefinedRes == ExecutionStatus::EXCEPTION)) {
         return ExecutionStatus::EXCEPTION;
       }
@@ -1465,7 +1460,7 @@ CallResult<bool> JSObject::putNamedWithReceiver_RJS(
         dpf = DefinePropertyFlags::getDefaultNewPropertyFlags();
       }
       return JSProxy::defineOwnProperty(
-          receiverHandle, runtime, name, dpf, valueHandle, opFlags);
+          receiverHandle, runtime, nameValHandle, dpf, valueHandle, opFlags);
     }
   }
 
@@ -1725,7 +1720,12 @@ CallResult<bool> JSObject::putComputedWithReceiver_RJS(
         dpf = DefinePropertyFlags::getDefaultNewPropertyFlags();
       }
       return JSProxy::defineOwnProperty(
-          receiverHandle, runtime, id, dpf, valueHandle, opFlags);
+          receiverHandle,
+          runtime,
+          nameValPrimitiveHandle,
+          dpf,
+          valueHandle,
+          opFlags);
     }
   }
 
@@ -2001,7 +2001,14 @@ CallResult<bool> JSObject::defineOwnPropertyInternal(
           selfHandle->flags_.lazyObject || selfHandle->flags_.proxyObject)) {
     if (selfHandle->flags_.proxyObject) {
       return JSProxy::defineOwnProperty(
-          selfHandle, runtime, name, dpFlags, valueOrAccessor, opFlags);
+          selfHandle,
+          runtime,
+          name.isUniqued() ? runtime->makeHandle(HermesValue::encodeStringValue(
+                                 runtime->getStringPrimFromSymbolID(name)))
+                           : runtime->makeHandle(name),
+          dpFlags,
+          valueOrAccessor,
+          opFlags);
     }
     assert(selfHandle->flags_.lazyObject && "descriptor flags are impossible");
     // if the property was not found and the object is lazy we need to

diff --git a/lib/VM/JSProxy.cpp b/lib/VM/JSProxy.cpp
@@ -626,7 +626,7 @@ CallResult<bool> JSProxy::getOwnProperty(
 CallResult<bool> JSProxy::defineOwnProperty(
     Handle<JSObject> selfHandle,
     Runtime *runtime,
-    SymbolID name,
+    Handle<> nameValHandle,
     DefinePropertyFlags dpFlags,
     Handle<> valueOrAccessor,
     PropOpFlags opFlags) {
@@ -642,8 +642,8 @@ CallResult<bool> JSProxy::defineOwnProperty(
   if (!*trapRes) {
     GCScope gcScope(runtime);
     //   a. Return ? target.[[GetOwnProperty]](P).
-    return JSObject::defineOwnProperty(
-        target, runtime, name, dpFlags, valueOrAccessor, opFlags);
+    return JSObject::defineOwnComputedPrimitive(
+        target, runtime, nameValHandle, dpFlags, valueOrAccessor, opFlags);
   }
   // 8. Let descObj be FromPropertyDescriptor(Desc).
   ComputedPropertyDescriptor desc;
@@ -663,7 +663,7 @@ CallResult<bool> JSProxy::defineOwnProperty(
       runtime,
       runtime->makeHandle(detail::slots(*selfHandle).handler),
       target.getHermesValue(),
-      HermesValue::encodeStringValue(runtime->getStringPrimFromSymbolID(name)),
+      nameValHandle.getHermesValue(),
       *descObjRes);
   if (trapResultRes == ExecutionStatus::EXCEPTION) {
     return ExecutionStatus::EXCEPTION;
@@ -682,12 +682,7 @@ CallResult<bool> JSProxy::defineOwnProperty(
   ComputedPropertyDescriptor targetDesc;
   MutableHandle<> targetDescValueOrAccessor{runtime};
   CallResult<bool> targetDescRes = JSObject::getOwnComputedDescriptor(
-      target,
-      runtime,
-      runtime->makeHandle(HermesValue::encodeStringValue(
-          runtime->getStringPrimFromSymbolID(name))),
-      targetDesc,
-      targetDescValueOrAccessor);
+      target, runtime, nameValHandle, targetDesc, targetDescValueOrAccessor);
   if (targetDescRes == ExecutionStatus::EXCEPTION) {
     return ExecutionStatus::EXCEPTION;
   }

diff --git a/test/hermes/proxy.js b/test/hermes/proxy.js
@@ -2121,5 +2121,9 @@ f.a = 1;
 f.b = 2;
 checkDeep({...f})(_ => ({a:1, b:2}))
 
+// Check that defining a property in a Proxy target which is an array
+// uses fast array access (this will trip an assert otherwise)
+new Proxy([], {}).unshift(0);
+
 print('done');
 // CHECK-LABEL: done