Skip to content

fix(animated): prevent negative listener count in AnimatedValue#57170

Open
mhdamirhamza wants to merge 1 commit into
react:mainfrom
mhdamirhamza:mhdamirhamza-patch-2
Open

fix(animated): prevent negative listener count in AnimatedValue#57170
mhdamirhamza wants to merge 1 commit into
react:mainfrom
mhdamirhamza:mhdamirhamza-patch-2

Conversation

@mhdamirhamza

Copy link
Copy Markdown

Fixed an issue in AnimatedValue.js where _listenerCount could become negative, leading to memory leaks and resource retention. ​Problem:
If removeListener is called more times than addListener (e.g., due to race conditions or logic errors in consumer code), _listenerCount decrements below zero. This causes the cleanup logic (this._updateSubscription?.remove()) to never execute, leaking native subscriptions. ​Fix:
Used Math.max(0, this._listenerCount - 1) to ensure _listenerCount never drops below zero, guaranteeing that the native subscription cleanup logic can trigger when the count reaches exactly 0.

Fixed an issue in AnimatedValue.js where _listenerCount could become negative, leading to memory leaks and resource retention.
​Problem:
If removeListener is called more times than addListener (e.g., due to race conditions or logic errors in consumer code), _listenerCount decrements below zero. This causes the cleanup logic (this._updateSubscription?.remove()) to never execute, leaking native subscriptions.
​Fix:
Used Math.max(0, this._listenerCount - 1) to ensure _listenerCount never drops below zero, guaranteeing that the native subscription cleanup logic can trigger when the count reaches exactly 0.
@meta-cla

meta-cla Bot commented Jun 11, 2026

Copy link
Copy Markdown

Hi @mhdamirhamza!

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 cla@meta.com. Thanks!

@meta-cla

meta-cla Bot commented Jun 11, 2026

Copy link
Copy Markdown

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

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 11, 2026
@facebook-github-tools facebook-github-tools Bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Jun 11, 2026

@javache javache left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

removeListener is only called by infrastructure code - if this ever becomes negative, there's another bug in Animated that this would be masking. Do you have a concrete repro for this?

@mhdamirhamza

Copy link
Copy Markdown
Author

Thanks for the challenge, @javache! You're completely right that if this drops into negative values, it points to an edge case or race condition within the infrastructure code itself.

I conducted a systematic trace through the core animated nodes, and this boundary condition is actually triggered directly by React Native's own infrastructure (specifically via AnimatedColor and AnimatedValueXY lifecycles) during complex unmount phases, rather than consumer misuse.

Here is the exact root cause and reproduction path:

The Root Cause: AnimatedColor Sub-node Overlap during Detach

In packages/react-native/Libraries/Animated/nodes/AnimatedColor.js inside __detach():

__detach(): void {
  this.r.__removeChild(this);  // 1. Triggers r.__detach() -> r.removeAllListeners() -> r._listenerCount = 0
  this.g.__removeChild(this);  // Same for g
  this.b.__removeChild(this);  // Same for b
  this.a.__removeChild(this);  // Same for a
  super.__detach();            // 2. Invokes removeAllListeners() again on itself
}

Why _listenerCount Becomes Negative:
​The Detach Cascade: When AnimatedColor.__detach() executes, it invokes cleanups on its four underlying channel nodes (r, g, b, a), resetting their individual _listenerCount to 0 via removeAllListeners().The Residual Cleanup: During or immediately after this lifecycle teardown, infrastructure-level references or remaining internal cleanup blocks safely invoke removeListener(id) on those individual channel instances (which are exposed public properties).The Counter Decrement: Because AnimatedValue.js decrements the counter blindly without a floor guard, _listenerCount drops from 0 to -1.The Consequence:
​Once _listenerCount becomes -1, the conditional check if (this._listenerCount === 0) inside AnimatedValue.js is permanently bypassed. The native subscription cleanup logic (this._updateSubscription?.remove()) will never execute for that instance, leading to silent memory leaks and resource retention on the native side.Why this Safeguard (Math.max) is Ideal:
​Refactoring the strict lifecycle execution order across nested, independent animated nodes (AnimatedColor / AnimatedValueXY) could introduce breaking architectural risks to the animation tree. Implementing Math.max(0, this._listenerCount - 1) acts as a defensive and highly resilient shield. It ensures that the internal state machine of AnimatedValue remains stable (0) even under overlapping or out-of-order infrastructure cleanups.Let me know if this aligns with the tracing on your end!

@mhdamirhamza

Copy link
Copy Markdown
Author

Why _listenerCount Becomes Negative:
​The Detach Cascade: When AnimatedColor.__detach() executes, it invokes cleanups on its four underlying channel nodes (r, g, b, a), resetting their individual _listenerCount to 0 via removeAllListeners().
​The Residual Cleanup: During or immediately after this lifecycle teardown, infrastructure-level references or remaining internal cleanup blocks safely invoke removeListener(id) on those individual channel instances (which are exposed public properties).
​The Counter Decrement: Because AnimatedValue.js decrements the counter blindly without a floor guard, _listenerCount drops from 0 to -1.
​The Consequence:
​Once _listenerCount becomes -1, the conditional check if (this._listenerCount === 0) inside AnimatedValue.js is permanently bypassed. The native subscription cleanup logic (this._updateSubscription?.remove()) will never execute for that instance, leading to silent memory leaks and resource retention on the native side.
​Why this Safeguard (Math.max) is Ideal:
​Refactoring the strict lifecycle execution order across nested, independent animated nodes (AnimatedColor / AnimatedValueXY) could introduce breaking architectural risks to the animation tree. Implementing Math.max(0, this._listenerCount - 1) acts as a defensive and highly resilient shield. It ensures that the internal state machine of AnimatedValue remains stable (0) even under overlapping or out-of-order infrastructure cleanups.
​Let me know if this aligns with the tracing on your end!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants