Skip to content

Commit

Permalink
Dont recreate maked context unless unmasked context changes
Browse files Browse the repository at this point in the history
Doing so can lead to infinite loops if componentWillReceiveProps() calls setState()
  • Loading branch information
Brian Vaughn committed Jan 7, 2017
1 parent 7f0dc41 commit a682059
Show file tree
Hide file tree
Showing 5 changed files with 88 additions and 9 deletions.
1 change: 1 addition & 0 deletions scripts/fiber/tests-passing.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1165,6 +1165,7 @@ src/renderers/shared/fiber/__tests__/ReactIncremental-test.js
* reads context when setState is above the provider
* maintains the correct context when providers bail out due to low priority
* maintains the correct context when unwinding due to an error in render
* should not recreate masked context unless inputs have changed

src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js
* catches render error in a boundary during full deferred mounting
Expand Down
7 changes: 5 additions & 2 deletions src/renderers/shared/fiber/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ var {
var ReactTypeOfWork = require('ReactTypeOfWork');
var {
getMaskedContext,
getUnmaskedContext,
hasContextChanged,
pushContextProvider,
pushTopLevelContextObject,
Expand Down Expand Up @@ -210,7 +211,8 @@ module.exports = function<T, P, I, TI, C, CX, CI>(
return bailoutOnAlreadyFinishedWork(current, workInProgress);
}

var context = getMaskedContext(workInProgress);
var unmaskedContext = getUnmaskedContext(workInProgress);
var context = getMaskedContext(workInProgress, unmaskedContext);

var nextChildren;

Expand Down Expand Up @@ -427,7 +429,8 @@ module.exports = function<T, P, I, TI, C, CX, CI>(
}
var fn = workInProgress.type;
var props = workInProgress.pendingProps;
var context = getMaskedContext(workInProgress);
var unmaskedContext = getUnmaskedContext(workInProgress);
var context = getMaskedContext(workInProgress, unmaskedContext);

var value;

Expand Down
22 changes: 17 additions & 5 deletions src/renderers/shared/fiber/ReactFiberClassComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import type { PriorityLevel } from 'ReactPriorityLevel';

var {
getMaskedContext,
getUnmaskedContext,
} = require('ReactFiberContext');
var {
addUpdate,
Expand Down Expand Up @@ -204,10 +205,17 @@ module.exports = function(
function constructClassInstance(workInProgress : Fiber) : any {
const ctor = workInProgress.type;
const props = workInProgress.pendingProps;
const context = getMaskedContext(workInProgress);
const unmaskedContext = getUnmaskedContext(workInProgress);
const context = getMaskedContext(workInProgress, unmaskedContext);
const instance = new ctor(props, context);
adoptClassInstance(workInProgress, instance);
checkClassInstance(workInProgress);

// Cache unmasked context so we can avoid recreating masked context unless necessary.
// ReactFiberContext usually updates this cache but can't for newly-created instances.
instance.__reactInternalMemoizedUnmaskedChildContext = unmaskedContext;
instance.__reactInternalMemoizedMaskedChildContext = context;

return instance;
}

Expand All @@ -221,9 +229,11 @@ module.exports = function(
throw new Error('There must be pending props for an initial mount.');
}

const unmaskedContext = getUnmaskedContext(workInProgress);

instance.props = props;
instance.state = state;
instance.context = getMaskedContext(workInProgress);
instance.context = getMaskedContext(workInProgress, unmaskedContext);

if (typeof instance.componentWillMount === 'function') {
instance.componentWillMount();
Expand Down Expand Up @@ -256,7 +266,8 @@ module.exports = function(
throw new Error('There should always be pending or memoized props.');
}
}
const newContext = getMaskedContext(workInProgress);
const newUnmaskedContext = getUnmaskedContext(workInProgress);
const newContext = getMaskedContext(workInProgress, newUnmaskedContext);

// TODO: Should we deal with a setState that happened after the last
// componentWillMount and before this componentWillMount? Probably
Expand All @@ -277,7 +288,7 @@ module.exports = function(
const newInstance = constructClassInstance(workInProgress);
newInstance.props = newProps;
newInstance.state = newState = newInstance.state || null;
newInstance.context = getMaskedContext(workInProgress);
newInstance.context = newContext;

if (typeof newInstance.componentWillMount === 'function') {
newInstance.componentWillMount();
Expand Down Expand Up @@ -314,7 +325,8 @@ module.exports = function(
}
}
const oldContext = instance.context;
const newContext = getMaskedContext(workInProgress);
const newUnmaskedContext = getUnmaskedContext(workInProgress);
const newContext = getMaskedContext(workInProgress, newUnmaskedContext);

// Note: During these life-cycles, instance.props/instance.state are what
// ever the previously attempted to render - not the "current". However,
Expand Down
21 changes: 19 additions & 2 deletions src/renderers/shared/fiber/ReactFiberContext.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,26 @@ function getUnmaskedContext(workInProgress : Fiber) : Object {
}
return contextStackCursor.current;
}
exports.getUnmaskedContext = getUnmaskedContext;

exports.getMaskedContext = function(workInProgress : Fiber) {
exports.getMaskedContext = function(workInProgress : Fiber, unmaskedContext : Object) {
const type = workInProgress.type;
const contextTypes = type.contextTypes;
if (!contextTypes) {
return emptyObject;
}

const unmaskedContext = getUnmaskedContext(workInProgress);
// Avoid recreating masked context unless unmasked context has changed.
// Failing to do this will result in unnecessary calls to componentWillReceiveProps.
// This may trigger infinite loops if componentWillReceiveProps calls setState.
const instance = workInProgress.stateNode;
if (
instance &&
instance.__reactInternalMemoizedUnmaskedChildContext === unmaskedContext
) {
return instance.__reactInternalMemoizedMaskedChildContext;
}

const context = {};
for (let key in contextTypes) {
context[key] = unmaskedContext[key];
Expand All @@ -74,6 +85,12 @@ exports.getMaskedContext = function(workInProgress : Fiber) {
checkReactTypeSpec(contextTypes, context, 'context', name, null, workInProgress);
}

// Cache unmasked context so we can avoid recreating masked context unless necessary.
if (instance) {
instance.__reactInternalMemoizedUnmaskedChildContext = unmaskedContext;
instance.__reactInternalMemoizedMaskedChildContext = context;
}

return context;
};

Expand Down
46 changes: 46 additions & 0 deletions src/renderers/shared/fiber/__tests__/ReactIncremental-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2021,4 +2021,50 @@ describe('ReactIncremental', () => {
});
ReactNoop.flush();
});

it('should not recreate masked context unless inputs have changed', () => {
const ops = [];

let scuCounter = 0;

class MyComponent extends React.Component {
static contextTypes = {};
componentDidMount(prevProps, prevState) {
ops.push('componentDidMount');
this.setState({ setStateInCDU: true });
}
componentDidUpdate(prevProps, prevState) {
ops.push('componentDidUpdate');
if (this.state.setStateInCDU) {
this.setState({ setStateInCDU: false });
}
}
componentWillReceiveProps(nextProps) {
ops.push('componentWillReceiveProps');
this.setState({ setStateInCDU: true });
}
render() {
ops.push('render');
return null;
}
shouldComponentUpdate(nextProps, nextState) {
ops.push('shouldComponentUpdate');
return scuCounter++ < 5; // Don't let test hang
}
}

ReactNoop.render(<MyComponent />);
ReactNoop.flush();

expect(ops).toEqual([
'render',
'componentDidMount',
'shouldComponentUpdate',
'render',
'componentDidUpdate',
'shouldComponentUpdate',
'render',
'componentDidUpdate',
]);
});
});

0 comments on commit a682059

Please sign in to comment.