Fix AnimatedPropsRegistry surface-stop resurrection race (#57376)#57376
Closed
bartlomiejbloniarz wants to merge 1 commit into
Closed
Fix AnimatedPropsRegistry surface-stop resurrection race (#57376)#57376bartlomiejbloniarz wants to merge 1 commit into
bartlomiejbloniarz wants to merge 1 commit into
Conversation
|
@bartlomiejbloniarz has exported this pull request. If you are a Meta employee, you can view the originating Diff in D109156094. |
meta-codesync Bot
pushed a commit
that referenced
this pull request
Jun 30, 2026
Summary: AnimatedPropsRegistry::update() runs on the UI thread every animation frame and created a surface's entry via operator[]. clearOnSurfaceStop() (run on the JS thread when a surface stops) erases that entry, but an in-flight animation frame landing after the stop re-created it via operator[] -- and since the surface is gone, nothing ever cleans it up again. The resurrected entry leaks its PropsSnapshot and ShadowNodeFamily for the lifetime of the registry. A surface's entry is legitimately created by getMap(), which AnimationBackendCommitHook calls on every React commit. stopSurface drains in-flight commits before unregistering the ShadowTree (ShadowTreeRegistry::remove takes the registry's unique lock, which excludes the shared-locked commit visits), so getMap() can never run for a stopped surface. That leaves update()'s operator[] as the only thing that can resurrect one. Fix: update() now only refines surfaces that already exist (find instead of operator[]) and never creates an entry; getMap() remains the sole creator. A stopped surface can no longer be resurrected, and there is no extra bookkeeping that could grow over time. Changelog: [General][Fixed] - Fix a surface-stop race in the C++ Animated shared backend that could permanently leak per-surface animated state Differential Revision: D109156094
f63eaec to
e1b6006
Compare
meta-codesync Bot
pushed a commit
that referenced
this pull request
Jun 30, 2026
Summary: AnimatedPropsRegistry::update() runs on the UI thread every animation frame and created a surface's entry via operator[]. clearOnSurfaceStop() (run on the JS thread when a surface stops) erases that entry, but an in-flight animation frame landing after the stop re-created it via operator[] -- and since the surface is gone, nothing ever cleans it up again. The resurrected entry leaks its PropsSnapshot and ShadowNodeFamily for the lifetime of the registry. A surface's entry is legitimately created by getMap(), which AnimationBackendCommitHook calls on every React commit. stopSurface drains in-flight commits before unregistering the ShadowTree (ShadowTreeRegistry::remove takes the registry's unique lock, which excludes the shared-locked commit visits), so getMap() can never run for a stopped surface. That leaves update()'s operator[] as the only thing that can resurrect one. Fix: update() now only refines surfaces that already exist (find instead of operator[]) and never creates an entry; getMap() remains the sole creator. A stopped surface can no longer be resurrected, and there is no extra bookkeeping that could grow over time. Changelog: [General][Fixed] - Fix a surface-stop race in the C++ Animated shared backend that could permanently leak per-surface animated state Differential Revision: D109156094
e1b6006 to
027e29f
Compare
meta-codesync Bot
pushed a commit
that referenced
this pull request
Jul 1, 2026
Summary: AnimatedPropsRegistry::update() runs on the UI thread every animation frame and created a surface's entry via operator[]. clearOnSurfaceStop() (run on the JS thread when a surface stops) erases that entry, but an in-flight animation frame landing after the stop re-created it via operator[] -- and since the surface is gone, nothing ever cleans it up again. The resurrected entry leaks its PropsSnapshot and ShadowNodeFamily for the lifetime of the registry. A surface's entry is legitimately created by getMap(), which AnimationBackendCommitHook calls on every React commit. stopSurface drains in-flight commits before unregistering the ShadowTree (ShadowTreeRegistry::remove takes the registry's unique lock, which excludes the shared-locked commit visits), so getMap() can never run for a stopped surface. That leaves update()'s operator[] as the only thing that can resurrect one. Fix: update() now only refines surfaces that already exist (find instead of operator[]) and never creates an entry; getMap() remains the sole creator. A stopped surface can no longer be resurrected, and there is no extra bookkeeping that could grow over time. Changelog: [General][Fixed] - Fix a surface-stop race in the C++ Animated shared backend that could permanently leak per-surface animated state Reviewed By: javache Differential Revision: D109156094
027e29f to
dd607d2
Compare
Summary: AnimatedPropsRegistry::update() runs on the UI thread every animation frame and created a surface's entry via operator[]. clearOnSurfaceStop() (run on the JS thread when a surface stops) erases that entry, but an in-flight animation frame landing after the stop re-created it via operator[] -- and since the surface is gone, nothing ever cleans it up again. The resurrected entry leaks its PropsSnapshot and ShadowNodeFamily for the lifetime of the registry. A surface's entry is legitimately created by getMap(), which AnimationBackendCommitHook calls on every React commit. stopSurface drains in-flight commits before unregistering the ShadowTree (ShadowTreeRegistry::remove takes the registry's unique lock, which excludes the shared-locked commit visits), so getMap() can never run for a stopped surface. That leaves update()'s operator[] as the only thing that can resurrect one. Fix: update() now only refines surfaces that already exist (find instead of operator[]) and never creates an entry; getMap() remains the sole creator. A stopped surface can no longer be resurrected, and there is no extra bookkeeping that could grow over time. Changelog: [General][Fixed] - Fix a surface-stop race in the C++ Animated shared backend that could permanently leak per-surface animated state Reviewed By: javache Differential Revision: D109156094
dd607d2 to
fa21f3f
Compare
|
This pull request has been merged in d9d2502. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
AnimatedPropsRegistry::update() runs on the UI thread every animation frame and
created a surface's entry via operator[]. clearOnSurfaceStop() (run on the JS
thread when a surface stops) erases that entry, but an in-flight animation frame
landing after the stop re-created it via operator[] -- and since the surface is
gone, nothing ever cleans it up again. The resurrected entry leaks its
PropsSnapshot and ShadowNodeFamily for the lifetime of the registry.
A surface's entry is legitimately created by getMap(), which
AnimationBackendCommitHook calls on every React commit. stopSurface drains
in-flight commits before unregistering the ShadowTree
(ShadowTreeRegistry::remove takes the registry's unique lock, which excludes the
shared-locked commit visits), so getMap() can never run for a stopped surface.
That leaves update()'s operator[] as the only thing that can resurrect one.
Fix: update() now only refines surfaces that already exist (find instead of
operator[]) and never creates an entry; getMap() remains the sole creator. A
stopped surface can no longer be resurrected, and there is no extra bookkeeping
that could grow over time.
Changelog: [General][Fixed] - Fix a surface-stop race in the C++ Animated shared backend that could permanently leak per-surface animated state
Reviewed By: javache
Differential Revision: D109156094