Don't allow recursive sync updates during reconciliation (begin phase)

acdlite · acdlite · commit 197b4617b44d · 2016-12-28T15:43:05.000-08:00
We can't allow this because it may lead to infinite loops. Recursion
is only safe during the commit phase.

Prints a warning and downgrades to Task priority.
diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt
@@ -56,8 +56,6 @@ src/renderers/shared/__tests__/ReactPerf-test.js
 * should include render time of functional components
 * should not count time in a portal towards lifecycle method
 * should work when measurement starts during reconciliation
-* should not print errant warnings if portal throws in render()
-* should not print errant warnings if portal throws in componentWillMount()
 
 src/renderers/shared/hooks/__tests__/ReactComponentTreeHook-test.js
 * can be retrieved by ID
@@ -72,7 +70,6 @@ src/renderers/shared/shared/__tests__/ReactComponentLifeCycle-test.js
 * should carry through each of the phases of setup
 
 src/renderers/shared/shared/__tests__/ReactCompositeComponent-test.js
-* should disallow nested render calls
 * should update refs if shouldComponentUpdate gives false
 
 src/renderers/shared/shared/__tests__/ReactEmptyComponent-test.js
@@ -95,5 +92,3 @@ src/renderers/shared/shared/__tests__/ReactUpdates-test.js
 
 src/renderers/shared/shared/__tests__/refs-test.js
 * Should increase refs with an increase in divs
-* attaches, detaches from fiber component with stack layer
-* attaches, detaches from stack component with fiber layer
diff --git a/scripts/fiber/tests-passing-except-dev.txt b/scripts/fiber/tests-passing-except-dev.txt
@@ -112,6 +112,7 @@ src/renderers/shared/shared/__tests__/ReactCompositeComponent-test.js
 * should warn about `setState` on unmounted components
 * should warn about `setState` in render
 * should warn about `setState` in getChildContext
+* should disallow nested render calls
 
 src/renderers/shared/shared/__tests__/ReactMultiChild-test.js
 * should warn for using maps as children with owner info
diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt
@@ -1117,6 +1117,8 @@ src/renderers/shared/__tests__/ReactPerf-test.js
 * should not print errant warnings if render() throws
 * should not print errant warnings if componentWillMount() throws
 * should not print errant warnings if componentDidMount() throws
+* should not print errant warnings if portal throws in render()
+* should not print errant warnings if portal throws in componentWillMount()
 * should not print errant warnings if portal throws in componentDidMount()
 
 src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js
@@ -1581,6 +1583,8 @@ src/renderers/shared/shared/__tests__/refs-test.js
 * ref called correctly for stateless component when __DEV__ = false
 * ref called correctly for stateless component when __DEV__ = true
 * coerces numbers to strings
+* attaches, detaches from fiber component with stack layer
+* attaches, detaches from stack component with fiber layer
 
 src/renderers/shared/shared/event/__tests__/EventPluginHub-test.js
 * should prevent non-function listeners, at dispatch
diff --git a/src/renderers/dom/fiber/ReactDOMFiber.js b/src/renderers/dom/fiber/ReactDOMFiber.js
@@ -67,9 +67,10 @@ type HostContextDev = {
 };
 type HostContextProd = string;
 type HostContext = HostContextDev | HostContextProd;
-
-let eventsEnabled : ?boolean = null;
-let selectionInformation : ?mixed = null;
+type CommitInfo = {
+  eventsEnabled: boolean,
+  selectionInformation: mixed,
+};
 
 var ELEMENT_NODE_TYPE = 1;
 var DOC_NODE_TYPE = 9;
@@ -123,17 +124,18 @@ var DOMRenderer = ReactFiberReconciler({
     return getChildNamespace(parentNamespace, type);
   },
 
-  prepareForCommit() : void {
-    eventsEnabled = ReactBrowserEventEmitter.isEnabled();
+  prepareForCommit() : CommitInfo {
+    const eventsEnabled = ReactBrowserEventEmitter.isEnabled();
     ReactBrowserEventEmitter.setEnabled(false);
-    selectionInformation = ReactInputSelection.getSelectionInformation();
+    return {
+      eventsEnabled,
+      selectionInformation: ReactInputSelection.getSelectionInformation(),
+    };
   },
 
-  resetAfterCommit() : void {
-    ReactInputSelection.restoreSelection(selectionInformation);
-    selectionInformation = null;
-    ReactBrowserEventEmitter.setEnabled(eventsEnabled);
-    eventsEnabled = null;
+  resetAfterCommit(commitInfo : CommitInfo) : void {
+    ReactInputSelection.restoreSelection(commitInfo.selectionInformation);
+    ReactBrowserEventEmitter.setEnabled(commitInfo.eventsEnabled);
   },
 
   createInstance(
diff --git a/src/renderers/shared/__tests__/ReactPerf-test.js b/src/renderers/shared/__tests__/ReactPerf-test.js
@@ -614,7 +614,7 @@ describe('ReactPerf', () => {
       }
     }
     class EvilPortal extends React.Component {
-      componentWillMount() {
+      componentDidMount() {
         var portalContainer = document.createElement('div');
         ReactDOM.render(<Evil />, portalContainer);
       }
@@ -644,6 +644,7 @@ describe('ReactPerf', () => {
   it('should not print errant warnings if portal throws in componentWillMount()', () => {
     var container = document.createElement('div');
     var thrownErr = new Error('Muhaha!');
+    spyOn(console, 'error');
 
     class Evil extends React.Component {
       componentWillMount() {
@@ -679,6 +680,13 @@ describe('ReactPerf', () => {
     }
     ReactDOM.unmountComponentAtNode(container);
     ReactPerf.stop();
+
+    // A sync `render` inside cWM will print a warning. That should be the
+    // only warning.
+    expect(console.error.calls.count()).toEqual(1);
+    expect(console.error.calls.argsFor(0)[0]).toMatch(
+      /Render methods should be a pure function of props and state/
+    );
   });
 
   it('should not print errant warnings if portal throws in componentDidMount()', () => {
@@ -694,7 +702,7 @@ describe('ReactPerf', () => {
       }
     }
     class EvilPortal extends React.Component {
-      componentWillMount() {
+      componentDidMount() {
         var portalContainer = document.createElement('div');
         ReactDOM.render(<Evil />, portalContainer);
       }
diff --git a/src/renderers/shared/fiber/ReactFiberBeginWork.js b/src/renderers/shared/fiber/ReactFiberBeginWork.js
@@ -66,8 +66,8 @@ if (__DEV__) {
   var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber');
 }
 
-module.exports = function<T, P, I, TI, C, CX>(
-  config : HostConfig<T, P, I, TI, C, CX>,
+module.exports = function<T, P, I, TI, C, CX, CI>(
+  config : HostConfig<T, P, I, TI, C, CX, CI>,
   hostContext : HostContext<C, CX>,
   scheduleUpdate : (fiber : Fiber, priorityLevel : PriorityLevel) => void,
   getPriorityContext : () => PriorityLevel,
diff --git a/src/renderers/shared/fiber/ReactFiberCommitWork.js b/src/renderers/shared/fiber/ReactFiberCommitWork.js
@@ -33,8 +33,8 @@ var {
   ContentReset,
 } = require('ReactTypeOfSideEffect');
 
-module.exports = function<T, P, I, TI, C, CX>(
-  config : HostConfig<T, P, I, TI, C, CX>,
+module.exports = function<T, P, I, TI, C, CX, CI>(
+  config : HostConfig<T, P, I, TI, C, CX, CI>,
   hostContext : HostContext<C, CX>,
   captureError : (failedFiber : Fiber, error: Error) => ?Fiber
 ) {
diff --git a/src/renderers/shared/fiber/ReactFiberCompleteWork.js b/src/renderers/shared/fiber/ReactFiberCompleteWork.js
@@ -46,8 +46,8 @@ if (__DEV__) {
   var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber');
 }
 
-module.exports = function<T, P, I, TI, C, CX>(
-  config : HostConfig<T, P, I, TI, C, CX>,
+module.exports = function<T, P, I, TI, C, CX, CI>(
+  config : HostConfig<T, P, I, TI, C, CX, CI>,
   hostContext : HostContext<C, CX>,
 ) {
   const {
diff --git a/src/renderers/shared/fiber/ReactFiberHostContext.js b/src/renderers/shared/fiber/ReactFiberHostContext.js
@@ -34,8 +34,8 @@ export type HostContext<C, CX> = {
   resetHostContainer() : void,
 };
 
-module.exports = function<T, P, I, TI, C, CX>(
-  config : HostConfig<T, P, I, TI, C, CX>
+module.exports = function<T, P, I, TI, C, CX, CI>(
+  config : HostConfig<T, P, I, TI, C, CX, CI>
 ) : HostContext<C, CX> {
   const {
     getChildHostContext,
diff --git a/src/renderers/shared/fiber/ReactFiberReconciler.js b/src/renderers/shared/fiber/ReactFiberReconciler.js
@@ -43,7 +43,7 @@ export type Deadline = {
 
 type OpaqueNode = Fiber;
 
-export type HostConfig<T, P, I, TI, C, CX> = {
+export type HostConfig<T, P, I, TI, C, CX, CI> = {
 
   getRootHostContext(rootContainerInstance : C) : CX,
   getChildHostContext(parentHostContext : CX, type : T) : CX,
@@ -68,8 +68,8 @@ export type HostConfig<T, P, I, TI, C, CX> = {
   scheduleAnimationCallback(callback : () => void) : void,
   scheduleDeferredCallback(callback : (deadline : Deadline) => void) : void,
 
-  prepareForCommit() : void,
-  resetAfterCommit() : void,
+  prepareForCommit() : CI,
+  resetAfterCommit(commitInfo : CI) : void,
 
   useSyncScheduling ?: boolean,
 };
@@ -99,7 +99,7 @@ getContextForSubtree._injectFiber(function(fiber : Fiber) {
     parentContext;
 });
 
-module.exports = function<T, P, I, TI, C, CX>(config : HostConfig<T, P, I, TI, C, CX>) : Reconciler<C, I, TI> {
+module.exports = function<T, P, I, TI, C, CX, CI>(config : HostConfig<T, P, I, TI, C, CX, CI>) : Reconciler<C, I, TI> {
 
   var {
     scheduleUpdate,
diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js
@@ -66,14 +66,16 @@ var {
   resetContext,
 } = require('ReactFiberContext');
 
+var warning = require('warning');
+
 if (__DEV__) {
   var ReactFiberInstrumentation = require('ReactFiberInstrumentation');
   var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber');
 }
 
 var timeHeuristicForUnitOfWork = 1;
 
-module.exports = function<T, P, I, TI, C, CX>(config : HostConfig<T, P, I, TI, C, CX>) {
+module.exports = function<T, P, I, TI, C, CX, CI>(config : HostConfig<T, P, I, TI, C, CX, CI>) {
   const hostContext = ReactFiberHostContext(config);
   const { popHostContainer, popHostContext, resetHostContainer } = hostContext;
   const { beginWork, beginFailedWork } = ReactFiberBeginWork(
@@ -347,7 +349,7 @@ module.exports = function<T, P, I, TI, C, CX>(config : HostConfig<T, P, I, TI, C
       firstEffect = finishedWork.firstEffect;
     }
 
-    prepareForCommit();
+    const commitInfo = prepareForCommit();
 
     // Commit all the side-effects within a tree. We'll do this in two passes.
     // The first pass performs all the host insertions, updates, deletions and
@@ -368,7 +370,7 @@ module.exports = function<T, P, I, TI, C, CX>(config : HostConfig<T, P, I, TI, C
       }
     }
 
-    resetAfterCommit();
+    resetAfterCommit(commitInfo);
 
     // In the second pass we'll perform all life-cycles and ref callbacks.
     // Life-cycles happen as a separate pass so that all placements, updates,
@@ -676,23 +678,25 @@ module.exports = function<T, P, I, TI, C, CX>(config : HostConfig<T, P, I, TI, C
     // If performWork is called recursively, we need to save the previous state
     // of the scheduler so it can be restored before the function exits.
     // Recursion is only possible when using syncUpdates.
-    const previousIsPerformingWork = isPerformingWork;
+    const previousPriorityContext = priorityContext;
     const previousPriorityContextBeforeReconciliation = priorityContextBeforeReconciliation;
+    const previousIsPerformingWork = isPerformingWork;
     const previousShouldDeferSyncUpdates = shouldDeferSyncUpdates;
     const previousNextEffect = nextEffect;
-    const previousCapturedErrors = capturedErrors;
-    const previousFailedBoundaries = failedBoundaries;
     const previousCommitPhaseBoundaries = commitPhaseBoundaries;
+    const previousFirstUncaughtError = firstUncaughtError;
+    const previousFatalError = fatalError;
     const previousIsCommitting = isCommitting;
     const previousIsUnmounting = isUnmounting;
 
-    isPerformingWork = true;
+    priorityContext = NoWork;
     priorityContextBeforeReconciliation = NoWork;
+    isPerformingWork = true;
     shouldDeferSyncUpdates = true;
     nextEffect = null;
-    capturedErrors = null;
-    failedBoundaries = null;
     commitPhaseBoundaries = null;
+    firstUncaughtError = null;
+    fatalError = null;
     isCommitting = false;
     isUnmounting = false;
 
@@ -801,30 +805,23 @@ module.exports = function<T, P, I, TI, C, CX>(config : HostConfig<T, P, I, TI, C
       }
     }
 
+    const errorToThrow = fatalError || firstUncaughtError;
+
     // We're done performing work. Restore the previous state of the scheduler.
-    isPerformingWork = previousIsPerformingWork;
+    priorityContext = previousPriorityContext;
     priorityContextBeforeReconciliation = previousPriorityContextBeforeReconciliation;
+    isPerformingWork = previousIsPerformingWork;
     shouldDeferSyncUpdates = previousShouldDeferSyncUpdates;
     nextEffect = previousNextEffect;
-    capturedErrors = previousCapturedErrors;
-    failedBoundaries = previousFailedBoundaries;
     commitPhaseBoundaries = previousCommitPhaseBoundaries;
+    firstUncaughtError = previousFirstUncaughtError;
+    fatalError = previousFatalError;
     isCommitting = previousIsCommitting;
     isUnmounting = previousIsUnmounting;
 
-    if (!isPerformingWork) {
-      // If we're done performing work, it's safe to throw errors.
-      if (fatalError) {
-        let e = fatalError;
-        fatalError = null;
-        firstUncaughtError = null;
-        throw e;
-      }
-      if (firstUncaughtError) {
-        let e = firstUncaughtError;
-        firstUncaughtError = null;
-        throw e;
-      }
+    // It's safe to throw any unhandled errors.
+    if (errorToThrow) {
+      throw errorToThrow;
     }
   }
 
@@ -1025,6 +1022,20 @@ module.exports = function<T, P, I, TI, C, CX>(config : HostConfig<T, P, I, TI, C
   }
 
   function scheduleUpdate(fiber : Fiber, priorityLevel : PriorityLevel) {
+    // Detect if a synchronous update is made during render (or begin phase).
+    if (priorityLevel === SynchronousPriority && isPerformingWork && !isCommitting) {
+      if (__DEV__) {
+        warning(
+          false,
+          'Render methods should be a pure function of props and state; ' +
+          'triggering nested component updates from render is not allowed. ' +
+          'If necessary, trigger nested updates in componentDidUpdate.'
+        );
+      }
+      // Downgrade to Task priority to prevent an infinite loop.
+      priorityLevel = TaskPriority;
+    }
+
     if (priorityLevel <= nextPriorityLevel) {
       // We must reset the current unit of work pointer so that we restart the
       // search from the root during the next tick, in case there is now higher
diff --git a/src/renderers/shared/hooks/__tests__/ReactComponentTreeHook-test.js b/src/renderers/shared/hooks/__tests__/ReactComponentTreeHook-test.js
@@ -1839,6 +1839,7 @@ describe('ReactComponentTreeHook', () => {
 
     it('is created during mounting', () => {
       // https://github.com/facebook/react/issues/7187
+      spyOn(console, 'error'); // Allow console warning
       var el = document.createElement('div');
       var portalEl = document.createElement('div');
       class Foo extends React.Component {
diff --git a/src/renderers/shared/shared/__tests__/ReactCompositeComponent-test.js b/src/renderers/shared/shared/__tests__/ReactCompositeComponent-test.js
@@ -1266,7 +1266,7 @@ describe('ReactCompositeComponent', () => {
     var layer = document.createElement('div');
 
     class Component extends React.Component {
-      componentWillMount() {
+      componentDidMount() {
         ReactDOM.render(<div />, layer);
       }
 

Original file line number	Diff line number	Diff line change
`@@ -614,7 +614,7 @@ describe('ReactPerf', () => {`
`614`	`614`	`}`
`615`	`615`	`}`
`616`	`616`	`class EvilPortal extends React.Component {`
`617`		`- componentWillMount() {`
	`617`	`+ componentDidMount() {`
`618`	`618`	`var portalContainer = document.createElement('div');`
`619`	`619`	`ReactDOM.render(<Evil />, portalContainer);`
`620`	`620`	`}`
`@@ -644,6 +644,7 @@ describe('ReactPerf', () => {`
`644`	`644`	`it('should not print errant warnings if portal throws in componentWillMount()', () => {`
`645`	`645`	`var container = document.createElement('div');`
`646`	`646`	`var thrownErr = new Error('Muhaha!');`
	`647`	`+ spyOn(console, 'error');`
`647`	`648`
`648`	`649`	`class Evil extends React.Component {`
`649`	`650`	`componentWillMount() {`
`@@ -679,6 +680,13 @@ describe('ReactPerf', () => {`
`679`	`680`	`}`
`680`	`681`	`ReactDOM.unmountComponentAtNode(container);`
`681`	`682`	`ReactPerf.stop();`
	`683`	`+`
	`684`	+ // A sync `render` inside cWM will print a warning. That should be the
	`685`	`+ // only warning.`
	`686`	`+ expect(console.error.calls.count()).toEqual(1);`
	`687`	`+ expect(console.error.calls.argsFor(0)[0]).toMatch(`
	`688`	`+ /Render methods should be a pure function of props and state/`
	`689`	`+ );`
`682`	`690`	`});`
`683`	`691`
`684`	`692`	`it('should not print errant warnings if portal throws in componentDidMount()', () => {`
`@@ -694,7 +702,7 @@ describe('ReactPerf', () => {`
`694`	`702`	`}`
`695`	`703`	`}`
`696`	`704`	`class EvilPortal extends React.Component {`
`697`		`- componentWillMount() {`
	`705`	`+ componentDidMount() {`
`698`	`706`	`var portalContainer = document.createElement('div');`
`699`	`707`	`ReactDOM.render(<Evil />, portalContainer);`
`700`	`708`	`}`