diff --git a/src/bun.js/bindings/JSYogaNode.cpp b/src/bun.js/bindings/JSYogaNode.cpp index fd2417dceb..8c92fec745 100644 --- a/src/bun.js/bindings/JSYogaNode.cpp +++ b/src/bun.js/bindings/JSYogaNode.cpp @@ -135,7 +135,15 @@ void JSYogaNode::visitOutputConstraints(JSC::JSCell* cell, Visitor& visitor) auto* thisObject = jsCast(cell); ASSERT_GC_OBJECT_INHERITS(thisObject, info()); Base::visitOutputConstraints(thisObject, visitor); + + // Re-visit after mutator execution in case callbacks changed references + // This is critical for objects whose reachability can change during runtime thisObject->visitAdditionalChildren(visitor); + + // Ensure we keep alive during active layout operations + if (thisObject->m_impl->isInLayoutCalculation()) { + visitor.addOpaqueRoot(&thisObject->m_impl.get()); + } } template void JSYogaNode::visitOutputConstraints(JSC::JSCell*, JSC::AbstractSlotVisitor&); diff --git a/src/bun.js/bindings/JSYogaNodeOwner.cpp b/src/bun.js/bindings/JSYogaNodeOwner.cpp index 78f98b51b5..de82aab66c 100644 --- a/src/bun.js/bindings/JSYogaNodeOwner.cpp +++ b/src/bun.js/bindings/JSYogaNodeOwner.cpp @@ -44,11 +44,25 @@ bool JSYogaNodeOwner::isReachableFromOpaqueRoots(JSC::Handle handl UNUSED_PARAM(handle); auto* impl = static_cast(context); + + // Enhanced reachability based on layout state and React Native patterns + + // Keep alive during active layout calculations (similar to EventTarget firing events) + if (impl->isInLayoutCalculation()) { + if (reason) *reason = "YogaNode active in layout calculation"_s; + return true; + } + + // Keep alive if this is a root node with children being laid out + if (impl->hasChildrenInLayout()) { + if (reason) *reason = "YogaNode has children in active layout"_s; + return true; + } // Check if the YogaNodeImpl itself is reachable as opaque root bool reachable = visitor.containsOpaqueRoot(impl); - if (reason) - *reason = "YogaNode reachable from root"_s; + if (reachable && reason) + *reason = "YogaNode reachable from opaque root"_s; return reachable; } diff --git a/src/bun.js/bindings/YogaNodeImpl.cpp b/src/bun.js/bindings/YogaNodeImpl.cpp index 4fc5b6dc93..c6a3f6fe62 100644 --- a/src/bun.js/bindings/YogaNodeImpl.cpp +++ b/src/bun.js/bindings/YogaNodeImpl.cpp @@ -8,21 +8,12 @@ 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) +// Simplified approach: trust Yoga's built-in parent-child management +static void simpleYGNodeFree(YGNodeRef node) { - if (!node) return; - - Locker locker { s_freedNodesLock }; - if (s_freedNodes.contains(node)) { - return; // Already freed + if (node) { + YGNodeFree(node); } - - s_freedNodes.add(node); - YGNodeFree(node); } Ref YogaNodeImpl::create(YGConfigRef config, JSYogaConfig* jsConfig) @@ -32,7 +23,7 @@ Ref YogaNodeImpl::create(YGConfigRef config, JSYogaConfig* jsConfi YogaNodeImpl::YogaNodeImpl(YGConfigRef config, JSYogaConfig* jsConfig) : m_jsConfig(jsConfig) - , m_ownsYogaNode(true) + , m_inLayoutCalculation(false) { if (config) { m_yogaNode = YGNodeNewWithConfig(config); @@ -47,16 +38,14 @@ YogaNodeImpl::YogaNodeImpl(YGConfigRef config, JSYogaConfig* jsConfig) YogaNodeImpl::~YogaNodeImpl() { if (m_yogaNode) { - // Clear the context pointer to avoid callbacks during cleanup + // Clear context immediately to prevent callbacks during cleanup YGNodeSetContext(m_yogaNode, nullptr); - - // 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); - } + + // Simplified pattern: only free root nodes (no parent) + // Let Yoga handle child cleanup automatically + YGNodeRef parent = YGNodeGetParent(m_yogaNode); + if (!parent) { + simpleYGNodeFree(m_yogaNode); } m_yogaNode = nullptr; } @@ -102,17 +91,42 @@ void YogaNodeImpl::replaceYogaNode(YGNodeRef newNode) { if (m_yogaNode) { YGNodeSetContext(m_yogaNode, nullptr); - // Only free the old node if we owned it - if (m_ownsYogaNode) { - safeYGNodeFree(m_yogaNode); + + // Simplified pattern: only free if no parent (root node) + YGNodeRef parent = YGNodeGetParent(m_yogaNode); + if (!parent) { + simpleYGNodeFree(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; } } +void YogaNodeImpl::setInLayoutCalculation(bool inLayout) +{ + m_inLayoutCalculation.store(inLayout); +} + +bool YogaNodeImpl::isInLayoutCalculation() const +{ + return m_inLayoutCalculation.load(); +} + +bool YogaNodeImpl::hasChildrenInLayout() const +{ + if (!m_yogaNode) return false; + + size_t childCount = YGNodeGetChildCount(m_yogaNode); + for (size_t i = 0; i < childCount; i++) { + YGNodeRef childNode = YGNodeGetChild(m_yogaNode, i); + YogaNodeImpl* childImpl = fromYGNode(childNode); + if (childImpl && childImpl->isInLayoutCalculation()) { + return true; + } + } + return false; +} + } // namespace Bun diff --git a/src/bun.js/bindings/YogaNodeImpl.h b/src/bun.js/bindings/YogaNodeImpl.h index 1db9e818e5..5123ee2663 100644 --- a/src/bun.js/bindings/YogaNodeImpl.h +++ b/src/bun.js/bindings/YogaNodeImpl.h @@ -5,6 +5,7 @@ #include #include #include +#include namespace Bun { @@ -33,6 +34,11 @@ public: // Replace the internal YGNodeRef (used for cloning) void replaceYogaNode(YGNodeRef newNode); + + // Layout state management for GC protection + void setInLayoutCalculation(bool inLayout); + bool isInLayoutCalculation() const; + bool hasChildrenInLayout() const; private: explicit YogaNodeImpl(YGConfigRef config, JSYogaConfig* jsConfig); @@ -40,7 +46,7 @@ private: YGNodeRef m_yogaNode; JSC::Weak m_wrapper; JSYogaConfig* m_jsConfig; - bool m_ownsYogaNode; // Track if we should free the YGNode in destructor + std::atomic m_inLayoutCalculation; // Track layout state for GC protection }; } // namespace Bun