Fix react-freeze #7053

bartlomiejbloniarz · 2025-02-19T12:59:29Z

Summary

This PR is very annoying. Until now we assumed that when componentWillUnmount is called, the component will never reappear. This is false when using <Suspense> (docs), which is exactly what react-freeze does. To fix the issue I had to move our animation cleanup functions to be called only when we are sure that the component will not reappear. This is how the new approach works for each registry:

AnimatedPropsRegistry: we remove a record from this registry only when we fail to apply it onto the ShadowTree (in the commit hook or perfromOperations)
CSSAnimationsRegistry: we remove a record from this registry only when we fail to apply it onto the ShadowTree (in the commit hook or perfromOperations)
CSSTransitionsRegistry: we remove a record from this registry when componentWillUnmount is called and there is no ongoing transition OR we failed to apply it onto the ShadowTree (in the commit hook or perfromOperations)
StaticPropsRegistry remains the same as it doesn't store any state that needs to be persisted when freeze is used

One problem with this approach is that we are running animations on components that are freezed. This is a waste of resources. This will be addressed in a separate PR, but the main idea is to pause animations in componentWillUnmount and resume them (with the new timestamp) in compnentDidMount.

Also currently the leak check will say that AnimatedPropsRegistry leaked if we don't reload it. This is because an animation frame comes after the cleanup from the commit hook. Normally this would be cleaned up in performOperations, but because of the sunchronouslyUpdateProps fast-path it won't. The cleanup will still happen on the next commit hook invocation (i.e. if you hit reload on the leak check). This issue soon will be gone as we are removing the fast path in #7014.

Before	After
Screen.Recording.2025-02-19.at.13.57.39.mov	Screen.Recording.2025-02-19.at.13.44.07.mov

Test plan

Go through the app, and then visit the FreezeExample. Use the check for registry leaks button.

tjzel · 2025-02-21T16:40:13Z

packages/react-native-reanimated/Common/cpp/reanimated/Fabric/ShadowTreeCloner.cpp

+      if (ancestors.empty()) {
+        tagsToRemove.push_back(family->getTag());
+      }


Let's annotate that empty ancestors means that the shadow node was (hopefully) irreversibly detached

tjzel · 2025-02-21T16:42:48Z

packages/react-native-reanimated/Common/cpp/reanimated/Fabric/ReanimatedCommitHook.cpp

+    std::vector<Tag> tagsToRemove;
+    rootNode = cloneShadowTreeWithNewProps(*rootNode, propsMap, tagsToRemove);
+    if (!tagsToRemove.empty()) {
+      updatesRegistryManager_->removeBatch(tagsToRemove);
+    }


I don't like returning values via parameters but let's let it slide

tjzel · 2025-02-21T16:44:03Z

packages/react-native-reanimated/Common/cpp/reanimated/CSS/registry/StaticPropsRegistry.h

@@ -20,7 +21,8 @@ class StaticPropsRegistry {
  folly::dynamic get(Tag viewTag) const;
  bool has(Tag viewTag) const;
  void remove(Tag viewTag);
-
+  void removeBatch(const std::vector<Tag> &tagsToRemove);
+  bool empty();


Suggested change

bool empty();

bool isEmpty();

tjzel · 2025-02-21T16:47:51Z

packages/react-native-reanimated/Common/cpp/reanimated/CSS/registry/CSSAnimationsRegistry.h

  void updateSettings(
      Tag viewTag,
      const SettingsUpdates &settingsUpdates,
      double timestamp);

  void update(double timestamp);
+  bool empty();


Suggested change

bool empty();

bool isEmpty();

tjzel · 2025-02-21T16:51:29Z

packages/react-native-reanimated/Common/cpp/reanimated/Fabric/updates/UpdatesRegistry.h

@@ -42,10 +42,13 @@ class UpdatesRegistry {

  void flushUpdates(UpdatesBatch &updatesBatch, bool merge);
  void collectProps(PropsMap &propsMap);
+  virtual void removeBatch(const std::vector<Tag> &tagsToRemove);
+  bool empty();


Suggested change

bool empty();

bool isEmpty();

tjzel · 2025-02-21T17:00:31Z

packages/react-native-reanimated/Common/cpp/reanimated/RuntimeDecorators/UIRuntimeDecorator.cpp

-#ifdef RCT_NEW_ARCH_ENABLED
-    const RemoveFromPropsRegistryFunction removeFromPropsRegistry,
-#else
+#ifndef RCT_NEW_ARCH_ENABLED


Suggested change

#ifndef RCT_NEW_ARCH_ENABLED

#ifdef RCT_NEW_ARCH_ENABLED

// nothing

#else

tjzel · 2025-02-21T17:00:46Z

packages/react-native-reanimated/Common/cpp/reanimated/RuntimeDecorators/UIRuntimeDecorator.h

-#ifdef RCT_NEW_ARCH_ENABLED
-      const RemoveFromPropsRegistryFunction removeFromPropsRegistry,
-#else
+#ifndef RCT_NEW_ARCH_ENABLED


Suggested change

#ifndef RCT_NEW_ARCH_ENABLED

#ifdef RCT_NEW_ARCH_ENABLED

// nothing

#else

tjzel · 2025-02-21T17:03:12Z

packages/react-native-reanimated/src/privateGlobals.d.ts

@@ -54,7 +54,6 @@ declare global {
        }[]
      ) => void)
    | undefined;
-  var _removeFromPropsRegistry: (viewTags: number[]) => void | undefined;


Add the leak check thing here.

tjzel · 2025-02-21T17:07:17Z

apps/common-app/src/components/LeakCheck.tsx

+    // eslint-disable-next-line no-underscore-dangle
+    return (global._registriesLeakCheck as () => string)();
+  }
+  return '';


return global?._registriesLeakCheck() ?? '';

tjzel · 2025-02-21T17:08:43Z

apps/common-app/src/App.tsx


 export default function App() {
+  const [droped, setNuked] = useState(false);


Suggested change

const [droped, setNuked] = useState(false);

const [nuked, setNuked] = useState(false);

bartlomiejbloniarz added 3 commits February 19, 2025 12:22

move registry cleanup from componenWillUnmount

f4b4c6b

typescript fix

d7ee1b6

leave the StaticPropsRegistry as it was

d044191

bartlomiejbloniarz requested review from kmagiera, piaskowyk and tomekzaw as code owners February 19, 2025 14:39

bartlomiejbloniarz force-pushed the @bartlomiejbloniarz/fix-react-freeze branch from 9435608 to d044191 Compare February 20, 2025 08:29

bartlomiejbloniarz added 6 commits February 20, 2025 09:46

Merge branch 'main' into @bartlomiejbloniarz/fix-react-freeze

9d603ca

format

0a5bc61

fix lost js utils

a46b27f

cpplint

f4bf715

sort imports

53d9a1c

format jsi utils :)

419d93c

MatiPl01 mentioned this pull request Feb 20, 2025

[BUG ] :: useAnimatedStyle reset style when screen refocus #7048

Open

bartlomiejbloniarz added 4 commits February 20, 2025 19:22

Merge branch 'main' into @bartlomiejbloniarz/fix-react-freeze

7d3f1bf

fix merge

8f1205b

Merge branch 'main' into @bartlomiejbloniarz/fix-react-freeze

3e4cf26

move leak check behind new arch flag

ec62b83

zfurtak mentioned this pull request Feb 21, 2025

[$250] iOS - Troubleshoot- The "Client side logging" toggle is automatically turned off Expensify/App#53713

Open

8 tasks

tjzel approved these changes Feb 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix react-freeze #7053

Fix react-freeze #7053

bartlomiejbloniarz commented Feb 19, 2025

tjzel Feb 21, 2025

tjzel Feb 21, 2025

tjzel Feb 21, 2025

tjzel Feb 21, 2025

tjzel Feb 21, 2025

tjzel Feb 21, 2025

tjzel Feb 21, 2025

tjzel Feb 21, 2025

tjzel Feb 21, 2025

tjzel Feb 21, 2025


		export default function App() {
		const [droped, setNuked] = useState(false);

	const [droped, setNuked] = useState(false);
	const [nuked, setNuked] = useState(false);

Fix react-freeze #7053

Are you sure you want to change the base?

Fix react-freeze #7053

Conversation

bartlomiejbloniarz commented Feb 19, 2025

Summary

Test plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment