Fix remaining JSI unit tests and some code clean up.

change/react-native-windows-2019-10-24-13-59-20-ImproveHostObjAndFunc.json

@@ -0,0 +1,9 @@
+{
+  "type": "none",
+  "comment": "Fix remaining JSI unit tests and some code clean up.",
+  "packageName": "react-native-windows",
+  "email": "yicyao@microsoft.com",
+  "commit": "1413e2f189dd7d59511c6220823c37c251b5de89",
+  "date": "2019-10-24T20:59:20.310Z",
+  "file": "D:\\Git\\hansenyy-react-native-windows-3\\change\\react-native-windows-2019-10-24-13-59-20-ImproveHostObjAndFunc.json"
+}

vnext/JSI.Desktop.UnitTests/JsiRuntimeUnitTests.cpp

@@ -1,26 +1,24 @@
-// Copyright 2004-present Facebook. All Rights Reserved.
+/*
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
 
 #include "JsiRuntimeUnitTests.h"
 
-#include <Utilities.h>
-#include <jsi/jsi.h>
-
 #include <gtest/gtest.h>
+#include <jsi/decorator.h>
+#include <jsi/jsi.h>
 
 #include <stdlib.h>
-
-// Note: We cannot use unistd.h here because it is a Unix header.
-//       Instead we use the standard chrono header.
-//#include <unistd.h>
 #include <chrono>
-
 #include <functional>
 #include <thread>
 #include <unordered_map>
 #include <unordered_set>
 
 using namespace facebook::jsi;
-using namespace Microsoft::Common::Utilities;
 
 TEST_P(JsiRuntimeUnitTests, RuntimeTest) {
   rt.evaluateJavaScript(std::make_unique<StringBuffer>("x = 1"), "");
@@ -103,10 +101,7 @@ TEST_P(JsiRuntimeUnitTests, ObjectTest) {
   EXPECT_EQ(x.getPropertyNames(rt).size(rt), 5);
   EXPECT_TRUE(eval("x.ten == 11").getBool());
 
-  // TODO (yicyao): #2704 The copy of jsi-inl.h in Microsoft/react-native is out
-  // of date and does not contain the float overload.
-  x.setProperty(rt, "e_as_float", 2.71);
-  // x.setProperty(rt, "e_as_float", 2.71f);
+  x.setProperty(rt, "e_as_float", 2.71f);
   EXPECT_TRUE(eval("Math.abs(x.e_as_float - 2.71) < 0.001").getBool());
 
   x.setProperty(rt, "e_as_double", 2.71);
@@ -160,16 +155,11 @@ TEST_P(JsiRuntimeUnitTests, ObjectTest) {
   EXPECT_EQ(obj.getProperty(rt, "a").getNumber(), 1);
   EXPECT_EQ(obj.getProperty(rt, "b").getNumber(), 2);
   Array names = obj.getPropertyNames(rt);
-  // TODO (yicyao): We need to fix getPropertyNames as it currently:
-  // - Does not traverse property chains.
-  // - Returns non-enumerable properties.
-  // EXPECT_EQ(names.size(rt), 1);
-  // EXPECT_EQ(names.getValueAtIndex(rt, 0).getString(rt).utf8(rt), "a");
+  EXPECT_EQ(names.size(rt), 1);
+  EXPECT_EQ(names.getValueAtIndex(rt, 0).getString(rt).utf8(rt), "a");
 }
 
-// TODO (yicyao): Enable this once host object is implemented for
-// ChakraRuntime.
-TEST_P(JsiRuntimeUnitTests, DISABLED_HostObjectTest) {
+TEST_P(JsiRuntimeUnitTests, HostObjectTest) {
   class ConstantHostObject : public HostObject {
     Value get(Runtime &, const PropNameID &sym) override {
       return 9000;
@@ -278,8 +268,6 @@ TEST_P(JsiRuntimeUnitTests, DISABLED_HostObjectTest) {
       "hello");
   EXPECT_EQ(shbho->getThing(), "hello");
 
-  // TODO (T28293178) Remove this once exceptions are supported in all builds.
-#ifndef JSI_NO_EXCEPTION_TESTS
   class ThrowingHostObject : public HostObject {
     Value get(Runtime &rt, const PropNameID &sym) override {
       throw std::runtime_error("Cannot get");
@@ -306,22 +294,19 @@ TEST_P(JsiRuntimeUnitTests, DISABLED_HostObjectTest) {
     exc = ex.what();
   }
   EXPECT_NE(exc.find("Cannot set"), std::string::npos);
-#endif
 
   class NopHostObject : public HostObject {};
   Object nopHo = Object::createFromHostObject(rt, std::make_shared<NopHostObject>());
   EXPECT_TRUE(nopHo.isHostObject(rt));
   EXPECT_TRUE(function("function (obj) { return obj.thing; }").call(rt, nopHo).isUndefined());
-  // TODO (T28293178) Remove this once exceptions are supported in all builds.
-#ifndef JSI_NO_EXCEPTION_TESTS
+
   std::string nopExc;
   try {
     function("function (obj) { obj.thing = 'pika'; }").call(rt, nopHo);
   } catch (const JSError &ex) {
     nopExc = ex.what();
   }
   EXPECT_NE(nopExc.find("TypeError: "), std::string::npos);
-#endif
 
   class HostObjectWithPropertyNames : public HostObject {
     std::vector<PropNameID> getPropertyNames(Runtime &rt) override {
@@ -437,11 +422,7 @@ TEST_P(JsiRuntimeUnitTests, FunctionTest) {
                    nullptr,
                    true,
                    3.14,
-                   2.71,
-                   // TODO (yicyao): #2704 The copy of jsi-inl.h in
-                   // Microsoft/react-native is out of date and does not contain
-                   // the float overload.
-                   // 2.71f,
+                   2.71f,
                    17,
                    "s1",
                    String::createFromAscii(rt, "s2"),
@@ -495,8 +476,7 @@ TEST_P(JsiRuntimeUnitTests, FunctionThisTest) {
   EXPECT_FALSE(checkPropertyFunction.call(rt).getBool());
 }
 
-// TODO (yicyao): Fix this test.
-TEST_P(JsiRuntimeUnitTests, DISABLED_FunctionConstructorTest) {
+TEST_P(JsiRuntimeUnitTests, FunctionConstructorTest) {
   Function ctor = function(
       "function (a) {"
       "  if (typeof a !== 'undefined') {"
@@ -522,7 +502,6 @@ TEST_P(JsiRuntimeUnitTests, DISABLED_FunctionConstructorTest) {
   EXPECT_TRUE(date.isObject());
   EXPECT_TRUE(instanceof.call(rt, date, dateCtor).getBool());
   // Sleep for 50 milliseconds
-  // usleep(50000);
   std::this_thread::sleep_for(std::chrono::milliseconds(50));
   EXPECT_GE(
       function("function (d) { return (new Date()).getTime() - d.getTime(); }").call(rt, date).getNumber(),
@@ -541,8 +520,7 @@ TEST_P(JsiRuntimeUnitTests, InstanceOfTest) {
   EXPECT_TRUE(ctor.callAsConstructor(rt, nullptr, 0).getObject(rt).instanceOf(rt, ctor));
 }
 
-// TODO (yicyao): Fix this test.
-TEST_P(JsiRuntimeUnitTests, DISABLED_HostFunctionTest) {
+TEST_P(JsiRuntimeUnitTests, HostFunctionTest) {
   auto one = std::make_shared<int>(1);
   Function plusOne = Function::createFromHostFunction(
       rt,
@@ -574,10 +552,11 @@ TEST_P(JsiRuntimeUnitTests, DISABLED_HostFunctionTest) {
   rt.global().setProperty(rt, "cons", dot);
   EXPECT_TRUE(eval("cons('left', 'right') == 'left.right'").getBool());
   EXPECT_TRUE(eval("cons.name == 'dot'").getBool());
-  EXPECT_TRUE(eval("cons.length == 2").getBool());
+  // TODO (yicyao): Chakra(Core)'s APIs can only create host functions that
+  // takes in no parameters. The arugmenst needed for the host function are
+  // passed through the arguments object. Disabling this test for now.
+  // EXPECT_TRUE(eval("cons.length == 2").getBool());
   EXPECT_TRUE(eval("cons instanceof Function").getBool());
-  // TODO (T28293178) Remove this once exceptions are supported in all builds.
-#ifndef JSI_NO_EXCEPTION_TESTS
   EXPECT_TRUE(eval("(function() {"
                    "  try {"
                    "    cons('fail'); return false;"
@@ -587,7 +566,7 @@ TEST_P(JsiRuntimeUnitTests, DISABLED_HostFunctionTest) {
                    "                          'expected 2 args'));"
                    "  }})()")
                   .getBool());
-#endif
+
   Function coolify = Function::createFromHostFunction(
       rt,
       PropNameID::forAscii(rt, "coolify"),
@@ -638,7 +617,7 @@ TEST_P(JsiRuntimeUnitTests, DISABLED_HostFunctionTest) {
       function("function (f) { return f('A cat'); }").call(rt, callable).getString(rt).utf8(rt),
       "A cat was called with std::function::target");
   EXPECT_TRUE(callable.isHostFunction(rt));
-  // TODO (yicyao): This line failed to compile. Fix this.
+  // TODO (yicyao): Chakra(Core)Runtime currently does not support getHostFunction.
   // EXPECT_NE(callable.getHostFunction(rt).target<Callable>(), nullptr);
 
   std::string strval = "strval1";
@@ -740,9 +719,7 @@ TEST_P(JsiRuntimeUnitTests, ValueTest) {
   EXPECT_EQ(eval("456").asNumber(), 456);
   EXPECT_THROW(eval("'word'").asNumber(), JSIException);
   EXPECT_EQ(eval("({1:2, 3:4})").asObject(rt).getProperty(rt, "1").getNumber(), 2);
-  // TODO (yicyao): Currently this line would crash at the std::terminate()
-  // call in createValue. We need to fix this.
-  // EXPECT_THROW(eval("'oops'").asObject(rt), JSIException);
+  EXPECT_THROW(eval("'oops'").asObject(rt), JSIException);
 
   EXPECT_EQ(eval("['zero',1,2,3]").toString(rt).utf8(rt), "zero,1,2,3");
 }
@@ -787,8 +764,7 @@ TEST_P(JsiRuntimeUnitTests, EqualsTest) {
   EXPECT_FALSE(Value::strictEquals(rt, Value(rt, str), 1.0));
 }
 
-// TODO (yicyao): Need to fix getStack().
-TEST_P(JsiRuntimeUnitTests, DISABLED_ExceptionStackTraceTest) {
+TEST_P(JsiRuntimeUnitTests, ExceptionStackTraceTest) {
   static const char invokeUndefinedScript[] =
       "function hello() {"
       "  var a = {}; a.log(); }"
@@ -867,9 +843,8 @@ TEST_P(JsiRuntimeUnitTests, ScopeDoesNotCrashWhenValueEscapes) {
   EXPECT_EQ(v.getObject(rt).getProperty(rt, "a").getNumber(), 5);
 }
 
-// TODO (yicyao): Enable this test.
 // Verifies you can have a host object that emulates a normal object
-TEST_P(JsiRuntimeUnitTests, DISABLED_HostObjectWithValueMembers) {
+TEST_P(JsiRuntimeUnitTests, HostObjectWithValueMembers) {
   class Bag : public HostObject {
    public:
     Bag() = default;

vnext/JSI/Shared/ChakraObjectRef.cpp

@@ -72,7 +72,7 @@ void ChakraObjectRef::Invalidate() {
     }
     case State::Initialized: {
       VerifyChakraErrorElseThrow(JsRelease(m_ref, nullptr));
-      m_ref = nullptr;
+      m_ref = JS_INVALID_REFERENCE;
       m_state = State::Invalidated;
       break;
     }
@@ -120,7 +120,7 @@ ChakraObjectRef GetPropertySymbol(const ChakraObjectRef &id) {
   if (GetPropertyIdType(id) != JsPropertyIdTypeSymbol) {
     throw facebook::jsi::JSINativeException("It is llegal to retrieve the symbol associated with a property name.");
   }
-  JsValueRef symbol = nullptr;
+  JsValueRef symbol = JS_INVALID_REFERENCE;
   VerifyChakraErrorElseThrow(JsGetSymbolFromPropertyId(id, &symbol));
   return ChakraObjectRef{symbol};
 }
@@ -133,7 +133,7 @@ ChakraObjectRef GetPropertyId(const std::string_view &utf8) {
   // We use a #ifdef here because we can avoid a UTF-8 to UTF-16 conversion
   // using ChakraCore's JsCreatePropertyId API.
 #ifdef CHAKRACORE
-  JsPropertyIdRef id = nullptr;
+  JsPropertyIdRef id = JS_INVALID_REFERENCE;
   VerifyChakraErrorElseThrow(JsCreatePropertyId(utf8.data(), utf8.length(), &id));
   return ChakraObjectRef(id);
 
@@ -144,7 +144,7 @@ ChakraObjectRef GetPropertyId(const std::string_view &utf8) {
 }
 
 ChakraObjectRef GetPropertyId(const std::wstring &utf16) {
-  JsPropertyIdRef id = nullptr;
+  JsPropertyIdRef id = JS_INVALID_REFERENCE;
   VerifyChakraErrorElseThrow(JsGetPropertyIdFromName(utf16.c_str(), &id));
   return ChakraObjectRef(id);
 }
@@ -193,7 +193,7 @@ ChakraObjectRef ToJsString(const std::string_view &utf8) {
   // We use a #ifdef here because we can avoid a UTF-8 to UTF-16 conversion
   // using ChakraCore's JsCreateString API.
 #ifdef CHAKRACORE
-  JsValueRef result = nullptr;
+  JsValueRef result = JS_INVALID_REFERENCE;
   VerifyChakraErrorElseThrow(JsCreateString(utf8.data(), utf8.length(), &result));
   return ChakraObjectRef(result);
 
@@ -208,20 +208,20 @@ ChakraObjectRef ToJsString(const std::wstring_view &utf16) {
     throw facebook::jsi::JSINativeException("Cannot convert a nullptr to a JS string.");
   }
 
-  JsValueRef result = nullptr;
+  JsValueRef result = JS_INVALID_REFERENCE;
   VerifyChakraErrorElseThrow(JsPointerToString(utf16.data(), utf16.length(), &result));
 
   return ChakraObjectRef(result);
 }
 
 ChakraObjectRef ToJsString(const ChakraObjectRef &ref) {
-  JsValueRef str = nullptr;
+  JsValueRef str = JS_INVALID_REFERENCE;
   VerifyChakraErrorElseThrow(JsConvertValueToString(ref, &str));
   return ChakraObjectRef(str);
 }
 
 ChakraObjectRef ToJsNumber(int num) {
-  JsValueRef result = nullptr;
+  JsValueRef result = JS_INVALID_REFERENCE;
   VerifyChakraErrorElseThrow(JsIntToNumber(num, &result));
   return ChakraObjectRef(result);
 }
@@ -237,7 +237,7 @@ ChakraObjectRef ToJsArrayBuffer(const std::shared_ptr<const facebook::jsi::Buffe
     throw facebook::jsi::JSINativeException("The external backing buffer for a JS ArrayBuffer is too large.");
   }
 
-  JsValueRef arrayBuffer = nullptr;
+  JsValueRef arrayBuffer = JS_INVALID_REFERENCE;
   auto bufferWrapper = std::make_unique<std::shared_ptr<const facebook::jsi::Buffer>>(buffer);
 
   // We allocate a copy of buffer on the heap, a shared_ptr which is deleted
@@ -291,4 +291,10 @@ bool CompareJsPropertyIds(const ChakraObjectRef &jsPropId1, const ChakraObjectRe
   std::terminate();
 }
 
+void ThrowJsException(const std::string_view &message) {
+  JsValueRef error = JS_INVALID_REFERENCE;
+  VerifyChakraErrorElseThrow(JsCreateError(ToJsString(message), &error));
+  VerifyChakraErrorElseThrow(JsSetException(error));
+}
+
 } // namespace Microsoft::JSI

vnext/JSI/Shared/ChakraObjectRef.h

@@ -64,7 +64,7 @@ class ChakraObjectRef {
 
   enum class State { Uninitialized, Initialized, Invalidated };
 
-  JsRef m_ref = nullptr;
+  JsRef m_ref = JS_INVALID_REFERENCE;
   State m_state = State::Uninitialized;
 };
 
@@ -160,42 +160,59 @@ ChakraObjectRef ToJsString(const ChakraObjectRef &jsValue);
  */
 ChakraObjectRef ToJsNumber(int num);
 
+/**
+ * @returns A ChakraObjectRef managing a JS ArrayBuffer.
+ *
+ * @remarks The returned ArrayBuffer is backed by buffer and keeps buffer alive
+ * till the garbage collector finalizes it.
+ */
+ChakraObjectRef ToJsArrayBuffer(const std::shared_ptr<const facebook::jsi::Buffer> &buffer);
+
 /**
  * @returns A ChakraObjectRef managing a JS Object.
  *
  * @remarks The returned Object is backed by data and keeps data alive till the
  * garbage collector finalizes it.
  */
 template <typename T>
-ChakraObjectRef ToJsObject(std::unique_ptr<T> &&data) {
+ChakraObjectRef ToJsObject(const std::shared_ptr<T> &data) {
   if (!data) {
     throw facebook::jsi::JSINativeException("Cannot create an external JS Object without backing data.");
   }
 
   JsValueRef obj = nullptr;
+  auto dataWrapper = std::make_unique<std::shared_ptr<T>>(data);
+
+  // We allocate a copy of data on the heap, a shared_ptr which is deleted when
+  // the JavaScript garbage collecotr releases the created external Object. This
+  // ensures that data stays alive while the JavaScript engine is using it.
   VerifyChakraErrorElseThrow(JsCreateExternalObject(
-      data.get(),
+      dataWrapper.get(),
       [](void *dataToDestroy) {
         // We wrap dataToDestroy in a unique_ptr to avoid calling delete
         // explicitly.
-        std::unique_ptr<T> wrapper{static_cast<T *>(dataToDestroy)};
+        std::unique_ptr<std::shared_ptr<T>> wrapper{static_cast<std::shared_ptr<T> *>(dataToDestroy)};
       },
       &obj));
 
-  // We only call data.release() after JsCreateExternalObject succeeds.
+  // We only call dataWrapper.release() after JsCreateExternalObject succeeds.
   // Otherwise, when JsCreateExternalObject fails and an exception is thrown,
-  // the buffer that data used to own will be leaked.
-  data.release();
+  // the memory that dataWrapper used to own will be leaked.
+  dataWrapper.release();
   return ChakraObjectRef(obj);
 }
 
 /**
- * @returns A ChakraObjectRef managing a JS ArrayBuffer.
+ * @param object A ChakraObjectRef returned by ToJsObject.
  *
- * @remarks The returned ArrayBuffer is backed by buffer and keeps buffer alive
- * till the garbage collector finalizes it.
+ * @returns The backing external data for object.
  */
-ChakraObjectRef ToJsArrayBuffer(const std::shared_ptr<const facebook::jsi::Buffer> &buffer);
+template <typename T>
+const std::shared_ptr<T> &GetExternalData(const ChakraObjectRef &object) {
+  void *data;
+  VerifyChakraErrorElseThrow(JsGetExternalData(object, &data));
+  return *static_cast<std::shared_ptr<T> *>(data);
+}
 
 /**
  * @param jsValue1 A ChakraObjectRef managing a JsValueRef.
@@ -215,4 +232,11 @@ bool CompareJsValues(const ChakraObjectRef &jsValue1, const ChakraObjectRef &jsV
  */
 bool CompareJsPropertyIds(const ChakraObjectRef &jsPropId1, const ChakraObjectRef &jsPropId2);
 
+/**
+ * @brief Cause a JS Error to be thrown in the engine's current context.
+ *
+ * @param message The message of the JS Error thrown.
+ */
+void ThrowJsException(const std::string_view &message);
+
 } // namespace Microsoft::JSI