Skip to content

Commit

Permalink
Fix memory leak with renderComponentToString()
Browse files Browse the repository at this point in the history
This is leaking memory. Move event registration to mount time.
  • Loading branch information
petehunt authored and zpao committed Jan 25, 2014
1 parent 4975113 commit 526be15
Show file tree
Hide file tree
Showing 7 changed files with 159 additions and 11 deletions.
23 changes: 14 additions & 9 deletions src/core/ReactDOMComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,16 +64,19 @@ function assertValidProps(props) {
);
}

function putListener(id, registrationName, listener) {
function putListener(id, registrationName, listener, transaction) {
var container = ReactMount.findReactContainerForID(id);
if (container) {
var doc = container.nodeType === ELEMENT_NODE_TYPE ?
container.ownerDocument :
container;
listenTo(registrationName, doc);
}

ReactEventEmitter.putListener(id, registrationName, listener);
transaction.getPutListenerQueue().enqueuePutListener(
id,
registrationName,
listener
);
}


Expand Down Expand Up @@ -112,7 +115,7 @@ ReactDOMComponent.Mixin = {
);
assertValidProps(this.props);
return (
this._createOpenTagMarkup() +
this._createOpenTagMarkupAndPutListeners(transaction) +
this._createContentMarkup(transaction) +
this._tagClose
);
Expand All @@ -128,9 +131,10 @@ ReactDOMComponent.Mixin = {
* @see http://jsperf.com/obj-vs-arr-iteration
*
* @private
* @param {ReactReconcileTransaction} transaction
* @return {string} Markup of opening tag.
*/
_createOpenTagMarkup: function() {
_createOpenTagMarkupAndPutListeners: function(transaction) {
var props = this.props;
var ret = this._tagOpen;

Expand All @@ -143,7 +147,7 @@ ReactDOMComponent.Mixin = {
continue;
}
if (registrationNameModules[propKey]) {
putListener(this._rootNodeID, propKey, propValue);
putListener(this._rootNodeID, propKey, propValue, transaction);
} else {
if (propKey === STYLE) {
if (propValue) {
Expand Down Expand Up @@ -222,7 +226,7 @@ ReactDOMComponent.Mixin = {
prevProps,
prevOwner
);
this._updateDOMProperties(prevProps);
this._updateDOMProperties(prevProps, transaction);
this._updateDOMChildren(prevProps, transaction);
}
),
Expand All @@ -240,8 +244,9 @@ ReactDOMComponent.Mixin = {
*
* @private
* @param {object} lastProps
* @param {ReactReconcileTransaction} transaction
*/
_updateDOMProperties: function(lastProps) {
_updateDOMProperties: function(lastProps, transaction) {
var nextProps = this.props;
var propKey;
var styleName;
Expand Down Expand Up @@ -302,7 +307,7 @@ ReactDOMComponent.Mixin = {
styleUpdates = nextProp;
}
} else if (registrationNameModules[propKey]) {
putListener(this._rootNodeID, propKey, nextProp);
putListener(this._rootNodeID, propKey, nextProp, transaction);
} else if (
DOMProperty.isStandardName[propKey] ||
DOMProperty.isCustomAttribute(propKey)) {
Expand Down
61 changes: 61 additions & 0 deletions src/core/ReactPutListenerQueue.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/**
* Copyright 2013 Facebook, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
* @providesModule ReactPutListenerQueue
*/

"use strict";

var PooledClass = require('PooledClass');
var ReactEventEmitter = require('ReactEventEmitter');

var mixInto = require('mixInto');

function ReactPutListenerQueue() {
this.listenersToPut = [];
}

mixInto(ReactPutListenerQueue, {
enqueuePutListener: function(rootNodeID, propKey, propValue) {
this.listenersToPut.push({
rootNodeID: rootNodeID,
propKey: propKey,
propValue: propValue
});
},

putListeners: function() {
for (var i = 0; i < this.listenersToPut.length; i++) {
var listenerToPut = this.listenersToPut[i];
ReactEventEmitter.putListener(
listenerToPut.rootNodeID,
listenerToPut.propKey,
listenerToPut.propValue
);
}
},

reset: function() {
this.listenersToPut.length = 0;
},

destructor: function() {
this.reset();
}
});

PooledClass.addPoolingTo(ReactPutListenerQueue);

module.exports = ReactPutListenerQueue;
20 changes: 20 additions & 0 deletions src/core/ReactReconcileTransaction.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ var PooledClass = require('PooledClass');
var ReactEventEmitter = require('ReactEventEmitter');
var ReactInputSelection = require('ReactInputSelection');
var ReactMountReady = require('ReactMountReady');
var ReactPutListenerQueue = require('ReactPutListenerQueue');
var Transaction = require('Transaction');

var mixInto = require('mixInto');
Expand Down Expand Up @@ -88,12 +89,23 @@ var ON_DOM_READY_QUEUEING = {
}
};

var PUT_LISTENER_QUEUEING = {
initialize: function() {
this.putListenerQueue.reset();
},

close: function() {
this.putListenerQueue.putListeners();
}
};

/**
* Executed within the scope of the `Transaction` instance. Consider these as
* being member methods, but with an implied ordering while being isolated from
* each other.
*/
var TRANSACTION_WRAPPERS = [
PUT_LISTENER_QUEUEING,
SELECTION_RESTORATION,
EVENT_SUPPRESSION,
ON_DOM_READY_QUEUEING
Expand All @@ -116,6 +128,7 @@ var TRANSACTION_WRAPPERS = [
function ReactReconcileTransaction() {
this.reinitializeTransaction();
this.reactMountReady = ReactMountReady.getPooled(null);
this.putListenerQueue = ReactPutListenerQueue.getPooled();
}

var Mixin = {
Expand All @@ -142,13 +155,20 @@ var Mixin = {
return this.reactMountReady;
},

getPutListenerQueue: function() {
return this.putListenerQueue;
},

/**
* `PooledClass` looks for this, and will invoke this before allowing this
* instance to be resused.
*/
destructor: function() {
ReactMountReady.release(this.reactMountReady);
this.reactMountReady = null;

ReactPutListenerQueue.release(this.putListenerQueue);
this.putListenerQueue = null;
}
};

Expand Down
2 changes: 1 addition & 1 deletion src/core/__tests__/ReactDOMComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ describe('ReactDOMComponent', function() {
mixInto(NodeStub, ReactDOMComponent.Mixin);

genMarkup = function(props) {
return (new NodeStub(props))._createOpenTagMarkup();
return (new NodeStub(props))._createOpenTagMarkupAndPutListeners();
};

this.addMatchers({
Expand Down
40 changes: 40 additions & 0 deletions src/core/putDOMComponentListener.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/**
* Copyright 2013 Facebook, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
* @providesModule putDOMComponentListener
*/

"use strict";

var ReactEventEmitter = require('ReactEventEmitter');
var ReactMount = require('ReactMount');

var listenTo = ReactEventEmitter.listenTo;

var ELEMENT_NODE_TYPE = 1;

function putDOMComponentListener(id, registrationName, listener) {
var container = ReactMount.findReactContainerForID(id);
if (container) {
var doc = container.nodeType === ELEMENT_NODE_TYPE ?
container.ownerDocument :
container;
listenTo(registrationName, doc);
}

ReactEventEmitter.putListener(id, registrationName, listener);
}

module.exports = putDOMComponentListener;
12 changes: 12 additions & 0 deletions src/environment/__tests__/ReactServerRendering-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,18 @@ describe('ReactServerRendering', function() {
);
});

it('should not register event listeners', function() {
var EventPluginHub = require('EventPluginHub');
var cb = mocks.getMockFunction();

ReactServerRendering.renderComponentToString(
<span onClick={cb}>hello world</span>,
function() {
expect(EventPluginHub.__getListenerBank()).toEqual({});
}
);
});

it('should render composite components', function() {
var Parent = React.createClass({
render: function() {
Expand Down
12 changes: 11 additions & 1 deletion src/event/EventPluginHub.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

var EventPluginRegistry = require('EventPluginRegistry');
var EventPluginUtils = require('EventPluginUtils');
var ExecutionEnvironment = require('ExecutionEnvironment');

var accumulate = require('accumulate');
var forEachAccumulated = require('forEachAccumulated');
Expand Down Expand Up @@ -150,6 +151,11 @@ var EventPluginHub = {
* @param {?function} listener The callback to store.
*/
putListener: function(id, registrationName, listener) {
invariant(
ExecutionEnvironment.canUseDOM,
'Cannot call putListener() in a non-DOM environment.'
);

if (__DEV__) {
// IE8 has no API for event capturing and the `onScroll` event doesn't
// bubble.
Expand Down Expand Up @@ -265,10 +271,14 @@ var EventPluginHub = {
},

/**
* This is needed for tests only. Do not use!
* These are needed for tests only. Do not use!
*/
__purge: function() {
listenerBank = {};
},

__getListenerBank: function() {
return listenerBank;
}

};
Expand Down

0 comments on commit 526be15

Please sign in to comment.