From 131bcab46c1d28b334c4d260d8f2a6575d45436b Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Sat, 26 Jan 2019 14:18:47 +0000 Subject: [PATCH 1/4] Fix react-dom/server context memory retention --- ...eactDOMServerIntegrationNewContext-test.js | 25 +++++++++++++++++++ .../src/server/ReactPartialRenderer.js | 13 +++++++++- 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js index 1060cb244baba..bdd26f6620407 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js @@ -482,5 +482,30 @@ describe('ReactDOMServerIntegration', () => { ); } }); + + it('frees context value reference when stream destroyed', () => { + const LoggedInUser = React.createContext('default'); + + const AppWithUser = user => ( + +
+ {whoAmI => whoAmI} +
+ + ); + + const stream = ReactDOMServer.renderToNodeStream( + AppWithUser('Amy'), + ).setEncoding('utf8'); + + const {threadID} = stream.partialRenderer; + + // Read enough to render Provider but not enough for it to be exited + stream._read(10); + expect(LoggedInUser[threadID]).toBe('Amy'); + + stream.destroy(); + expect(LoggedInUser[threadID]).toBe('default'); + }); }); }); diff --git a/packages/react-dom/src/server/ReactPartialRenderer.js b/packages/react-dom/src/server/ReactPartialRenderer.js index 1d9a7157b21cb..bb2bd6a231531 100644 --- a/packages/react-dom/src/server/ReactPartialRenderer.js +++ b/packages/react-dom/src/server/ReactPartialRenderer.js @@ -715,6 +715,7 @@ class ReactDOMServerRenderer { destroy() { if (!this.exhausted) { this.exhausted = true; + this.clearProviders(); freeThreadID(this.threadID); } } @@ -748,7 +749,7 @@ class ReactDOMServerRenderer { context[threadID] = provider.props.value; } - popProvider(provider: ReactProvider): void { + popProvider(provider?: ReactProvider): void { const index = this.contextIndex; if (__DEV__) { warningWithoutStack( @@ -776,6 +777,16 @@ class ReactDOMServerRenderer { context[this.threadID] = previousValue; } + clearProviders(): void { + while (this.contextIndex > -1) { + if (__DEV__) { + this.popProvider((this.contextProviderStack: any)[this.contextIndex]); + } else { + this.popProvider(); + } + } + } + read(bytes: number): string | null { if (this.exhausted) { return null; From 781dd45d0cdad8d16a5ac00849734ed5838d09d2 Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Sun, 27 Jan 2019 01:55:11 +0000 Subject: [PATCH 2/4] Test for pollution of later renders --- ...eactDOMServerIntegrationNewContext-test.js | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js index bdd26f6620407..cb7e7534344f6 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js @@ -483,6 +483,46 @@ describe('ReactDOMServerIntegration', () => { } }); + it('does not pollute later renders when stream destroyed', () => { + const LoggedInUser = React.createContext('default'); + + const AppWithUser = user => ( + +
+ {whoAmI => whoAmI} +
+ + ); + + const stream = ReactDOMServer.renderToNodeStream( + AppWithUser('Amy'), + ).setEncoding('utf8'); + + const {threadID} = stream.partialRenderer; + + // Read enough to render Provider but not enough for it to be exited + stream._read(10); + expect(LoggedInUser[threadID]).toBe('Amy'); + + stream.destroy(); + + const AppWithUserNoProvider = () => ( + {whoAmI => whoAmI} + ); + + const stream2 = ReactDOMServer.renderToNodeStream( + AppWithUserNoProvider(), + ).setEncoding('utf8'); + + // Sanity check to ensure 2nd render has same threadID as 1st render, + // otherwise this test is not testing what it's meant to + expect(stream2.partialRenderer.threadID).toBe(threadID); + + const markup = stream2.read(Infinity); + + expect(markup).toBe('default'); + }); + it('frees context value reference when stream destroyed', () => { const LoggedInUser = React.createContext('default'); From 94f70f555fd10c09333b5ba2896b89175a2a66a1 Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Thu, 14 Feb 2019 08:32:34 +0000 Subject: [PATCH 3/4] Inline loop --- .../react-dom/src/server/ReactPartialRenderer.js | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/packages/react-dom/src/server/ReactPartialRenderer.js b/packages/react-dom/src/server/ReactPartialRenderer.js index bb2bd6a231531..120403e89dcc8 100644 --- a/packages/react-dom/src/server/ReactPartialRenderer.js +++ b/packages/react-dom/src/server/ReactPartialRenderer.js @@ -749,7 +749,7 @@ class ReactDOMServerRenderer { context[threadID] = provider.props.value; } - popProvider(provider?: ReactProvider): void { + popProvider(provider: ReactProvider): void { const index = this.contextIndex; if (__DEV__) { warningWithoutStack( @@ -778,12 +778,11 @@ class ReactDOMServerRenderer { } clearProviders(): void { - while (this.contextIndex > -1) { - if (__DEV__) { - this.popProvider((this.contextProviderStack: any)[this.contextIndex]); - } else { - this.popProvider(); - } + // Restore any remaining providers on the stack to previous values + for (let index = this.contextIndex; index >= 0; index--) { + const context: ReactContext = this.contextStack[index]; + const previousValue = this.contextValueStack[index]; + context[this.threadID] = previousValue; } } From 855cc272f1c610f762b07851190f04f70bf154c7 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 14 Feb 2019 19:45:41 +0000 Subject: [PATCH 4/4] More tests --- ...eactDOMServerIntegrationNewContext-test.js | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js index cb7e7534344f6..071dd8715751c 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js @@ -483,6 +483,7 @@ describe('ReactDOMServerIntegration', () => { } }); + // Regression test for https://github.com/facebook/react/issues/14705 it('does not pollute later renders when stream destroyed', () => { const LoggedInUser = React.createContext('default'); @@ -498,6 +499,7 @@ describe('ReactDOMServerIntegration', () => { AppWithUser('Amy'), ).setEncoding('utf8'); + // This is an implementation detail because we test a memory leak const {threadID} = stream.partialRenderer; // Read enough to render Provider but not enough for it to be exited @@ -523,6 +525,7 @@ describe('ReactDOMServerIntegration', () => { expect(markup).toBe('default'); }); + // Regression test for https://github.com/facebook/react/issues/14705 it('frees context value reference when stream destroyed', () => { const LoggedInUser = React.createContext('default'); @@ -538,6 +541,7 @@ describe('ReactDOMServerIntegration', () => { AppWithUser('Amy'), ).setEncoding('utf8'); + // This is an implementation detail because we test a memory leak const {threadID} = stream.partialRenderer; // Read enough to render Provider but not enough for it to be exited @@ -547,5 +551,29 @@ describe('ReactDOMServerIntegration', () => { stream.destroy(); expect(LoggedInUser[threadID]).toBe('default'); }); + + it('does not pollute sync renders after an error', () => { + const LoggedInUser = React.createContext('default'); + const Crash = () => { + throw new Error('Boo!'); + }; + const AppWithUser = user => ( + + {whoAmI => whoAmI} + + + ); + + expect(() => { + ReactDOMServer.renderToString(AppWithUser('Casper')); + }).toThrow('Boo'); + + // Should not report a value from failed render + expect( + ReactDOMServer.renderToString( + {whoAmI => whoAmI}, + ), + ).toBe('default'); + }); }); });