From a129259ad6a61790fe1912a87cb45018d942f90c Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 23 Jan 2019 15:51:57 +0000 Subject: [PATCH] Disallow reading context during useMemo etc (#14653) * Revert "Revert "Double-render function components with Hooks in DEV in StrictMode" (#14652)" This reverts commit 3fbebb2a0b3b806f428c4154700af8e75e561895. * Revert "Revert "Disallow reading context during useMemo etc" (#14651)" This reverts commit 5fce6488ce1dc97e31515a47ff409d32ab722d65. * Add extra passing test for an edge case Mentioned by @acdlite to watch out for * More convoluted test * Don't rely on expirationTime Addresses @acdlite's concerns * Edge case: forbid readContext() during eager reducer --- .../src/ReactFiberClassComponent.js | 9 +- .../react-reconciler/src/ReactFiberHooks.js | 20 ++- .../src/ReactFiberNewContext.js | 25 ++++ .../src/__tests__/ReactHooks-test.internal.js | 141 ++++++++++++++++++ 4 files changed, 186 insertions(+), 9 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 16a3e39fc316b..3875371714f38 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -20,7 +20,6 @@ import { import ReactStrictModeWarnings from './ReactStrictModeWarnings'; import {isMounted} from 'react-reconciler/reflection'; import {get as getInstance, set as setInstance} from 'shared/ReactInstanceMap'; -import ReactSharedInternals from 'shared/ReactSharedInternals'; import shallowEqual from 'shared/shallowEqual'; import getComponentName from 'shared/getComponentName'; import invariant from 'shared/invariant'; @@ -48,6 +47,7 @@ import { hasContextChanged, emptyContextObject, } from './ReactFiberContext'; +import {readContext} from './ReactFiberNewContext'; import { requestCurrentTime, computeExpirationForFiber, @@ -55,13 +55,6 @@ import { flushPassiveEffects, } from './ReactFiberScheduler'; -const ReactCurrentDispatcher = ReactSharedInternals.ReactCurrentDispatcher; - -function readContext(contextType: any): any { - const dispatcher = ReactCurrentDispatcher.current; - return dispatcher.readContext(contextType); -} - const fakeInternalInstance = {}; const isArray = Array.isArray; diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 825ea9c08503c..66c777d828dc9 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -14,7 +14,11 @@ import type {HookEffectTag} from './ReactHookEffectTags'; import {NoWork} from './ReactFiberExpirationTime'; import {enableHooks} from 'shared/ReactFeatureFlags'; -import {readContext} from './ReactFiberNewContext'; +import { + readContext, + stashContextDependencies, + unstashContextDependencies, +} from './ReactFiberNewContext'; import { Update as UpdateEffect, Passive as PassiveEffect, @@ -600,8 +604,10 @@ export function useReducer( const action = update.action; // Temporarily clear to forbid calling Hooks in a reducer. currentlyRenderingFiber = null; + stashContextDependencies(); newState = reducer(newState, action); currentlyRenderingFiber = fiber; + unstashContextDependencies(); update = update.next; } while (update !== null); @@ -672,8 +678,10 @@ export function useReducer( const action = update.action; // Temporarily clear to forbid calling Hooks in a reducer. currentlyRenderingFiber = null; + stashContextDependencies(); newState = reducer(newState, action); currentlyRenderingFiber = fiber; + unstashContextDependencies(); } } prevUpdate = update; @@ -704,6 +712,7 @@ export function useReducer( } // Temporarily clear to forbid calling Hooks in a reducer. currentlyRenderingFiber = null; + stashContextDependencies(); // There's no existing queue, so this is the initial render. if (reducer === basicStateReducer) { // Special case for `useState`. @@ -714,6 +723,7 @@ export function useReducer( initialState = reducer(initialState, initialAction); } currentlyRenderingFiber = fiber; + unstashContextDependencies(); workInProgressHook.memoizedState = workInProgressHook.baseState = initialState; queue = workInProgressHook.queue = { last: null, @@ -947,8 +957,10 @@ export function useMemo( // Temporarily clear to forbid calling Hooks. currentlyRenderingFiber = null; + stashContextDependencies(); const nextValue = nextCreate(); currentlyRenderingFiber = fiber; + unstashContextDependencies(); workInProgressHook.memoizedState = [nextValue, nextDeps]; currentHookNameInDev = null; return nextValue; @@ -1044,7 +1056,13 @@ function dispatchAction( if (eagerReducer !== null) { try { const currentState: S = (queue.eagerState: any); + // Temporarily clear to forbid calling Hooks in a reducer. + let maybeFiber = currentlyRenderingFiber; // Note: likely null now unlike `fiber` + currentlyRenderingFiber = null; + stashContextDependencies(); const eagerState = eagerReducer(currentState, action); + currentlyRenderingFiber = maybeFiber; + unstashContextDependencies(); // Stash the eagerly computed state, and the reducer used to compute // it, on the update object. If the reducer hasn't changed by the // time we enter the render phase, then the eager state can be used diff --git a/packages/react-reconciler/src/ReactFiberNewContext.js b/packages/react-reconciler/src/ReactFiberNewContext.js index 9687f40d7f12b..cbba426499d0a 100644 --- a/packages/react-reconciler/src/ReactFiberNewContext.js +++ b/packages/react-reconciler/src/ReactFiberNewContext.js @@ -52,12 +52,37 @@ let currentlyRenderingFiber: Fiber | null = null; let lastContextDependency: ContextDependency | null = null; let lastContextWithAllBitsObserved: ReactContext | null = null; +// We stash the variables above before entering user code in Hooks. +let stashedCurrentlyRenderingFiber: Fiber | null = null; +let stashedLastContextDependency: ContextDependency | null = null; +let stashedLastContextWithAllBitsObserved: ReactContext | null = null; + export function resetContextDependences(): void { // This is called right before React yields execution, to ensure `readContext` // cannot be called outside the render phase. currentlyRenderingFiber = null; lastContextDependency = null; lastContextWithAllBitsObserved = null; + + stashedCurrentlyRenderingFiber = null; + stashedLastContextDependency = null; + stashedLastContextWithAllBitsObserved = null; +} + +export function stashContextDependencies(): void { + stashedCurrentlyRenderingFiber = currentlyRenderingFiber; + stashedLastContextDependency = lastContextDependency; + stashedLastContextWithAllBitsObserved = lastContextWithAllBitsObserved; + + currentlyRenderingFiber = null; + lastContextDependency = null; + lastContextWithAllBitsObserved = null; +} + +export function unstashContextDependencies(): void { + currentlyRenderingFiber = stashedCurrentlyRenderingFiber; + lastContextDependency = stashedLastContextDependency; + lastContextWithAllBitsObserved = stashedLastContextWithAllBitsObserved; } export function pushProvider(providerFiber: Fiber, nextValue: T): void { diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 45810597c87ab..bf4483986313e 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -672,6 +672,147 @@ describe('ReactHooks', () => { expect(root.toJSON()).toEqual('123'); }); + it('throws when reading context inside useMemo', () => { + const {useMemo, createContext} = React; + const ReactCurrentDispatcher = + React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED + .ReactCurrentDispatcher; + + const ThemeContext = createContext('light'); + function App() { + return useMemo(() => { + return ReactCurrentDispatcher.current.readContext(ThemeContext); + }, []); + } + + expect(() => ReactTestRenderer.create()).toThrow( + 'Context can only be read while React is rendering', + ); + }); + + it('throws when reading context inside useMemo after reading outside it', () => { + const {useMemo, createContext} = React; + const ReactCurrentDispatcher = + React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED + .ReactCurrentDispatcher; + + const ThemeContext = createContext('light'); + let firstRead, secondRead; + function App() { + firstRead = ReactCurrentDispatcher.current.readContext(ThemeContext); + useMemo(() => {}); + secondRead = ReactCurrentDispatcher.current.readContext(ThemeContext); + return useMemo(() => { + return ReactCurrentDispatcher.current.readContext(ThemeContext); + }, []); + } + + expect(() => ReactTestRenderer.create()).toThrow( + 'Context can only be read while React is rendering', + ); + expect(firstRead).toBe('light'); + expect(secondRead).toBe('light'); + }); + + it('throws when reading context inside useEffect', () => { + const {useEffect, createContext} = React; + const ReactCurrentDispatcher = + React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED + .ReactCurrentDispatcher; + + const ThemeContext = createContext('light'); + function App() { + useEffect(() => { + ReactCurrentDispatcher.current.readContext(ThemeContext); + }); + return null; + } + + const root = ReactTestRenderer.create(); + expect(() => root.update()).toThrow( + // The exact message doesn't matter, just make sure we don't allow this + "Cannot read property 'readContext' of null", + ); + }); + + it('throws when reading context inside useLayoutEffect', () => { + const {useLayoutEffect, createContext} = React; + const ReactCurrentDispatcher = + React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED + .ReactCurrentDispatcher; + + const ThemeContext = createContext('light'); + function App() { + useLayoutEffect(() => { + ReactCurrentDispatcher.current.readContext(ThemeContext); + }); + return null; + } + + expect(() => ReactTestRenderer.create()).toThrow( + // The exact message doesn't matter, just make sure we don't allow this + "Cannot read property 'readContext' of null", + ); + }); + + it('throws when reading context inside useReducer', () => { + const {useReducer, createContext} = React; + const ReactCurrentDispatcher = + React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED + .ReactCurrentDispatcher; + + const ThemeContext = createContext('light'); + function App() { + useReducer( + () => { + ReactCurrentDispatcher.current.readContext(ThemeContext); + }, + null, + {}, + ); + return null; + } + + expect(() => ReactTestRenderer.create()).toThrow( + 'Context can only be read while React is rendering', + ); + }); + + // Edge case. + it('throws when reading context inside eager useReducer', () => { + const {useState, createContext} = React; + const ThemeContext = createContext('light'); + + const ReactCurrentDispatcher = + React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED + .ReactCurrentDispatcher; + + let _setState; + function Fn() { + const [, setState] = useState(0); + _setState = setState; + return null; + } + + class Cls extends React.Component { + render() { + _setState(() => { + ReactCurrentDispatcher.current.readContext(ThemeContext); + }); + return null; + } + } + + expect(() => + ReactTestRenderer.create( + + + + , + ), + ).toThrow('Context can only be read while React is rendering'); + }); + it('throws when calling hooks inside useReducer', () => { const {useReducer, useRef} = React; function App() {