From 03e1f40c1e74681ea43b73cecb2face05ceab7ba Mon Sep 17 00:00:00 2001 From: Pieter De Baets Date: Fri, 26 May 2017 03:43:20 -0700 Subject: [PATCH] Cleanup and document Value wrapper (retry) Reviewed By: mhorowitz Differential Revision: D5120975 fbshipit-source-id: 6e9c80a57fdcf7f3dad21d5521fb928b52c924c7 --- React/CxxBridge/RCTJSCHelpers.mm | 8 ++- ReactCommon/cxxreact/JSCExecutor.cpp | 4 +- ReactCommon/cxxreact/JSCExecutor.h | 4 ++ ReactCommon/jschelpers/JSCHelpers.cpp | 92 ++++++++++++++------------- ReactCommon/jschelpers/JSCHelpers.h | 63 +++++++++++------- ReactCommon/jschelpers/Value.cpp | 63 ++++++++---------- ReactCommon/jschelpers/Value.h | 70 ++++++++++---------- 7 files changed, 158 insertions(+), 146 deletions(-) diff --git a/React/CxxBridge/RCTJSCHelpers.mm b/React/CxxBridge/RCTJSCHelpers.mm index 63c67acb6..e9c379004 100644 --- a/React/CxxBridge/RCTJSCHelpers.mm +++ b/React/CxxBridge/RCTJSCHelpers.mm @@ -14,6 +14,7 @@ #import #import +#import #import #import #import @@ -30,8 +31,9 @@ JSValueRef nativeLoggingHook( level = MAX(level, (RCTLogLevel)Value(ctx, arguments[1]).asNumber()); } if (argumentCount > 0) { - String message = Value(ctx, arguments[0]).toString(); - _RCTLogJavaScriptInternal(level, @(message.str().c_str())); + JSContext *contextObj = contextForGlobalContextRef(JSC_JSContextGetGlobalContext(ctx)); + JSValue *msg = [JSC_JSValue(ctx) valueWithJSValueRef:arguments[0] inContext:contextObj]; + _RCTLogJavaScriptInternal(level, [msg toString]); } return Value::makeUndefined(ctx); } @@ -45,7 +47,7 @@ JSValueRef nativePerformanceNow( } void RCTPrepareJSCExecutor() { - ReactMarker::logTaggedMarker = [](const ReactMarker::ReactMarkerId, const char* tag) {}; + ReactMarker::logTaggedMarker = [](const ReactMarker::ReactMarkerId, const char *tag) {}; PerfLogging::installNativeHooks = RCTFBQuickPerformanceLoggerConfigureHooks; JSNativeHooks::loggingHook = nativeLoggingHook; JSNativeHooks::nowHook = nativePerformanceNow; diff --git a/ReactCommon/cxxreact/JSCExecutor.cpp b/ReactCommon/cxxreact/JSCExecutor.cpp index 14cd32fec..c08dab694 100644 --- a/ReactCommon/cxxreact/JSCExecutor.cpp +++ b/ReactCommon/cxxreact/JSCExecutor.cpp @@ -346,7 +346,7 @@ void JSCExecutor::loadApplicationScript(std::unique_ptr scrip JSValueRef jsError; JSValueRef result = JSC_JSEvaluateBytecodeBundle(m_context, NULL, sourceFD, jsSourceURL, &jsError); if (result == nullptr) { - formatAndThrowJSException(m_context, jsError, jsSourceURL); + throw JSException(m_context, jsError, jsSourceURL); } } else #endif @@ -390,7 +390,7 @@ void JSCExecutor::bindBridge() throw(JSException) { batchedBridgeValue = requireBatchedBridge.asObject().callAsFunction({}); } if (batchedBridgeValue.isUndefined()) { - throwJSExecutionException("Could not get BatchedBridge, make sure your bundle is packaged correctly"); + throw JSException("Could not get BatchedBridge, make sure your bundle is packaged correctly"); } } diff --git a/ReactCommon/cxxreact/JSCExecutor.h b/ReactCommon/cxxreact/JSCExecutor.h index 473cca7c7..7c8aafa0c 100644 --- a/ReactCommon/cxxreact/JSCExecutor.h +++ b/ReactCommon/cxxreact/JSCExecutor.h @@ -15,6 +15,10 @@ #include #include +#ifndef RN_EXPORT +#define RN_EXPORT __attribute__((visibility("default"))) +#endif + namespace facebook { namespace react { diff --git a/ReactCommon/jschelpers/JSCHelpers.cpp b/ReactCommon/jschelpers/JSCHelpers.cpp index ade9eaf9e..8e2040415 100644 --- a/ReactCommon/jschelpers/JSCHelpers.cpp +++ b/ReactCommon/jschelpers/JSCHelpers.cpp @@ -68,6 +68,51 @@ JSObjectRef makeFunction( } +void JSException::buildMessage(JSContextRef ctx, JSValueRef exn, JSStringRef sourceURL, const char* errorMsg) { + std::ostringstream msgBuilder; + if (errorMsg && strlen(errorMsg) > 0) { + msgBuilder << errorMsg << ": "; + } + + Value exnValue = Value(ctx, exn); + msgBuilder << exnValue.toString().str(); + + // The null/empty-ness of source tells us if the JS came from a + // file/resource, or was a constructed statement. The location + // info will include that source, if any. + std::string locationInfo = sourceURL != nullptr ? String::ref(ctx, sourceURL).str() : ""; + Object exnObject = exnValue.asObject(); + auto line = exnObject.getProperty("line"); + if (line != nullptr && line.isNumber()) { + if (locationInfo.empty() && line.asInteger() != 1) { + // If there is a non-trivial line number, but there was no + // location info, we include a placeholder, and the line + // number. + locationInfo = folly::to(":", line.asInteger()); + } else if (!locationInfo.empty()) { + // If there is location info, we always include the line + // number, regardless of its value. + locationInfo += folly::to(":", line.asInteger()); + } + } + + if (!locationInfo.empty()) { + msgBuilder << " (" << locationInfo << ")"; + } + + auto exceptionText = msgBuilder.str(); + LOG(ERROR) << "Got JS Exception: " << exceptionText; + msg_ = std::move(exceptionText); + + Value jsStack = exnObject.getProperty("stack"); + if (jsStack.isString()) { + auto stackText = jsStack.toString().str(); + LOG(ERROR) << "Got JS Stack: " << stackText; + stack_ = std::move(stackText); + } +} + + JSObjectRef makeFunction( JSContextRef ctx, const char* name, @@ -122,11 +167,11 @@ void removeGlobal(JSGlobalContextRef ctx, const char* name) { Object::getGlobalObject(ctx).setProperty(name, Value::makeUndefined(ctx)); } -JSValueRef evaluateScript(JSContextRef context, JSStringRef script, JSStringRef source) { +JSValueRef evaluateScript(JSContextRef context, JSStringRef script, JSStringRef sourceURL) { JSValueRef exn, result; - result = JSC_JSEvaluateScript(context, script, NULL, source, 0, &exn); + result = JSC_JSEvaluateScript(context, script, NULL, sourceURL, 0, &exn); if (result == nullptr) { - formatAndThrowJSException(context, exn, source); + throw JSException(context, exn, sourceURL); } return result; } @@ -136,51 +181,12 @@ JSValueRef evaluateSourceCode(JSContextRef context, JSSourceCodeRef source, JSSt JSValueRef exn, result; result = JSEvaluateSourceCode(context, source, NULL, &exn); if (result == nullptr) { - formatAndThrowJSException(context, exn, sourceURL); + throw JSException(context, exn, sourceURL); } return result; } #endif -void formatAndThrowJSException(JSContextRef context, JSValueRef exn, JSStringRef source) { - Value exception = Value(context, exn); - std::string exceptionText = exception.toString().str(); - - // The null/empty-ness of source tells us if the JS came from a - // file/resource, or was a constructed statement. The location - // info will include that source, if any. - std::string locationInfo = source != nullptr ? String::ref(context, source).str() : ""; - Object exObject = exception.asObject(); - auto line = exObject.getProperty("line"); - if (line != nullptr && line.isNumber()) { - if (locationInfo.empty() && line.asInteger() != 1) { - // If there is a non-trivial line number, but there was no - // location info, we include a placeholder, and the line - // number. - locationInfo = folly::to(":", line.asInteger()); - } else if (!locationInfo.empty()) { - // If there is location info, we always include the line - // number, regardless of its value. - locationInfo += folly::to(":", line.asInteger()); - } - } - - if (!locationInfo.empty()) { - exceptionText += " (" + locationInfo + ")"; - } - - LOG(ERROR) << "Got JS Exception: " << exceptionText; - - Value jsStack = exObject.getProperty("stack"); - if (jsStack.isNull() || !jsStack.isString()) { - throwJSExecutionException("%s", exceptionText.c_str()); - } else { - LOG(ERROR) << "Got JS Stack: " << jsStack.toString().str(); - throwJSExecutionExceptionWithStack( - exceptionText.c_str(), jsStack.toString().str().c_str()); - } -} - JSValueRef translatePendingCppExceptionToJSError(JSContextRef ctx, const char *exceptionLocation) { std::ostringstream msg; try { diff --git a/ReactCommon/jschelpers/JSCHelpers.h b/ReactCommon/jschelpers/JSCHelpers.h index c30a562af..542b29f2b 100644 --- a/ReactCommon/jschelpers/JSCHelpers.h +++ b/ReactCommon/jschelpers/JSCHelpers.h @@ -2,33 +2,53 @@ #pragma once -#include -#include - -#include #include #include +#include + +#include +#include +#include namespace facebook { namespace react { -inline void throwJSExecutionException(const char* msg) { - throw JSException(msg); -} +class JSException : public std::exception { +public: + explicit JSException(const char* msg) + : msg_(msg) {} -template -inline void throwJSExecutionException(const char* fmt, Args... args) { - int msgSize = snprintf(nullptr, 0, fmt, args...); - msgSize = std::min(512, msgSize + 1); - char *msg = (char*) alloca(msgSize); - snprintf(msg, msgSize, fmt, args...); - throw JSException(msg); -} + template + explicit JSException(const char* fmt, Args... args) + : msg_(folly::stringPrintf(fmt, args...)) {} -template -inline void throwJSExecutionExceptionWithStack(const char* msg, const char* stack) { - throw JSException(msg, stack); -} + explicit JSException(JSContextRef ctx, JSValueRef exn, const char* msg) { + buildMessage(ctx, exn, nullptr, msg); + } + + explicit JSException(JSContextRef ctx, JSValueRef exn, JSStringRef sourceURL) { + buildMessage(ctx, exn, sourceURL, nullptr); + } + + template + explicit JSException(JSContextRef ctx, JSValueRef exn, JSStringRef sourceURL, const char* fmt, Args... args) { + buildMessage(ctx, exn, sourceURL, folly::stringPrintf(fmt, args...).c_str()); + } + + const std::string& getStack() const { + return stack_; + } + + virtual const char* what() const noexcept override { + return msg_.c_str(); + } + +private: + std::string msg_; + std::string stack_; + + void buildMessage(JSContextRef ctx, JSValueRef exn, JSStringRef sourceURL, const char* errorMsg); +}; using JSFunction = std::function; @@ -71,11 +91,6 @@ JSValueRef evaluateSourceCode( JSStringRef sourceURL); #endif -void formatAndThrowJSException( - JSContextRef ctx, - JSValueRef exn, - JSStringRef sourceURL); - JSValueRef translatePendingCppExceptionToJSError(JSContextRef ctx, const char *exceptionLocation); JSValueRef translatePendingCppExceptionToJSError(JSContextRef ctx, JSObjectRef jsFunctionCause); diff --git a/ReactCommon/jschelpers/Value.cpp b/ReactCommon/jschelpers/Value.cpp index 7f00ea973..399a307f1 100644 --- a/ReactCommon/jschelpers/Value.cpp +++ b/ReactCommon/jschelpers/Value.cpp @@ -17,24 +17,11 @@ namespace facebook { namespace react { -Value::Value(JSContextRef context, JSValueRef value) : - m_context(context), - m_value(value) -{ -} +Value::Value(JSContextRef context, JSValueRef value) + : m_context(context), m_value(value) {} -Value::Value(JSContextRef context, JSStringRef str) : - m_context(context), - m_value(JSC_JSValueMakeString(context, str)) -{ -} - -Value::Value(Value&& other) : - m_context(other.m_context), - m_value(other.m_value) -{ - other.m_value = nullptr; -} +Value::Value(JSContextRef context, JSStringRef str) + : m_context(context), m_value(JSC_JSValueMakeString(context, str)) {} JSContextRef Value::context() const { return m_context; @@ -43,9 +30,8 @@ JSContextRef Value::context() const { std::string Value::toJSONString(unsigned indent) const { JSValueRef exn; auto stringToAdopt = JSC_JSValueCreateJSONString(m_context, m_value, indent, &exn); - if (stringToAdopt == nullptr) { - std::string exceptionText = Value(m_context, exn).toString().str(); - throwJSExecutionException("Exception creating JSON string: %s", exceptionText.c_str()); + if (!stringToAdopt) { + throw JSException(m_context, exn, "Exception creating JSON string"); } return String::adopt(m_context, stringToAdopt).str(); } @@ -54,7 +40,7 @@ std::string Value::toJSONString(unsigned indent) const { Value Value::fromJSON(JSContextRef ctx, const String& json) { auto result = JSC_JSValueMakeFromJSONString(ctx, json); if (!result) { - throwJSExecutionException("Failed to create String from JSON: %s", json.str().c_str()); + throw JSException("Failed to create Value from JSON: %s", json.str().c_str()); } return Value(ctx, result); } @@ -133,24 +119,31 @@ JSValueRef Value::fromDynamicInner(JSContextRef ctx, const folly::dynamic& obj) } } -Object Value::asObject() { +Object Value::asObject() const { JSValueRef exn; JSObjectRef jsObj = JSC_JSValueToObject(context(), m_value, &exn); if (!jsObj) { - std::string exceptionText = Value(m_context, exn).toString().str(); - throwJSExecutionException("Failed to convert to object: %s", exceptionText.c_str()); + throw JSException(m_context, exn, "Failed to convert to object"); } return Object(context(), jsObj); } +String Value::toString() const { + JSValueRef exn; + JSStringRef jsStr = JSC_JSValueToStringCopy(context(), m_value, &exn); + if (!jsStr) { + throw JSException(m_context, exn, "Failed to convert to string"); + } + return String::adopt(context(), jsStr); +} + Value Value::makeError(JSContextRef ctx, const char *error) { JSValueRef exn; JSValueRef args[] = { Value(ctx, String(ctx, error)) }; JSObjectRef errorObj = JSC_JSObjectMakeError(ctx, 1, args, &exn); if (!errorObj) { - std::string exceptionText = Value(ctx, exn).toString().str(); - throwJSExecutionException("%s", exceptionText.c_str()); + throw JSException(ctx, exn, "Exception making error"); } return Value(ctx, errorObj); } @@ -179,8 +172,7 @@ Value Object::callAsFunction(JSObjectRef thisObj, int nArgs, const JSValueRef ar JSValueRef exn; JSValueRef result = JSC_JSObjectCallAsFunction(m_context, m_obj, thisObj, nArgs, args, &exn); if (!result) { - std::string exceptionText = Value(m_context, exn).toString().str(); - throwJSExecutionException("Exception calling object as function: %s", exceptionText.c_str()); + throw JSException(m_context, exn, "Exception calling object as function"); } return Value(m_context, result); } @@ -189,8 +181,7 @@ Object Object::callAsConstructor(std::initializer_list args) const { JSValueRef exn; JSObjectRef result = JSC_JSObjectCallAsConstructor(m_context, m_obj, args.size(), args.begin(), &exn); if (!result) { - std::string exceptionText = Value(m_context, exn).toString().str(); - throwJSExecutionException("Exception calling object as constructor: %s", exceptionText.c_str()); + throw JSException(m_context, exn, "Exception calling object as constructor"); } return Object(m_context, result); } @@ -199,8 +190,7 @@ Value Object::getProperty(const String& propName) const { JSValueRef exn; JSValueRef property = JSC_JSObjectGetProperty(m_context, m_obj, propName, &exn); if (!property) { - std::string exceptionText = Value(m_context, exn).toString().str(); - throwJSExecutionException("Failed to get property: %s", exceptionText.c_str()); + throw JSException(m_context, exn, nullptr, "Failed to get property '%s'", propName.str().c_str()); } return Value(m_context, property); } @@ -209,8 +199,7 @@ Value Object::getPropertyAtIndex(unsigned int index) const { JSValueRef exn; JSValueRef property = JSC_JSObjectGetPropertyAtIndex(m_context, m_obj, index, &exn); if (!property) { - std::string exceptionText = Value(m_context, exn).toString().str(); - throwJSExecutionException("Failed to get property at index %u: %s", index, exceptionText.c_str()); + throw JSException(m_context, exn, nullptr, "Failed to get property at index %d", index); } return Value(m_context, property); } @@ -223,8 +212,7 @@ void Object::setProperty(const String& propName, const Value& value) { JSValueRef exn = nullptr; JSC_JSObjectSetProperty(m_context, m_obj, propName, value, kJSPropertyAttributeNone, &exn); if (exn) { - std::string exceptionText = Value(m_context, exn).toString().str(); - throwJSExecutionException("Failed to set property: %s", exceptionText.c_str()); + throw JSException(m_context, exn, nullptr, "Failed to set property '%s'", propName.str().c_str()); } } @@ -232,8 +220,7 @@ void Object::setPropertyAtIndex(unsigned int index, const Value& value) { JSValueRef exn = nullptr; JSC_JSObjectSetPropertyAtIndex(m_context, m_obj, index, value, &exn); if (exn) { - std::string exceptionText = Value(m_context, exn).toString().str(); - throwJSExecutionException("Failed to set property: %s", exceptionText.c_str()); + throw JSException(m_context, exn, nullptr, "Failed to set property at index %d", index); } } diff --git a/ReactCommon/jschelpers/Value.h b/ReactCommon/jschelpers/Value.h index d90cedee3..92dce0834 100644 --- a/ReactCommon/jschelpers/Value.h +++ b/ReactCommon/jschelpers/Value.h @@ -15,30 +15,13 @@ namespace facebook { namespace react { +#ifndef RN_EXPORT +#define RN_EXPORT __attribute__((visibility("default"))) +#endif + class Value; -class Context; - -class JSException : public std::exception { -public: - explicit JSException(const char* msg) - : msg_(msg), stack_("") {} - - JSException(const char* msg, const char* stack) - : msg_(msg), stack_(stack) {} - - const std::string& getStack() const { - return stack_; - } - - virtual const char* what() const noexcept override { - return msg_.c_str(); - } - -private: - std::string msg_; - std::string stack_; -}; +// C++ object wrapper for JSStringRef class String : public noncopyable { public: explicit String(): m_context(nullptr), m_string(nullptr) {} // dummy empty constructor @@ -82,12 +65,16 @@ public: return m_string; } + JSContextRef context() const { + return m_context; + } + // Length in characters size_t length() const { return m_string ? JSC_JSStringGetLength(m_context, m_string) : 0; } - // Length in bytes of a null-terminated utf8 encoded value + // Length in bytes of a nul-terminated utf8 encoded value size_t utf8Size() const { return m_string ? JSC_JSStringGetMaximumUTF8CStringSize(m_context, m_string) : 0; } @@ -115,7 +102,7 @@ public: return unicode::utf16toUTF8(utf16, stringLength); } - // Assumes that utf8 is null terminated + // Assumes that utf8 is nul-terminated bool equals(const char* utf8) { return m_string ? JSC_JSStringIsEqualToUTF8CString(m_context, m_string, utf8) : false; } @@ -133,10 +120,13 @@ public: return createExpectingAscii(context, ascii.c_str(), ascii.size()); } + // Creates a String wrapper and increases the refcount of the JSStringRef static String ref(JSContextRef context, JSStringRef string) { return String(context, string, false); } + // Creates a String wrapper that takes over ownership of the string. The + // JSStringRef passed in must previously have been created or retained. static String adopt(JSContextRef context, JSStringRef string) { return String(context, string, true); } @@ -154,6 +144,9 @@ private: JSStringRef m_string; }; +// C++ object wrapper for JSObjectRef. The underlying JSObjectRef can be +// optionally protected. You must protect the object if it is ever +// heap-allocated, since otherwise you may end up with an invalid reference. class Object : public noncopyable { public: Object(JSContextRef context, JSObjectRef obj) : @@ -248,11 +241,16 @@ private: Value callAsFunction(JSObjectRef thisObj, int nArgs, const JSValueRef args[]) const; }; +// C++ object wrapper for JSValueRef. The underlying JSValueRef is not +// protected, so this class should always be used as a stack-allocated +// variable. class Value : public noncopyable { public: - __attribute__((visibility("default"))) Value(JSContextRef context, JSValueRef value); - __attribute__((visibility("default"))) Value(JSContextRef context, JSStringRef value); - __attribute__((visibility("default"))) Value(Value&&); + RN_EXPORT Value(JSContextRef context, JSValueRef value); + RN_EXPORT Value(JSContextRef context, JSStringRef value); + + RN_EXPORT Value(const Value &o) : Value(o.m_context, o.m_value) {} + RN_EXPORT Value(const String &o) : Value(o.context(), o) {} Value& operator=(Value&& other) { m_context = other.m_context; @@ -309,15 +307,13 @@ public: return getType() == kJSTypeObject; } - Object asObject(); + Object asObject() const; bool isString() const { return getType() == kJSTypeString; } - String toString() noexcept { - return String::adopt(context(), JSC_JSValueToStringCopy(context(), m_value, nullptr)); - } + String toString() const; static Value makeError(JSContextRef ctx, const char *error); @@ -333,13 +329,15 @@ public: return Value(ctx, JSC_JSValueMakeNull(ctx)); } - __attribute__((visibility("default"))) std::string toJSONString(unsigned indent = 0) const; - __attribute__((visibility("default"))) static Value fromJSON(JSContextRef ctx, const String& json); - __attribute__((visibility("default"))) static Value fromDynamic(JSContextRef ctx, const folly::dynamic& value); - __attribute__((visibility("default"))) JSContextRef context() const; -protected: + RN_EXPORT std::string toJSONString(unsigned indent = 0) const; + RN_EXPORT static Value fromJSON(JSContextRef ctx, const String& json); + RN_EXPORT static Value fromDynamic(JSContextRef ctx, const folly::dynamic& value); + RN_EXPORT JSContextRef context() const; + +private: JSContextRef m_context; JSValueRef m_value; + static JSValueRef fromDynamicInner(JSContextRef ctx, const folly::dynamic& obj); };