Skip to content
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

Open
anotherCoward opened this issue Jun 3, 2024 · 7 comments
Open

[memory leak] window.autoNumericGlobalList #788

anotherCoward opened this issue Jun 3, 2024 · 7 comments

Comments

@anotherCoward
Copy link

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.

image

Expected behavior

Make the garbage collectable. WeakMap has no .clear() method anymore.

Steps to reproduce the problem

Example

@anotherCoward
Copy link
Author

anotherCoward commented Jun 3, 2024

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
  });

@AlexandreBonneau
Copy link
Member

AlexandreBonneau commented Jun 3, 2024

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 .nuke() function?
If you do, then there are no problems as seen here.

@anotherCoward
Copy link
Author

anotherCoward commented Jun 4, 2024

Sadly, there is no disconnectedCallback for default elements.

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 isConnected state. If false, "nuke" it. When empty, stop the interval and start it again if the respective form is displayed again. This would be the same result as the MutationObserver I'm using right now.

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 onbeforeleave.

Just to give you an idea of what Camunda is:
image
(example from: https://docs.camunda.org/manual/7.21/webapps/tasklist/)

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).

@blutorange
Copy link
Contributor

blutorange commented Aug 30, 2024

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 nuke. Just a few notes in case it helps anybody:

I changed the code from the CodePen slightly so it does not use nuke, but just removes the element from the DOM.
Then on Chrome, I observe the following behavior.

  • It fills the map, the heap grows to about 200 MB
  • Nothing gets cleared just by waiting.
    • Presumably because it does not run a garbage collection cycle as it does not consider it large enough?
  • Nothing gets cleared when I go to the dev tools memory tab and trigger a garbage collection cycle.
    • This is because the console tab keeps a reference to the element -- by virtue of having it logged to the console.
  • After I clear the console and trigger a garbage collection cycle again, the heap shrinks back ~10-20MB and the weak map gets cleared.

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();
}

@AlexandreBonneau
Copy link
Member

@anotherCoward maybe the bug you are experiencing is more on Camunda' side, with a reference being kept to the data you want garbage-collected?

@anotherCoward
Copy link
Author

anotherCoward commented Aug 30, 2024

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 autoNumericGlobalList is created by one of the forms, it won't be cleared after a form was submitted or a different form is loaded. Only when the user refreshes the page manually.

You don't keep track of items, that are no longer attached to the DOM inside that list.

image

I simply removed the container from the DOM using the DEV tools, similar to what Camunda does, cleared the console, looked at autoNumericGlobalList and it still has 1000 references to elements that are no longer attached to the DOM.

EDIT:
image

@blutorange
Copy link
Contributor

blutorange commented Aug 31, 2024

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 listener

Issue

AutoNumeric adds a global listener to the document. The listener is an anonymous lambda function with a closure that captures the this context of the auto numeric instance, and the instance contains a reference to the node.

autonumeric-mem-leak-1-document-listener.webm

In 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 parentNode and children, te first input element actually references all other input elements indirectly as well.

Suggestion

onKeydownGlobal does not seem to actually need the instance, it only accesses the static method this.constructor._unformatAltHovered, which can be done more idiomatically via AutoNumeric._unformatAltHovered. onKeydownGlobal could be a static method. (Instead of static methods, I prefer to just write a function in the same file, but outside the class).

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 parentNode is nulled). So they are not reachable and available for garbage collection.

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 listener

Issue

Same as above, but AutoNumeric also adds listener to the form element, if present.

autonumeric-mem-leak-3-form-listener.webm

Suggestion

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 form element, so no listener was registered.

Window-scoped form handler map

Issue

Also similar to the above. AutoNumeric defines a global aNFormHandlerMap property on the window. This is a map with values that also reference the _onFormSubmitFunc from above, which in turn captures an auto numeric instance, which references a DOM node.

autonumeric-mem-leak-2-form-handler-map.webm

Suggestion

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 form element, so no listener was registered.


I simply removed the container from the DOM using the DEV tools, similar to what Camunda does, cleared the console, looked at autoNumericGlobalList and it still has 1000 references to elements that are no longer attached to the DOM.

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 anFoo, which is actually a global variable and therefore also establishes a reference (so the map won't get cleared).

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()

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants