-
Notifications
You must be signed in to change notification settings - Fork 46.2k
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.
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
Clean up top-level event listeners after unmounting all roots #7128
Comments
I think we don’t want to provide a new API for this.
This sounds sensible. (Aside from the new API bit—this should be an implementation detail.) |
I havent tried a poc, the furthest i got was simply grepping for the abstraction (sorry cant remember the name just now) calls and noticed that many times after "adding" a listener for some event, the returned function to "remove" is never stored anywhere. The "add" a listener but never store the remover happens in a few scattered places. My check was rather quick and i didnt try to a poc in anyway. |
It would be similar to what if (listeners) {
for (var i = 0; i < listeners.length; i++) {
listeners[i].remove();
}
} The event system is pretty complex though, it might not be that easy. |
In addition to what @aweary describes, ive also had to 'reset' the document as it stashes some state under its unique 'react' key. Without the document reset thing react remains broken after the last unmount, because some state thinks its already added event listeners, that have now been removed during the unmount of the last component. Im using the simple examples but react seems to correctly remove all global document event listeners (during the last comp unmount), and add them back when a new component is rendered and mounted. Hope to do a bit more work soon enuff. |
I think the right solution to this is to attach event listeners per root. |
If you're still following this, could you elaborate on what you mean by resetting the document? I have a bunch of hanging memory leaks for Do you mean clearing the innerHTML of the document or what? |
Sorry for the generic use of the word "reset". Reset has nothing to do with changing any HTML or styling properties on the document object or anything else, reset if I recall means to clear or empty "the only" react state hash added to the document object. I cant recall the name but it may very well be the same HTH. |
In React 17 we're attaching events per root which should fix this. |
If you want to try it: |
Do you want to request a feature or report a bug?
Bug - maybe intended behaviour.
What is the current behavior?
Background
I have an app that needs to be embedded by other apps (other customers). The idea being "our" react app has its javascript loaded in an iframe, but the "main" window hosts dom elements from the customers and our react app. That bit works fine. As time goes on "our" react UI is no longer needed, and then react root is removed, and the iframe destroyed. These apps are often long lived so there will be times when the react app needs to appear again, and the iframe is recreated and everything reloaded. This can and will happen many times.
Goal
We would like to NOT keep the iframe around when its not actually needed, but rather re-create just in time when it is needed. This app is used by customers and they would like to embed our "react" app, without interference with their "app" and all its javascript, which is why we are doing the iframe thing.
Problem
It is evident by watching the chrome dev tools "timeline" memory graph that memory always increases each time a new iframe is created and the react UI is init'd. Unmounting and destroying the iframe, never causes the memory to drop to "near" original before load value. Repeating this process multiple times slowly show an increase memory.
This also causes a more immediate problem, in that react is throwing exceptions on every event (click, type etc) because the window of the iframe is now null.
Proof: First symptom - Event exceptions (only happens in my app)
These exceptions only happen in my (cant share) app, i cant repo them, but parts of this apply to all react apps. Please read thru - it will all make sense when you get to the end and if you examine my poc.
Destroying the Iframe, leaves React and its event dispatching system in memory. I have a mixture of x-tag, webcomponents which are used to "create" the iframe and load the react app. After the custom element is used (lets call it ), the console starts showing exceptions all within react code. This is a side effect of the react dispatchEvent still being active and trying to do stuff.
I know about
ReactEventListener.dispatchEvent
(snip below) where i can disable react( i havent actually tried) to avoid the exceptions, but that would leave the memory leak.https://github.com/facebook/react/blob/master/src/renderers/dom/client/ReactEventListener.js#158
Its rather easy to prove that react remains in memory, simply goto the compiled app, find the
React dispatchEvent
and insert a console.log and watch as it continues to "print" stuff after unmounting the last component, even though there are no listeners. In my case the exception is caused because allextractEvents
eventually default to "window" as the "target".There are multiple copies of the same basic idea in various react functions, where it tries to get a target that it assumes will never be null. If one doesnt load react in an iframe, then window is always defined.
Later the
shouldUseChangeEvent
tries to read the nodeName of the now "undefined" window, because its iframe has been destroyed, but that now results in an exception (null pointer etc).react/src/renderers/dom/client/eventPlugins/ChangeEventPlugin.js
Line 72 in 045f1a7
If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via https://jsfiddle.net or similar (template: https://jsfiddle.net/reactjs/69z2wepo/).
What is the expected behavior?
There are probably two possible solutions, that work in tandem.
Firstly React should provide an API that will remove all its global event listeners. Naturally it could complain if there are any active components that remain mounted. This API may be internal/private (not public), if [docs] Fix button links on bottom of home #2 was implemented. It might be called something like
React.shutdownAll
Because everything is gone, the next React render would setup all its globals again.React should dispose of all its global event handlers when the last or "root" component is unmounted. This would call the new api mentioned in 1.
Either option solves my problem, where i wish to either let react shutdown gracefully. With this in mind i could.
React.shutdownAll
then this step is skipped.Proof #2
Goto your compiled out, locate the
dispatchEvent
and add a console.log, notice even after the last / root container is unmounted stuff will continue to be printed because the event listeners are still active.I did a very quick scan of the abstraction around adding listeners, and i couldnt see the remove function being stored and then called to cleanup.
Proof #3
Look at my last section below where i have a proof of concept form of the popular todomvc react example.
Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
React 15.0.2
React-Dom 15.0.2
React-redux 4.4.5 (might be useful to know)
Reproducable use case
Sorry i tried but decided that using the facebook jsfiddle wasnt really a smart thing for the following reasons.
I have forked the popular todomvc app and added a few minor edits to recreate, reload, render+unmount x100, destroy everything about the app, and try again in a loop separated by a sleep.
Hopefully we can trust the todomvc guys are doing the right thing, no dumb memory leaks. If you examine it should be obvious the only thing im adding is support for my horrible create app, run app, render+unmount many times, render, unmount, sleep a bit and then loop again until counter exhausted.
Sorry if this is boring but as a convenience i will list the basic instructions to "run" the react version of my branch on your local machines...
4A. navigate to http://localhost:8000/examples/react/index.html
4B. navigate to http://localhost:8000/examples/react/index3.html
// /examples/react corresponds to the dist/examples/react directory that gulp built into.
My poc supports 3 concepts.
If you look at my p/r against todomvc you will see many helpful pictures with memory leak graphs from chrome dev tools for each of the 3 described scenarios and some commentary.
The text was updated successfully, but these errors were encountered: