diff --git a/src/bun.js/bindings/JSYogaNode.cpp b/src/bun.js/bindings/JSYogaNode.cpp index 02568a0ff7..9d956c2592 100644 --- a/src/bun.js/bindings/JSYogaNode.cpp +++ b/src/bun.js/bindings/JSYogaNode.cpp @@ -62,6 +62,11 @@ void JSYogaNode::finishCreation(JSC::VM& vm, YGConfigRef config, JSYogaConfig* j if (jsConfig) { m_config.set(vm, this, jsConfig); } + + // Initialize children array to maintain strong references + // This mirrors React Native's _reactSubviews NSMutableArray + JSC::JSGlobalObject* globalObject = this->globalObject(); + m_children.set(vm, this, JSC::constructEmptyArray(globalObject, nullptr, 0)); } void JSYogaNode::finishCreation(JSC::VM& vm) @@ -72,6 +77,11 @@ void JSYogaNode::finishCreation(JSC::VM& vm) m_impl->setJSWrapper(this); // No JSYogaConfig in this path - it's only set when explicitly provided + + // Initialize children array to maintain strong references + // This mirrors React Native's _reactSubviews NSMutableArray + JSC::JSGlobalObject* globalObject = this->globalObject(); + m_children.set(vm, this, JSC::constructEmptyArray(globalObject, nullptr, 0)); } JSC::Structure* JSYogaNode::createStructure(JSC::VM& vm, JSC::JSGlobalObject* globalObject, JSC::JSValue prototype) @@ -118,6 +128,7 @@ void JSYogaNode::visitAdditionalChildren(Visitor& visitor) visitor.append(m_dirtiedFunc); visitor.append(m_baselineFunc); visitor.append(m_config); + visitor.append(m_children); // Use the YogaNodeImpl pointer as opaque root instead of YGNodeRef // This avoids use-after-free when YGNode memory is freed but YogaNodeImpl still exists diff --git a/src/bun.js/bindings/JSYogaNode.h b/src/bun.js/bindings/JSYogaNode.h index dc4b3a4677..473f2cf3a1 100644 --- a/src/bun.js/bindings/JSYogaNode.h +++ b/src/bun.js/bindings/JSYogaNode.h @@ -49,6 +49,10 @@ public: // Store the JSYogaConfig that was used to create this node JSC::WriteBarrier m_config; + + // Store children to prevent GC while still part of Yoga tree + // This mirrors React Native's _reactSubviews NSMutableArray pattern + JSC::WriteBarrier m_children; private: JSYogaNode(JSC::VM&, JSC::Structure*); diff --git a/src/bun.js/bindings/JSYogaNodeOwner.cpp b/src/bun.js/bindings/JSYogaNodeOwner.cpp index b016a078b2..d157e10dfd 100644 --- a/src/bun.js/bindings/JSYogaNodeOwner.cpp +++ b/src/bun.js/bindings/JSYogaNodeOwner.cpp @@ -34,6 +34,9 @@ void JSYogaNodeOwner::finalize(JSC::Handle handle, void* context) // The context contains our YogaNodeImpl auto* impl = static_cast(context); + // TEMP: Skip YGNodeFree during GC to debug double-free issue + // TODO: Find proper solution for safe Yoga node cleanup during GC + // Deref the YogaNodeImpl - this will decrease its reference count // and potentially destroy it if no other references exist impl->deref(); diff --git a/src/bun.js/bindings/JSYogaPrototype.cpp b/src/bun.js/bindings/JSYogaPrototype.cpp index 5748892785..53e8e39a18 100644 --- a/src/bun.js/bindings/JSYogaPrototype.cpp +++ b/src/bun.js/bindings/JSYogaPrototype.cpp @@ -1177,7 +1177,28 @@ JSC_DEFINE_HOST_FUNCTION(jsYogaNodeProtoFuncRemoveChild, (JSC::JSGlobalObject * return {}; } + // Remove from Yoga tree YGNodeRemoveChild(thisObject->impl().yogaNode(), childNode->impl().yogaNode()); + + // Remove strong reference from children array to allow GC + // This mirrors React Native's [_reactSubviews removeObject:subview] pattern + JSC::JSArray* childrenArray = jsCast(thisObject->m_children.get()); + if (childrenArray) { + uint32_t length = childrenArray->length(); + for (uint32_t i = 0; i < length; i++) { + JSC::JSValue element = childrenArray->getIndex(globalObject, i); + if (element == childNode) { + // Remove this element by shifting everything down + for (uint32_t j = i; j < length - 1; j++) { + JSC::JSValue nextElement = childrenArray->getIndex(globalObject, j + 1); + childrenArray->putDirectIndex(globalObject, j, nextElement); + } + childrenArray->setLength(globalObject, length - 1); + break; + } + } + } + return JSC::JSValue::encode(JSC::jsUndefined()); } @@ -2333,7 +2354,30 @@ JSC_DEFINE_HOST_FUNCTION(jsYogaNodeProtoFuncInsertChild, (JSC::JSGlobalObject * int index = callFrame->uncheckedArgument(1).toInt32(globalObject); RETURN_IF_EXCEPTION(scope, {}); + // Insert into Yoga tree YGNodeInsertChild(thisObject->impl().yogaNode(), child->impl().yogaNode(), index); + + // Add strong reference to children array to prevent GC + // This mirrors React Native's [_reactSubviews insertObject:subview atIndex:atIndex] pattern + JSC::JSArray* childrenArray = jsCast(thisObject->m_children.get()); + if (childrenArray) { + // Insert at the specified index by shifting existing elements + uint32_t length = childrenArray->length(); + uint32_t insertIndex = std::min(static_cast(index), length); + + // Grow array by 1 + childrenArray->setLength(globalObject, length + 1); + + // Shift elements to make room + for (uint32_t i = length; i > insertIndex; i--) { + JSC::JSValue element = childrenArray->getIndex(globalObject, i - 1); + childrenArray->putDirectIndex(globalObject, i, element); + } + + // Insert the new child + childrenArray->putDirectIndex(globalObject, insertIndex, child); + } + return JSC::JSValue::encode(JSC::jsUndefined()); } @@ -3075,10 +3119,12 @@ JSC_DEFINE_HOST_FUNCTION(jsYogaNodeProtoFuncClone, (JSC::JSGlobalObject * global auto* zigGlobalObject = defaultGlobalObject(globalObject); JSC::Structure* structure = zigGlobalObject->m_JSYogaNodeClassStructure.get(zigGlobalObject); - // Create a new JSYogaNode wrapper - JSYogaNode* jsClonedNode = JSYogaNode::create(vm, structure, nullptr); - // Replace the internal node with the cloned one - jsClonedNode->impl().replaceYogaNode(clonedNode); + // Create YogaNodeImpl directly with the cloned node to avoid double creation + auto clonedImpl = YogaNodeImpl::create(nullptr); + clonedImpl->replaceYogaNode(clonedNode); + + // Create JSYogaNode wrapper with the impl + JSYogaNode* jsClonedNode = JSYogaNode::create(vm, structure, WTFMove(clonedImpl)); // Copy JavaScript callbacks from the original node if (thisObject->m_measureFunc) { @@ -3106,9 +3152,12 @@ JSC_DEFINE_HOST_FUNCTION(jsYogaNodeProtoFuncClone, (JSC::JSGlobalObject * global YGNodeRef originalChild = YGNodeGetChild(pair.original, i); if (clonedChild && originalChild) { + // Create YogaNodeImpl directly with cloned child to avoid double creation + auto clonedChildImpl = YogaNodeImpl::create(nullptr); + clonedChildImpl->replaceYogaNode(clonedChild); + // Create JS wrapper for cloned child - JSYogaNode* jsClonedChild = JSYogaNode::create(vm, structure, nullptr); - jsClonedChild->impl().replaceYogaNode(clonedChild); + JSYogaNode* jsClonedChild = JSYogaNode::create(vm, structure, WTFMove(clonedChildImpl)); // Copy callbacks from original child JSYogaNode* jsOriginalChild = JSYogaNode::fromYGNode(originalChild); diff --git a/src/bun.js/bindings/YogaNodeImpl.cpp b/src/bun.js/bindings/YogaNodeImpl.cpp index 2cd5e23cf0..27efd9a78b 100644 --- a/src/bun.js/bindings/YogaNodeImpl.cpp +++ b/src/bun.js/bindings/YogaNodeImpl.cpp @@ -8,13 +8,6 @@ namespace Bun { -// Simplified approach: trust Yoga's built-in parent-child management -static void simpleYGNodeFree(YGNodeRef node) -{ - if (node) { - YGNodeFree(node); - } -} Ref YogaNodeImpl::create(YGConfigRef config) { @@ -35,8 +28,8 @@ YogaNodeImpl::YogaNodeImpl(YGConfigRef config) YogaNodeImpl::~YogaNodeImpl() { - // React Native pattern: Don't access potentially freed YGNode memory - // Let Yoga handle all cleanup automatically - safer than checking parent/child status + // Don't call YGNodeFree here - let JS finalizer handle it to control timing + // This avoids double-free issues during GC when nodes may be freed in arbitrary order m_yogaNode = nullptr; } diff --git a/test/js/bun/yoga-config.test.js b/test/js/bun/yoga/yoga-config.test.js similarity index 100% rename from test/js/bun/yoga-config.test.js rename to test/js/bun/yoga/yoga-config.test.js diff --git a/test/js/bun/yoga-constants.test.js b/test/js/bun/yoga/yoga-constants.test.js similarity index 100% rename from test/js/bun/yoga-constants.test.js rename to test/js/bun/yoga/yoga-constants.test.js diff --git a/test/js/bun/yoga-layout-comprehensive.test.js b/test/js/bun/yoga/yoga-layout-comprehensive.test.js similarity index 100% rename from test/js/bun/yoga-layout-comprehensive.test.js rename to test/js/bun/yoga/yoga-layout-comprehensive.test.js diff --git a/test/js/bun/yoga-node-extended.test.js b/test/js/bun/yoga/yoga-node-extended.test.js similarity index 100% rename from test/js/bun/yoga-node-extended.test.js rename to test/js/bun/yoga/yoga-node-extended.test.js diff --git a/test/js/bun/yoga-node.test.js b/test/js/bun/yoga/yoga-node.test.js similarity index 100% rename from test/js/bun/yoga-node.test.js rename to test/js/bun/yoga/yoga-node.test.js