From 15df051c94533c70b065d51c41cf9b6b2f1dc6a6 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 11 Nov 2020 09:06:37 -0600 Subject: [PATCH] Add warning if return pointer is inconsistent (#20219) Bugs caused by inconsistent return pointers are tricky to diagnose because the source of the error is often in a different part of the codebase from the actual mistake. For example, you might forget to set a return pointer during the render phase, which later causes a crash in the commit phase. This adds a dev-only invariant to the commit phase to check for inconsistencies. With this in place, we'll hopefully catch return pointer errors quickly during local development, when we have the most context for what might have caused it. --- .../__tests__/ReactWrongReturnPointer-test.js | 61 +++++++++++++++++++ .../src/createReactNoop.js | 5 ++ .../src/ReactFiberCommitWork.new.js | 27 ++++++++ 3 files changed, 93 insertions(+) create mode 100644 packages/react-dom/src/__tests__/ReactWrongReturnPointer-test.js diff --git a/packages/react-dom/src/__tests__/ReactWrongReturnPointer-test.js b/packages/react-dom/src/__tests__/ReactWrongReturnPointer-test.js new file mode 100644 index 0000000000000..688fb5c926ed5 --- /dev/null +++ b/packages/react-dom/src/__tests__/ReactWrongReturnPointer-test.js @@ -0,0 +1,61 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @emails react-core + */ + +let React; +let ReactNoop; + +beforeEach(() => { + React = require('react'); + ReactNoop = require('react-noop-renderer'); +}); + +// Don't feel too guilty if you have to delete this test. +// @gate new +// @gate __DEV__ +test('warns in DEV if return pointer is inconsistent', async () => { + const {useRef, useLayoutEffect} = React; + + let ref = null; + function App({text}) { + ref = useRef(null); + return ( + <> + +
{text}
+ + ); + } + + function Sibling({text}) { + useLayoutEffect(() => { + if (text === 'B') { + // Mutate the return pointer of the div to point to the wrong alternate. + // This simulates the most common type of return pointer inconsistency. + const current = ref.current.fiber; + const workInProgress = current.alternate; + workInProgress.return = current.return; + } + }, [text]); + return null; + } + + const root = ReactNoop.createRoot(); + await ReactNoop.act(async () => { + root.render(); + }); + + spyOnDev(console, 'error'); + await ReactNoop.act(async () => { + root.render(); + }); + expect(console.error.calls.count()).toBe(1); + expect(console.error.calls.argsFor(0)[0]).toMatch( + 'Internal React error: Return pointer is inconsistent with parent.', + ); +}); diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index 386d040e90757..ec2a6d714b4f9 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -275,6 +275,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { props: Props, rootContainerInstance: Container, hostContext: HostContext, + internalInstanceHandle: Object, ): Instance { if (type === 'errorInCompletePhase') { throw new Error('Error in host config.'); @@ -300,6 +301,10 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { value: inst.context, enumerable: false, }); + Object.defineProperty(inst, 'fiber', { + value: internalInstanceHandle, + enumerable: false, + }); return inst; }, diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 55c7c986effc7..e45db46500faf 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -463,6 +463,7 @@ function iterativelyCommitBeforeMutationEffects_begin() { (fiber.subtreeFlags & BeforeMutationMask) !== NoFlags && child !== null ) { + warnIfWrongReturnPointer(fiber, child); nextEffect = child; } else { iterativelyCommitBeforeMutationEffects_complete(); @@ -496,6 +497,7 @@ function iterativelyCommitBeforeMutationEffects_complete() { const sibling = fiber.sibling; if (sibling !== null) { + warnIfWrongReturnPointer(fiber.return, sibling); nextEffect = sibling; return; } @@ -713,6 +715,7 @@ function iterativelyCommitMutationEffects_begin( const child = fiber.child; if ((fiber.subtreeFlags & MutationMask) !== NoFlags && child !== null) { + warnIfWrongReturnPointer(fiber, child); nextEffect = child; } else { iterativelyCommitMutationEffects_complete(root, renderPriorityLevel); @@ -751,6 +754,7 @@ function iterativelyCommitMutationEffects_complete( const sibling = fiber.sibling; if (sibling !== null) { + warnIfWrongReturnPointer(fiber.return, sibling); nextEffect = sibling; return; } @@ -1172,12 +1176,14 @@ function iterativelyCommitLayoutEffects_begin( } const sibling = finishedWork.sibling; if (sibling !== null) { + warnIfWrongReturnPointer(finishedWork.return, sibling); nextEffect = sibling; } else { nextEffect = finishedWork.return; iterativelyCommitLayoutEffects_complete(subtreeRoot, finishedRoot); } } else { + warnIfWrongReturnPointer(finishedWork, firstChild); nextEffect = firstChild; } } else { @@ -1224,6 +1230,7 @@ function iterativelyCommitLayoutEffects_complete( const sibling = fiber.sibling; if (sibling !== null) { + warnIfWrongReturnPointer(fiber.return, sibling); nextEffect = sibling; return; } @@ -1757,12 +1764,14 @@ function iterativelyCommitPassiveMountEffects_begin( } const sibling = fiber.sibling; if (sibling !== null) { + warnIfWrongReturnPointer(fiber.return, sibling); nextEffect = sibling; } else { nextEffect = fiber.return; iterativelyCommitPassiveMountEffects_complete(subtreeRoot, root); } } else { + warnIfWrongReturnPointer(fiber, firstChild); nextEffect = firstChild; } } else { @@ -1808,6 +1817,7 @@ function iterativelyCommitPassiveMountEffects_complete( const sibling = fiber.sibling; if (sibling !== null) { + warnIfWrongReturnPointer(fiber.return, sibling); nextEffect = sibling; return; } @@ -1886,6 +1896,7 @@ function iterativelyCommitPassiveUnmountEffects_begin() { } if ((fiber.subtreeFlags & PassiveMask) !== NoFlags && child !== null) { + warnIfWrongReturnPointer(fiber, child); nextEffect = child; } else { iterativelyCommitPassiveUnmountEffects_complete(); @@ -1904,6 +1915,7 @@ function iterativelyCommitPassiveUnmountEffects_complete() { const sibling = fiber.sibling; if (sibling !== null) { + warnIfWrongReturnPointer(fiber.return, sibling); nextEffect = sibling; return; } @@ -1941,6 +1953,7 @@ function iterativelyCommitPassiveUnmountEffectsInsideOfDeletedTree_begin( const fiber = nextEffect; const child = fiber.child; if ((fiber.subtreeFlags & PassiveStatic) !== NoFlags && child !== null) { + warnIfWrongReturnPointer(fiber, child); nextEffect = child; } else { iterativelyCommitPassiveUnmountEffectsInsideOfDeletedTree_complete( @@ -1968,6 +1981,7 @@ function iterativelyCommitPassiveUnmountEffectsInsideOfDeletedTree_complete( const sibling = fiber.sibling; if (sibling !== null) { + warnIfWrongReturnPointer(fiber.return, sibling); nextEffect = sibling; return; } @@ -3178,3 +3192,16 @@ function invokeEffectsInDev( } } } + +let didWarnWrongReturnPointer = false; +function warnIfWrongReturnPointer(returnFiber, child) { + if (__DEV__) { + if (!didWarnWrongReturnPointer && child.return !== returnFiber) { + didWarnWrongReturnPointer = true; + console.error( + 'Internal React error: Return pointer is inconsistent ' + + 'with parent.', + ); + } + } +}