diff --git a/src/bun.js/bindings/JSYogaConfigOwner.cpp b/src/bun.js/bindings/JSYogaConfigOwner.cpp index 697533cf87..a469f57271 100644 --- a/src/bun.js/bindings/JSYogaConfigOwner.cpp +++ b/src/bun.js/bindings/JSYogaConfigOwner.cpp @@ -23,13 +23,9 @@ void JSYogaConfigOwner::finalize(JSC::Handle handle, void* context bool JSYogaConfigOwner::isReachableFromOpaqueRoots(JSC::Handle handle, void* context, JSC::AbstractSlotVisitor& visitor, ASCIILiteral* reason) { UNUSED_PARAM(handle); - - auto* impl = static_cast(context); - + UNUSED_PARAM(context); // YogaConfig doesn't currently use opaque roots, so always return false // This allows normal GC collection based on JS reference reachability - fprintf(stderr, "[DEBUG] JSYogaConfigOwner::isReachableFromOpaqueRoots called for YogaConfigImpl %p, reachable: false\n", impl); - if (reason) *reason = "YogaConfig not using opaque roots"_s; diff --git a/src/bun.js/bindings/JSYogaNode.cpp b/src/bun.js/bindings/JSYogaNode.cpp index 787f2f4d69..09ad66df43 100644 --- a/src/bun.js/bindings/JSYogaNode.cpp +++ b/src/bun.js/bindings/JSYogaNode.cpp @@ -117,8 +117,6 @@ JSC::GCClient::IsoSubspace* JSYogaNode::subspaceFor(JSC::VM& vm) template void JSYogaNode::visitAdditionalChildren(Visitor& visitor) { - fprintf(stderr, "[DEBUG] JSYogaNode::visitAdditionalChildren called for %p\n", this); - visitor.append(m_measureFunc); visitor.append(m_dirtiedFunc); visitor.append(m_baselineFunc); @@ -126,7 +124,6 @@ void JSYogaNode::visitAdditionalChildren(Visitor& visitor) // Use the YogaNodeImpl pointer as opaque root instead of YGNodeRef // This avoids use-after-free when YGNode memory is freed but YogaNodeImpl still exists - fprintf(stderr, "[DEBUG] JSYogaNode::visitAdditionalChildren adding YogaNodeImpl %p as opaque root for JSYogaNode %p\n", &m_impl.get(), this); visitor.addOpaqueRoot(&m_impl.get()); } diff --git a/src/bun.js/bindings/JSYogaNodeOwner.cpp b/src/bun.js/bindings/JSYogaNodeOwner.cpp index 79302d0bae..78f98b51b5 100644 --- a/src/bun.js/bindings/JSYogaNodeOwner.cpp +++ b/src/bun.js/bindings/JSYogaNodeOwner.cpp @@ -34,8 +34,6 @@ void JSYogaNodeOwner::finalize(JSC::Handle handle, void* context) // The context contains our YogaNodeImpl auto* impl = static_cast(context); - fprintf(stderr, "[DEBUG] JSYogaNodeOwner::finalize called for YogaNodeImpl %p\n", impl); - // Deref the YogaNodeImpl - this will decrease its reference count // and potentially destroy it if no other references exist impl->deref(); @@ -49,9 +47,6 @@ bool JSYogaNodeOwner::isReachableFromOpaqueRoots(JSC::Handle handl // Check if the YogaNodeImpl itself is reachable as opaque root bool reachable = visitor.containsOpaqueRoot(impl); - fprintf(stderr, "[DEBUG] JSYogaNodeOwner::isReachableFromOpaqueRoots called for YogaNodeImpl %p, reachable: %s\n", - impl, reachable ? "true" : "false"); - if (reason) *reason = "YogaNode reachable from root"_s; diff --git a/src/bun.js/bindings/YogaConfigImpl.cpp b/src/bun.js/bindings/YogaConfigImpl.cpp index 118741412e..a2693460c3 100644 --- a/src/bun.js/bindings/YogaConfigImpl.cpp +++ b/src/bun.js/bindings/YogaConfigImpl.cpp @@ -32,10 +32,7 @@ void YogaConfigImpl::setJSWrapper(JSYogaConfig* wrapper) // This prevents ref count leaks if setJSWrapper is called multiple times if (!m_wrapper) { // Increment ref count for the weak handle context - fprintf(stderr, "[DEBUG] YogaConfigImpl::setJSWrapper %p calling ref() for JS wrapper %p\n", this, wrapper); this->ref(); - } else { - fprintf(stderr, "[DEBUG] YogaConfigImpl::setJSWrapper %p already has wrapper, replacing with %p\n", this, wrapper); } // Create weak reference with our JS owner diff --git a/src/bun.js/bindings/YogaNodeImpl.cpp b/src/bun.js/bindings/YogaNodeImpl.cpp index d7aac3abb4..95db41cfbe 100644 --- a/src/bun.js/bindings/YogaNodeImpl.cpp +++ b/src/bun.js/bindings/YogaNodeImpl.cpp @@ -3,9 +3,27 @@ #include "JSYogaConfig.h" #include "JSYogaNodeOwner.h" #include +#include +#include namespace Bun { +// Global set to track freed YGNodes to prevent double-freeing +static Lock s_freedNodesLock; +static HashSet s_freedNodes; + +static void safeYGNodeFree(YGNodeRef node) { + if (!node) return; + + Locker locker { s_freedNodesLock }; + if (s_freedNodes.contains(node)) { + return; // Already freed + } + + s_freedNodes.add(node); + YGNodeFree(node); +} + Ref YogaNodeImpl::create(YGConfigRef config, JSYogaConfig* jsConfig) { return adoptRef(*new YogaNodeImpl(config, jsConfig)); @@ -13,6 +31,7 @@ Ref YogaNodeImpl::create(YGConfigRef config, JSYogaConfig* jsConfi YogaNodeImpl::YogaNodeImpl(YGConfigRef config, JSYogaConfig* jsConfig) : m_jsConfig(jsConfig) + , m_ownsYogaNode(true) { if (config) { m_yogaNode = YGNodeNewWithConfig(config); @@ -30,13 +49,14 @@ YogaNodeImpl::~YogaNodeImpl() // Clear the context pointer to avoid callbacks during cleanup YGNodeSetContext(m_yogaNode, nullptr); - // Remove from parent to avoid use-after-free when parent tries to clear owner - YGNodeRef parent = YGNodeGetParent(m_yogaNode); - if (parent) { - YGNodeRemoveChild(parent, m_yogaNode); + // Only free the node if we own it and it has no parent + // Nodes with parents should be freed when the parent is freed + if (m_ownsYogaNode) { + YGNodeRef parent = YGNodeGetParent(m_yogaNode); + if (!parent) { + safeYGNodeFree(m_yogaNode); + } } - - YGNodeFree(m_yogaNode); m_yogaNode = nullptr; } } @@ -47,10 +67,7 @@ void YogaNodeImpl::setJSWrapper(JSYogaNode* wrapper) // This prevents ref count leaks if setJSWrapper is called multiple times if (!m_wrapper) { // Increment ref count for the weak handle context - fprintf(stderr, "[DEBUG] YogaNodeImpl::setJSWrapper %p calling ref() for JS wrapper %p\n", this, wrapper); this->ref(); - } else { - fprintf(stderr, "[DEBUG] YogaNodeImpl::setJSWrapper %p already has wrapper, replacing with %p\n", this, wrapper); } // Create weak reference with our JS owner @@ -84,11 +101,16 @@ void YogaNodeImpl::replaceYogaNode(YGNodeRef newNode) { if (m_yogaNode) { YGNodeSetContext(m_yogaNode, nullptr); - YGNodeFree(m_yogaNode); + // Only free the old node if we owned it + if (m_ownsYogaNode) { + safeYGNodeFree(m_yogaNode); + } } m_yogaNode = newNode; if (newNode) { YGNodeSetContext(newNode, this); + // Cloned nodes are owned by us - YGNodeClone creates a new node we must free + m_ownsYogaNode = true; } } diff --git a/src/bun.js/bindings/YogaNodeImpl.h b/src/bun.js/bindings/YogaNodeImpl.h index f0e20ecd2e..1db9e818e5 100644 --- a/src/bun.js/bindings/YogaNodeImpl.h +++ b/src/bun.js/bindings/YogaNodeImpl.h @@ -40,6 +40,7 @@ private: YGNodeRef m_yogaNode; JSC::Weak m_wrapper; JSYogaConfig* m_jsConfig; + bool m_ownsYogaNode; // Track if we should free the YGNode in destructor }; } // namespace Bun