Fix crash when throwing an exception from napi (#13664)

src/bun.js/bindings/napi.cpp

@@ -45,7 +45,6 @@
 #include <JavaScriptCore/JSArrayBuffer.h>
 #include "JSFFIFunction.h"
 #include <JavaScriptCore/JavaScript.h>
-#include <JavaScriptCore/JSWeakValue.h>
 #include "napi.h"
 #include <JavaScriptCore/GetterSetter.h>
 #include <JavaScriptCore/JSSourceCode.h>
@@ -64,6 +63,7 @@
 #include <JavaScriptCore/FunctionPrototype.h>
 #include "CommonJSModuleRecord.h"
 #include "wtf/text/ASCIIFastPath.h"
+#include "JavaScriptCore/WeakInlines.h"
 
 // #include <iostream>
 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<JSString*>(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<JSCell>(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<JSString>(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<void*>(finalize_cb);
     globalObject->napiInstanceDataFinalizerHint = finalize_hint;

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<JSCell> cell;
+        JSC::Weak<JSString> 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<JSC::JSGlobalObject> globalObject;
-    JSC::JSWeakValue weakValueRef;
+    NapiWeakValue weakValueRef;
     JSC::Strong<JSC::Unknown> strongRef;
     NapiFinalizer finalizer;
     void* data = nullptr;