From e4b50e8c5900d569f16b761833e3e8afbf3a81ce Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 20 Mar 2019 16:09:43 +0000 Subject: [PATCH 1/4] Isolate ReactUpdates-test cases This ensures their behavior is consistent when run in isolation, and that they actually test the cases they're describing. --- packages/react-dom/src/__tests__/ReactUpdates-test.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/react-dom/src/__tests__/ReactUpdates-test.js b/packages/react-dom/src/__tests__/ReactUpdates-test.js index a6b788902b1b7..ea09c91b7c7b6 100644 --- a/packages/react-dom/src/__tests__/ReactUpdates-test.js +++ b/packages/react-dom/src/__tests__/ReactUpdates-test.js @@ -15,6 +15,7 @@ let ReactTestUtils; describe('ReactUpdates', () => { beforeEach(() => { + jest.resetModules(); React = require('react'); ReactDOM = require('react-dom'); ReactTestUtils = require('react-dom/test-utils'); From 63c3456baedc8045e369125167112c6ec74f3652 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 20 Mar 2019 16:10:38 +0000 Subject: [PATCH 2/4] Add coverage for cases where we reset nestedUpdateCounter These cases explicitly verify that we reset the counter in right places. --- .../src/__tests__/ReactUpdates-test.js | 80 +++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactUpdates-test.js b/packages/react-dom/src/__tests__/ReactUpdates-test.js index ea09c91b7c7b6..cdff8f93ae4cb 100644 --- a/packages/react-dom/src/__tests__/ReactUpdates-test.js +++ b/packages/react-dom/src/__tests__/ReactUpdates-test.js @@ -1312,6 +1312,46 @@ describe('ReactUpdates', () => { ReactDOM.render(, container); }); + it('resets the update counter for unrelated updates', () => { + const container = document.createElement('div'); + const ref = React.createRef(); + + class EventuallyTerminating extends React.Component { + state = {step: 0}; + componentDidMount() { + this.setState({step: 1}); + } + componentDidUpdate() { + if (this.state.step < limit) { + this.setState({step: this.state.step + 1}); + } + } + render() { + return this.state.step; + } + } + + let limit = 55; + expect(() => { + ReactDOM.render(, container); + }).toThrow('Maximum'); + + // Verify that we don't go over the limit if these updates are unrelated. + limit -= 10; + ReactDOM.render(, container); + expect(container.textContent).toBe(limit.toString()); + ref.current.setState({step: 0}); + expect(container.textContent).toBe(limit.toString()); + ref.current.setState({step: 0}); + expect(container.textContent).toBe(limit.toString()); + + limit += 10; + expect(() => { + ref.current.setState({step: 0}); + }).toThrow('Maximum'); + expect(ref.current).toBe(null); + }); + it('does not fall into an infinite update loop', () => { class NonTerminating extends React.Component { state = {step: 0}; @@ -1337,6 +1377,46 @@ describe('ReactUpdates', () => { }).toThrow('Maximum'); }); + it('can recover after falling into an infinite update loop', () => { + class NonTerminating extends React.Component { + state = {step: 0}; + componentDidMount() { + this.setState({step: 1}); + } + componentDidUpdate() { + this.setState({step: 2}); + } + render() { + return this.state.step; + } + } + + class Terminating extends React.Component { + state = {step: 0}; + componentDidMount() { + this.setState({step: 1}); + } + render() { + return this.state.step; + } + } + + const container = document.createElement('div'); + expect(() => { + ReactDOM.render(, container); + }).toThrow('Maximum'); + + ReactDOM.render(, container); + expect(container.textContent).toBe('1'); + + expect(() => { + ReactDOM.render(, container); + }).toThrow('Maximum'); + + ReactDOM.render(, container); + expect(container.textContent).toBe('1'); + }); + it('does not fall into an infinite error loop', () => { function BadRender() { throw new Error('error'); From d2303133fd09e1980af5c720de85dcaa571fbaa5 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 20 Mar 2019 17:29:57 +0000 Subject: [PATCH 3/4] Add a mutually recursive test case --- .../src/__tests__/ReactUpdates-test.js | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactUpdates-test.js b/packages/react-dom/src/__tests__/ReactUpdates-test.js index cdff8f93ae4cb..6f6dc2ea6086f 100644 --- a/packages/react-dom/src/__tests__/ReactUpdates-test.js +++ b/packages/react-dom/src/__tests__/ReactUpdates-test.js @@ -1417,6 +1417,33 @@ describe('ReactUpdates', () => { expect(container.textContent).toBe('1'); }); + it('does not fall into mutually recursive infinite update loop with same container', () => { + // Note: this test would fail if there were two or more different roots. + + class A extends React.Component { + componentDidMount() { + ReactDOM.render(, container); + } + render() { + return null; + } + } + + class B extends React.Component { + componentDidMount() { + ReactDOM.render(, container); + } + render() { + return null; + } + } + + const container = document.createElement('div'); + expect(() => { + ReactDOM.render(, container); + }).toThrow('Maximum'); + }); + it('does not fall into an infinite error loop', () => { function BadRender() { throw new Error('error'); From 36be02f5888111d9287870e3766e7dccbc467d30 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 20 Mar 2019 18:17:46 +0000 Subject: [PATCH 4/4] Add test coverage for useLayoutEffect loop --- .../react-dom/src/__tests__/ReactUpdates-test.js | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactUpdates-test.js b/packages/react-dom/src/__tests__/ReactUpdates-test.js index 6f6dc2ea6086f..533ce0743a9a0 100644 --- a/packages/react-dom/src/__tests__/ReactUpdates-test.js +++ b/packages/react-dom/src/__tests__/ReactUpdates-test.js @@ -1377,6 +1377,21 @@ describe('ReactUpdates', () => { }).toThrow('Maximum'); }); + it('does not fall into an infinite update loop with useLayoutEffect', () => { + function NonTerminating() { + const [step, setStep] = React.useState(0); + React.useLayoutEffect(() => { + setStep(x => x + 1); + }); + return step; + } + + const container = document.createElement('div'); + expect(() => { + ReactDOM.render(, container); + }).toThrow('Maximum'); + }); + it('can recover after falling into an infinite update loop', () => { class NonTerminating extends React.Component { state = {step: 0};