From 4f9d76ef66a0a06e3fdf530c7020881612d16fbc Mon Sep 17 00:00:00 2001 From: Matt Ellis Date: Tue, 12 Nov 2024 10:48:57 +0000 Subject: [PATCH] Fix memory leak with non-disposed listeners Fixes VIM-3319 --- .../idea/vim/listener/VimListenerManager.kt | 48 +++++++------- .../ideavim/listener/VimListenersTest.kt | 66 +++++++++++++++++-- 2 files changed, 86 insertions(+), 28 deletions(-) diff --git a/src/main/java/com/maddyhome/idea/vim/listener/VimListenerManager.kt b/src/main/java/com/maddyhome/idea/vim/listener/VimListenerManager.kt index 4b9f37f690..e5d59ae187 100644 --- a/src/main/java/com/maddyhome/idea/vim/listener/VimListenerManager.kt +++ b/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) } diff --git a/src/test/java/org/jetbrains/plugins/ideavim/listener/VimListenersTest.kt b/src/test/java/org/jetbrains/plugins/ideavim/listener/VimListenersTest.kt index d184d69eda..4c789ce1b4 100644 --- a/src/test/java/org/jetbrains/plugins/ideavim/listener/VimListenersTest.kt +++ b/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) + } } -} \ No newline at end of file +}