Fix Ref Lifecycles in Hidden Subtrees by yungsters · Pull Request #31379 · facebook/react

yungsters · 2024-10-28T23:32:37Z

Summary

We're seeing certain situations in React Native development where ref callbacks in <Activity mode="hidden"> are sometimes invoked exactly once with null without ever being called with a "current" value.

This violates the contract for refs because refs are expected to always attach before detach (and to always eventually detach after attach). This is particularly bad for refs that return cleanup functions, because refs that return cleanup functions expect to never be invoked with null. This bug causes such refs to be invoked with null (because since safelyAttachRef was never called, safelyDetachRef thinks the ref does not return a cleanup function and invokes it with null).

This fix makes use of offscreenSubtreeWasHidden in commitDeletionEffectsOnFiber, similar to how ec52a56 did this for commitDeletionEffectsOnFiber.

How did you test this change?

We were able to isolate the repro steps to isolate the React Native experimental changes. However, the repro steps depend on Fast Refresh.

function callbackRef(current) {
  // Called once with `current` as null, upon triggering Fast Refresh.
}

<Activity mode="hidden">
  <View ref={callbackRef} />;
</Activity>

Ideally, we would have a unit test that verifies this behavior without Fast Refresh. (We have evidence that this bug occurs without Fast Refresh in real product implementations. However, we have not successfully deduced the root cause, yet.)

This PR currently includes a unit test that reproduces the Fast Refresh scenario, which is also demonstrated in this CodeSandbox: https://codesandbox.io/p/sandbox/hungry-darkness-33wxy7

Verified unit tests pass:

$ yarn
$ yarn test
# Run with `-r=www-classic` for `enableScopeAPI` tests.
$ yarn test -r=www-classic

Verified on the internal React Native development branch that the bug no longer repros.

vercel · 2024-10-28T23:32:42Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
react-compiler-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 30, 2024 9:23pm

react-sizebot · 2024-10-28T23:36:09Z

The size diff is too large to display in a single comment. The GitHub action for this pull request contains an artifact called 'sizebot-message.md' with the full message.

Generated by 🚫 dangerJS against 82bea69

yungsters · 2024-10-28T23:57:00Z

By the way, I don't add these checks to the safelyDetachRef calls in disappearLayoutEffects because recursivelyTraverseDisappearLayoutEffects (which calls disappearLayoutEffects) already checks for offscreenSubtreeWasHidden on Line 2017.

kassens · 2024-10-29T14:26:44Z

        // TODO: This is a temporary solution that allowed us to transition away
        // from React Flare on www.
-        if (flags & Ref) {
+        if (flags & Ref && !offscreenSubtreeWasHidden) {


It doesn't seem clear to me if the safelyAttachRef call should be done conditional here.

Conceptually, it makes sense to skip both. I'm not sure why Scope refs are even being attached here in the first place. 😕

sammy-SC · 2024-10-29T15:34:41Z

I believe this is a good approach to fix it.

Could you explain in the PR description why offscreenSubtreeWasHidden is the right choice here?

yungsters · 2024-10-29T16:02:27Z

I believe this is a good approach to fix it.

Could you explain in the PR description why offscreenSubtreeWasHidden is the right choice here?

@sammy-SC Added this to the PR summary:

This fix makes use of offscreenSubtreeWasHidden in commitDeletionEffectsOnFiber, similar to how ec52a56 did this for commitDeletionEffectsOnFiber.

yungsters · 2024-10-29T18:40:59Z

      commitReconciliationEffects(finishedWork);

-      if (flags & Ref) {
+      if (flags & Ref && !offscreenSubtreeWasHidden) {


For the Jest unit test that @rickhanlonii added in a2c9e7a, this is the only line that is actually necessary to fix the unit test.

yungsters · 2024-10-29T23:18:12Z

+          if (!offscreenSubtreeWasHidden && current !== null) {
            safelyDetachRef(finishedWork, finishedWork.return);
          }
-          safelyAttachRef(finishedWork, finishedWork.return);
+          if (!offscreenSubtreeIsHidden) {
+            safelyAttachRef(finishedWork, finishedWork.return);
+          }


@kassens, I think this is the correct behavior.

You were right that !offscreenSubtreeWasHidden should only be scoped to safelyDetachRef, and we were missing a !offscreenSubtreeIsHidden check around safelyAttachRef.

Are you sure about this behavior? We don't check for Offscreen anywhere else that we attach.

We don't check for Offscreen anywhere else that we attach.

We actually do, but it's implicit.

Every other type calls safelyAttachRef from reappearLayoutEffects, which is called only by recursivelyTraverseReappearLayoutEffects. These occur during:

commitLayoutEffectOnFiber, and

recursivelyTraverseReappearLayoutEffects

In both of these, we prune traversal when we encounter a hidden Offscreen. (See the links above for where this happens.)

Are you sure about this behavior?

I'm not sure why ScopeComponent requires safelyAttachRef to be here, but I assume it has something to do with the comment above:

// TODO: This is a temporary solution that allowed us to transition away // from React Flare on www.

I don't plan to change this as part of fixing refs, though. I considered an alternative in which I leave ScopeComponent alone, but I think this is actually the correct fix.

I'm not sure why ScopeComponent requires safelyAttachRef to be here, [...]

I traced the origin of that comment (which was harder than I expected due to the reconciler forking and unforking that happened) and found this: #19264

To unblock internal progression on replacing React Flare, let's instead move resolution of Scope API refs to the mutation phase, so we can attach event listeners to scopes before the layout phase begins.

I don't fully understand the context, but I figured that it would be helpful to know its origin.

~~After writing unit tests for this bug, I've confirmed that these changes are actually not needed to prevent <Scope ref={X}> from invoking X in <Activity mode="hidden">.

I've dropped these changes for now.~~

Oh… just kidding, I was not running yarn test with -r=www-classic (which is where enableScopeAPI is enabled). These changes are needed after all, will update.

I confirmed that Scope was previously completely ignoring any parent Activity that declared the subtree as hidden.

yungsters · 2024-10-30T21:04:41Z

+      if (!offscreenSubtreeWasHidden) {
+        safelyDetachRef(deletedFiber, nearestMountedAncestor);
+      }


In the unit test for "ignores ref for Activity in hidden subtree", I discovered that this and the call from commitMutationEffectsOnFiber are triggered, causing X to be incorrectly invoked twice! 🤯

<Activity mode="hidden"> <Activity mode="visible" ref={X}> <div /> </Activity> </Activity>

yungsters · 2024-10-30T21:05:42Z

I've expanded on the unit tests and verified that 1) each new test fails without this PR, and 2) each line modified by this PR (except for that one comment misspelling) is necessary for one of the new tests to succeed.

## Summary We're seeing certain situations in React Native development where ref callbacks in `<Activity mode="hidden">` are sometimes invoked exactly once with `null` without ever being called with a "current" value. This violates the contract for refs because refs are expected to always attach before detach (and to always eventually detach after attach). This is *particularly* bad for refs that return cleanup functions, because refs that return cleanup functions expect to never be invoked with `null`. This bug causes such refs to be invoked with `null` (because since `safelyAttachRef` was never called, `safelyDetachRef` thinks the ref does not return a cleanup function and invokes it with `null`). This fix makes use of `offscreenSubtreeWasHidden` in `commitDeletionEffectsOnFiber`, similar to how ec52a56 did this for `commitDeletionEffectsOnFiber`. ## How did you test this change? We were able to isolate the repro steps to isolate the React Native experimental changes. However, the repro steps depend on Fast Refresh. ``` function callbackRef(current) { // Called once with `current` as null, upon triggering Fast Refresh. } <Activity mode="hidden"> <View ref={callbackRef} />; </Activity> ``` Ideally, we would have a unit test that verifies this behavior without Fast Refresh. (We have evidence that this bug occurs without Fast Refresh in real product implementations. However, we have not successfully deduced the root cause, yet.) This PR currently includes a unit test that reproduces the Fast Refresh scenario, which is also demonstrated in this CodeSandbox: https://codesandbox.io/p/sandbox/hungry-darkness-33wxy7 Verified unit tests pass: ``` $ yarn $ yarn test # Run with `-r=www-classic` for `enableScopeAPI` tests. $ yarn test -r=www-classic ``` Verified on the internal React Native development branch that the bug no longer repros. --------- Co-authored-by: Rick Hanlon <rickhanlonii@fb.com> DiffTrain build for [ea3ac58](ea3ac58)

@rickhanlonii

## Summary While fixing ref lifecycles in hidden subtrees in #31379, @rickhanlonii noticed that we could also add more unit tests for other types of tags to prevent future regressions during code refactors. This PR adds more unit tests in the same vein as those added in #31379. ## How did you test this change? Verified unit tests pass: ``` $ yarn $ yarn test ReactFreshIntegration-test.js ```

@yungsters

In the unit test for "ignores ref for Activity in hidden subtree", I discovered that this *and* the call from `commitMutationEffectsOnFiber` are triggered, causing `X` to be incorrectly invoked *twice*! 🤯 ``` <Activity mode="hidden"> <Activity mode="visible" ref={X}> <div /> </Activity> </Activity> ``` _Originally posted by @yungsters in facebook/react#31379 (comment)

Avoid Detaching Refs on Hidden Subtrees

bdfe9fe

facebook-github-bot added the CLA Signed label Oct 28, 2024

yungsters requested review from acdlite and sammy-SC October 28, 2024 23:32

yungsters assigned rickhanlonii and unassigned rickhanlonii Oct 28, 2024

yungsters requested a review from rickhanlonii October 28, 2024 23:33

vercel Bot deployed to Preview October 28, 2024 23:34 View deployment

kassens reviewed Oct 29, 2024

View reviewed changes

sammy-SC approved these changes Oct 29, 2024

View reviewed changes

Add fast refresh test

a2c9e7a

vercel Bot deployed to Preview October 29, 2024 18:22 View deployment

yungsters commented Oct 29, 2024

View reviewed changes

Isolate !offscreenSubtreeWasHidden checks from flags checks

2101de0

vercel Bot deployed to Preview October 29, 2024 22:55 View deployment

Hoist __DEV__ check into @gate pragma

1426f6d

vercel Bot deployed to Preview October 29, 2024 23:10 View deployment

Check offscreenSubtreeIsHidden for attaching ref of Scope

52c0e97

yungsters commented Oct 29, 2024

View reviewed changes

yungsters requested review from kassens and sammy-SC October 29, 2024 23:18

vercel Bot deployed to Preview October 29, 2024 23:18 View deployment

kassens approved these changes Oct 30, 2024

View reviewed changes

Simplify test code (in preparation for adding more)

a0685eb

vercel Bot deployed to Preview October 30, 2024 21:00 View deployment

yungsters added 2 commits October 30, 2024 14:00

Add more test cases

4036c91

Drop changes to ScopeComponent in commitMutationEffectsOnFiber

1b7c7ee

yungsters force-pushed the hidden-detach-refs branch from a21f919 to 1b7c7ee Compare October 30, 2024 21:01

vercel Bot deployed to Preview October 30, 2024 21:02 View deployment

yungsters commented Oct 30, 2024

View reviewed changes

Fix missing Activity import in test case

dc0c3a5

vercel Bot deployed to Preview October 30, 2024 21:09 View deployment

Fix ScopeComponent detach/attach behavior

82bea69

vercel Bot deployed to Preview October 30, 2024 21:23 View deployment

yungsters changed the title ~~Avoid Detaching Refs on Hidden Subtrees~~ Fix Ref Lifecycles in Hidden Subtrees Oct 31, 2024

rickhanlonii approved these changes Oct 31, 2024

View reviewed changes

yungsters merged commit ea3ac58 into facebook:main Oct 31, 2024

yungsters deleted the hidden-detach-refs branch October 31, 2024 20:25

vercel-release-bot mentioned this pull request Nov 1, 2024

Upgrade React from 603e6108-20241029 to b7e21579-20241031 vercel/next.js#72187

Closed

yungsters mentioned this pull request Nov 1, 2024

More Unit Tests for Refs in Hidden Subtrees #31404

Merged

This was referenced Nov 1, 2024

Upgrade React from 603e6108-20241029 to 16409d05-20241101 vercel/next.js#72198

Closed

Upgrade React from 603e6108-20241029 to 7c8e5e7a-20241101 vercel/next.js#72199

Merged

7XGH mentioned this pull request Nov 2, 2024

Conceptually, it makes sense to skip both. I'm not sure why Scope refs are even being attached here in the first place. 😕 #31408

Closed

Conversation

yungsters commented Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

How did you test this change?

Uh oh!

vercel Bot commented Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

react-sizebot commented Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yungsters commented Oct 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sammy-SC commented Oct 29, 2024

Uh oh!

yungsters commented Oct 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yungsters Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yungsters commented Oct 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

yungsters commented Oct 28, 2024 •

edited

Loading

vercel Bot commented Oct 28, 2024 •

edited

Loading

react-sizebot commented Oct 28, 2024 •

edited

Loading

yungsters Oct 30, 2024 •

edited

Loading