From e2f094614f8e8dd5111f19690ad32cea00d407f2 Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Thu, 2 Jan 2014 16:36:58 -0700 Subject: [PATCH 1/2] Fix potential memory leak when unmounting Fixes #781. --- src/core/ReactCompositeComponent.js | 3 +- src/core/ReactDOMComponent.js | 2 +- .../__tests__/ReactCompositeComponent-test.js | 39 +++++++++++++++++++ 3 files changed, 42 insertions(+), 2 deletions(-) diff --git a/src/core/ReactCompositeComponent.js b/src/core/ReactCompositeComponent.js index 82d99539ec..d3c3303230 100644 --- a/src/core/ReactCompositeComponent.js +++ b/src/core/ReactCompositeComponent.js @@ -736,10 +736,11 @@ var ReactCompositeComponentMixin = { this._defaultProps = null; - ReactComponent.Mixin.unmountComponent.call(this); this._renderedComponent.unmountComponent(); this._renderedComponent = null; + ReactComponent.Mixin.unmountComponent.call(this); + if (this.refs) { this.refs = null; } diff --git a/src/core/ReactDOMComponent.js b/src/core/ReactDOMComponent.js index 540ed03ade..9041629805 100644 --- a/src/core/ReactDOMComponent.js +++ b/src/core/ReactDOMComponent.js @@ -364,9 +364,9 @@ ReactDOMComponent.Mixin = { * @internal */ unmountComponent: function() { + this.unmountChildren(); ReactEventEmitter.deleteAllListeners(this._rootNodeID); ReactComponent.Mixin.unmountComponent.call(this); - this.unmountChildren(); } }; diff --git a/src/core/__tests__/ReactCompositeComponent-test.js b/src/core/__tests__/ReactCompositeComponent-test.js index 10f668eecc..7915fcbe7e 100644 --- a/src/core/__tests__/ReactCompositeComponent-test.js +++ b/src/core/__tests__/ReactCompositeComponent-test.js @@ -22,6 +22,7 @@ var MorphingComponent; var ChildUpdates; var React; +var ReactComponent; var ReactCurrentOwner; var ReactPropTypes; var ReactTestUtils; @@ -40,6 +41,7 @@ describe('ReactCompositeComponent', function() { reactComponentExpect = require('reactComponentExpect'); React = require('React'); + ReactComponent = require('ReactComponent'); ReactCurrentOwner = require('ReactCurrentOwner'); ReactDoNotBindDeprecated = require('ReactDoNotBindDeprecated'); ReactPropTypes = require('ReactPropTypes'); @@ -506,6 +508,43 @@ describe('ReactCompositeComponent', function() { }); }); + it('should call componentWillUnmount before unmounting', function() { + var container = document.createElement('div'); + var innerUnmounted = false; + + spyOn(ReactComponent, 'unmountIDFromEnvironment').andCallThrough(); + + var Component = React.createClass({ + render: function() { + return
+ +
; + } + }); + var Inner = React.createClass({ + componentWillUnmount: function() { + // It's important that unmountIDFromEnvironment (which clears + // ReactMount's node cache) be called after any component lifecycle + // methods, because a componentWillMount implementation is likely call + // this.getDOMNode(), which will repopulate the node cache after it's + // been cleared, causing a memory leak. + expect(ReactComponent.unmountIDFromEnvironment.callCount).toBe(0); + innerUnmounted = true; + }, + render: function() { + return
; + } + }); + + React.renderComponent(, container); + React.unmountComponentAtNode(container); + expect(innerUnmounted).toBe(true); + + // , , and both
elements each call + // unmountIDFromEnvironment, for a total of 4. + expect(ReactComponent.unmountIDFromEnvironment.callCount).toBe(4); + }); + it('should detect valid CompositeComponent classes', function() { var Component = React.createClass({ render: function() { From 2716f388611bacaf3906b92a2d9628318da9338b Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Wed, 8 Jan 2014 21:48:48 -0800 Subject: [PATCH 2/2] Spy on purgeID instead of unmountIDFromEnvironment --- .../__tests__/ReactCompositeComponent-test.js | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/core/__tests__/ReactCompositeComponent-test.js b/src/core/__tests__/ReactCompositeComponent-test.js index 7915fcbe7e..03c13d7197 100644 --- a/src/core/__tests__/ReactCompositeComponent-test.js +++ b/src/core/__tests__/ReactCompositeComponent-test.js @@ -512,7 +512,7 @@ describe('ReactCompositeComponent', function() { var container = document.createElement('div'); var innerUnmounted = false; - spyOn(ReactComponent, 'unmountIDFromEnvironment').andCallThrough(); + spyOn(ReactMount, 'purgeID').andCallThrough(); var Component = React.createClass({ render: function() { @@ -523,12 +523,11 @@ describe('ReactCompositeComponent', function() { }); var Inner = React.createClass({ componentWillUnmount: function() { - // It's important that unmountIDFromEnvironment (which clears - // ReactMount's node cache) be called after any component lifecycle - // methods, because a componentWillMount implementation is likely call - // this.getDOMNode(), which will repopulate the node cache after it's - // been cleared, causing a memory leak. - expect(ReactComponent.unmountIDFromEnvironment.callCount).toBe(0); + // It's important that ReactMount.purgeID be called after any component + // lifecycle methods, because a componentWillMount implementation is + // likely call this.getDOMNode(), which will repopulate the node cache + // after it's been cleared, causing a memory leak. + expect(ReactMount.purgeID.callCount).toBe(0); innerUnmounted = true; }, render: function() { @@ -541,8 +540,8 @@ describe('ReactCompositeComponent', function() { expect(innerUnmounted).toBe(true); // , , and both
elements each call - // unmountIDFromEnvironment, for a total of 4. - expect(ReactComponent.unmountIDFromEnvironment.callCount).toBe(4); + // unmountIDFromEnvironment which calls purgeID, for a total of 4. + expect(ReactMount.purgeID.callCount).toBe(4); }); it('should detect valid CompositeComponent classes', function() {