From f19c36116c8ef61c043fd77c599906f59585da6c Mon Sep 17 00:00:00 2001 From: David Vacca Date: Fri, 22 Jun 2018 11:29:05 -0700 Subject: [PATCH] Clean-up parent / owner reference of children during clonning Summary: This diff cleans up the parent / owner references for children of ReactShadowNode / YogaNode during cloning. The reason of this behavior is to avoid retaining every generation of trees during cloning. This fixes a memory leak detected when running the ProgressBarExample.android.js in catalyst app Reviewed By: fkgozali Differential Revision: D8019894 fbshipit-source-id: b0d38f0c836ffec534f64fa1adbd7511ecf3473d --- .../react/uimanager/ReactShadowNodeImpl.java | 16 ++++++++++++++-- .../main/java/com/facebook/yoga/YogaNode.java | 12 +++++++++--- ReactCommon/yoga/yoga/Yoga.cpp | 12 ++++++++---- 3 files changed, 31 insertions(+), 9 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactShadowNodeImpl.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactShadowNodeImpl.java index 35406a11a..6a334c3a7 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactShadowNodeImpl.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactShadowNodeImpl.java @@ -193,12 +193,24 @@ public class ReactShadowNodeImpl implements ReactShadowNode // Virtual ReactShadowNode do not have a YogaNode associated copy.mYogaNode = null; } - copy.mNativeChildren = mNativeChildren == null ? null : new ArrayList<>(mNativeChildren); copy.mTotalNativeChildren = mTotalNativeChildren; - copy.mChildren = mChildren == null ? null : new ArrayList<>(mChildren); + copy.mNativeChildren = copyChildren(mNativeChildren); + copy.mChildren = copyChildren(mChildren); + return copy; } + @Nullable + private ArrayList copyChildren(@Nullable List list){ + ArrayList result = list == null ? null : new ArrayList<>(list); + if (result != null) { + for (ReactShadowNodeImpl child : result) { + child.mParent = null; + } + } + return result; + } + @Override public ReactShadowNodeImpl mutableCopyWithNewChildren(long instanceHandle) { ReactShadowNodeImpl copy = copy(); diff --git a/ReactAndroid/src/main/java/com/facebook/yoga/YogaNode.java b/ReactAndroid/src/main/java/com/facebook/yoga/YogaNode.java index 90caf8f81..f8b895b16 100644 --- a/ReactAndroid/src/main/java/com/facebook/yoga/YogaNode.java +++ b/ReactAndroid/src/main/java/com/facebook/yoga/YogaNode.java @@ -1,8 +1,9 @@ /* - * Copyright (c) 2014-present, Facebook, Inc. + * Copyright (c) 2014-present, Facebook, Inc. + * + * This source code is licensed under the MIT license found in the LICENSE + * file in the root directory of this source tree. * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. */ package com.facebook.yoga; @@ -185,6 +186,11 @@ public class YogaNode implements Cloneable { clonedYogaNode.mOwner = null; clonedYogaNode.mChildren = mChildren != null ? (List) ((ArrayList) mChildren).clone() : null; + if (clonedYogaNode.mChildren != null) { + for (YogaNode child : clonedYogaNode.mChildren) { + child.mOwner = null; + } + } return clonedYogaNode; } catch (CloneNotSupportedException ex) { // This class implements Cloneable, this should not happen diff --git a/ReactCommon/yoga/yoga/Yoga.cpp b/ReactCommon/yoga/yoga/Yoga.cpp index c6a8ffd2e..3c83ce080 100644 --- a/ReactCommon/yoga/yoga/Yoga.cpp +++ b/ReactCommon/yoga/yoga/Yoga.cpp @@ -1,8 +1,9 @@ -/** - * Copyright (c) 2014-present, Facebook, Inc. +/* + * Copyright (c) 2014-present, Facebook, Inc. + * + * This source code is licensed under the MIT license found in the LICENSE + * file in the root directory of this source tree. * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. */ #include "Yoga.h" @@ -243,6 +244,9 @@ YGNodeRef YGNodeClone(YGNodeRef oldNode) { oldNode->getConfig(), node != nullptr, "Could not allocate memory for node"); + for (auto &item : oldNode->getChildren()) { + item->setOwner(nullptr); + } gNodeInstanceCount++; node->setOwner(nullptr); return node;