[FALSE POSITIVE][BUG]: Possible Memory leak due to not null `EventTarget#onclick()` property cause nodes to not be cleaned( DOM spec )

[skatcat31]

This leads to a slow memory leak as long as the person stays on the page if this is a memory leak.

This only applies to non permanently referenced nodes that are not re-used. Will be crawling code base to identify areas of problem.

[skatcat31]

Possible potential fixes:

• instead use the EventTarget.addEventListener(function, { once: true }) automatic removal strategy so that when cleaned up there will be no event listener on the deleted children
• upon removal crawl all and set onclick to null to remove

Places to investigate:

https://github.com/skatcat31/william-sonoma-challenge/blob/master/source/modules/slideShow.class.js#L100-L111

https://github.com/skatcat31/william-sonoma-challenge/blob/master/source/modules/slideShow.class.js#L139-L141

Possible methods of investigation:

• Direct heap traversal looking for orphan memory references to nodes
• Interval that expires after x * 1000 runs to see if the memory size grows after a garbage collection

[skatcat31]

Test interval code:

let intervalReference;
let iterations = 0;
let maxIterations = 10000;
let openClick = document.querySelector('#container > div > div:nth-child(3)');
let ping = true;
const test = () => {
  // keep track of iteration
  iterations++;

  if(ping){
    //open bad actor
    openClick.click();
  } else {
    //close bad actor
    let closeClick = document.querySelector('#slideshow > div');
    closeClick.click();
  }

  //break out of
  if (iterations >= maxIterations * 2){
    window.clearInterval(intervalReference);
    ping = true;
    console.log('finished iterations')
  }
}

intervalReference = window.setInterval(test)

[skatcat31]

Profile before:

heap: 5 246 216 Documents: 2 Nodes: 400 Listeners: 21

Profile after first run:

heap: 7 012 384 Documents: 2 Nodes: 40 639 Listeners: 23

Profile after first garbage collections:

heap: 8 178 416 Documents: 2 Nodes: 409 Listeners: 23

Profile after second run:

heap: 10 559 016 Documents: 2 Nodes: 12 641 Listeners: 23

Profile after second garbage collection:

heap: 10 771 672 Documents: 2 Nodes: 409 Listeners: 23

Profile after third run:

heap: 13 540 288 Documents: 2 nodes: 22 608 listeners: 23

Profile after third garbage collection:

heap: 13 478 120 documents: 2 nodes: 409 listeners: 23

It seems to be an extremely slow memory leak or not enough memory to cause the JS engine to collect orphans aggressively. According the spec this could be a leak caused by a not null eventhandler directly on the child, but I may have read it incorrectly. It is known that a registered event listener can cause the node to not be cleaned up.

This can possibly be fixed by setting the event handler on the image to null on change and on the modal to null on close

[skatcat31]

Local testing after implementing change to the hide method:

fix:

https://github.com/skatcat31/william-sonoma-challenge/blob/master/source/modules/slideShow.class.js#L141

      container.parentNode.replaceChild(newContainer, container);
      // clear event listeners
      const modal = container.querySelector('.modal');
      const img = container.querySelector('img');
      if (modal) modal.onclick = null;
      if (img) img.onclick = null;

Before:

heap: 7 861 344 documents: 3 nodes: 658 listeners: 25

First run:

heap: 8 970 632 documents: 3 nodes: 26 648 listeners: 27

First garbage collection:

heap: 8 219 128 documents: 3 nodes: 667 listeners: 27

Second run:

heap: 9 332 944 documents: 3 nodes: 39 239 listeners: 27

Second garbage collection

heap: 8 227 624 documents: 3 nodes: 667 listeners: 27

Third run:

heap: 8 777 272 documents: 3 nodes: 7 613(max 57 869) listeners: 27

Since this version seems to not climb in memory as fast and can climb back down close to initial levels instead of constantly climbing it seems the fix is valid and it was in fact a memory leak.

This possibly confirms the theory from above that this is a memory leak due to non null onclick event listeners.

[skatcat31]

Reminder: While this growth seems quick since it was visible in just three profiles, it was after 10,000 iterations that it grew a substantial amount. Had this possible leak not been noticed by @psyrendust or someone else deeply familiar with the DOM spec as it regards to Event Handlers, it would have possibly gone unnoticed for quite some time assuming human interaction speed. At 5ms of interaction speed(the fastest known limit of human nerves theorized as of 2018) it would take 8.33 hours for one iteration and a growth of ~2.5 MBs.

[skatcat31]

I will be doing one last confirmation of the leak against the current code and upon confirmation opening a PR referencing this issue.

[skatcat31]

Before:

heap:7 844 544 documents: 3 nodes: 664 listeners: 25

First run:

heap: 9 481 448 documents: 3 nodes: 25 589 listeners: 27

First garbage collection:

heap: 8 215 368 documents: 3 nodes: 672 listeners: 27

Second run:

heap: 10 058 848 documents: 3 nodes: 58 232 listeners: 27

Second garbage collection:

heap: 8 220 528 documents: 3 nodes: 673 listeners: 27

Third run:

heap: 10 235 624 documents: 3 nodes: 58983 listeners: 27

Third garbage collection:

heap: 8 220 632 documents: 3 nodes: 673 listeners: 27

These results do not confirm the first test run. Inspecting loaded sources it was confirmed it was the version the bug is suspected of being in. I will run another three after a fresh page load and see what happens.

[skatcat31]

Fresh browser instance reload:

before:

heap: 7 928 232 documents: 3 nodes: 658 listeners: 25

first run:

heap: 9 419 272 documents: 3 nodes: 28 479 listeners: 27

first garbage collection:

heap: 8 225 352 documents: 3 nodes: 667 listeners: 27

second run:

heap: 9 780 608 documents: 3 nodes: 24 123 listeners: 27

second garbage collection

heap: 8 229 280 documents: 3 nodes: 667 listeners: 27

third run

heap: 10 624 408 documents: 3 nodes: 58270 listeners: 27

third garbage collection:

heap: 8 229 424 documents: 3 nodes: 667 listeners: 27

Again the results seem somewhat flat. I'm curious what happened in the first test that gave such outrageous numbers. Anomaly detected?

[skatcat31]

I will be running the current master 100K times to see what sort of results I receive. Since this will take some time I will post the results in another comment.

[skatcat31]

100K:

heap: 12 090 872 documents: 3 nodes: 50 732 listeners: 27

After garbage collection:

heap: 8 224 792 documents: 3 nodes: 667 listeners: 27

I am uncertain this causes a leak anymore. I will be re-reading the spec for event handler properties

[skatcat31]

relevant spec:

https://www.w3.org/TR/html50/webappapis.html#event-handler-attributes

Upon re-reading I cannot see how this spec would cause a memory leak since it does not add it to the event listener list.

I have also read the https://dom.spec.whatwg.org/#dom-eventtarget-addeventlistener spec and this one seems more likely to cause a memory leak by keeping a reference to the event target in the listener list and preventing cleanup. I will run a test with this method of registering the event listener and see what happens.

[skatcat31]

Even after moving to the EventTarget#addEventListener() method of registering an event I cannot seem to create a growing memory footprint even after 100K iterations:

https://github.com/skatcat31/william-sonoma-challenge/blob/master/source/modules/slideShow.class.js#L100-L111

      // modal.onclick = this.hide;
      modal.addEventListener('click', this.hide);

      // image.onclick = this.next;
      image.addEventListener('click', this.next);

after 100K:

heap: 8 050 096 documents: 1 nodes: 109 791 listeners: 23

garbage collected:

heap: 5 104 984 documents: 1 nodes: 259 listeners: 23

I can only conclude that garbage collection techniques have changed recently and that neither method would lead to a leak.

The changes to the document numbers and the heap discrepancies can be looked at as individual testing anomalies as none of the tests runs(except the first) have created an upward trend in heap or memory profile with iterations in the tens of thousands to hundreds of thousands.

[skatcat31]

I can only guess that the garbage collector has been updated in Chrome to be able to clean up EventTarget instances that are the only reference to the event listener list or itself through it's event listener list.

[skatcat31]

I think I'm going to leave this issue open so that it can be seen that this possible issue has been investigated by other potential viewers.