Skip to content

Commit

Permalink
Disallow reading context during useMemo etc (#14653)
Browse files Browse the repository at this point in the history
* Revert "Revert "Double-render function components with Hooks in DEV in StrictMode" (#14652)"

This reverts commit 3fbebb2.

* Revert "Revert "Disallow reading context during useMemo etc" (#14651)"

This reverts commit 5fce648.

* 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
  • Loading branch information
gaearon authored Jan 23, 2019
1 parent c068d31 commit a129259
Show file tree
Hide file tree
Showing 4 changed files with 186 additions and 9 deletions.
9 changes: 1 addition & 8 deletions packages/react-reconciler/src/ReactFiberClassComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -48,20 +47,14 @@ import {
hasContextChanged,
emptyContextObject,
} from './ReactFiberContext';
import {readContext} from './ReactFiberNewContext';
import {
requestCurrentTime,
computeExpirationForFiber,
scheduleWork,
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;

Expand Down
20 changes: 19 additions & 1 deletion packages/react-reconciler/src/ReactFiberHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -600,8 +604,10 @@ export function useReducer<S, A>(
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);

Expand Down Expand Up @@ -672,8 +678,10 @@ export function useReducer<S, A>(
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;
Expand Down Expand Up @@ -704,6 +712,7 @@ export function useReducer<S, A>(
}
// 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`.
Expand All @@ -714,6 +723,7 @@ export function useReducer<S, A>(
initialState = reducer(initialState, initialAction);
}
currentlyRenderingFiber = fiber;
unstashContextDependencies();
workInProgressHook.memoizedState = workInProgressHook.baseState = initialState;
queue = workInProgressHook.queue = {
last: null,
Expand Down Expand Up @@ -947,8 +957,10 @@ export function useMemo<T>(

// Temporarily clear to forbid calling Hooks.
currentlyRenderingFiber = null;
stashContextDependencies();
const nextValue = nextCreate();
currentlyRenderingFiber = fiber;
unstashContextDependencies();
workInProgressHook.memoizedState = [nextValue, nextDeps];
currentHookNameInDev = null;
return nextValue;
Expand Down Expand Up @@ -1044,7 +1056,13 @@ function dispatchAction<S, A>(
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
Expand Down
25 changes: 25 additions & 0 deletions packages/react-reconciler/src/ReactFiberNewContext.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,37 @@ let currentlyRenderingFiber: Fiber | null = null;
let lastContextDependency: ContextDependency<mixed> | null = null;
let lastContextWithAllBitsObserved: ReactContext<any> | null = null;

// We stash the variables above before entering user code in Hooks.
let stashedCurrentlyRenderingFiber: Fiber | null = null;
let stashedLastContextDependency: ContextDependency<mixed> | null = null;
let stashedLastContextWithAllBitsObserved: ReactContext<any> | 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<T>(providerFiber: Fiber, nextValue: T): void {
Expand Down
141 changes: 141 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(<App />)).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(<App />)).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(<App />);
expect(() => root.update(<App />)).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(<App />)).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(<App />)).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(
<React.Fragment>
<Fn />
<Cls />
</React.Fragment>,
),
).toThrow('Context can only be read while React is rendering');
});

it('throws when calling hooks inside useReducer', () => {
const {useReducer, useRef} = React;
function App() {
Expand Down

0 comments on commit a129259

Please sign in to comment.