Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Patch release branch for 16.8.3 #14902

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fix react-dom/server context leaks when render stream destroyed early (
…#14706)

* Fix react-dom/server context memory retention

* Test for pollution of later renders

* Inline loop

* More tests
  • Loading branch information
overlookmotel authored and acdlite committed Feb 20, 2019
commit b668168d4dec542e9022bc779661f691b730dd44
Original file line number Diff line number Diff line change
Expand Up @@ -482,5 +482,98 @@ 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');

const AppWithUser = user => (
<LoggedInUser.Provider value={user}>
<header>
<LoggedInUser.Consumer>{whoAmI => whoAmI}</LoggedInUser.Consumer>
</header>
</LoggedInUser.Provider>
);

const stream = ReactDOMServer.renderToNodeStream(
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
stream._read(10);
expect(LoggedInUser[threadID]).toBe('Amy');

stream.destroy();

const AppWithUserNoProvider = () => (
<LoggedInUser.Consumer>{whoAmI => whoAmI}</LoggedInUser.Consumer>
);

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');
});

// Regression test for https://github.com/facebook/react/issues/14705
it('frees context value reference when stream destroyed', () => {
const LoggedInUser = React.createContext('default');

const AppWithUser = user => (
<LoggedInUser.Provider value={user}>
<header>
<LoggedInUser.Consumer>{whoAmI => whoAmI}</LoggedInUser.Consumer>
</header>
</LoggedInUser.Provider>
);

const stream = ReactDOMServer.renderToNodeStream(
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
stream._read(10);
expect(LoggedInUser[threadID]).toBe('Amy');

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 => (
<LoggedInUser.Provider value={user}>
<LoggedInUser.Consumer>{whoAmI => whoAmI}</LoggedInUser.Consumer>
<Crash />
</LoggedInUser.Provider>
);

expect(() => {
ReactDOMServer.renderToString(AppWithUser('Casper'));
}).toThrow('Boo');

// Should not report a value from failed render
expect(
ReactDOMServer.renderToString(
<LoggedInUser.Consumer>{whoAmI => whoAmI}</LoggedInUser.Consumer>,
),
).toBe('default');
});
});
});
10 changes: 10 additions & 0 deletions packages/react-dom/src/server/ReactPartialRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -715,6 +715,7 @@ class ReactDOMServerRenderer {
destroy() {
if (!this.exhausted) {
this.exhausted = true;
this.clearProviders();
freeThreadID(this.threadID);
}
}
Expand Down Expand Up @@ -776,6 +777,15 @@ class ReactDOMServerRenderer {
context[this.threadID] = previousValue;
}

clearProviders(): void {
// Restore any remaining providers on the stack to previous values
for (let index = this.contextIndex; index >= 0; index--) {
const context: ReactContext<any> = this.contextStack[index];
const previousValue = this.contextValueStack[index];
context[this.threadID] = previousValue;
}
}

read(bytes: number): string | null {
if (this.exhausted) {
return null;
Expand Down