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.

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

Jump to bottom

Memory leak in Shortcuts class #2631

Open
melenudo opened this issue Feb 21, 2024 · 6 comments
Open

Memory leak in Shortcuts class #2631

melenudo opened this issue Feb 21, 2024 · 6 comments
Labels
good first issue

Comments

@melenudo
Copy link

melenudo commented Feb 21, 2024 •
edited
Loading

Editor.js Version

v2.29.0

Issue description

Hi!

I've found a memory leak in the Shortcuts class. When you remove the shortcut for an element you don't check if the element's shortcuts are empty and left the element in the registeredShortcuts map (with an empty array):

editor.js/src/components/utils/shortcuts.ts

Line 89 in b619946

this.registeredShortcuts.set(element, shortcuts.filter(el => el !== shortcut));

If you create a new EditorJS and destroys it, you will see a bunch of detached elements referenced by registeredShortcuts map.

Steps to reproduce:

You can reproduce the issue with this simple html:

<html>
  <body>
    <button id="remove">Remove</button>
    <div id="editor" style="min-width: 700px"></div>
    <script src="https://cdn.jsdelivr.net/npm/@editorjs/editorjs@latest"></script>
    <script>
      let editor = new EditorJS({
        holder: 'editor',
      });

      function remove() {
        document.querySelector('#editor').remove();
        editor.isReady.then(() => {
          editor.destroy();
          editor = undefined;
        });
      }
      document.querySelector('#remove').addEventListener('click', remove);
    </script>
  </body>
</html>

Press "Remove" button and take a snapshot in Chrome dev tools. You will see detached HTMLDivElement's referenced by registeredShortcuts:

screenshot

If you remove the element from the map when the shortcuts array is empty, the problem disappears:

const newShortcuts = shortcuts.filter(el => el !== shortcut);

newShortcuts.length===0?
  this.registeredShortcuts.delete(element):
  this.registeredShortcuts.set(element, newShortcuts);

screenshot2
@obafemitayor
Copy link
Contributor

Hi, I am a first time contributor. Is it ok if I take a look at this?

@neSpecc
Copy link
Member

neSpecc commented Dec 17, 2024

Hi, I am a first time contributor. Is it ok if I take a look at this?

Absolutely

@obafemitayor
Copy link
Contributor

👀

@obafemitayor obafemitayor mentioned this issue Dec 21, 2024
Merged
@obafemitayor
Copy link
Contributor

@neSpecc I have created a PR to fix this issue
can you help take a look when you have the chance?
#2893

@obafemitayor
Copy link
Contributor

Hello @neSpecc, have you had some time to review my latest changes on this PR (#2893)?

@obafemitayor
Copy link
Contributor

@neSpecc As this PR is approved, do you know when it will be merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@melenudo @neSpecc @obafemitayor