Fix Memory Leak in AnimatedWithChildren

[c-miles]

Summary:

This pull request fixes an issue in AnimatedWithChildren where duplicate children were being added during repeated re-renders.

In our long-lived app, which uses many animations, this caused the children array to grow unchecked, eventually degrading performance and freezing the app.

Here's a reproduction that visually demonstrates performance degradation over 20–30 seconds.

This change prevents adding the same child more than once, eliminating the memory leak and restoring smooth performance.

Changelog:

[General] [FIXED] - Prevent duplicate child additions in AnimatedWithChildren to prevent memory leak.

Test Plan:

Added a new test file (AnimatedWithChildren-test.js) that verifies:

• A child is added only once when __addChild is called multiple times.
• The children array is correctly updated when children are removed.

Manual Verification:

• I configured a test app to use my custom React Native build from my fork and confirmed that the memory leak was eliminated by monitoring the children array during repeated animations and updates with memory dev tools.

Below are before-and-after screenshots from my test app’s memory dev tools. You can see that within one minute, the test app running on main grew by 95 MB, compared to the test app running with the fix that prevents duplicates. Note that the dips on the right side of the image are caused by taking heap snapshots.

Main

This branch

closes #48860

[facebook-github-bot]

Hi @c-miles!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[c-miles]

Assumed duplicate removals are expected during cleanup; with __addChild now preventing duplicates, the warning in __removeChild causes unnecessary test failures and log noise.

[c-miles]

This check prevents duplicate animated nodes from being added, which fixes the memory leak we observed.

The fact that _addChild is being called repeatedly with the same node may suggest an opportunity for further architectural review, though this fix addresses the immediate issue.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[c-miles]

I’m following up on this pull request. Could someone please review it and let me know if any further modifications are needed to move it forward? Thanks in advance.

[rodriguescarson]

Approved

[javache]

I would expect that if addChild is called multiple times, so would removeChild. Are there any callers which may be missing this?

Could you find a test for this which doesn't rely on calling __addChild explicitly?

[c-miles]

@javache Your expectation about removeChild being called for each addChild is correct.

All classes extending AnimatedWithChildren (AnimatedValue, AnimatedTransform, AnimatedInterpolation, AnimatedStyle, etc.) properly call removeChild when unmounting or detaching.

The issue occurs when multiple addChild calls happen with the same child node (common with reused AnimatedValues in transforms or interpolation chains). The current implementation only removes one instance when removeChild is called:

__removeChild(child: AnimatedNode): void {
  const index = this._children.indexOf(child);
  if (index === -1) {
    // Remove fails if child not found
    return;
  }
  this._children.splice(index, 1);
}

I chose to prevent duplication at the __addChild level rather than modifying removeChild to find all instances.

I've added a test demonstrating the 1:1 relationship without explicitly calling __addChild.

An alternative solution would be using a Set instead of an Array for _children, which would naturally prevent duplicates.

[yungsters]

Thanks for the detailed pull request, @c-miles.

If I understand @javache's point, it should always be the case that if __addChild is called N times with the same AnimatedValue child, there will be N invocations of __removeChild on unmount. And it seems like you agree.

Do I understand correctly that the problem you're raising here is the unbounded growth of this._children (which is probably exacerbated by linear operations like this._children.indexOf(child)?

If so, then I understand why you are approaching the solution in this way.

I think @javache's concern with this fix is the expectation that __addChild and __removeChild operate like reference counting, in which two invocations of __addChild and one invocation of __removeChild should still retain a reference to the child in AnimatedWithChildren.

An alternate approach that prevents unbounded growth of this._children (and slow linear operations) would be to change this._children to instead be a Map<AnimatedNode, number> — a map from AnimatedNode child to references.

• In __addChild, we add child to the map if it does not exist. Then, we increment its value.
• In __removeChild, we decrement the value of child in the map. If it would become zero, delete child.

Side effects (e.g. __makeNative) would only be triggered when adding or removing child from the map.

[micahlt]

Anything preventing this from being merged?? This is a critical issue in a few of my apps, would love to see this fix merged.

[c-miles]

@yungsters Thank you for the detailed feedback. Your Map based reference counting suggestion makes complete architectural sense.

After digging deeper, I found the root cause is actually in React Native Web, not core. React Native Web's AnimatedProps constructor calls this.__attach(), then the lifecycle hook calls it again, causing duplicate children. Core doesn't have this bug since the constructor doesn't call __attach().

Removing that constructor call in React Native Web completely fixes the leak. I'll open a pull request there to address it at the source.

@micahlt Sorry for the delay! We forked RN with this fix and I moved roles and forgot about the PR. If you're using React Native Web, that's where the fix needs to be. If you're on iOS/Android core, I'm curious about your setup as I couldn't reproduce it there.

Closing this since the issue doesn't affect core. Thanks for pushing me to investigate further!

[c-miles]

@micahlt

Here's the pr on web: Fix memory leak in AnimatedProps constructor

[react-native-bot]

	Warnings
⚠️	❗ JavaScript API change detected - This PR commits an update to ReactNativeApi.d.ts, indicating a change to React Native's public JavaScript API. Please include a clear changelog message. This change will be subject to extra review. This change was flagged as: BREAKING

Generated by 🚫 dangerJS against 5afcda0