Skip to content

Commit

Permalink
Fix a memory leak in ReactComponentTreeDevtool
Browse files Browse the repository at this point in the history 
`ReactDebugTool` used to only call `purgeUnmountedComponents()` while profiling, so information about unmounted instances kept accumulating when not profiling.

Additionally, unmounting in React Native and rendering to string did not correctly clean up the devtool.

Finally, the tests tested the wrong behavior and relied on explicit `purgeUnmountedComponent()` calls.

To fix this, we:

* Test specifically that unmounting is enough to clean up the tree devtool.
* Add missing `onBeginFlush` and `onEndFlush` calls to server and native rendering so `ReactDebugTool` knows when to copy the tree.

Fixes #6750
  • Loading branch information
@gaearon
gaearon committed May 12, 2016
1 parent 027d9a9 commit d912a94
Show file tree
Hide file tree
Showing 9 changed files with 166 additions and 78 deletions.
49 changes: 29 additions & 20 deletions src/isomorphic/ReactDebugTool.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,45 +48,54 @@ var currentTimerDebugID = null;
var currentTimerStartTime = null;
var currentTimerType = null;

function clearHistory() {
ReactComponentTreeDevtool.purgeUnmountedComponents();
ReactNativeOperationHistoryDevtool.clearHistory();
}

function getTreeSnapshot(registeredIDs) {
return registeredIDs.reduce((tree, id) => {
var ownerID = ReactComponentTreeDevtool.getOwnerID(id);
var parentID = ReactComponentTreeDevtool.getParentID(id);
tree[id] = {
displayName: ReactComponentTreeDevtool.getDisplayName(id),
text: ReactComponentTreeDevtool.getText(id),
updateCount: ReactComponentTreeDevtool.getUpdateCount(id),
childIDs: ReactComponentTreeDevtool.getChildIDs(id),
// Text nodes don't have owners but this is close enough.
ownerID: ownerID || ReactComponentTreeDevtool.getOwnerID(parentID),
parentID,
};
return tree;
}, {});
}

function resetMeasurements() {
if (__DEV__) {
var previousStartTime = currentFlushStartTime;
var previousMeasurements = currentFlushMeasurements || [];
var previousOperations = ReactNativeOperationHistoryDevtool.getHistory();

if (!isProfiling || currentFlushNesting === 0) {
currentFlushStartTime = null;
currentFlushMeasurements = null;
clearHistory();
return;
}

var previousStartTime = currentFlushStartTime;
var previousMeasurements = currentFlushMeasurements || [];
var previousOperations = ReactNativeOperationHistoryDevtool.getHistory();

if (previousMeasurements.length || previousOperations.length) {
var registeredIDs = ReactComponentTreeDevtool.getRegisteredIDs();
flushHistory.push({
duration: performanceNow() - previousStartTime,
measurements: previousMeasurements || [],
operations: previousOperations || [],
treeSnapshot: registeredIDs.reduce((tree, id) => {
var ownerID = ReactComponentTreeDevtool.getOwnerID(id);
var parentID = ReactComponentTreeDevtool.getParentID(id);
tree[id] = {
displayName: ReactComponentTreeDevtool.getDisplayName(id),
text: ReactComponentTreeDevtool.getText(id),
updateCount: ReactComponentTreeDevtool.getUpdateCount(id),
childIDs: ReactComponentTreeDevtool.getChildIDs(id),
// Text nodes don't have owners but this is close enough.
ownerID: ownerID || ReactComponentTreeDevtool.getOwnerID(parentID),
parentID,
};
return tree;
}, {}),
treeSnapshot: getTreeSnapshot(registeredIDs),
});
}

clearHistory();
currentFlushStartTime = performanceNow();
currentFlushMeasurements = [];
ReactComponentTreeDevtool.purgeUnmountedComponents();
ReactNativeOperationHistoryDevtool.clearHistory();
}
}

Expand Down
15 changes: 15 additions & 0 deletions src/isomorphic/__tests__/ReactPerf-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,21 @@ describe('ReactPerf', function() {
});
});

it('should include stats for components unmounted during measurement', function() {
var container = document.createElement('div');
var measurements = measure(() => {
ReactDOM.render(<Div><Div key="a" /></Div>, container);
ReactDOM.render(<Div><Div key="b" /></Div>, container);
});
expect(ReactPerf.getExclusive(measurements)).toEqual([{
key: 'Div',
instanceCount: 3,
counts: { ctor: 3, render: 4 },
durations: { ctor: 3, render: 4 },
totalDuration: 7,
}]);
});

it('warns once when using getMeasurementsSummaryMap', function() {
var measurements = measure(() => {});
spyOn(console, 'error');
Expand Down
5 changes: 5 additions & 0 deletions src/isomorphic/devtools/ReactComponentTreeDevtool.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,11 @@ var ReactComponentTreeDevtool = {
},

purgeUnmountedComponents() {
if (ReactComponentTreeDevtool._preventPurging) {
// Should only be used for testing.
return;
}

Object.keys(tree)
.filter(id => !tree[id].isMounted)
.forEach(purgeDeep);
Expand Down
5 changes: 5 additions & 0 deletions src/isomorphic/devtools/ReactNativeOperationHistoryDevtool.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ var ReactNativeOperationHistoryDevtool = {
},

clearHistory() {
if (ReactNativeOperationHistoryDevtool._preventClearing) {
// Should only be used for tests.
return;
}

history = [];
},

Expand Down
56 changes: 28 additions & 28 deletions src/isomorphic/devtools/__tests__/ReactComponentTreeDevtool-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,13 +97,20 @@ describe('ReactComponentTreeDevtool', () => {
// Ensure the tree is correct on every step.
pairs.forEach(([element, expectedTree]) => {
currentElement = element;

// Mount a new tree or update the existing tree.
ReactDOM.render(<Wrapper />, node);
expect(getActualTree()).toEqual(expectedTree);

// Purging should have no effect
// on the tree we expect to see.
ReactComponentTreeDevtool.purgeUnmountedComponents();
expect(getActualTree()).toEqual(expectedTree);
});

// Unmounting the root node should purge
// the whole subtree automatically.
ReactDOM.unmountComponentAtNode(node);
ReactComponentTreeDevtool.purgeUnmountedComponents();
expect(getActualTree()).toBe(undefined);
expect(getRootDisplayNames()).toEqual([]);
expect(getRegisteredDisplayNames()).toEqual([]);
Expand All @@ -112,8 +119,23 @@ describe('ReactComponentTreeDevtool', () => {
// Ensure the tree is correct on every step.
pairs.forEach(([element, expectedTree]) => {
currentElement = element;

// Rendering to string while not profiling
// should not produce any entries after it finishes.
ReactDOMServer.renderToString(<Wrapper />);
expect(getActualTree()).toBe(undefined);
expect(getRootDisplayNames()).toEqual([]);
expect(getRegisteredDisplayNames()).toEqual([]);

// To test it, we tell the devtool to ignore next purge
// so the cleanup request by ReactDebugTool is ignored.
// This lets us make assertions on the tree before it
// is destroyed.
ReactComponentTreeDevtool._preventPurging = true;
ReactDOMServer.renderToString(<Wrapper />);
ReactComponentTreeDevtool._preventPurging = false;
expect(getActualTree()).toEqual(expectedTree);

ReactComponentTreeDevtool.purgeUnmountedComponents();
expect(getActualTree()).toBe(undefined);
expect(getRootDisplayNames()).toEqual([]);
Expand Down Expand Up @@ -1631,7 +1653,7 @@ describe('ReactComponentTreeDevtool', () => {
assertTreeMatches([element, tree], {includeOwnerDisplayName: true});
});

it('preserves unmounted components until purge', () => {
it('purges unmounted components automatically', () => {
var node = document.createElement('div');
var renderBar = true;
var fooInstance;
Expand Down Expand Up @@ -1666,31 +1688,15 @@ describe('ReactComponentTreeDevtool', () => {
renderBar = false;
ReactDOM.render(<Foo />, node);
expect(
getTree(barInstance._debugID, {
includeParentDisplayName: true,
expectedParentID: fooInstance._debugID,
})
getTree(barInstance._debugID, {expectedParentID: null})
).toEqual({
displayName: 'Bar',
parentDisplayName: 'Foo',
displayName: 'Unknown',
children: [],
});

ReactDOM.unmountComponentAtNode(node);
expect(
getTree(barInstance._debugID, {
includeParentDisplayName: true,
expectedParentID: fooInstance._debugID,
})
).toEqual({
displayName: 'Bar',
parentDisplayName: 'Foo',
children: [],
});

ReactComponentTreeDevtool.purgeUnmountedComponents();
expect(
getTree(barInstance._debugID, {includeParentDisplayName: true})
getTree(barInstance._debugID, {expectedParentID: null})
).toEqual({
displayName: 'Unknown',
children: [],
Expand Down Expand Up @@ -1719,7 +1725,7 @@ describe('ReactComponentTreeDevtool', () => {

ReactDOM.unmountComponentAtNode(node);
expect(ReactComponentTreeDevtool.getUpdateCount(divID)).toEqual(0);
expect(ReactComponentTreeDevtool.getUpdateCount(spanID)).toEqual(2);
expect(ReactComponentTreeDevtool.getUpdateCount(spanID)).toEqual(0);
});

it('does not report top-level wrapper as a root', () => {
Expand All @@ -1733,12 +1739,6 @@ describe('ReactComponentTreeDevtool', () => {

ReactDOM.unmountComponentAtNode(node);
expect(getRootDisplayNames()).toEqual([]);

ReactComponentTreeDevtool.purgeUnmountedComponents();
expect(getRootDisplayNames()).toEqual([]);

// This currently contains TopLevelWrapper until purge
// so we only check it at the very end.
expect(getRegisteredDisplayNames()).toEqual([]);
});
});
48 changes: 18 additions & 30 deletions src/isomorphic/devtools/__tests__/ReactComponentTreeDevtool-test.native.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,13 +119,20 @@ describe('ReactComponentTreeDevtool', () => {
// Ensure the tree is correct on every step.
pairs.forEach(([element, expectedTree]) => {
currentElement = element;

// Mount a new tree or update the existing tree.
ReactNative.render(<Wrapper />, 1);
expect(getActualTree()).toEqual(expectedTree);

// Purging should have no effect
// on the tree we expect to see.
ReactComponentTreeDevtool.purgeUnmountedComponents();
expect(getActualTree()).toEqual(expectedTree);
});

// Unmounting the root node should purge
// the whole subtree automatically.
ReactNative.unmountComponentAtNode(1);
ReactComponentTreeDevtool.purgeUnmountedComponents();
expect(getActualTree()).toBe(undefined);
expect(getRootDisplayNames()).toEqual([]);
expect(getRegisteredDisplayNames()).toEqual([]);
Expand All @@ -134,10 +141,13 @@ describe('ReactComponentTreeDevtool', () => {
// Ensure the tree is correct on every step.
pairs.forEach(([element, expectedTree]) => {
currentElement = element;

// Mount a new tree.
ReactNative.render(<Wrapper />, 1);
ReactNative.unmountComponentAtNode(1);
expect(getActualTree()).toEqual(expectedTree);
ReactComponentTreeDevtool.purgeUnmountedComponents();

// Unmounting should clean it up.
ReactNative.unmountComponentAtNode(1);
expect(getActualTree()).toBe(undefined);
expect(getRootDisplayNames()).toEqual([]);
expect(getRegisteredDisplayNames()).toEqual([]);
Expand Down Expand Up @@ -1620,7 +1630,7 @@ describe('ReactComponentTreeDevtool', () => {
assertTreeMatches([element, tree], {includeOwnerDisplayName: true});
});

it('preserves unmounted components until purge', () => {
it('purges unmounted components automatically', () => {
var renderBar = true;
var fooInstance;
var barInstance;
Expand Down Expand Up @@ -1654,31 +1664,15 @@ describe('ReactComponentTreeDevtool', () => {
renderBar = false;
ReactNative.render(<Foo />, 1);
expect(
getTree(barInstance._debugID, {
includeParentDisplayName: true,
expectedParentID: fooInstance._debugID,
})
getTree(barInstance._debugID, {expectedParentID: null})
).toEqual({
displayName: 'Bar',
parentDisplayName: 'Foo',
displayName: 'Unknown',
children: [],
});

ReactNative.unmountComponentAtNode(1);
expect(
getTree(barInstance._debugID, {
includeParentDisplayName: true,
expectedParentID: fooInstance._debugID,
})
).toEqual({
displayName: 'Bar',
parentDisplayName: 'Foo',
children: [],
});

ReactComponentTreeDevtool.purgeUnmountedComponents();
expect(
getTree(barInstance._debugID, {includeParentDisplayName: true})
getTree(barInstance._debugID, {expectedParentID: null})
).toEqual({
displayName: 'Unknown',
children: [],
Expand All @@ -1705,7 +1699,7 @@ describe('ReactComponentTreeDevtool', () => {

ReactNative.unmountComponentAtNode(1);
expect(ReactComponentTreeDevtool.getUpdateCount(viewID)).toEqual(0);
expect(ReactComponentTreeDevtool.getUpdateCount(imageID)).toEqual(2);
expect(ReactComponentTreeDevtool.getUpdateCount(imageID)).toEqual(0);
});

it('does not report top-level wrapper as a root', () => {
Expand All @@ -1717,12 +1711,6 @@ describe('ReactComponentTreeDevtool', () => {

ReactNative.unmountComponentAtNode(1);
expect(getRootDisplayNames()).toEqual([]);

ReactComponentTreeDevtool.purgeUnmountedComponents();
expect(getRootDisplayNames()).toEqual([]);

// This currently contains TopLevelWrapper until purge
// so we only check it at the very end.
expect(getRegisteredDisplayNames()).toEqual([]);
});
});
Loading

0 comments on commit d912a94

Please sign in to comment.