Switch to synchronous strategy for unprotect

Summary:
Older versions of JSC (ios 11 and before) have a bug which I
believe the ProtectionQueue mechanism tickles:
https://bugs.webkit.org/show_bug.cgi?id=186827

This removes the ProtectionQueue and replaces it with an atomic flag
to avoid calling unprotect after VM shutdown.

This also fixes a race condition in shutdown.

Reviewed By: danzimm

Differential Revision: D12969953

fbshipit-source-id: fa3a14f3207be67a987ac3cf0fc1c9ce88837b0b

Author: mhorowitz

ReactCommon/jsi/JSCRuntime.cpp

@@ -6,23 +6,19 @@
 #include "JSCRuntime.h"
 
 #include <JavaScriptCore/JavaScript.h>
+#include <atomic>
 #include <condition_variable>
 #include <cstdlib>
 #include <mutex>
 #include <queue>
 #include <sstream>
 #include <thread>
 
-#ifndef NDEBUG
-#include <atomic>
-#endif
-
 namespace facebook {
 namespace jsc {
 
 namespace detail {
 class ArgsConverter;
-class ProtectionQueue;
 } // namespace detail
 
 class JSCRuntime;
@@ -65,7 +61,6 @@ class JSCRuntime : public jsi::Runtime {
 
  protected:
   friend class detail::ArgsConverter;
-  friend class detail::ProtectionQueue;
   class JSCStringValue final : public PointerValue {
 #ifndef NDEBUG
     JSCStringValue(JSStringRef str, std::atomic<intptr_t>& counter);
@@ -83,31 +78,26 @@ class JSCRuntime : public jsi::Runtime {
   };
 
   class JSCObjectValue final : public PointerValue {
-#ifndef NDEBUG
     JSCObjectValue(
         JSGlobalContextRef ctx,
-        detail::ProtectionQueue& pq,
-        JSObjectRef obj,
-        std::atomic<intptr_t>& counter);
-#else
-    JSCObjectValue(
-        JSGlobalContextRef context,
-        detail::ProtectionQueue& pq,
-        JSObjectRef obj);
+        const std::atomic<bool>& ctxInvalid,
+        JSObjectRef obj
+#ifndef NDEBUG
+        ,
+        std::atomic<intptr_t>& counter
 #endif
+                   );
 
     void invalidate() override;
-    void unprotect();
 
     JSGlobalContextRef ctx_;
+    const std::atomic<bool>& ctxInvalid_;
     JSObjectRef obj_;
-    detail::ProtectionQueue& protectionQueue_;
 #ifndef NDEBUG
     std::atomic<intptr_t>& counter_;
 #endif
    protected:
     friend class JSCRuntime;
-    friend class detail::ProtectionQueue;
   };
 
   PointerValue* cloneString(const Runtime::PointerValue* pv) override;
@@ -202,10 +192,8 @@ class JSCRuntime : public jsi::Runtime {
   void checkException(JSValueRef res, JSValueRef exc, const char* msg);
 
   JSGlobalContextRef ctx_;
+  std::atomic<bool> ctxInvalid_;
   std::string desc_;
-  // We make this a pointer so that we can control explicitly when it's deleted
-  // namely before the context is released.
-  mutable std::unique_ptr<detail::ProtectionQueue> protectionQueue_;
 #ifndef NDEBUG
   mutable std::atomic<intptr_t> objectCounter_;
   mutable std::atomic<intptr_t> stringCounter_;
@@ -293,106 +281,14 @@ std::string to_string(void* value) {
 }
 } // namespace
 
-// UnprotectQueue
-namespace detail {
-class ProtectionQueue {
- public:
-  ProtectionQueue()
-      : ctxInvalid_(false)
-      , shuttingDown_(false)
-#ifndef NDEBUG
-        ,
-        didShutdown_ {
-    false
-  }
-#endif
-  , unprotectorThread_(&ProtectionQueue::unprotectThread, this) {}
-
-  void setContextInvalid() {
-    std::lock_guard<std::mutex> locker(mutex_);
-    ctxInvalid_ = true;
-  }
-
-  void shutdown() {
-    {
-      std::lock_guard<std::mutex> locker(mutex_);
-      assert(ctxInvalid_);
-      shuttingDown_ = true;
-      notEmpty_.notify_one();
-    }
-    unprotectorThread_.join();
-  }
-
-  void push(JSCRuntime::JSCObjectValue* value) {
-    std::lock_guard<std::mutex> locker(mutex_);
-    assert(!didShutdown_);
-    queue_.push(value);
-    notEmpty_.notify_one();
-  }
-
- private:
-  // This this the function that runs in the background deleting (and thus
-  // unprotecting JSObjectRefs as need be). This needs to be explicitly on a
-  // separate thread so that we don't have the API lock when `JSValueUnprotect`
-  // is called already (i.e. if we did this on the same thread that calls
-  // invalidate() on an Object then we might be in the middle of a GC pass, and
-  // already have the API lock).
-  void unprotectThread() {
-#if defined(__APPLE__)
-    pthread_setname_np("jsc-protectionqueue-unprotectthread");
-#endif
-
-    std::unique_lock<std::mutex> locker(mutex_);
-    while (!shuttingDown_ || !queue_.empty()) {
-      if (queue_.empty()) {
-        // This will wake up when shuttingDown_ becomes true
-        notEmpty_.wait(locker);
-      } else {
-        JSCRuntime::JSCObjectValue* value = queue_.front();
-        queue_.pop();
-        // We need to drop the lock here since this calls JSValueUnprotect and
-        // that may make another GC pass, which could call another finalizer
-        // and thus attempt to push to this queue then, and deadlock.
-        locker.unlock();
-        if (ctxInvalid_) {
-          value->ctx_ = nullptr;
-        }
-        value->unprotect();
-        locker.lock();
-      }
-    }
-#ifndef NDEBUG
-    didShutdown_ = true;
-#endif
-  }
-  // Used to lock the queue_/shuttingDown_ ivars
-  std::mutex mutex_;
-  // Used to signal queue_ empty status changing
-  std::condition_variable notEmpty_;
-  // The actual underlying queue
-  std::queue<JSCRuntime::JSCObjectValue*> queue_;
-  // A flag which is set before shutting down JSC
-  bool ctxInvalid_;
-  // A flag dictating whether or not we need to stop all execution
-  bool shuttingDown_;
-#ifndef NDEBUG
-  bool didShutdown_;
-#endif
-  // The thread that dequeues and processes the queue. Note this is the last
-  // member on purpose so the thread starts up after all state has been
-  // properly initialized
-  std::thread unprotectorThread_;
-};
-} // namespace detail
-
 JSCRuntime::JSCRuntime()
     : JSCRuntime(JSGlobalContextCreateInGroup(nullptr, nullptr)) {
   JSGlobalContextRelease(ctx_);
 }
 
 JSCRuntime::JSCRuntime(JSGlobalContextRef ctx)
     : ctx_(JSGlobalContextRetain(ctx)),
-      protectionQueue_(std::make_unique<detail::ProtectionQueue>())
+      ctxInvalid_(false)
 #ifndef NDEBUG
       ,
       objectCounter_(0),
@@ -405,15 +301,11 @@ JSCRuntime::~JSCRuntime() {
   // On shutting down and cleaning up: when JSC is actually torn down,
   // it calls JSC::Heap::lastChanceToFinalize internally which
   // finalizes anything left over.  But at this point,
-  // JSUnprotectValue() can no longer be called.  So there's a
-  // multiphase shutdown here.  We tell the protection queue that the
-  // VM is invalid, which causes it not to call JSUnprotectValue.  but
-  // it will decrement its counters, if !NDEBUG.  Then we shut down
-  // the VM, which will clean everything up.  Finally, we shut down
-  // the queue itself.
-  protectionQueue_->setContextInvalid();
+  // JSValueUnprotect() can no longer be called.  We use an
+  // atomic<bool> to avoid unsafe unprotects happening after shutdown
+  // has started.
+  ctxInvalid_ = true;
   JSGlobalContextRelease(ctx_);
-  protectionQueue_->shutdown();
 #ifndef NDEBUG
   assert(
       objectCounter_ == 0 && "JSCRuntime destroyed with a dangling API object");
@@ -473,15 +365,9 @@ JSCRuntime::JSCStringValue::JSCStringValue(JSStringRef str)
 #endif
 
 void JSCRuntime::JSCStringValue::invalidate() {
-  // We want to immediately JSStringRelease once a String is released,
-  // and queue a JSObjectRef to unprotected (see comment on
-  // ProtectionQueue::unprotectThread above).
-  //
   // These JSC{String,Object}Value objects are implicitly owned by the
   // {String,Object} objects, thus when a String/Object is destructed
-  // the JSC{String,Object}Value should be released (again this has
-  // the caveat that objects must be unprotected on a separate
-  // thread).
+  // the JSC{String,Object}Value should be released.
 #ifndef NDEBUG
   counter_ -= 1;
 #endif
@@ -492,16 +378,16 @@ void JSCRuntime::JSCStringValue::invalidate() {
 
 JSCRuntime::JSCObjectValue::JSCObjectValue(
     JSGlobalContextRef ctx,
-    detail::ProtectionQueue& pq,
+    const std::atomic<bool>& ctxInvalid,
     JSObjectRef obj
 #ifndef NDEBUG
     ,
     std::atomic<intptr_t>& counter
 #endif
     )
     : ctx_(ctx),
-      obj_(obj),
-      protectionQueue_(pq)
+      ctxInvalid_(ctxInvalid),
+      obj_(obj)
 #ifndef NDEBUG
       ,
       counter_(counter)
@@ -514,16 +400,32 @@ JSCRuntime::JSCObjectValue::JSCObjectValue(
 }
 
 void JSCRuntime::JSCObjectValue::invalidate() {
-  // See comment in JSCRuntime::JSCStringValue::invalidate as well as
-  // on ProtectionQueue::unprotectThread.
-  protectionQueue_.push(this);
-}
-
-void JSCRuntime::JSCObjectValue::unprotect() {
 #ifndef NDEBUG
   counter_ -= 1;
 #endif
-  if (ctx_) {
+  // When shutting down the VM, if there is a HostObject which
+  // contains or otherwise owns a jsi::Object, then the final GC will
+  // finalize the HostObject, leading to a call to invalidate().  But
+  // at that point, making calls to JSValueUnprotect will crash.
+  // It is up to the application to make sure that any other calls to
+  // invalidate() happen before VM destruction; see the comment on
+  // jsi::Runtime.
+  //
+  // Another potential concern here is that in the non-shutdown case,
+  // if a HostObject is GCd, JSValueUnprotect will be called from the
+  // JSC finalizer.  The documentation warns against this: "You must
+  // not call any function that may cause a garbage collection or an
+  // allocation of a garbage collected object from within a
+  // JSObjectFinalizeCallback. This includes all functions that have a
+  // JSContextRef parameter." However, an audit of the source code for
+  // JSValueUnprotect in late 2018 shows that it cannot cause
+  // allocation or a GC, and further, this code has not changed in
+  // about two years.  In the future, we may choose to reintroduce the
+  // mechanism previously used here which uses a separate thread for
+  // JSValueUnprotect, in order to conform to the documented API, but
+  // use the "unsafe" synchronous version on iOS 11 and earlier.
+
+  if (!ctxInvalid_) {
     JSValueUnprotect(ctx_, obj_);
   }
   delete this;
@@ -1222,9 +1124,9 @@ jsi::Runtime::PointerValue* JSCRuntime::makeObjectValue(
     objectRef = JSObjectMake(ctx_, nullptr, nullptr);
   }
 #ifndef NDEBUG
-  return new JSCObjectValue(ctx_, *protectionQueue_, objectRef, objectCounter_);
+  return new JSCObjectValue(ctx_, ctxInvalid_, objectRef, objectCounter_);
 #else
-  return new JSCObjectValue(ctx_, *protectionQueue_, objectRef);
+  return new JSCObjectValue(ctx_, ctxInvalid_, objectRef);
 #endif
 }
 

ReactCommon/jsi/jsi.h

@@ -109,7 +109,7 @@ class JSI_EXPORT HostObject {
 };
 
 /// Represents a JS runtime.  Movable, but not copyable.  Note that
-/// this object is not thread-aware, but cannot be used safely from
+/// this object may not be thread-aware, but cannot be used safely from
 /// multiple threads at once.  The application is responsible for
 /// ensuring that it is used safely.  This could mean using the
 /// Runtime from a single thread, using a mutex, doing all work on a
@@ -118,7 +118,14 @@ class JSI_EXPORT HostObject {
 /// argument.  Destructors (all but ~Scope), operators, or other methods
 /// which do not take Runtime& as an argument are safe to call from any
 /// thread, but it is still forbidden to make write operations on a single
-/// instance of any class from more than one thread.
+/// instance of any class from more than one thread.  In addition, to
+/// make shutdown safe, destruction of objects associated with the Runtime
+/// must be destroyed before the Runtime is destroyed, or from the
+/// destructor of a managed HostObject or HostFunction.  Informally, this
+/// means that the main source of unsafe behavior is to hold a jsi object
+/// in a non-Runtime-managed object, and not clean it up before the Runtime
+/// is shut down.  If your lifecycle is such that avoiding this is hard,
+/// you will probably need to do use your own locks.
 class Runtime {
  public:
   virtual ~Runtime();