Skip to content

7128 - React document listener memory leak fix. #7209

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

7128 - React document listener memory leak fix. #7209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
61 changes: 51 additions & 10 deletions src/renderers/dom/client/ReactBrowserEventEmitter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,15 @@ var ReactBrowserEventEmitter = Object.assign({}, ReactEventEmitterMixin, {
ReactEventListener.setHandleTopLevel(
ReactBrowserEventEmitter.handleTopLevel
);
ReactEventListener.setDocumentResetCallback(function(doc) {
// This must be called during a document reset (@see ReactEventListener.resetDocument)
// after unmounting the last component, otherwise the next render will be visible,
// but document level event listeners will be missing, and input (clicks, typing etc)
// wont work.
reactTopListenersCounter = 0;
alreadyListeningTo = {};
delete doc[topListenersIDKey];
});
ReactBrowserEventEmitter.ReactEventListener = ReactEventListener;
},
},
Expand Down Expand Up @@ -234,13 +243,37 @@ var ReactBrowserEventEmitter = Object.assign({}, ReactEventEmitterMixin, {
*
* @param {string} registrationName Name of listener (e.g. `onClick`).
* @param {object} contentDocumentHandle Document which owns the container
* @return {function} A function that removes ALL listeners that were added.
*/
listenTo: function(registrationName, contentDocumentHandle) {
var mountAt = contentDocumentHandle;
var isListening = getListeningForDocument(mountAt);
var dependencies =
EventPluginRegistry.registrationNameDependencies[registrationName];

// aggregates handler remove functions...
var listenerRemovers = [];
var trapBubbledEvent = function(topLevelType, handlerBaseName) {
var remover = ReactBrowserEventEmitter.ReactEventListener.trapBubbledEvent(
topLevelType,
handlerBaseName,
mountAt
);
if (remover && remover.remove) {
listenerRemovers.push(remover.remove);
}
};
var trapCapturedEvent = function(topLevelType, handlerBaseName, handle) {
var remover = ReactBrowserEventEmitter.ReactEventListener.trapCapturedEvent(
topLevelType,
handlerBaseName,
handle
);
if (remover && remover.remove) {
listenerRemovers.push(remover.remove);
}
};
Copy link
Author

@mP1 mP1 Jul 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if the names $trapBubbledEvent and $trapCapturedEvent should be different so theres no confusion about them being refs to the ones that live in ReactEventListener.


var topLevelTypes = EventConstants.topLevelTypes;
for (var i = 0; i < dependencies.length; i++) {
var dependency = dependencies[i];
Expand All @@ -250,21 +283,21 @@ var ReactBrowserEventEmitter = Object.assign({}, ReactEventEmitterMixin, {
)) {
if (dependency === topLevelTypes.topWheel) {
if (isEventSupported('wheel')) {
ReactBrowserEventEmitter.ReactEventListener.trapBubbledEvent(
trapBubbledEvent(
topLevelTypes.topWheel,
'wheel',
mountAt
);
} else if (isEventSupported('mousewheel')) {
ReactBrowserEventEmitter.ReactEventListener.trapBubbledEvent(
trapBubbledEvent(
topLevelTypes.topWheel,
'mousewheel',
mountAt
);
} else {
// Firefox needs to capture a different mouse scroll event.
// @see http://www.quirksmode.org/dom/events/tests/scroll.html
ReactBrowserEventEmitter.ReactEventListener.trapBubbledEvent(
trapBubbledEvent(
topLevelTypes.topWheel,
'DOMMouseScroll',
mountAt
Expand All @@ -273,13 +306,13 @@ var ReactBrowserEventEmitter = Object.assign({}, ReactEventEmitterMixin, {
} else if (dependency === topLevelTypes.topScroll) {

if (isEventSupported('scroll', true)) {
ReactBrowserEventEmitter.ReactEventListener.trapCapturedEvent(
trapCapturedEvent(
topLevelTypes.topScroll,
'scroll',
mountAt
);
} else {
ReactBrowserEventEmitter.ReactEventListener.trapBubbledEvent(
trapBubbledEvent(
topLevelTypes.topScroll,
'scroll',
ReactBrowserEventEmitter.ReactEventListener.WINDOW_HANDLE
Expand All @@ -289,25 +322,25 @@ var ReactBrowserEventEmitter = Object.assign({}, ReactEventEmitterMixin, {
dependency === topLevelTypes.topBlur) {

if (isEventSupported('focus', true)) {
ReactBrowserEventEmitter.ReactEventListener.trapCapturedEvent(
trapCapturedEvent(
topLevelTypes.topFocus,
'focus',
mountAt
);
ReactBrowserEventEmitter.ReactEventListener.trapCapturedEvent(
trapCapturedEvent(
topLevelTypes.topBlur,
'blur',
mountAt
);
} else if (isEventSupported('focusin')) {
// IE has `focusin` and `focusout` events which bubble.
// @see http://www.quirksmode.org/blog/archives/2008/04/delegating_the.html
ReactBrowserEventEmitter.ReactEventListener.trapBubbledEvent(
trapBubbledEvent(
topLevelTypes.topFocus,
'focusin',
mountAt
);
ReactBrowserEventEmitter.ReactEventListener.trapBubbledEvent(
trapBubbledEvent(
topLevelTypes.topBlur,
'focusout',
mountAt
Expand All @@ -318,7 +351,7 @@ var ReactBrowserEventEmitter = Object.assign({}, ReactEventEmitterMixin, {
isListening[topLevelTypes.topBlur] = true;
isListening[topLevelTypes.topFocus] = true;
} else if (topEventMapping.hasOwnProperty(dependency)) {
ReactBrowserEventEmitter.ReactEventListener.trapBubbledEvent(
trapBubbledEvent(
dependency,
topEventMapping[dependency],
mountAt
Expand All @@ -328,6 +361,14 @@ var ReactBrowserEventEmitter = Object.assign({}, ReactEventEmitterMixin, {
isListening[dependency] = true;
}
}

return function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quickly reading this I'm having a little trouble understanding the exact flow, if I understand this correctly, this seems like a wasteful way of doing it. Allocating a new callback which holds an array of callbacks, instead of keeping a single global array of callbacks? Or am I missing something.

Copy link
Author

@mP1 mP1 Jul 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@syranide

This function can accept documents and other types of targets. In both these cases it returns a remover that removes all registered listeners. To give a consistent interface it must therefore always return a remover for all types of target.

In this case, the caller knows if its adding to a document, and it does the right thing with the returned remover.

Its simply trying to match the style that whoever registers a listener also takes care of removing them at the right time.

Regarding the wastefulness of creating an array for each call, it hardly matters, as this isnt called that frequently, actually it only happens once if your "root" is the document, or once for the other "targets", which again wont be that many.

listenerRemovers.forEach(
function(remover) {
remover();
}
);
};
},

trapBubbledEvent: function(topLevelType, handlerBaseName, handle) {
Expand Down
45 changes: 45 additions & 0 deletions src/renderers/dom/client/ReactEventListener.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,17 @@ var ReactUpdates = require('ReactUpdates');
var getEventTarget = require('getEventTarget');
var getUnboundedScrollPosition = require('getUnboundedScrollPosition');

/**
* Tracks all listeners added to a document, for later batch removal.
* @type {Array}
*/
var documentEventListenerRemovers = [];

/**
* This is invoked when the document is reset.
*/
var documentResetCallback = function() {};

/**
* Find the deepest React component completely containing the root of the
* passed-in instance (for use when entire React trees are nested within each
Expand Down Expand Up @@ -172,6 +183,40 @@ var ReactEventListener = {
TopLevelCallbackBookKeeping.release(bookKeeping);
}
},
/**
* Records the remover for a previously added document event listener.
* @param {function} remover A function that removes an added listener.
*/
addDocumentEventListenerRemover: function(remover) {
if (remover) {
documentEventListenerRemovers.push(remover);
}
},

/**
* This callback will be invoked when resetDocument is also called.
* @param callback
*/
setDocumentResetCallback: function(callback) {
documentResetCallback = callback;
},

/**
* Removes all previously added document or global event listeners.
* @param {object} The document holding the last component.
*/
resetDocument: function(doc) {
for (;;) {
var remover = documentEventListenerRemovers.pop();
if (!remover) {
break;
}
remover();
}

documentResetCallback(doc);
},

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately i had to add these "public" or "visible" apis because other places need to call them.

};

module.exports = ReactEventListener;
5 changes: 5 additions & 0 deletions src/renderers/dom/client/ReactMount.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ var ReactDOMComponentTree = require('ReactDOMComponentTree');
var ReactDOMContainerInfo = require('ReactDOMContainerInfo');
var ReactDOMFeatureFlags = require('ReactDOMFeatureFlags');
var ReactElement = require('ReactElement');
var ReactEventListener = require('ReactEventListener');
var ReactFeatureFlags = require('ReactFeatureFlags');
var ReactInstanceMap = require('ReactInstanceMap');
var ReactInstrumentation = require('ReactInstrumentation');
Expand Down Expand Up @@ -593,6 +594,10 @@ var ReactMount = {
container,
false
);

if (Object.keys(instancesByReactRootID).length===0) {
ReactEventListener.resetDocument(container.ownerDocument);
}
return true;
},

Expand Down
48 changes: 48 additions & 0 deletions src/renderers/dom/client/__tests__/ReactEventListener-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,4 +209,52 @@ describe('ReactEventListener', function() {
expect(calls[0][EVENT_TARGET_PARAM])
.toBe(ReactDOMComponentTree.getInstanceFromNode(instance.getInner()));
});

it('documentResetCallback called when resetDocument called with provided document', function() {
var iframe =document.createElement('iframe');
var doc = iframe.contentDocument;
var resetCalled = false;
ReactEventListener.setDocumentResetCallback(function(d) {
expect(d).toBe(doc);
resetCalled = true;
});
ReactEventListener.resetDocument(doc);
expect(resetCalled).toBe(true);
});

it('document event remover called once and removed upon first resetDocument', function() {
var removed = false;
ReactEventListener.addDocumentEventListenerRemover(function() {
expect(removed).toBe(false);
removed = true;
});
ReactEventListener.resetDocument(document);
expect(removed).toBe(true);
ReactEventListener.resetDocument(document);
ReactEventListener.resetDocument(document);
});

it('multiple document event removers each called once and removed upon first resetDocument call', function() {
var i = false;
ReactEventListener.addDocumentEventListenerRemover(function() {
expect(i).toBe(false);
i = true;
});
var j = false;
ReactEventListener.addDocumentEventListenerRemover(function() {
expect(j).toBe(false);
j = true;
});
var k = false;
ReactEventListener.addDocumentEventListenerRemover(function() {
expect(k).toBe(false);
k = true;
});
ReactEventListener.resetDocument(document);
expect(i).toBe(true);
expect(j).toBe(true);
expect(k).toBe(true);
ReactEventListener.resetDocument(document);
ReactEventListener.resetDocument(document);
});
});
67 changes: 67 additions & 0 deletions src/renderers/dom/client/__tests__/ReactMount-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
var React;
var ReactDOM;
var ReactDOMServer;
var ReactEventListener;
var ReactMount;
var ReactTestUtils;
var WebComponents;
Expand All @@ -25,6 +26,7 @@ describe('ReactMount', function() {
React = require('React');
ReactDOM = require('ReactDOM');
ReactDOMServer = require('ReactDOMServer');
ReactEventListener = require('ReactEventListener');
ReactMount = require('ReactMount');
ReactTestUtils = require('ReactTestUtils');

Expand Down Expand Up @@ -278,6 +280,71 @@ describe('ReactMount', function() {
expect(Object.keys(ReactMount._instancesByReactRootID).length).toBe(1);
});

it('only calls ReactEventListener.resetDocument when without any roots', function() {
var mustResetNow = false;
var documentResetted = false;

ReactEventListener.setDocumentResetCallback(function() {
expect(mustResetNow).toBe(true);
documentResetted = true;
});

var container = document.createElement('div');
document.body.appendChild(container);
ReactDOM.render(<span />, container);

mustResetNow = true;
ReactDOM.unmountComponentAtNode(container);
expect(documentResetted).toBe(true);

// second time...
mustResetNow = false;
documentResetted=false;
ReactDOM.render(<span />, container);

mustResetNow = true;
ReactDOM.unmountComponentAtNode(container);
expect(documentResetted).toBe(true);
});

it('multiple containers only calls ReactEventListener.resetDocument when without any roots', function() {
var mustResetNow = false;
var documentResetted = false;
ReactEventListener.setDocumentResetCallback(function() {
expect(mustResetNow).toBe(true);
documentResetted = true;
});

var container1 = document.createElement('div');
document.body.appendChild(container1);
ReactDOM.render(<span />, container1);

var container2 = document.createElement('div');
document.body.appendChild(container2);
ReactDOM.render(<span />, container2);

ReactDOM.unmountComponentAtNode(container1);

mustResetNow = true;
ReactDOM.unmountComponentAtNode(container2);
expect(documentResetted).toBe(true);
});

it('only calls ReactEventListener.resetDocument when without any roots #2', function() {
var documentResetted = false;
ReactEventListener.setDocumentResetCallback(function() {
documentResetted=true;
});

var container = document.createElement('div');
document.body.appendChild(container);
ReactDOM.render(<div />, container);
ReactDOM.render(<span />, container);

ReactDOM.unmountComponentAtNode(container);
expect(documentResetted).toBe(true);
});

it('marks top-level mounts', function() {
var ReactFeatureFlags = require('ReactFeatureFlags');

Expand Down
7 changes: 6 additions & 1 deletion src/renderers/dom/shared/ReactDOMComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ var ReactDOMInput = require('ReactDOMInput');
var ReactDOMOption = require('ReactDOMOption');
var ReactDOMSelect = require('ReactDOMSelect');
var ReactDOMTextarea = require('ReactDOMTextarea');
var ReactEventListener = require('ReactEventListener');
var ReactInstrumentation = require('ReactInstrumentation');
var ReactMultiChild = require('ReactMultiChild');
var ReactServerRenderingTransaction = require('ReactServerRenderingTransaction');
Expand Down Expand Up @@ -224,7 +225,11 @@ function enqueuePutListener(inst, registrationName, listener, transaction) {
var containerInfo = inst._hostContainerInfo;
var isDocumentFragment = containerInfo._node && containerInfo._node.nodeType === DOC_FRAGMENT_TYPE;
var doc = isDocumentFragment ? containerInfo._node : containerInfo._ownerDocument;
listenTo(registrationName, doc);
var listenerRemover = listenTo(registrationName, doc, !isDocumentFragment);
if (!isDocumentFragment) {
ReactEventListener.addDocumentEventListenerRemover(listenerRemover);
}

transaction.getReactMountReady().enqueue(putListener, {
inst: inst,
registrationName: registrationName,
Expand Down