[Bug]: Memory leak when discarding used editors

[schontz]

Affected Packages

pm, react, starter-kit, extension-color, extension-list-item, extension-list-style

Version(s)

2.7.2

Bug Description

I have observed Editor memory leaks when the editor instance is re-created. I create a repro that simply uses the demo app.

Browser Used

Chrome

Code Example URL

https://github.com/schontz/tiptap-react-leak

Expected Behavior

There should only be a single editor in memory, while instead there are many.

Additional Context (Optional)

Dependency Updates

• Yes, I've updated all my dependencies.

[nperez0111]

Would this tell you where the memory is still being referenced from?

It is detached from the DOM so I'm not terribly concerned to be honest

[schontz]

For our application, we are seeing memory balloon over time. As soon as we stopped the editor from being re-created, memory stabilized. We have many more extensions that take up a decent amount of memory, so the editor not going away leads to huge memory consumption over time. Our application is not reloaded for weeks at a time.

[schontz]

It should be noted that detached DOM nodes in a memory snapshot are ones that are referenced and cannot be cleaned up by the garbage collector. GC is forced before each snapshot.

[nperez0111]

Alright, well then this will need some research into what is actually keeping these references around. Unfortunately, a pretty broad surface area to look into.

I would be willing to take a patch that resolves this

[schontz]

The problem is here:

tiptap/packages/core/src/Editor.ts

Lines 371 to 376 in f76515a

	// Let’s store the editor instance in the DOM element.
	// So we’ll have access to it for tests.
	// @ts-ignore
	const dom = this.view.dom as TiptapEditorHTMLElement
	dom.editor = this

There is a circular reference between the editor and the DOM node. How this gets exercised actually has nothing to do with editing. Rather, it is something along these lines:

1. Editor is created in place
2. Put cursor in editor. The text node in that DOM will be used here
3. Click the count button, which causes the editor to be replaced.
4. The PM code runs before the actual replacement, so trackWrites is pointing to a text node that is part of the old editor root.
5. The old editor gets destroyed, but that text node still points to the root. The root had root.editor = editor, so it never lets it go.

The problem isn't just with trackWrites, though. I changed it to a WeakRef, but that just moved the leak to the command manager. Plugging that moved it to extension manager, etc., etc. The real issue is that a DOM node should never have a reference to the editor. I commented out the reference and the leaks vanished.

It looks like the ref is only there for testing, which means you should only run that code during testing. If you truly need a reference, it should be done via a data attribute to look things up. Something along the lines of:

// Expose this to testing env
Editor.aliveEditors = new Map<string, Editor>()

// in Editor constructor:
this.id = nextEditorId++
Editor.aliveEditors.set(this.id, this)

// in Editor destroy:
Editor.aliveEditors.delete(this.id)

[nperez0111]

What if we remove this reference on destroy?

[schontz]

Tested and that works. I suggest assigning and destroying in EditorView instead of Editor, where it is currently assigned. The EditorView is in ProseMirror, so the Editor needs to take care of it.

[schontz]

I posted a CR that only runs the code in Cypress. I can't get tests to run, so you'll need to verify that they work. The alternative is to deref on destroy, but if any errors occur that prevent a clean destroy, the leak would persist. As the code is test-only, it is best if it just runs there.

[nperez0111]

I do prefer just de-referencing it in the destroy method because I personally have found it quite useful to inspect the editor and issue commands from the dom element. This also could be relied upon by users.

The PR you made was into your own repo, but I'll make one into here

[nperez0111]

I released this with v2.7.4