Implement ChakraRuntime::getPropertyNames() properly in native.

change/react-native-windows-2019-10-30-16-44-34-FixGetPropertyNames.json

@@ -0,0 +1,9 @@
+{
+  "type": "none",
+  "comment": "Implement ChakraRuntime::getPropertyNames() properly in native.",
+  "packageName": "react-native-windows",
+  "email": "yicyao@microsoft.com",
+  "commit": "7c96aa9beae65248b1b4c4c668b08ea56e4b4392",
+  "date": "2019-10-30T23:44:34.909Z",
+  "file": "D:\\Git\\hansenyy-react-native-windows-3\\change\\react-native-windows-2019-10-30-16-44-34-FixGetPropertyNames.json"
+}

vnext/JSI.Desktop.UnitTests/JsiRuntimeUnitTests.cpp

@@ -79,7 +79,7 @@ TEST_P(JsiRuntimeUnitTests, StringTest) {
   EXPECT_EQ(movedQuux.utf8(rt), "quux2");
 }
 
-TEST_P(JsiRuntimeUnitTests, DISABLED_ObjectTest) {
+TEST_P(JsiRuntimeUnitTests, ObjectTest) {
   eval("x = {1:2, '3':4, 5:'six', 'seven':['eight', 'nine']}");
   Object x = rt.global().getPropertyAsObject(rt, "x");
   EXPECT_EQ(x.getPropertyNames(rt).size(rt), 4);

vnext/JSI/Shared/ChakraObjectRef.cpp

@@ -220,6 +220,12 @@ ChakraObjectRef ToJsString(const ChakraObjectRef &ref) {
   return ChakraObjectRef(str);
 }
 
+int ToInteger(const ChakraObjectRef &jsNumber) {
+  int result = 0;
+  VerifyChakraErrorElseThrow(JsNumberToInt(jsNumber, &result));
+  return result;
+}
+
 ChakraObjectRef ToJsNumber(int num) {
   JsValueRef result = JS_INVALID_REFERENCE;
   VerifyChakraErrorElseThrow(JsIntToNumber(num, &result));

vnext/JSI/Shared/ChakraObjectRef.h

@@ -148,13 +148,20 @@ ChakraObjectRef ToJsString(const std::string_view &utf8);
 ChakraObjectRef ToJsString(const std::wstring_view &utf16);
 
 /**
- * @param jsValue A ChakraObjectRef mananing a JsValueRef.
+ * @param jsValue A ChakraObjectRef managing a JsValueRef.
  *
  * @returns A ChakraObjectRef managing the return value of the JS .toString
  * function.
  */
 ChakraObjectRef ToJsString(const ChakraObjectRef &jsValue);
 
+/**
+ * @param jsNumber A ChakraObjectRef managing a JS number.
+ *
+ * @returns The value of jsNumber converted to an integer.
+ */
+int ToInteger(const ChakraObjectRef &jsNumber);
+
 /**
  * @returns A ChakraObjectRef managing a JS number.
  */

vnext/JSI/Shared/ChakraRuntime.cpp

@@ -118,8 +118,7 @@ facebook::jsi::Value ChakraRuntime::evaluateJavaScript(
   if (!runtimeArgs().scriptStore) {
     if (!buffer)
       throw facebook::jsi::JSINativeException("Script buffer is empty!");
-    evaluateJavaScriptSimple(*buffer, sourceURL);
-    return facebook::jsi::Value::undefined();
+    return evaluateJavaScriptSimple(*buffer, sourceURL);
   }
 
   uint64_t scriptVersion = 0;
@@ -140,8 +139,7 @@ facebook::jsi::Value ChakraRuntime::evaluateJavaScript(
 
   // Simple evaluate if script version can't be computed.
   if (scriptVersion == 0) {
-    evaluateJavaScriptSimple(*scriptBuffer, sourceURL);
-    return facebook::jsi::Value::undefined();
+    return evaluateJavaScriptSimple(*scriptBuffer, sourceURL);
   }
 
   auto sharedScriptBuffer = std::shared_ptr<const facebook::jsi::Buffer>(std::move(scriptBuffer));
@@ -305,9 +303,7 @@ facebook::jsi::HostFunctionType &ChakraRuntime::getHostFunction(const facebook::
 facebook::jsi::Value ChakraRuntime::getProperty(
     const facebook::jsi::Object &obj,
     const facebook::jsi::PropNameID &name) {
-  JsValueRef result = JS_INVALID_REFERENCE;
-  VerifyJsErrorElseThrow(JsGetProperty(GetChakraObjectRef(obj), GetChakraObjectRef(name), &result));
-  return ToJsiValue(ChakraObjectRef(result));
+  return ToJsiValue(GetProperty(GetChakraObjectRef(obj), GetChakraObjectRef(name)));
 }
 
 facebook::jsi::Value ChakraRuntime::getProperty(const facebook::jsi::Object &obj, const facebook::jsi::String &name) {
@@ -376,25 +372,60 @@ bool ChakraRuntime::isHostFunction(const facebook::jsi::Function &obj) const {
 }
 
 facebook::jsi::Array ChakraRuntime::getPropertyNames(const facebook::jsi::Object &object) {
-  JsValueRef propertyNamesArrayRef;
-  VerifyJsErrorElseThrow(JsGetOwnPropertyNames(GetChakraObjectRef(object), &propertyNamesArrayRef));
+  // Handle to the null JS value.
+  ChakraObjectRef jsNull = ToChakraObjectRef(facebook::jsi::Value::null());
+
+  // Handle to the Object constructor.
+  ChakraObjectRef objectConstructor = GetProperty(GetChakraObjectRef(global()), "Object");
+
+  // Handle to the Object.prototype Object.
+  ChakraObjectRef objectPrototype = GetProperty(objectConstructor, "prototype");
+
+  // Handle to the Object.prototype.propertyIsEnumerable() Function.
+  ChakraObjectRef objectPrototypePropertyIsEnumerable = GetProperty(objectPrototype, "propertyIsEnumerable");
+
+  std::vector<ChakraObjectRef> enumerablePropNames{};
+  ChakraObjectRef currentObject = GetChakraObjectRef(object);
 
-  JsPropertyIdRef propertyId;
-  VerifyJsErrorElseThrow(JsGetPropertyIdFromName(L"length", &propertyId));
-  JsValueRef countRef;
-  VerifyJsErrorElseThrow(JsGetProperty(propertyNamesArrayRef, propertyId, &countRef));
-  int count;
+  // We have a small optimization here where we stop traversing the prototype
+  // chain as soon as we hit Object.prototype. However, we still need to check
+  // for null here, as one can create an Object with no prototype through
+  // Object.create(null).
+  while (!CompareJsValues(currentObject, objectPrototype) && !CompareJsValues(currentObject, jsNull)) {
+    JsValueRef propNamesRef = JS_INVALID_REFERENCE;
+    VerifyJsErrorElseThrow(JsGetOwnPropertyNames(currentObject, &propNamesRef));
+    ChakraObjectRef propNames(propNamesRef);
 
-  VerifyJsErrorElseThrow(JsNumberToInt(countRef, &count));
+    int numPropNames = ToInteger(GetProperty(propNames, "length"));
 
-  auto result = createArray(count);
-  for (int i = 0; i < count; i++) {
-    JsValueRef index;
-    VerifyJsErrorElseThrow(JsIntToNumber(i, &index));
-    JsValueRef propertyName;
-    VerifyJsErrorElseThrow(JsGetIndexedProperty(propertyNamesArrayRef, index, &propertyName));
+    for (int i = 0; i < numPropNames; ++i) {
+      JsValueRef propNameRef = JS_INVALID_REFERENCE;
+      VerifyJsErrorElseThrow(JsGetIndexedProperty(propNames, ToJsNumber(i), &propNameRef));
+      ChakraObjectRef propName(propNameRef);
 
-    result.setValueAtIndex(*this, i, MakePointer<facebook::jsi::String>(propertyName));
+      std::vector<JsValueRef> args = {currentObject, propName};
+
+      JsValueRef result;
+      VerifyJsErrorElseThrow(JsCallFunction(
+          objectPrototypePropertyIsEnumerable, args.data(), static_cast<unsigned short>(args.size()), &result));
+
+      bool propIsEnumerable = ToJsiValue(ChakraObjectRef(result)).getBool();
+
+      if (propIsEnumerable) {
+        enumerablePropNames.emplace_back(propName);
+      }
+    }
+
+    JsValueRef prototype = JS_INVALID_REFERENCE;
+    VerifyJsErrorElseThrow(JsGetPrototype(currentObject, &prototype));
+    currentObject = ChakraObjectRef(prototype);
+  }
+
+  size_t numEnumerablePropNames = enumerablePropNames.size();
+  facebook::jsi::Array result = createArray(numEnumerablePropNames);
+
+  for (size_t i = 0; i < numEnumerablePropNames; ++i) {
+    result.setValueAtIndex(*this, i, MakePointer<facebook::jsi::String>(enumerablePropNames[i]));
   }
 
   return result;
@@ -425,12 +456,26 @@ facebook::jsi::Array ChakraRuntime::createArray(size_t length) {
 
 size_t ChakraRuntime::size(const facebook::jsi::Array &arr) {
   assert(isArray(arr));
-  return static_cast<size_t>(GetProperty(arr, "length").asNumber());
+
+  int result = ToInteger(GetProperty(GetChakraObjectRef(arr), "length"));
+
+  if (result < 0 || result > SIZE_MAX) {
+    throw facebook::jsi::JSINativeException("Invalid JS array length detected.");
+  }
+
+  return static_cast<size_t>(result);
 }
 
 size_t ChakraRuntime::size(const facebook::jsi::ArrayBuffer &arrBuf) {
   assert(isArrayBuffer(arrBuf));
-  return static_cast<size_t>(GetProperty(arrBuf, "bytelength").asNumber());
+
+  int result = ToInteger(GetProperty(GetChakraObjectRef(arrBuf), "bytelength"));
+
+  if (result < 0 || result > SIZE_MAX) {
+    throw facebook::jsi::JSINativeException("Invalid JS array buffer bytelength detected.");
+  }
+
+  return static_cast<size_t>(result);
 }
 
 uint8_t *ChakraRuntime::data(const facebook::jsi::ArrayBuffer &arrBuf) {
@@ -566,13 +611,6 @@ bool ChakraRuntime::instanceOf(const facebook::jsi::Object &obj, const facebook:
 
 #pragma endregion Functions_inherited_from_Runtime
 
-facebook::jsi::Value ChakraRuntime::GetProperty(const facebook::jsi::Object &obj, const char *const name) const {
-  // We have to use const_casts here because createPropNameIDFromAscii and
-  // getProperty are not marked as const.
-  facebook::jsi::PropNameID propId = const_cast<ChakraRuntime *>(this)->createPropNameIDFromAscii(name, strlen(name));
-  return const_cast<ChakraRuntime *>(this)->getProperty(obj, propId);
-}
-
 void ChakraRuntime::VerifyJsErrorElseThrow(JsErrorCode error) {
   switch (error) {
     case JsNoError: {
@@ -697,6 +735,12 @@ std::vector<ChakraObjectRef> ChakraRuntime::ToChakraObjectRefs(const facebook::j
   return result;
 }
 
+ChakraObjectRef ChakraRuntime::GetProperty(const ChakraObjectRef &obj, const ChakraObjectRef &id) {
+  JsValueRef result = JS_INVALID_REFERENCE;
+  VerifyJsErrorElseThrow(JsGetProperty(obj, id, &result));
+  return ChakraObjectRef(result);
+}
+
 JsValueRef CALLBACK ChakraRuntime::HostFunctionCall(
     JsValueRef callee,
     bool isConstructCall,

vnext/JSI/Shared/ChakraRuntime.h

@@ -154,15 +154,6 @@ class ChakraRuntime : public facebook::jsi::Runtime {
   }
 
  private:
-  // Since the function
-  //   Object::getProperty(Runtime& runtime, const char* name)
-  // causes mulitple copies of name, we do not want to use it when implementing
-  // ChakraRuntime methods. This function does the same thing as
-  // Object::getProperty, but without the extra overhead. This function is
-  // declared as const so that it can be used when implementing
-  // isHostFunction and isHostObject.
-  facebook::jsi::Value GetProperty(const facebook::jsi::Object &obj, const char *const name) const;
-
   void VerifyJsErrorElseThrow(JsErrorCode error);
 
   // ChakraPointerValue is needed for working with Facebook's jsi::Pointer class
@@ -248,15 +239,35 @@ class ChakraRuntime : public facebook::jsi::Runtime {
   ChakraObjectRef ToChakraObjectRef(const facebook::jsi::Value &value);
   std::vector<ChakraObjectRef> ToChakraObjectRefs(const facebook::jsi::Value *value, size_t count);
 
-  // Host function and host object helpers
+  // Convenience functions for property access.
+  ChakraObjectRef GetProperty(const ChakraObjectRef &obj, const ChakraObjectRef &id);
+  inline ChakraObjectRef GetProperty(const ChakraObjectRef &obj, const char *const name) {
+    return GetProperty(obj, GetChakraObjectRef(createPropNameIDFromAscii(name, strlen(name))));
+  }
+
+  // Since the function
+  //   Object::getProperty(Runtime& runtime, const char* name)
+  // causes mulitple copies of name, we do not want to use it when implementing
+  // ChakraRuntime methods. This function does the same thing as
+  // Object::getProperty, but without the extra overhead. This function is
+  // declared as const so that it can be used when implementing
+  // isHostFunction and isHostObject.
+  inline facebook::jsi::Value GetProperty(const facebook::jsi::Object &obj, const char *const name) const {
+    // We have to use const_casts here because ToJsiValue and GetProperty cannnot
+    // be marked as const.
+    return const_cast<ChakraRuntime *>(this)->ToJsiValue(
+        const_cast<ChakraRuntime *>(this)->GetProperty(GetChakraObjectRef(obj), name));
+  }
+
+  // Host function helper
   static JsValueRef CALLBACK HostFunctionCall(
       JsValueRef callee,
       bool isConstructCall,
       JsValueRef *argumentsIncThis,
       unsigned short argumentCountIncThis,
       void *callbackState);
 
-  // For the following functions, runtime must be referring to a ChakraRuntime.
+  // Host object helpers; runtime must be referring to a ChakraRuntime.
   static facebook::jsi::Value HostObjectGetTrap(
       Runtime &runtime,
       const facebook::jsi::Value & /*thisVal*/,
@@ -272,7 +283,6 @@ class ChakraRuntime : public facebook::jsi::Runtime {
       const facebook::jsi::Value & /*thisVal*/,
       const facebook::jsi::Value *args,
       size_t count);
-
   facebook::jsi::Object createHostObjectProxyHandler() noexcept;
 
   // Promise Helpers