Refactor shutdown so that debug asserts can pass
Summary: We were not accounting for shutdown properly when counting jsi Objects at shutdown. Reviewed By: danzimm Differential Revision: D10451732 fbshipit-source-id: 7f0eb357aa3a011b7b2a97e44c22549e06e311c5
This commit is contained in:
parent
f867db366a
commit
2a44054e99
|
@ -72,8 +72,6 @@ class JSCRuntime : public jsi::Runtime {
|
||||||
#else
|
#else
|
||||||
JSCStringValue(JSStringRef str);
|
JSCStringValue(JSStringRef str);
|
||||||
#endif
|
#endif
|
||||||
~JSCStringValue();
|
|
||||||
|
|
||||||
void invalidate() override;
|
void invalidate() override;
|
||||||
|
|
||||||
JSStringRef str_;
|
JSStringRef str_;
|
||||||
|
@ -97,9 +95,9 @@ class JSCRuntime : public jsi::Runtime {
|
||||||
detail::ProtectionQueue& pq,
|
detail::ProtectionQueue& pq,
|
||||||
JSObjectRef obj);
|
JSObjectRef obj);
|
||||||
#endif
|
#endif
|
||||||
~JSCObjectValue();
|
|
||||||
|
|
||||||
void invalidate() override;
|
void invalidate() override;
|
||||||
|
void unprotect();
|
||||||
|
|
||||||
JSGlobalContextRef ctx_;
|
JSGlobalContextRef ctx_;
|
||||||
JSObjectRef obj_;
|
JSObjectRef obj_;
|
||||||
|
@ -300,7 +298,8 @@ namespace detail {
|
||||||
class ProtectionQueue {
|
class ProtectionQueue {
|
||||||
public:
|
public:
|
||||||
ProtectionQueue()
|
ProtectionQueue()
|
||||||
: shuttingDown_(false)
|
: ctxInvalid_(false)
|
||||||
|
, shuttingDown_(false)
|
||||||
#ifndef NDEBUG
|
#ifndef NDEBUG
|
||||||
,
|
,
|
||||||
didShutdown_ {
|
didShutdown_ {
|
||||||
|
@ -309,9 +308,15 @@ class ProtectionQueue {
|
||||||
#endif
|
#endif
|
||||||
, unprotectorThread_(&ProtectionQueue::unprotectThread, this) {}
|
, unprotectorThread_(&ProtectionQueue::unprotectThread, this) {}
|
||||||
|
|
||||||
|
void setContextInvalid() {
|
||||||
|
std::lock_guard<std::mutex> locker(mutex_);
|
||||||
|
ctxInvalid_ = true;
|
||||||
|
}
|
||||||
|
|
||||||
void shutdown() {
|
void shutdown() {
|
||||||
{
|
{
|
||||||
std::lock_guard<std::mutex> locker(mutex_);
|
std::lock_guard<std::mutex> locker(mutex_);
|
||||||
|
assert(ctxInvalid_);
|
||||||
shuttingDown_ = true;
|
shuttingDown_ = true;
|
||||||
notEmpty_.notify_one();
|
notEmpty_.notify_one();
|
||||||
}
|
}
|
||||||
|
@ -349,7 +354,10 @@ class ProtectionQueue {
|
||||||
// that may make another GC pass, which could call another finalizer
|
// that may make another GC pass, which could call another finalizer
|
||||||
// and thus attempt to push to this queue then, and deadlock.
|
// and thus attempt to push to this queue then, and deadlock.
|
||||||
locker.unlock();
|
locker.unlock();
|
||||||
delete value;
|
if (ctxInvalid_) {
|
||||||
|
value->ctx_ = nullptr;
|
||||||
|
}
|
||||||
|
value->unprotect();
|
||||||
locker.lock();
|
locker.lock();
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
@ -363,10 +371,12 @@ class ProtectionQueue {
|
||||||
std::condition_variable notEmpty_;
|
std::condition_variable notEmpty_;
|
||||||
// The actual underlying queue
|
// The actual underlying queue
|
||||||
std::queue<JSCRuntime::JSCObjectValue*> 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
|
// A flag dictating whether or not we need to stop all execution
|
||||||
bool shuttingDown_;
|
bool shuttingDown_;
|
||||||
#ifndef NDEBUG
|
#ifndef NDEBUG
|
||||||
std::atomic<bool> didShutdown_;
|
bool didShutdown_;
|
||||||
#endif
|
#endif
|
||||||
// The thread that dequeues and processes the queue. Note this is the last
|
// 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
|
// member on purpose so the thread starts up after all state has been
|
||||||
|
@ -392,6 +402,17 @@ JSCRuntime::JSCRuntime(JSGlobalContextRef ctx)
|
||||||
}
|
}
|
||||||
|
|
||||||
JSCRuntime::~JSCRuntime() {
|
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();
|
||||||
|
JSGlobalContextRelease(ctx_);
|
||||||
protectionQueue_->shutdown();
|
protectionQueue_->shutdown();
|
||||||
#ifndef NDEBUG
|
#ifndef NDEBUG
|
||||||
assert(
|
assert(
|
||||||
|
@ -399,7 +420,6 @@ JSCRuntime::~JSCRuntime() {
|
||||||
assert(
|
assert(
|
||||||
stringCounter_ == 0 && "JSCRuntime destroyed with a dangling API string");
|
stringCounter_ == 0 && "JSCRuntime destroyed with a dangling API string");
|
||||||
#endif
|
#endif
|
||||||
JSGlobalContextRelease(ctx_);
|
|
||||||
}
|
}
|
||||||
|
|
||||||
void JSCRuntime::evaluateJavaScript(
|
void JSCRuntime::evaluateJavaScript(
|
||||||
|
@ -453,27 +473,21 @@ JSCRuntime::JSCStringValue::JSCStringValue(JSStringRef str)
|
||||||
#endif
|
#endif
|
||||||
|
|
||||||
void JSCRuntime::JSCStringValue::invalidate() {
|
void JSCRuntime::JSCStringValue::invalidate() {
|
||||||
// JSI needs to be flexible enough to allow Runtime to act as a root in
|
// We want to immediately JSStringRelease once a String is released,
|
||||||
// hermes' case and just an interface in JSC's case. In hermes the
|
// and queue a JSObjectRef to unprotected (see comment on
|
||||||
// objects/strings must be tracked in a list so that they can be marked
|
// ProtectionQueue::unprotectThread above).
|
||||||
// on a GC sweep, while for JSC we want to immediately JSStringRelease once a
|
|
||||||
// String is released, and queue a JSObjectRef to unprotected (see comment
|
|
||||||
// on ProtectionQueue::unprotectThread above).
|
|
||||||
//
|
//
|
||||||
// In JSC's case these JSC{String,Object}Value objects are implicitly owned
|
// These JSC{String,Object}Value objects are implicitly owned by the
|
||||||
// by the {String,Object} objects, thus when a String/Object is destructed
|
// {String,Object} objects, thus when a String/Object is destructed
|
||||||
// the JSC{String,Object}Value should be released (again this has the caveat
|
// the JSC{String,Object}Value should be released (again this has
|
||||||
// that objects must be unprotected on a separate thread).
|
// the caveat that objects must be unprotected on a separate
|
||||||
//
|
// thread).
|
||||||
// Angery reaccs only
|
|
||||||
delete this;
|
|
||||||
}
|
|
||||||
|
|
||||||
JSCRuntime::JSCStringValue::~JSCStringValue() {
|
|
||||||
#ifndef NDEBUG
|
#ifndef NDEBUG
|
||||||
counter_ -= 1;
|
counter_ -= 1;
|
||||||
#endif
|
#endif
|
||||||
JSStringRelease(str_);
|
JSStringRelease(str_);
|
||||||
|
// Angery reaccs only
|
||||||
|
delete this;
|
||||||
}
|
}
|
||||||
|
|
||||||
JSCRuntime::JSCObjectValue::JSCObjectValue(
|
JSCRuntime::JSCObjectValue::JSCObjectValue(
|
||||||
|
@ -505,11 +519,14 @@ void JSCRuntime::JSCObjectValue::invalidate() {
|
||||||
protectionQueue_.push(this);
|
protectionQueue_.push(this);
|
||||||
}
|
}
|
||||||
|
|
||||||
JSCRuntime::JSCObjectValue::~JSCObjectValue() {
|
void JSCRuntime::JSCObjectValue::unprotect() {
|
||||||
#ifndef NDEBUG
|
#ifndef NDEBUG
|
||||||
counter_ -= 1;
|
counter_ -= 1;
|
||||||
#endif
|
#endif
|
||||||
|
if (ctx_) {
|
||||||
JSValueUnprotect(ctx_, obj_);
|
JSValueUnprotect(ctx_, obj_);
|
||||||
|
}
|
||||||
|
delete this;
|
||||||
}
|
}
|
||||||
|
|
||||||
jsi::Runtime::PointerValue* JSCRuntime::cloneString(
|
jsi::Runtime::PointerValue* JSCRuntime::cloneString(
|
||||||
|
|
Loading…
Reference in New Issue