diff --git a/src/bun.js/bindings/napi.cpp b/src/bun.js/bindings/napi.cpp index 10c4335b99..6c431277fc 100644 --- a/src/bun.js/bindings/napi.cpp +++ b/src/bun.js/bindings/napi.cpp @@ -45,7 +45,6 @@ #include #include "JSFFIFunction.h" #include -#include #include "napi.h" #include #include @@ -64,6 +63,7 @@ #include #include "CommonJSModuleRecord.h" #include "wtf/text/ASCIIFastPath.h" +#include "JavaScriptCore/WeakInlines.h" // #include using namespace JSC; @@ -157,13 +157,8 @@ void NapiRef::ref() ++refCount; if (refCount == 1 && !weakValueRef.isClear()) { auto& vm = globalObject.get()->vm(); - if (weakValueRef.isString()) { - strongRef.set(vm, JSC::JSValue(weakValueRef.string())); - } else if (weakValueRef.isObject()) { - strongRef.set(vm, JSC::JSValue(weakValueRef.object())); - } else { - strongRef.set(vm, weakValueRef.primitive()); - } + strongRef.set(vm, weakValueRef.get()); + // isSet() will return always true after being set once // We cannot rely on isSet() to check if the value is set we need to use isClear() // .setString/.setObject/.setPrimitive will assert fail if called more than once (even after clear()) @@ -227,6 +222,101 @@ static uint32_t getPropertyAttributes(napi_property_descriptor prop) return result; } +NapiWeakValue::~NapiWeakValue() +{ + clear(); +} + +void NapiWeakValue::clear() +{ + switch (m_tag) { + case WeakTypeTag::Cell: { + m_value.cell.clear(); + break; + } + case WeakTypeTag::String: { + m_value.string.clear(); + break; + } + default: { + break; + } + } + + m_tag = WeakTypeTag::NotSet; +} + +bool NapiWeakValue::isClear() const +{ + return m_tag == WeakTypeTag::NotSet; +} + +void NapiWeakValue::setPrimitive(JSValue value) +{ + switch (m_tag) { + case WeakTypeTag::Cell: { + m_value.cell.clear(); + break; + } + case WeakTypeTag::String: { + m_value.string.clear(); + break; + } + default: { + break; + } + } + m_tag = WeakTypeTag::Primitive; + m_value.primitive = value; +} + +void NapiWeakValue::set(JSValue value, WeakHandleOwner& owner, void* context) +{ + if (value.isCell()) { + auto* cell = value.asCell(); + if (cell->isString()) { + setString(jsCast(cell), owner, context); + } else { + setCell(cell, owner, context); + } + } else { + setPrimitive(value); + } +} + +void NapiWeakValue::setCell(JSCell* cell, WeakHandleOwner& owner, void* context) +{ + switch (m_tag) { + case WeakTypeTag::Cell: { + m_value.cell = JSC::Weak(cell, &owner, context); + break; + } + case WeakTypeTag::String: { + m_value.string.clear(); + break; + } + default: { + break; + } + } +} + +void NapiWeakValue::setString(JSString* string, WeakHandleOwner& owner, void* context) +{ + switch (m_tag) { + case WeakTypeTag::Cell: { + m_value.cell.clear(); + break; + } + default: { + break; + } + } + + m_value.string = JSC::Weak(string, &owner, context); + m_tag = WeakTypeTag::String; +} + class NAPICallFrame { public: NAPICallFrame(const JSC::ArgList args, void* dataPtr) @@ -679,7 +769,7 @@ extern "C" napi_status napi_set_named_property(napi_env env, napi_value object, return napi_object_expected; } - if (UNLIKELY(utf8name == nullptr || !*utf8name)) { + if (UNLIKELY(utf8name == nullptr || !*utf8name || !value)) { return napi_invalid_arg; } @@ -970,7 +1060,8 @@ extern "C" napi_status napi_wrap(napi_env env, } auto* ref = new NapiRef(globalObject, 0); - ref->weakValueRef.setObject(value.getObject(), weakValueHandleOwner(), ref); + + ref->weakValueRef.set(value, weakValueHandleOwner(), ref); if (finalize_cb) { ref->finalizer.finalize_cb = finalize_cb; @@ -1272,13 +1363,13 @@ extern "C" napi_status napi_create_reference(napi_env env, napi_value value, { NAPI_PREMABLE - if (UNLIKELY(!result)) { + if (UNLIKELY(!result || !env)) { return napi_invalid_arg; } JSC::JSValue val = toJS(value); - if (!val || !val.isObject()) { + if (!val || !val.isCell()) { return napi_object_expected; } @@ -1288,14 +1379,7 @@ extern "C" napi_status napi_create_reference(napi_env env, napi_value value, if (initial_refcount > 0) { ref->strongRef.set(globalObject->vm(), val); } - // we dont have a finalizer but we can use the weak value to re-ref again or get the value until the GC is called - if (val.isString()) { - ref->weakValueRef.setString(val.toString(globalObject), weakValueHandleOwner(), ref); - } else if (val.isObject()) { - ref->weakValueRef.setObject(val.getObject(), weakValueHandleOwner(), ref); - } else { - ref->weakValueRef.setPrimitive(val); - } + ref->weakValueRef.set(val, weakValueHandleOwner(), ref); *result = toNapi(ref); @@ -2526,8 +2610,7 @@ extern "C" napi_status napi_set_instance_data(napi_env env, NAPI_PREMABLE Zig::GlobalObject* globalObject = toJS(env); - if (data) - globalObject->napiInstanceData = data; + globalObject->napiInstanceData = data; globalObject->napiInstanceDataFinalizer = reinterpret_cast(finalize_cb); globalObject->napiInstanceDataFinalizerHint = finalize_hint; diff --git a/src/bun.js/bindings/napi.h b/src/bun.js/bindings/napi.h index 76d2401be4..5e4e173f6c 100644 --- a/src/bun.js/bindings/napi.h +++ b/src/bun.js/bindings/napi.h @@ -61,6 +61,87 @@ public: void call(JSC::JSGlobalObject* globalObject, void* data); }; + +// This is essentially JSC::JSWeakValue, except with a JSCell* instead of a +// JSObject*. Sometimes, a napi embedder might want to store a JSC::Exception, a +// JSC::HeapBigInt, JSC::Symbol, etc inside of a NapiRef. So we can't limit it +// to just JSObject*. It has to be JSCell*. It's not clear that we benefit from +// not simply making this JSC::Unknown. +class NapiWeakValue { +public: + NapiWeakValue() = default; + ~NapiWeakValue(); + + void clear(); + bool isClear() const; + + bool isSet() const { return m_tag != WeakTypeTag::NotSet; } + bool isPrimitive() const { return m_tag == WeakTypeTag::Primitive; } + bool isCell() const { return m_tag == WeakTypeTag::Cell; } + bool isString() const { return m_tag == WeakTypeTag::String; } + + void setPrimitive(JSValue); + void setCell(JSCell*, WeakHandleOwner&, void* context); + void setString(JSString*, WeakHandleOwner&, void* context); + void set(JSValue, WeakHandleOwner&, void* context); + + JSValue get() const + { + switch (m_tag) { + case WeakTypeTag::Primitive: + return m_value.primitive; + case WeakTypeTag::Cell: + return JSC::JSValue(m_value.cell.get()); + case WeakTypeTag::String: + return JSC::JSValue(m_value.string.get()); + default: + return JSC::JSValue(); + } + } + + JSCell* cell() const + { + ASSERT(isCell()); + return m_value.cell.get(); + } + + JSValue primitive() const + { + ASSERT(isPrimitive()); + return m_value.primitive; + } + + JSString* string() const + { + ASSERT(isString()); + return m_value.string.get(); + } + +private: + enum class WeakTypeTag { NotSet, + Primitive, + Cell, + String }; + + WeakTypeTag m_tag { WeakTypeTag::NotSet }; + + union WeakValueUnion { + WeakValueUnion() + : primitive(JSValue()) + { + } + + ~WeakValueUnion() + { + ASSERT(!primitive); + } + + JSValue primitive; + JSC::Weak cell; + JSC::Weak string; + } m_value; +}; + class NapiRef { WTF_MAKE_ISO_ALLOCATED(NapiRef); @@ -80,22 +161,7 @@ public: JSC::JSValue value() const { if (refCount == 0) { - // isSet() can return true even if the value was cleared - // so we must check if the value is clear - // if the value is unset, isClear() will return true - if (weakValueRef.isClear()) { - return JSC::JSValue {}; - } - - if (weakValueRef.isString()) { - return JSC::JSValue(weakValueRef.string()); - } - - if (weakValueRef.isObject()) { - return JSC::JSValue(weakValueRef.object()); - } - - return weakValueRef.primitive(); + return weakValueRef.get(); } return strongRef.get(); @@ -110,7 +176,7 @@ public: } JSC::Weak globalObject; - JSC::JSWeakValue weakValueRef; + NapiWeakValue weakValueRef; JSC::Strong strongRef; NapiFinalizer finalizer; void* data = nullptr;