Merge pull request facebook#6753 from facebook/fix-6750

Fix a memory leak in ReactComponentTreeDevtool

src/isomorphic/ReactDebugTool.js

@@ -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();
   }
 }
 

src/isomorphic/__tests__/ReactPerf-test.js

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

src/isomorphic/devtools/ReactComponentTreeDevtool.js

@@ -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);

src/isomorphic/devtools/ReactNativeOperationHistoryDevtool.js

@@ -23,6 +23,11 @@ var ReactNativeOperationHistoryDevtool = {
   },
 
   clearHistory() {
+    if (ReactNativeOperationHistoryDevtool._preventClearing) {
+      // Should only be used for tests.
+      return;
+    }
+
     history = [];
   },
 

src/isomorphic/devtools/__tests__/ReactComponentTreeDevtool-test.js

@@ -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([]);
@@ -112,8 +119,23 @@ describe('ReactComponentTreeDevtool', () => {
     // Ensure the tree is correct on every step.
     pairs.forEach(([element, expectedTree]) => {
       currentElement = element;
+
+      // Rendering to string should not produce any entries
+      // because ReactDebugTool purges it when the flush ends.
+      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 actual tree.
+      ReactComponentTreeDevtool._preventPurging = true;
       ReactDOMServer.renderToString(<Wrapper />);
+      ReactComponentTreeDevtool._preventPurging = false;
       expect(getActualTree()).toEqual(expectedTree);
+
+      // Purge manually since we skipped the automatic purge.
       ReactComponentTreeDevtool.purgeUnmountedComponents();
       expect(getActualTree()).toBe(undefined);
       expect(getRootDisplayNames()).toEqual([]);
@@ -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;
@@ -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: [],
@@ -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', () => {
@@ -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([]);
   });
 });

src/isomorphic/devtools/__tests__/ReactComponentTreeDevtool-test.native.js

@@ -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([]);
@@ -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([]);
@@ -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;
@@ -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: [],
@@ -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', () => {
@@ -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([]);
   });
 });