Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix context propagation for offscreen/fallback trees #23095

Merged
merged 7 commits into from
Jan 19, 2022

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Jan 11, 2022

Fixes #19701.

As #19702 shows, this assumption is not always true:

// Neither alternate was updated, which means the rest of the
// ancestor path already has sufficient priority.
break;

In particular, commenting out this break; fixes #19701.

@acdlite told me this is because inside Offscreen/fallback trees, childLanes may not necessarily be consistent with surroundings because we commit surroundings without the children.

If I just comment out that break; we'll always traverse to the root. This seems bad, especially inside deep trees. Since this function is used during propagation. So I'm changing this to stop at the propagation root.

Aside from fixing what happens inside Offscreen/fallback trees, we also need to consider what happens when the loop goes outside the propagation root. Implementation-wise, this change is not a no-op for nodes outside. You can verify that we used to traverse upwards (past the propagation root) in some cases and modify the childLanes of ancestors. However, @acdlite said that from the model perspective, this shouldn't be necessary to do — we wouldn't have reached the propagation root if there weren't work already scheduled on it. And a parent of a propagation root doesn't need to have dirty childLanes.

So this adds some extra traversal to propagation but buys back some of the cost where we used to do deeper unnecessary traversal upwards past the propagation root. (But now we always stop at the propagation root.) Suspense call from beginWork is an exception though, as it will now always traverse to the root but it used to be able to bail out in the middle. Nvm I fixed that too.

I've included a failing test from #19702 to show it now passes. I imagine there may be other cases we don't know about where context failed to propagate so it's not just about this bug alone. My concern isn't so much Consumer API but the flaw in modeling. The assumption seems fundamentally flawed and I'm worried how else it might bite us.

@gaearon gaearon requested a review from acdlite January 11, 2022 19:16
@facebook-github-bot facebook-github-bot added the React Core Team Opened by a member of the React Core Team label Jan 11, 2022
@gaearon gaearon changed the title Fix context propagation for offscreen trees Fix context propagation for offscreen/fallback trees Jan 11, 2022
@sizebot
Copy link

sizebot commented Jan 11, 2022

Comparing: 9a7e6bf...d465a97

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 129.52 kB 129.53 kB +0.05% 41.53 kB 41.55 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 134.65 kB 134.66 kB +0.05% 43.01 kB 43.04 kB
facebook-www/ReactDOM-prod.classic.js +0.08% 427.76 kB 428.10 kB +0.06% 78.53 kB 78.58 kB
facebook-www/ReactDOM-prod.modern.js +0.08% 417.45 kB 417.79 kB +0.06% 77.06 kB 77.11 kB
facebook-www/ReactDOMForked-prod.classic.js +0.08% 427.76 kB 428.10 kB +0.06% 78.53 kB 78.58 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against d465a97

@gaearon
Copy link
Collaborator Author

gaearon commented Jan 11, 2022

There is a different fix in #21414. I'm not sure what makes more sense.

@okmttdhr
Copy link
Contributor

I added a comment to make it clear for me. #21414 (comment)

Copy link
Collaborator

@acdlite acdlite left a comment

Choose a reason for hiding this comment

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

Mostly nits except for the comment about checking the alternate. I'm curious if one of your test cases broke without that check or if you added it out of caution. If it does break without it, that indicates something else is amiss.

packages/react-reconciler/src/ReactFiberNewContext.new.js Outdated Show resolved Hide resolved
packages/react-reconciler/src/ReactFiberNewContext.new.js Outdated Show resolved Hide resolved
packages/react-reconciler/src/ReactFiberNewContext.new.js Outdated Show resolved Hide resolved
@gaearon
Copy link
Collaborator Author

gaearon commented Jan 18, 2022

I'm curious if one of your test cases broke without that check or if you added it out of caution.

No, just in case. I've amended.

This test will fail if we remove propagation, or if we propagate with a root node like fiber.return or fiber.return.return. The additional DEV-only error helps detect a different kind of mistake, like if the thing being passed hasn't actually been encountered on the way up. However, we still leave the actual production loop to check against null so that there is no way we loop forever if the propagation root is wrong.
Copy link
Collaborator

@acdlite acdlite left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for the thorough test cases!

packages/react-reconciler/src/ReactFiberNewContext.new.js Outdated Show resolved Hide resolved
@gaearon gaearon merged commit d8cfeaf into facebook:main Jan 19, 2022
@gaearon gaearon deleted the suspense-bug branch January 19, 2022 16:41
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Feb 9, 2022
Summary:
This sync includes the following changes:
- **[a3bde7974](facebook/react@a3bde7974 )**: Exclude react-dom/unstable_testing entry point from stable releases ([#23258](facebook/react#23258)) //<Sebastian Markbåge>//
- **[569093276](facebook/react@569093276 )**: Add onErrorShell Callback ([#23247](facebook/react#23247)) //<Sebastian Markbåge>//
- **[0dedfcc68](facebook/react@0dedfcc68 )**: Update the exports field ([#23257](facebook/react#23257)) //<Sebastian Markbåge>//
- **[9d4e8e84f](facebook/react@9d4e8e84f )**: React Native raw event EventEmitter - intended for app-specific perf listeners and debugging ([#23232](facebook/react#23232)) //<Joshua Gross>//
- **[1dece5235](facebook/react@1dece5235 )**: Add back warning with component stack on Hydration mismatch ([#23241](facebook/react#23241)) //<salazarm>//
- **[cd4eb116c](facebook/react@cd4eb116c )**: Revert "update node.js version for CI ([#23236](facebook/react#23236))" ([#23239](facebook/react#23239)) //<dan>//
- **[1d7728bf9](facebook/react@1d7728bf9 )**: update node.js version for CI ([#23236](facebook/react#23236)) //<sunderls>//
- **[848e802d2](facebook/react@848e802d2 )**: Add onRecoverableError option to hydrateRoot, createRoot ([#23207](facebook/react#23207)) //<Andrew Clark>//
- **[5318971f5](facebook/react@5318971f5 )**: Remove logic for multiple error recovery attempts ([#23227](facebook/react#23227)) //<Andrew Clark>//
- **[3a4462129](facebook/react@3a4462129 )**: Disable avoidThisFallback support in Fizz  ([#23224](facebook/react#23224)) //<salazarm>//
- **[0318ac2c4](facebook/react@0318ac2c4 )**: Revert 4f5449 //<Ricky>//
- **[4f5449eb4](facebook/react@4f5449eb4 )**: Remove main from scheduler `index.js` //<Ricky>//
- **[3f5ff16c1](facebook/react@3f5ff16c1 )**: [Hydration] Fallback to client render if server rendered extra nodes ([#23176](facebook/react#23176)) //<salazarm>//
- **[529dc3ce8](facebook/react@529dc3ce8 )**: Fix context providers in SSR when handling multiple requests ([#23171](facebook/react#23171)) //<Fran Dios>//
- **[505c15c9e](facebook/react@505c15c9e )**: Don't inject timeline hooks unless React supports profiling ([#23151](facebook/react#23151)) //<Brian Vaughn>//
- **[e12a9dfc9](facebook/react@e12a9dfc9 )**: Fix production-only updateSyncExternalStore() crash when doing setState in render ([#23150](facebook/react#23150)) //<Douglas Armstrong>//
- **[e48940255](facebook/react@e48940255 )**: Warn when a callback ref returns a function ([#23145](facebook/react#23145)) //<Dan Abramov>//
- **[d8cfeaf22](facebook/react@d8cfeaf22 )**: Fix context propagation for offscreen/fallback trees ([#23095](facebook/react#23095)) //<Dan Abramov>//
- **[d50482478](facebook/react@d50482478 )**: Enable scheduling profiler flag in react-dom/testing builds ([#23142](facebook/react#23142)) //<Brian Vaughn>//
- **[790b5246f](facebook/react@790b5246f )**: Fix setState ignored in Safari when iframe is added to DOM in the same commit ([#23111](facebook/react#23111)) //<Dan Abramov>//

Changelog:
[General][Changed] - React Native sync for revisions 51947a1...a3bde79

jest_e2e[run_all_tests]

Reviewed By: ShikaSD

Differential Revision: D34108924

fbshipit-source-id: 96acb66c1a7da79d6ef9403490cd0e29ad23d9fb
@gaearon gaearon mentioned this pull request Mar 29, 2022
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
* Failing test for Context.Consumer in suspended Suspense

See issue facebook#19701.

* Fix context propagation for offscreen trees

* Address nits

* Specify propagation root for Suspense too

* Pass correct propagation root

* Harden test coverage

This test will fail if we remove propagation, or if we propagate with a root node like fiber.return or fiber.return.return. The additional DEV-only error helps detect a different kind of mistake, like if the thing being passed hasn't actually been encountered on the way up. However, we still leave the actual production loop to check against null so that there is no way we loop forever if the propagation root is wrong.

* Remove superfluous warning

Co-authored-by: overlookmotel <theoverlookmotel@gmail.com>
nevilm-lt pushed a commit to nevilm-lt/react that referenced this pull request Apr 22, 2022
* Failing test for Context.Consumer in suspended Suspense

See issue facebook#19701.

* Fix context propagation for offscreen trees

* Address nits

* Specify propagation root for Suspense too

* Pass correct propagation root

* Harden test coverage

This test will fail if we remove propagation, or if we propagate with a root node like fiber.return or fiber.return.return. The additional DEV-only error helps detect a different kind of mistake, like if the thing being passed hasn't actually been encountered on the way up. However, we still leave the actual production loop to check against null so that there is no way we loop forever if the propagation root is wrong.

* Remove superfluous warning

Co-authored-by: overlookmotel <theoverlookmotel@gmail.com>
shwanton pushed a commit to shwanton/react-native-macos that referenced this pull request Feb 13, 2023
Summary:
This sync includes the following changes:
- **[a3bde7974](facebook/react@a3bde7974 )**: Exclude react-dom/unstable_testing entry point from stable releases ([facebook#23258](facebook/react#23258)) //<Sebastian Markbåge>//
- **[569093276](facebook/react@569093276 )**: Add onErrorShell Callback ([facebook#23247](facebook/react#23247)) //<Sebastian Markbåge>//
- **[0dedfcc68](facebook/react@0dedfcc68 )**: Update the exports field ([facebook#23257](facebook/react#23257)) //<Sebastian Markbåge>//
- **[9d4e8e84f](facebook/react@9d4e8e84f )**: React Native raw event EventEmitter - intended for app-specific perf listeners and debugging ([facebook#23232](facebook/react#23232)) //<Joshua Gross>//
- **[1dece5235](facebook/react@1dece5235 )**: Add back warning with component stack on Hydration mismatch ([facebook#23241](facebook/react#23241)) //<salazarm>//
- **[cd4eb116c](facebook/react@cd4eb116c )**: Revert "update node.js version for CI ([facebook#23236](facebook/react#23236))" ([facebook#23239](facebook/react#23239)) //<dan>//
- **[1d7728bf9](facebook/react@1d7728bf9 )**: update node.js version for CI ([facebook#23236](facebook/react#23236)) //<sunderls>//
- **[848e802d2](facebook/react@848e802d2 )**: Add onRecoverableError option to hydrateRoot, createRoot ([facebook#23207](facebook/react#23207)) //<Andrew Clark>//
- **[5318971f5](facebook/react@5318971f5 )**: Remove logic for multiple error recovery attempts ([facebook#23227](facebook/react#23227)) //<Andrew Clark>//
- **[3a4462129](facebook/react@3a4462129 )**: Disable avoidThisFallback support in Fizz  ([facebook#23224](facebook/react#23224)) //<salazarm>//
- **[0318ac2c4](facebook/react@0318ac2c4 )**: Revert 4f5449 //<Ricky>//
- **[4f5449eb4](facebook/react@4f5449eb4 )**: Remove main from scheduler `index.js` //<Ricky>//
- **[3f5ff16c1](facebook/react@3f5ff16c1 )**: [Hydration] Fallback to client render if server rendered extra nodes ([facebook#23176](facebook/react#23176)) //<salazarm>//
- **[529dc3ce8](facebook/react@529dc3ce8 )**: Fix context providers in SSR when handling multiple requests ([facebook#23171](facebook/react#23171)) //<Fran Dios>//
- **[505c15c9e](facebook/react@505c15c9e )**: Don't inject timeline hooks unless React supports profiling ([facebook#23151](facebook/react#23151)) //<Brian Vaughn>//
- **[e12a9dfc9](facebook/react@e12a9dfc9 )**: Fix production-only updateSyncExternalStore() crash when doing setState in render ([facebook#23150](facebook/react#23150)) //<Douglas Armstrong>//
- **[e48940255](facebook/react@e48940255 )**: Warn when a callback ref returns a function ([facebook#23145](facebook/react#23145)) //<Dan Abramov>//
- **[d8cfeaf22](facebook/react@d8cfeaf22 )**: Fix context propagation for offscreen/fallback trees ([facebook#23095](facebook/react#23095)) //<Dan Abramov>//
- **[d50482478](facebook/react@d50482478 )**: Enable scheduling profiler flag in react-dom/testing builds ([facebook#23142](facebook/react#23142)) //<Brian Vaughn>//
- **[790b5246f](facebook/react@790b5246f )**: Fix setState ignored in Safari when iframe is added to DOM in the same commit ([facebook#23111](facebook/react#23111)) //<Dan Abramov>//

Changelog:
[General][Changed] - React Native sync for revisions 51947a1...a3bde79

jest_e2e[run_all_tests]

Reviewed By: ShikaSD

Differential Revision: D34108924

fbshipit-source-id: 96acb66c1a7da79d6ef9403490cd0e29ad23d9fb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Context.Consumer inside Suspense does not receive context updates while suspended
6 participants