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

[editable] Fix Memory Leak when switching between focused editables #5542

Merged
merged 3 commits into from
Oct 31, 2023

Conversation

hellsan631
Copy link
Contributor

@hellsan631 hellsan631 commented Oct 28, 2023

Description
I've found a memory leak in slate-react where-in when you'd switch between two pages, each with their own editor instance, auto-focusing on these elements causes a memory leak, where there would be a lot of Detached HTML Element's just floating around.

At first I thought it a bunch of different bugs with chromium, but noticed that when I force removed the input elements from the dom before the component would unmount, we would still see small leak referencing "latestElement" (while simultaneously, I would still the element & children references in the IS_FOCUSED WeakMap) Looking at editable's code, I realized that we're storing state in a weird way, directly mutating it using useMemo, and React isn't removing all references (probably because its still stored in the WeakMap.).

The simple fix for this is what I've commited; using useLayoutEffect, we forcably remove the latestElement and references to it, before react unmounts the component.

Example
(Both of these examples were run using memlab, in an isolated test environment)
Before (Has Big Dom Leak)
https://github.com/ianstormtaylor/slate/assets/1474758/1ac76419-c48d-48ff-a759-994c268ea72f

After (No Memory Leak)
https://github.com/ianstormtaylor/slate/assets/1474758/e0a3961c-18cb-42d2-93a5-b01edbb3e52c

Context
We're using an older version of slate (0.91), but even when doing an upgrade in an isolated branch, we still see this issue. We're also using the most recent version of React (18.2.0).

Checks

  • The new code matches the existing patterns and styles.
  • The tests pass with yarn test.
  • The linter passes with yarn lint. (Fix errors with yarn fix.)
  • The relevant examples still work. (Run examples with yarn start.)
  • You've added a changeset if changing functionality. (Add one with yarn changeset add.)

…ntainers

I've found a memory leak in Slate React where-in when you'd switch between two pages, each with their own editor instance, auto-focusing on these elements causes a memory leak, where there would be a lot of Detached HTML Element's just floating around.

At first I thought it a bunch of different bugs with chromium, but noticed that when I force removed the input elements from the dom before the component would unmount, we would still see small leak referencing "latestElement" (while simultaniously, I would still the element & children references in the `IN_FOCUSE` WeakMap) Looking at editable's code, I realized that we're storing state in a weird way, directly mutating it using `useMemo`, and React isn't removing all references (probably because its still stored in the WeakMap.

The simple fix for this is what I've commited; using `useLayoutEffect`, we forcably remove the `latestElement` and references to it.
@changeset-bot
Copy link

changeset-bot bot commented Oct 28, 2023

🦋 Changeset detected

Latest commit: 40aa9d4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
slate-react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Collaborator

@dylans dylans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a simple fix with great results. I'll land and release within the next day or two.

if you have time, please add a changeset, otherwise I will. Thanks!

@hellsan631
Copy link
Contributor Author

ty for the changeset! 🙏

@dylans dylans merged commit 8688ed5 into ianstormtaylor:main Oct 31, 2023
11 checks passed
This was referenced Oct 31, 2023
@dylans
Copy link
Collaborator

dylans commented Nov 6, 2023

This one has introduced some regressions for us.

My colleague's thinking:

"If state goes, so should the reference and the element will be garbage collected.

State is updated in a weird way, mutating memo is certainly not something that should be done like this. Perhaps using memo in such a way is the thing that is introducing the memory leak, because it don't think it gets cleared deterministically, but remove shouldn't be called

It feels like the issue is useMemo."

@philicious
Copy link

@dylans this also breaks our implementation

  useEffect(() => {
    editor.children = value
    editor.onChange()

    ReactEditor.focus(editor) // this line now causes the editor to blank out!! if removed, its working but editor doesnt get initial focus
  }, [value, editor])
...
    <Slate
      editor={editor}
      initialValue={[]}
    >

@floklein
Copy link

floklein commented Nov 9, 2023

Crash for us too on unmount.

Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.

I recreated our setup: https://stackblitz.com/edit/react-egzyvh?file=src%2FApp.js
Created an issue: #5553

@dylans
Copy link
Collaborator

dylans commented Nov 10, 2023

Reverting per the above discussion.

dylans added a commit that referenced this pull request Nov 10, 2023
Revert due to issues introduced.
@dylans dylans mentioned this pull request Nov 10, 2023
dylans added a commit that referenced this pull request Nov 10, 2023
* Revert #5542

Revert due to issues introduced.

* Add changeset
@github-actions github-actions bot mentioned this pull request Nov 10, 2023
@philicious
Copy link

@dylans for what its worth: I tried 0.101.0 which claims to revert this #5542 but it still crashes for us on ReactEditor.focus(editor) with code seen in #5542 (comment) so I assume 0.101.0 does sth else thats also problematic

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

Successfully merging this pull request may close these issues.

4 participants