Handle errors building JSErrors better

Summary:
If building a JSError causes a JSError, this would lead to infinite recursion.  This changes the error handling path so the problem is visible to JS.  There's also a few related cleanups included, and a new test case.

Changelog: [General] [Fixed] - If Error global is not callable when building an error, jsi will throw a JS exception back to JS code. #158

Reviewed By: tmikov, dulinriley

Differential Revision: D18796269

fbshipit-source-id: 57a45d144fa2ea5e78d18c27d3130611737dda96

Author: mhorowitz

ReactCommon/jsi/jsi/jsi.cpp

@@ -37,6 +37,27 @@ std::string kindToString(const Value& v, Runtime* rt = nullptr) {
   }
 }
 
+// getPropertyAsFunction() will try to create a JSError.  If the
+// failure is in building a JSError, this will lead to infinite
+// recursion.  This function is used in place of getPropertyAsFunction
+// when building JSError, to avoid that infinite recursion.
+Value callGlobalFunction(Runtime& runtime, const char* name, const Value& arg) {
+  Value v = runtime.global().getProperty(runtime, name);
+  if (!v.isObject()) {
+    throw JSINativeException(
+        std::string("callGlobalFunction: JS global property '") + name +
+        "' is " + kindToString(v, &runtime) + ", expected a Function");
+  }
+  Object o = v.getObject(runtime);
+  if (!o.isFunction(runtime)) {
+    throw JSINativeException(
+        std::string("callGlobalFunction: JS global property '") + name +
+        "' is a non-callable Object, expected a Function");
+  }
+  Function f = std::move(o).getFunction(runtime);
+  return f.call(runtime, arg);
+}
+
 } // namespace
 
 namespace detail {
@@ -142,9 +163,7 @@ Function Object::getPropertyAsFunction(Runtime& runtime, const char* name)
             kindToString(std::move(obj), &runtime) + ", expected a Function");
   };
 
-  Runtime::PointerValue* value = obj.ptr_;
-  obj.ptr_ = nullptr;
-  return Function(value);
+  return std::move(obj).getFunction(runtime);
 }
 
 Array Object::asArray(Runtime& runtime) const& {
@@ -347,7 +366,11 @@ JSError::JSError(Runtime& rt, Value&& value) {
 JSError::JSError(Runtime& rt, std::string msg) : message_(std::move(msg)) {
   try {
     setValue(
-        rt, rt.global().getPropertyAsFunction(rt, "Error").call(rt, message_));
+        rt,
+        callGlobalFunction(rt, "Error", String::createFromUtf8(rt, message_)));
+  } catch (const std::exception& ex) {
+    message_ = std::string(ex.what()) + " (while raising " + message_ + ")";
+    setValue(rt, String::createFromUtf8(rt, message_));
   } catch (...) {
     setValue(rt, Value());
   }
@@ -360,6 +383,8 @@ JSError::JSError(Runtime& rt, std::string msg, std::string stack)
     e.setProperty(rt, "message", String::createFromUtf8(rt, message_));
     e.setProperty(rt, "stack", String::createFromUtf8(rt, stack_));
     setValue(rt, std::move(e));
+  } catch (const std::exception& ex) {
+    setValue(rt, String::createFromUtf8(rt, ex.what()));
   } catch (...) {
     setValue(rt, Value());
   }
@@ -380,20 +405,23 @@ void JSError::setValue(Runtime& rt, Value&& value) {
       if (message_.empty()) {
         jsi::Value message = obj.getProperty(rt, "message");
         if (!message.isUndefined()) {
-          message_ = message.toString(rt).utf8(rt);
+          message_ =
+              callGlobalFunction(rt, "String", message).getString(rt).utf8(rt);
         }
       }
 
       if (stack_.empty()) {
         jsi::Value stack = obj.getProperty(rt, "stack");
         if (!stack.isUndefined()) {
-          stack_ = stack.toString(rt).utf8(rt);
+          stack_ =
+              callGlobalFunction(rt, "String", stack).getString(rt).utf8(rt);
         }
       }
     }
 
     if (message_.empty()) {
-      message_ = value_->toString(rt).utf8(rt);
+      message_ =
+          callGlobalFunction(rt, "String", *value_).getString(rt).utf8(rt);
     }
 
     if (stack_.empty()) {
@@ -403,6 +431,13 @@ void JSError::setValue(Runtime& rt, Value&& value) {
     if (what_.empty()) {
       what_ = message_ + "\n\n" + stack_;
     }
+  } catch (const std::exception& ex) {
+    message_ = std::string("[Exception while creating message string: ") +
+        ex.what() + "]";
+    stack_ = std::string("Exception while creating stack string: ") +
+        ex.what() + "]";
+    what_ =
+        std::string("Exception while getting value fields: ") + ex.what() + "]";
   } catch (...) {
     message_ = "[Exception caught creating message string]";
     stack_ = "[Exception caught creating stack string]";

ReactCommon/jsi/jsi/test/testlib.cpp

@@ -939,7 +939,7 @@ unsigned countOccurences(const std::string& of, const std::string& in) {
 } // namespace
 
 TEST_P(JSITest, JSErrorsArePropagatedNicely) {
-  unsigned callsBeoreError = 5;
+  unsigned callsBeforeError = 5;
 
   Function sometimesThrows = function(
       "function sometimesThrows(shouldThrow, callback) {"
@@ -953,9 +953,9 @@ TEST_P(JSITest, JSErrorsArePropagatedNicely) {
       rt,
       PropNameID::forAscii(rt, "callback"),
       0,
-      [&sometimesThrows, &callsBeoreError](
+      [&sometimesThrows, &callsBeforeError](
           Runtime& rt, const Value& thisVal, const Value* args, size_t count) {
-        return sometimesThrows.call(rt, --callsBeoreError == 0, args[0]);
+        return sometimesThrows.call(rt, --callsBeforeError == 0, args[0]);
       });
 
   try {
@@ -975,6 +975,55 @@ TEST_P(JSITest, JSErrorsCanBeConstructedWithStack) {
   EXPECT_EQ(err.getStack(), "stack");
 }
 
+TEST_P(JSITest, JSErrorDoesNotInfinitelyRecurse) {
+  Value globalString = rt.global().getProperty(rt, "String");
+  rt.global().setProperty(rt, "String", Value::undefined());
+  try {
+    eval("throw Error('whoops')");
+    FAIL() << "expected exception";
+  } catch (const JSError& ex) {
+    EXPECT_EQ(
+        ex.getMessage(),
+        "[Exception while creating message string: callGlobalFunction: "
+        "JS global property 'String' is undefined, expected a Function]");
+  }
+  rt.global().setProperty(rt, "String", globalString);
+
+  Value globalError = rt.global().getProperty(rt, "Error");
+  rt.global().setProperty(rt, "Error", Value::undefined());
+  try {
+    rt.global().getPropertyAsFunction(rt, "NotAFunction");
+    FAIL() << "expected exception";
+  } catch (const JSError& ex) {
+    EXPECT_EQ(
+        ex.getMessage(),
+        "callGlobalFunction: JS global property 'Error' is undefined, "
+        "expected a Function (while raising getPropertyAsObject: "
+        "property 'NotAFunction' is undefined, expected an Object)");
+  }
+
+  // If Error is missing, this is fundamentally a problem with JS code
+  // messing up the global object, so it should present in JS code as
+  // a catchable string.  Not an Error (because that's broken), or as
+  // a C++ failure.
+
+  auto fails = [](Runtime& rt, const Value&, const Value*, size_t) -> Value {
+    return rt.global().getPropertyAsObject(rt, "NotAProperty");
+  };
+  EXPECT_EQ(
+      function("function (f) { try { f(); return 'undefined'; }"
+               "catch (e) { return typeof e; } }")
+          .call(
+              rt,
+              Function::createFromHostFunction(
+                  rt, PropNameID::forAscii(rt, "fails"), 0, fails))
+          .getString(rt)
+          .utf8(rt),
+      "string");
+
+  rt.global().setProperty(rt, "Error", globalError);
+}
+
 TEST_P(JSITest, ScopeDoesNotCrashTest) {
   Scope scope(rt);
   Object o(rt);