-
Notifications
You must be signed in to change notification settings - Fork 417
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
[memory leak] window.autoNumericGlobalList #788
Comments
For now, I have solved it with a simple MutationObserver on the respective form element. if (!window.autoNumericGlobalListObserver) {
const nuke = (removedNode) => {
if (window.autoNumericGlobalList.has(removedNode)) {
window.autoNumericGlobalList.delete(removedNode);
}
};
window.autoNumericGlobalListObserver = new MutationObserver(function(records, observer) {
for (const record of records) {
for (const removedNode of record.removedNodes) {
if (removedNode.nodeName == "INPUT") {
nuke(removedNode);
} else {
removedNode.querySelectorAll("input").forEach(nuke);
}
}
}
});
}
// inside the script
window.autoNumericGlobalListObserver.observe(FORMELEMENT, {
childList: true,
subtree: true
}); |
Weakmap are supposed to garbage collect automatically. I'm not sure why this does not work in that case. Have you tried keeping a reference to your AutoNumeric elements you need to remove, and use the |
Sadly, there is no I kept Camunda Tasklist open (without the the cleanup observer) and after ~24hrs the WeakMap was still filled. Using a reference list like suggest, would only work, if I check the list with intervals and test for the Otherwise I can't keep the references that easily, as the form is implemented inside/uploaded into Camunda. Like select one from multiple tasks, complete the form, do another task... and the user can switch easily between forms without completing them directly. So, if the user moves away (selects a different task) without completing the form and there is no signal/event fired like Just to give you an idea of what Camunda is: On the right side there are plain HTML Form blocks and script-elements. The scripts get executed each time, the form gets attached to the DOM (or in other words every time, the user clicks on the 2nd list and selects a task). |
I'm also integrating AutoNumeric currently (a different system) and just saw this issue. Since performance is also an issue for us, I quickly tried this as well. Seems to be working as expected though, even without I changed the code from the CodePen slightly so it does not use
Afaik, this is all the expected behavior. for (let i = 0; i < 30001; i++) {
const input = document.createElement("input");
mmm.append(input);
const an = new AutoNumeric(input);
an.node().remove();
} |
@anotherCoward maybe the bug you are experiencing is more on Camunda' side, with a reference being kept to the data you want garbage-collected? |
Camunda is not refreshing the site, when the forms get submitted or when other forms are loaded. It just loads the HTML, attaches them, parses the scripts and executes them inside an Angular scope. So if You don't keep track of items, that are no longer attached to the DOM inside that list. I simply removed the container from the DOM using the DEV tools, similar to what Camunda does, cleared the console, looked at |
Hmm, I did a bit more testing and found 3 issues with AutoNumeric where it keeps a reference to DOM elements. The weak map definitely isn't the issue though, it works as it should. Global document listenerIssue AutoNumeric adds a global listener to the document. The listener is an anonymous lambda function with a closure that captures the autonumeric-mem-leak-1-document-listener.webmIn the video above, I removed the div container with all auto numeric inputs. The global event listener only references the first input element -- at least directly. Since all nodes in a DOM tree are connected via Suggestion onKeydownGlobal does not seem to actually need the instance, it only accesses the static method Why this did not happen before when I tested with codepen As can be seen in the code from my post above, I removed the input element directly after initializing auto numeric, instead of removing a container later. Since I removed each input element directly, these input do not reference each other anymore (their Only the first input that is captured by the global event listener keeps being reachable. And indeed, when I try this again on codepen, every entry from the weak map is cleared, except for one. (I think I remember the map getting cleared completely when I tested this yesterday, but either I misremember or there was another difference.) Global form listenerIssue Same as above, but AutoNumeric also adds listener to the form element, if present. autonumeric-mem-leak-3-form-listener.webmSuggestion Same as above, it seems _onFormSubmit does not need to be an instance method. Why this did not happen before when I tested with codepen My example did not have a Window-scoped form handler mapIssue Also similar to the above. AutoNumeric defines a global aNFormHandlerMap property on the window. This is a map with values that also reference the autonumeric-mem-leak-2-form-handler-map.webmSuggestion Same as above, should be solved by ensuring that the global form event listeners do not leak references. Why this did not happen before when I tested with codepen My example did not have a
Just to make sure there's no confusion: it only gets cleared when the JS engine runs a garbage collection cycle, which can be whenever it feels like doing so. When I tested with Chrome, it did when the heap size (RAM usage) got high enough, several hundred MB; or when I triggered a GC cycle manually via dev tools -> memory. Sometimes I had to close the dev tools and reopen, as it still contained some reference even though I cleared the console. Also note that the code pen linked above declares a variable PS: While testing with one of our apps, I think I also found a few "leaks" unrelated to auto numeric, it's really easy to create those. You can use the memory tab of the Chrome dev tools to take a heap snapshot, which shows the retainers of an object (i.e. what still references them). class AutoNumericTag {}
const Leak = []; // simulate a leak
const an = new AutoNumeric(...);
Leak.push(an);
// Add this to the instance, now you can search the heap snapshot for "AutoNumericTag"
// to quickly find the auto numeric instance in the snapshot
an.__tag = new AutoNumericTag() |
Current behavior
Inside Camunda (BPMN Software) a form is generated depending on the users selection.
Every time the user picks something, the input elements are generated by a script. After it was attached to the form,
new AutoNumeric(el)
is used on every of these dynamical generated input elements that require formating/decimal places and so on. There are ~300 input elements per selection in some cases.After the user submitted the form, the references are still present in
window.autoNumericGlobalList
.If the user fills multiple forms during one session, it can happen that the browser tab runs out of memory.
Expected behavior
Make the garbage collectable. WeakMap has no
.clear()
method anymore.Steps to reproduce the problem
Example
The text was updated successfully, but these errors were encountered: