Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: Memory leak in Animated API by managing parent-child references #48993

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

FIX: Memory leak in Animated API by managing parent-child references #48993

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 44 additions & 19 deletions packages/react-native/Libraries/Animated/nodes/AnimatedWithChildren.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,41 +10,34 @@

'use strict';

import type {PlatformConfig} from '../AnimatedPlatformConfig';

import NativeAnimatedHelper from '../../../src/private/animated/NativeAnimatedHelper';
import AnimatedNode from './AnimatedNode';

Check warning on line 13 in packages/react-native/Libraries/Animated/nodes/AnimatedWithChildren.js

View workflow job for this annotation

GitHub Actions / test_js (20) 

Requires should be sorted alphabetically

Check warning on line 13 in packages/react-native/Libraries/Animated/nodes/AnimatedWithChildren.js

View workflow job for this annotation

GitHub Actions / test_js (18) 

Requires should be sorted alphabetically
import NativeAnimatedHelper from '../../../src/private/animated/NativeAnimatedHelper';
import type {PlatformConfig} from '../AnimatedPlatformConfig';

const {connectAnimatedNodes, disconnectAnimatedNodes} =
NativeAnimatedHelper.API;

export default class AnimatedWithChildren extends AnimatedNode {
_children: Array<AnimatedNode> = [];
_parents: Array<AnimatedNode> = [];

__makeNative(platformConfig: ?PlatformConfig) {
if (!this.__isNative) {
this.__isNative = true;
// Public method to add a child
addChild(child: AnimatedNode): void {
this.__addChild(child);
}

const children = this._children;
let length = children.length;
if (length > 0) {
for (let ii = 0; ii < length; ii++) {
const child = children[ii];
child.__makeNative(platformConfig);
connectAnimatedNodes(this.__getNativeTag(), child.__getNativeTag());
}
}
}
super.__makeNative(platformConfig);
// Public method to remove a child
removeChild(child: AnimatedNode): void {
this.__removeChild(child);
}

__addChild(child: AnimatedNode): void {
if (this._children.length === 0) {
this.__attach();
}
this._children.push(child);
child.addParent(this); // Maintain bidirectional parent-child relationship
Comment on lines 38 to +39
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m still seeing a memory leak as we keep adding the same child to _children multiple times. Here's an example where there's 334 children in the array while only 2 is expected.

Screenshot 2025-01-28 at 8 08 38 PM

I resolved this locally by adding an !this._children.includes(child) check just like you're doing in addParent below

Here's my original issue on react-native-web for reference, where I implemented just that change to resolve it: RN-web issue

I was able to achieve the same result by tweaking the changes to your updated __addChild method, which resolves the issue

  __addChild(child: AnimatedNode): void {
    if (!this._children.includes(child)) {
      if (this._children.length === 0) {
        this.__attach()
      }
  
      this._children.push(child)
  
      if (typeof child.addParent === 'function') {
        child.addParent(this)
      }
  
      if (this.__isNative) {
        child.__makeNative(this.__getPlatformConfig())
        connectAnimatedNodes(this.__getNativeTag(), child.__getNativeTag())
      }
    }
  }

if (this.__isNative) {
// Only accept "native" animated nodes as children
child.__makeNative(this.__getPlatformConfig());
connectAnimatedNodes(this.__getNativeTag(), child.__getNativeTag());
}
Expand All @@ -56,6 +49,7 @@
console.warn("Trying to remove a child that doesn't exist");
return;
}
child.removeParent(this); // Remove parent reference from the child
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yarn build was failing because child doesn't always implement addParent or removeParent.

I had to add checks (typeof child.addParent === 'function' and typeof child.removeParent === 'function') to avoid calling nonexistent methods.

I’m not necessarily recommending to adopt this; I just wanted to share what I had to do locally to test the rest of these changes.

(See attached screenshot of the terminal output.)

yarn build error

if (this.__isNative && child.__isNative) {
disconnectAnimatedNodes(this.__getNativeTag(), child.__getNativeTag());
}
Expand All @@ -65,6 +59,38 @@
}
}

// New method to add a parent
addParent(parent: AnimatedNode): void {
if (!this._parents.includes(parent)) {
this._parents.push(parent);
}
}

// New method to remove a parent
removeParent(parent: AnimatedNode): void {
const index = this._parents.indexOf(parent);
if (index !== -1) {
this._parents.splice(index, 1);
}
}

__makeNative(platformConfig: ?PlatformConfig) {
if (!this.__isNative) {
this.__isNative = true;

const children = this._children;
let length = children.length;
if (length > 0) {
for (let ii = 0; ii < length; ii++) {
const child = children[ii];
child.__makeNative(platformConfig);
connectAnimatedNodes(this.__getNativeTag(), child.__getNativeTag());
}
}
}
super.__makeNative(platformConfig);
}

__getChildren(): $ReadOnlyArray<AnimatedNode> {
return this._children;
}
Expand All @@ -75,7 +101,6 @@
const children = this._children;
for (let ii = 0, length = children.length; ii < length; ii++) {
const child = children[ii];
// $FlowFixMe[method-unbinding] added when improving typing for this parameters
if (child.__getValue) {
child.__callListeners(child.__getValue());
}
Expand Down
Loading