Skip to content

Commit

Permalink
Switch to synchronous strategy for unprotect
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@mhorowitz @facebook-github-bot
mhorowitz authored and facebook-github-bot committed Nov 9, 2018
1 parent 2b01da0 commit bf2500e
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 143 deletions.
184 changes: 43 additions & 141 deletions ReactCommon/jsi/JSCRuntime.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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;
Expand Down Expand Up @@ -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_;
Expand Down Expand Up @@ -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),
Expand All @@ -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");
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand All @@ -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;
Expand Down Expand Up @@ -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
}

Expand Down
11 changes: 9 additions & 2 deletions ReactCommon/jsi/jsi.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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();
Expand Down

0 comments on commit bf2500e

Please sign in to comment.