Fix memory leak with non-disposed listeners

Fixes VIM-3319

src/main/java/com/maddyhome/idea/vim/listener/VimListenerManager.kt

@@ -43,6 +43,7 @@ import com.intellij.openapi.fileEditor.ex.FileEditorManagerEx
 import com.intellij.openapi.fileEditor.ex.FileEditorWithProvider
 import com.intellij.openapi.fileEditor.impl.EditorComposite
 import com.intellij.openapi.fileEditor.impl.EditorWindow
+import com.intellij.openapi.observable.util.addKeyListener
 import com.intellij.openapi.project.ProjectManager
 import com.intellij.openapi.util.Disposer
 import com.intellij.openapi.util.Key
@@ -102,7 +103,6 @@ import com.maddyhome.idea.vim.ui.widgets.macro.MacroWidgetListener
 import com.maddyhome.idea.vim.ui.widgets.macro.macroWidgetOptionListener
 import com.maddyhome.idea.vim.ui.widgets.mode.listeners.ModeWidgetListener
 import com.maddyhome.idea.vim.ui.widgets.mode.modeWidgetOptionListener
-import com.maddyhome.idea.vim.vimDisposable
 import java.awt.event.MouseAdapter
 import java.awt.event.MouseEvent
 import javax.swing.SwingUtilities
@@ -276,49 +276,52 @@ internal object VimListenerManager {
       // TODO: If the user changes the 'ideavimsupport' option, existing editors won't be initialised
       if (vimDisabled(editor)) return
 
-      // As I understand, there is no need to pass a disposable that also disposes on editor close
-      //   because all editor resources will be garbage collected anyway on editor close
-      // Note that this uses the plugin's main disposable, rather than VimPlugin.onOffDisposable, because we don't need
-      // to - we explicitly call VimListenerManager.removeAll from VimPlugin.turnOffPlugin, and this disposes each
-      // editor's disposable individually.
-      val disposable = editor.project?.vimDisposable ?: return
-
       // Protect against double initialisation
       if (editor.getUserData(editorListenersDisposableKey) != null) {
         return
       }
 
-      val listenersDisposable = Disposer.newDisposable(disposable)
-      editor.putUserData(editorListenersDisposableKey, listenersDisposable)
-
-      Disposer.register(listenersDisposable) {
+      // Make sure we explicitly dispose this per-editor disposable!
+      // Because the listeners are registered with a parent disposable, they add child disposables that have to call a
+      // method on the editor to remove the listener. This means the disposable contains a reference to the editor (even
+      // if the listener handler is a singleton that doesn't hold a reference).
+      // Unless the per-editor disposable is disposed, all of these disposables sit in the disposer tree until the
+      // parent disposable is disposed, which will mean we leak editor instances.
+      // The per-editor disposable is explicitly disposed when the editor is released, and disposed via its parent when
+      // the plugin's on/off functionality is toggled, and so also when the plugin is disabled/unloaded by the platform.
+      // It doesn't matter if we explicitly remove all listeners before disposing onOffDisposable, as that will remove
+      // the per-editor disposable from the disposer tree.
+      val perEditorDisposable = Disposer.newDisposable(VimPlugin.getInstance().onOffDisposable)
+      editor.putUserData(editorListenersDisposableKey, perEditorDisposable)
+
+      Disposer.register(perEditorDisposable) {
         if (VimListenerTestObject.enabled) {
           VimListenerTestObject.disposedCounter += 1
         }
       }
 
-      editor.contentComponent.addKeyListener(VimKeyListener)
-      Disposer.register(listenersDisposable) { editor.contentComponent.removeKeyListener(VimKeyListener) }
+      // This listener and several below add a reference to the editor to the disposer tree
+      editor.contentComponent.addKeyListener(perEditorDisposable, VimKeyListener)
 
       // Initialise the local options. We MUST do this before anything has the chance to query options
       val vimEditor = editor.vim
       VimPlugin.getOptionGroup().initialiseLocalOptions(vimEditor, openingEditor, scenario)
 
       val eventFacade = EventFacade.getInstance()
-      eventFacade.addEditorMouseListener(editor, EditorMouseHandler, listenersDisposable)
-      eventFacade.addEditorMouseMotionListener(editor, EditorMouseHandler, listenersDisposable)
-      eventFacade.addEditorSelectionListener(editor, EditorSelectionHandler, listenersDisposable)
-      eventFacade.addComponentMouseListener(editor.contentComponent, ComponentMouseListener, listenersDisposable)
-      eventFacade.addCaretListener(editor, EditorCaretHandler, listenersDisposable)
+      eventFacade.addEditorMouseListener(editor, EditorMouseHandler, perEditorDisposable)
+      eventFacade.addEditorMouseMotionListener(editor, EditorMouseHandler, perEditorDisposable)
+      eventFacade.addEditorSelectionListener(editor, EditorSelectionHandler, perEditorDisposable)
+      eventFacade.addComponentMouseListener(editor.contentComponent, ComponentMouseListener, perEditorDisposable)
+      eventFacade.addCaretListener(editor, EditorCaretHandler, perEditorDisposable)
 
       VimPlugin.getEditor().editorCreated(editor)
-      VimPlugin.getChange().editorCreated(editor, listenersDisposable)
+      VimPlugin.getChange().editorCreated(editor, perEditorDisposable)
 
-      (editor as EditorEx).addFocusListener(VimFocusListener, listenersDisposable)
+      (editor as EditorEx).addFocusListener(VimFocusListener, perEditorDisposable)
 
       injector.listenersNotifier.notifyEditorCreated(vimEditor)
 
-      Disposer.register(listenersDisposable) {
+      Disposer.register(perEditorDisposable) {
         VimPlugin.getEditor().editorDeinit(editor)
       }
     }
@@ -460,6 +463,7 @@ internal object VimListenerManager {
     override fun editorReleased(event: EditorFactoryEvent) {
       if (vimDisabled(event.editor)) return
       val vimEditor = event.editor.vim
+      EditorListeners.remove(event.editor)
       injector.listenersNotifier.notifyEditorReleased(vimEditor)
       injector.markService.editorReleased(vimEditor)
     }

src/test/java/org/jetbrains/plugins/ideavim/listener/VimListenersTest.kt

@@ -8,30 +8,84 @@
 
 package org.jetbrains.plugins.ideavim.listener
 
+import com.intellij.openapi.components.ComponentManagerEx
+import com.intellij.openapi.fileEditor.FileEditorManager
+import com.intellij.openapi.fileEditor.ex.FileEditorManagerEx
+import com.intellij.openapi.fileEditor.impl.FileEditorManagerImpl
+import com.intellij.platform.util.coroutines.childScope
+import com.intellij.testFramework.fixtures.CodeInsightTestFixture
+import com.intellij.testFramework.fixtures.IdeaTestFixtureFactory
+import com.intellij.testFramework.replaceService
 import com.maddyhome.idea.vim.VimPlugin
 import com.maddyhome.idea.vim.listener.VimListenerTestObject
 import org.jetbrains.plugins.ideavim.VimTestCase
 import org.junit.jupiter.api.AfterEach
+import org.junit.jupiter.api.BeforeEach
 import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.TestInfo
 import kotlin.test.assertEquals
 
 class VimListenersTest : VimTestCase() {
+  @BeforeEach
+  override fun setUp(testInfo: TestInfo) {
+    super.setUp(testInfo)
+
+    val manager = FileEditorManagerImpl(fixture.project, (fixture.project as ComponentManagerEx).getCoroutineScope().childScope())
+    fixture.project.replaceService(FileEditorManager::class.java, manager, fixture.testRootDisposable)
+
+    VimListenerTestObject.disposedCounter = 0
+    VimListenerTestObject.enabled = true
+  }
+
   @AfterEach
   fun tearDown() {
     VimListenerTestObject.disposedCounter = 0
     VimListenerTestObject.enabled = false
   }
 
+  override fun createFixture(factory: IdeaTestFixtureFactory): CodeInsightTestFixture {
+    val fixture = factory.createFixtureBuilder("IdeaVim").fixture
+    return factory.createCodeInsightFixture(fixture)
+  }
+
   @Test
-  fun `disposable is called on plugin disable`() {
+  fun `disposable is called when disabling IdeaVim functionality`() {
     configureByText("XYZ")
-    VimListenerTestObject.disposedCounter = 0
-    VimListenerTestObject.enabled = true
 
-    VimPlugin.setEnabled(false)
+    try {
+      VimPlugin.setEnabled(false)
+      assertEquals(1, VimListenerTestObject.disposedCounter)
+    }
+    finally {
+      VimPlugin.setEnabled(true)
+    }
+  }
+
+  @Test
+  fun `disposable is called when closing editor`() {
+    configureByText("XYZ")
+
+    val fileManager = FileEditorManagerEx.getInstanceEx(fixture.project)
+    fileManager.closeFile(fixture.editor.virtualFile)
 
     assertEquals(1, VimListenerTestObject.disposedCounter)
+  }
+
+  @Test
+  fun `disposable is called once when disabling IdeaVim functionality and then closing the editor`() {
+    configureByText("XYZ")
+
+    try {
+      VimPlugin.setEnabled(false)
+      assertEquals(1, VimListenerTestObject.disposedCounter)
+
+      val fileManager = FileEditorManagerEx.getInstanceEx(fixture.project)
+      fileManager.closeFile(fixture.editor.virtualFile)
 
-    VimPlugin.setEnabled(true)
+      assertEquals(1, VimListenerTestObject.disposedCounter)
+    }
+    finally {
+      VimPlugin.setEnabled(true)
+    }
   }
-}
+}