Add locking around calls from JSCExecuter

Reviewed By: javache

Differential Revision: D5488452

fbshipit-source-id: bda18e7948574117b8ce95894782e0e6e9c321de

Author: Kathy Gray

ReactCommon/cxxreact/JSCExecutor.cpp

@@ -287,6 +287,7 @@ void JSCExecutor::loadApplicationScript(std::unique_ptr<const JSBigString> scrip
   // TODO t15069155: reduce the number of overrides here
 #ifdef WITH_FBJSCEXTENSIONS
   if (auto fileStr = dynamic_cast<const JSBigFileString *>(script.get())) {
+    JSContextLock lock(m_context);
     JSLoadSourceStatus jsStatus;
     auto bcSourceCode = JSCreateSourceCodeFromFile(fileStr->fd(), jsSourceURL, nullptr, &jsStatus);
 
@@ -295,7 +296,6 @@ void JSCExecutor::loadApplicationScript(std::unique_ptr<const JSBigString> scrip
       if (!bcSourceCode) {
         throw std::runtime_error("Unexpected error opening compiled bundle");
       }
-
       evaluateSourceCode(m_context, bcSourceCode, jsSourceURL);
 
       flush();
@@ -332,6 +332,7 @@ void JSCExecutor::loadApplicationScript(std::unique_ptr<const JSBigString> scrip
 #endif
   {
     String jsScript;
+    JSContextLock lock(m_context);
     {
       SystraceSection s_("JSCExecutor::loadApplicationScript-createExpectingAscii");
       ReactMarker::logMarker(ReactMarker::JS_BUNDLE_STRING_CONVERT_START);
@@ -433,10 +434,10 @@ void JSCExecutor::flush() {
 
 void JSCExecutor::callFunction(const std::string& moduleId, const std::string& methodId, const folly::dynamic& arguments) {
   SystraceSection s("JSCExecutor::callFunction");
-
   // This weird pattern is because Value is not default constructible.
   // The lambda is inlined, so there's no overhead.
   auto result = [&] {
+    JSContextLock lock(m_context);
     try {
       if (!m_callFunctionReturnResultAndFlushedQueueJS) {
         bindBridge();
@@ -451,13 +452,13 @@ void JSCExecutor::callFunction(const std::string& moduleId, const std::string& m
         std::runtime_error("Error calling " + moduleId + "." + methodId));
     }
   }();
-
   callNativeModules(std::move(result));
 }
 
 void JSCExecutor::invokeCallback(const double callbackId, const folly::dynamic& arguments) {
   SystraceSection s("JSCExecutor::invokeCallback");
   auto result = [&] {
+    JSContextLock lock(m_context);
     try {
       if (!m_invokeCallbackAndReturnFlushedQueueJS) {
         bindBridge();
@@ -471,29 +472,29 @@ void JSCExecutor::invokeCallback(const double callbackId, const folly::dynamic&
         std::runtime_error(folly::to<std::string>("Error invoking callback ", callbackId)));
     }
   }();
-
   callNativeModules(std::move(result));
 }
 
 Value JSCExecutor::callFunctionSyncWithValue(
     const std::string& module, const std::string& method, Value args) {
   SystraceSection s("JSCExecutor::callFunction");
-
-  if (!m_callFunctionReturnResultAndFlushedQueueJS) {
-    bindBridge();
-  }
-  Object result = m_callFunctionReturnResultAndFlushedQueueJS->callAsFunction({
-    Value(m_context, String::createExpectingAscii(m_context, module)),
-    Value(m_context, String::createExpectingAscii(m_context, method)),
-    std::move(args),
-  }).asObject();
+  Object result = [&] {
+    JSContextLock lock(m_context);
+    if (!m_callFunctionReturnResultAndFlushedQueueJS) {
+      bindBridge();
+    }
+    return m_callFunctionReturnResultAndFlushedQueueJS->callAsFunction({
+      Value(m_context, String::createExpectingAscii(m_context, module)),
+      Value(m_context, String::createExpectingAscii(m_context, method)),
+      std::move(args),
+    }).asObject();
+    }();
 
   Value length = result.getProperty("length");
 
   if (!length.isNumber() || length.asInteger() != 2) {
     std::runtime_error("Return value of a callFunction must be an array of size 2");
   }
-
   callNativeModules(result.getPropertyAtIndex(1));
   return result.getPropertyAtIndex(0);
 }

ReactCommon/jschelpers/JSCHelpers.cpp

@@ -8,9 +8,24 @@
 
 #include <glog/logging.h>
 
+#if WITH_FBJSCEXTENSIONS
+#include <pthread.h>
+#endif
+
 #include "JavaScriptCore.h"
 #include "Value.h"
 
+#if WITH_FBJSCEXTENSIONS
+#undef ASSERT
+#undef WTF_EXPORT_PRIVATE
+
+#include <JavaScriptCore/config.h>
+#include <wtf/WTFThreadData.h>
+
+#undef TRUE
+#undef FALSE
+#endif
+
 namespace facebook {
 namespace react {
 
@@ -203,6 +218,42 @@ JSValueRef evaluateSourceCode(JSContextRef context, JSSourceCodeRef source, JSSt
 }
 #endif
 
+JSContextLock::JSContextLock(JSGlobalContextRef ctx) noexcept
+#if WITH_FBJSCEXTENSIONS
+  : ctx_(ctx),
+   globalLock_(PTHREAD_MUTEX_INITIALIZER)
+   {
+  WTFThreadData& threadData = wtfThreadData();
+
+  // Code below is responsible for acquiring locks. It should execute
+  // atomically, thus none of the functions invoked from now on are allowed to
+  // throw an exception
+  try {
+    if (!threadData.isDebuggerThread()) {
+      CHECK(0 == pthread_mutex_lock(&globalLock_));
+    }
+    JSLock(ctx_);
+  } catch (...) {
+    abort();
+  }
+}
+#else
+{}
+#endif
+
+
+JSContextLock::~JSContextLock() noexcept {
+  #if WITH_FBJSCEXTENSIONS
+  WTFThreadData& threadData = wtfThreadData();
+
+  JSUnlock(ctx_);
+  if (!threadData.isDebuggerThread()) {
+    CHECK(0 == pthread_mutex_unlock(&globalLock_));
+  }
+  #endif
+}
+
+
 JSValueRef translatePendingCppExceptionToJSError(JSContextRef ctx, const char *exceptionLocation) {
   try {
     throw;

ReactCommon/jschelpers/JSCHelpers.h

@@ -96,6 +96,23 @@ JSValueRef evaluateSourceCode(
     JSStringRef sourceURL);
 #endif
 
+/**
+ * A lock for protecting accesses to the JSGlobalContext
+ * This will be a no-op for most compilations, where #if WITH_FBJSCEXTENSIONS is false,
+ * but avoids deadlocks in execution environments with advanced locking requirements,
+ * particularly with uses of the pthread mutex lock
+**/
+class JSContextLock {
+public:
+  JSContextLock(JSGlobalContextRef ctx) noexcept;
+  ~JSContextLock() noexcept;
+private:
+#if WITH_FBJSCEXTENSIONS
+  JSGlobalContextRef ctx_;
+  pthread_mutex_t globalLock_;
+#endif
+};
+
 JSValueRef translatePendingCppExceptionToJSError(JSContextRef ctx, const char *exceptionLocation);
 JSValueRef translatePendingCppExceptionToJSError(JSContextRef ctx, JSObjectRef jsFunctionCause);