diff --git a/change/react-native-windows-2019-10-30-16-44-34-FixGetPropertyNames.json b/change/react-native-windows-2019-10-30-16-44-34-FixGetPropertyNames.json new file mode 100644 index 00000000000..5c41b07616e --- /dev/null +++ b/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" +} \ No newline at end of file diff --git a/vnext/JSI.Desktop.UnitTests/JsiRuntimeUnitTests.cpp b/vnext/JSI.Desktop.UnitTests/JsiRuntimeUnitTests.cpp index 15cf9744dc1..372adb6e7d0 100644 --- a/vnext/JSI.Desktop.UnitTests/JsiRuntimeUnitTests.cpp +++ b/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); diff --git a/vnext/JSI/Shared/ChakraObjectRef.cpp b/vnext/JSI/Shared/ChakraObjectRef.cpp index f60647712ae..ebbb0643e56 100644 --- a/vnext/JSI/Shared/ChakraObjectRef.cpp +++ b/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)); diff --git a/vnext/JSI/Shared/ChakraObjectRef.h b/vnext/JSI/Shared/ChakraObjectRef.h index 3ccab6bb4d5..2784f23a99d 100644 --- a/vnext/JSI/Shared/ChakraObjectRef.h +++ b/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. */ diff --git a/vnext/JSI/Shared/ChakraRuntime.cpp b/vnext/JSI/Shared/ChakraRuntime.cpp index d8e3b81c2bc..04eaac21dc1 100644 --- a/vnext/JSI/Shared/ChakraRuntime.cpp +++ b/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(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 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(propertyName)); + std::vector args = {currentObject, propName}; + + JsValueRef result; + VerifyJsErrorElseThrow(JsCallFunction( + objectPrototypePropertyIsEnumerable, args.data(), static_cast(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(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(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(result); } size_t ChakraRuntime::size(const facebook::jsi::ArrayBuffer &arrBuf) { assert(isArrayBuffer(arrBuf)); - return static_cast(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(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(this)->createPropNameIDFromAscii(name, strlen(name)); - return const_cast(this)->getProperty(obj, propId); -} - void ChakraRuntime::VerifyJsErrorElseThrow(JsErrorCode error) { switch (error) { case JsNoError: { @@ -697,6 +735,12 @@ std::vector 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, diff --git a/vnext/JSI/Shared/ChakraRuntime.h b/vnext/JSI/Shared/ChakraRuntime.h index 9dbdf49c029..5d53b534796 100644 --- a/vnext/JSI/Shared/ChakraRuntime.h +++ b/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,7 +239,27 @@ class ChakraRuntime : public facebook::jsi::Runtime { ChakraObjectRef ToChakraObjectRef(const facebook::jsi::Value &value); std::vector 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(this)->ToJsiValue( + const_cast(this)->GetProperty(GetChakraObjectRef(obj), name)); + } + + // Host function helper static JsValueRef CALLBACK HostFunctionCall( JsValueRef callee, bool isConstructCall, @@ -256,7 +267,7 @@ class ChakraRuntime : public facebook::jsi::Runtime { 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