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

⚡️ Fix ResizeObserver memory leak #66

Merged
merged 1 commit into from
May 27, 2021
Merged

Conversation

alecgibson
Copy link
Collaborator

Fixes #61

At the moment, the ResizeObserver causes a memory leak, because we
never unobserve the Quill container, which then keeps a reference to
that node in memory (even if the node is disconnected from the DOM).

This change tweaks our ResizeObserver callback to check if the Quill
container is still connected to the DOM. If not, we unobserve, which
allows the ResizeObserver and DOM node to both be correctly garbage-
collected.

However, we can't just leave it there, because consumers may potentially
disconnect their Quill instance, and then reconnect it to the DOM. In
order to address this edge case, we re-register the ResizeObserver
whenever an update is made to a cursor, which should start listening
correctly the next time any cursor is updated.

Note that there's still a corner case:

  1. Consumer disconnects Quill from the DOM
  2. ResizeObserver stops observing
  3. Consumer reconnects Quill to the DOM
  4. User resizes window (before any updates are made)
  5. Cursors aren't correctly redrawn because the ResizeObserver hasn't
    been re-registered

However, this case should be pretty unusual, and is hard to remedy
without even more observers, so let's leave it there. Consumers can
manually fire cursors.update() when reconnecting Quill to the DOM to
correctly re-register the ResizeObserver.

Fixes #61

At the moment, the `ResizeObserver` causes a memory leak, because we
never unobserve the Quill container, which then keeps a reference to
that node in memory (even if the node is disconnected from the DOM).

This change tweaks our `ResizeObserver` callback to check if the Quill
container is still connected to the DOM. If not, we unobserve, which
allows the `ResizeObserver` and DOM node to both be correctly garbage-
collected.

However, we can't just leave it there, because consumers may potentially
disconnect their Quill instance, and then reconnect it to the DOM. In
order to address this edge case, we re-register the `ResizeObserver`
whenever an update is made to a cursor, which should start listening
correctly the next time any cursor is updated.

Note that there's still a corner case:

 1. Consumer disconnects Quill from the DOM
 2. `ResizeObserver` stops observing
 3. Consumer reconnects Quill to the DOM
 4. User resizes window (before any updates are made)
 5. Cursors aren't correctly redrawn because the `ResizeObserver` hasn't
    been re-registered

However, this case should be pretty unusual, and is hard to remedy
without even more observers, so let's leave it there. Consumers can
manually fire `cursors.update()` when reconnecting Quill to the DOM to
correctly re-register the `ResizeObserver`.
Copy link

@ricardoferrolho ricardoferrolho left a comment

Choose a reason for hiding this comment

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

👍

@alecgibson alecgibson merged commit 1a0db90 into master May 27, 2021
@alecgibson alecgibson deleted the resize-observer-leak branch May 27, 2021 07:06
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.

Quill-cursors has a memory leak due to missed cleanup for global listeners
2 participants