Mobile: Plugins: Fix event listener memory leak when disabling/uninstalling plugins (#10280)

Co-authored-by: Laurent Cozic <laurent22@users.noreply.github.com>

packages/app-cli/tests/services/plugins/api/JoplinSettings.ts

@@ -2,6 +2,7 @@ import Setting from '@joplin/lib/models/Setting';
 import { waitForFolderCount, setupDatabaseAndSynchronizer, switchClient, afterEachCleanUp } from '@joplin/lib/testing/test-utils';
 import Folder from '@joplin/lib/models/Folder';
 import { newPluginScript, newPluginService } from '../../../testUtils';
+import eventManager, { EventName } from '@joplin/lib/eventManager';
 
 describe('JoplinSettings', () => {
 
@@ -66,6 +67,38 @@ describe('JoplinSettings', () => {
 		await service.destroy();
 	});
 
+	test('should de-register settings change listeners when a plugin is unloaded', async () => {
+		const service = newPluginService();
+
+		const pluginScript = newPluginScript(`
+			joplin.plugins.register({
+				onStart: async function() {
+					await joplin.settings.registerSettings({
+						'test-setting': {
+							value: 1234,
+							type: 1,
+							public: false,
+							label: 'Test',
+						}
+					});
+
+					// Register 8 listeners to improve test reliability in the case
+					// where listeners are added/removed from other sources.
+					for (let i = 0; i < 8; i++) {
+						await joplin.settings.onChange((event) => { });
+					}
+				},
+			});
+		`);
+
+		const plugin = await service.loadPluginFromJsBundle('', pluginScript);
+		await service.runPlugin(plugin);
+
+		const listenerCounter = eventManager.listenerCounter_(EventName.SettingsChange);
+		plugin.onUnload();
+		expect(listenerCounter.getCountRemoved()).toBeGreaterThanOrEqual(8);
+	});
+
 	test('should allow registering multiple settings', async () => {
 		const service = newPluginService();
 

packages/app-cli/tests/services/plugins/api/JoplinWorkspace.ts

@@ -4,6 +4,7 @@ import Note from '@joplin/lib/models/Note';
 import Folder from '@joplin/lib/models/Folder';
 import ItemChange from '@joplin/lib/models/ItemChange';
 import { newPluginScript, newPluginService } from '../../../testUtils';
+import eventManager, { EventName } from '@joplin/lib/eventManager';
 
 describe('JoplinWorkspace', () => {
 
@@ -81,4 +82,33 @@ describe('JoplinWorkspace', () => {
 		expect(modFolder.title).toBe('changedtitle');
 	});
 
+	test('should remove event listeners when plugins are unloaded', async () => {
+		const service = newPluginService();
+
+		const pluginScript = newPluginScript(`
+			joplin.plugins.register({
+				onStart: async () => {
+					// Register each listener 8 times to improve test reliability -- it's possible
+					// for listeners for the same events to be added/removed by other sources.
+					for (let i = 0; i < 8; i++) {
+						await joplin.workspace.onNoteChange(async (event) => { });
+						await joplin.workspace.onResourceChange(async (event) => { });
+						await joplin.workspace.filterEditorContextMenu(async (event) => { });
+					}
+				}
+			})
+		`);
+		const plugin = await service.loadPluginFromJsBundle('', pluginScript);
+		await service.runPlugin(plugin);
+
+		const itemChangeListenerCounter = eventManager.listenerCounter_(EventName.ItemChange);
+		const resourceChangeListenerCounter = eventManager.listenerCounter_(EventName.ResourceChange);
+
+		plugin.onUnload();
+
+		expect(itemChangeListenerCounter.getCountRemoved()).toBeGreaterThanOrEqual(8);
+		expect(resourceChangeListenerCounter.getCountRemoved()).toBeGreaterThanOrEqual(8);
+
+		await service.destroy();
+	});
 });

packages/lib/eventManager.ts

@@ -158,6 +158,22 @@ export class EventManager {
 		return this.emitter_.once(eventName, callback);
 	}
 
+	// For testing only; only applies to listeners registered with .on.
+	public listenerCounter_(event: EventName) {
+		const initialListeners = this.emitter_.listeners(event);
+		return {
+			getCountRemoved: () => {
+				const currentListeners = this.emitter_.listeners(event);
+				let countRemoved = 0;
+				for (const listener of initialListeners) {
+					if (!currentListeners.includes(listener)) {
+						countRemoved ++;
+					}
+				}
+				return countRemoved;
+			},
+		};
+	}
 }
 
 const eventManager = new EventManager();

packages/lib/services/plugins/Plugin.ts

@@ -21,6 +21,8 @@ interface ContentScripts {
 	[type: string]: ContentScript[];
 }
 
+type OnUnloadListener = ()=> void;
+
 export default class Plugin {
 
 	private baseDir_: string;
@@ -41,6 +43,7 @@ export default class Plugin {
 	private dataDir_: string;
 	private dataDirCreated_ = false;
 	private hasErrors_ = false;
+	private onUnloadListeners_: OnUnloadListener[] = [];
 
 	// eslint-disable-next-line @typescript-eslint/ban-types -- Old code before rule was applied
 	public constructor(baseDir: string, manifest: PluginManifest, scriptText: string, dispatch: Function, dataDir: string) {
@@ -205,7 +208,16 @@ export default class Plugin {
 		return this.contentScriptMessageListeners_[id](message);
 	}
 
+	public addOnUnloadListener(callback: OnUnloadListener) {
+		this.onUnloadListeners_.push(callback);
+	}
+
 	public onUnload() {
+		for (const callback of this.onUnloadListeners_) {
+			callback();
+		}
+		this.onUnloadListeners_ = [];
+
 		this.dispatch_({
 			type: 'PLUGIN_UNLOAD',
 			pluginId: this.id,

packages/lib/services/plugins/api/Joplin.ts

@@ -49,7 +49,7 @@ export default class Joplin {
 		this.data_ = new JoplinData(plugin);
 		this.plugins_ = new JoplinPlugins(plugin);
 		this.imaging_ = new JoplinImaging(implementation.imaging);
-		this.workspace_ = new JoplinWorkspace(store);
+		this.workspace_ = new JoplinWorkspace(plugin, store);
 		this.filters_ = new JoplinFilters();
 		this.commands_ = new JoplinCommands();
 		this.views_ = new JoplinViews(implementation.joplin.views, plugin, store);

packages/lib/services/plugins/api/JoplinSettings.ts

@@ -5,6 +5,7 @@ import Setting, { SettingItem as InternalSettingItem, SettingSectionSource } fro
 import Plugin from '../Plugin';
 import getPluginNamespacedSettingKey from '../utils/getPluginNamespacedSettingKey';
 import getPluginSettingKeyPrefix from '../utils/getPluginSettingKeyPrefix';
+import makeListener from '../utils/makeListener';
 import { SettingItem, SettingSection } from './types';
 
 // That's all the plugin as of 27/08/21 - any new plugin after that will not be
@@ -180,12 +181,13 @@ export default class JoplinSettings {
 	 */
 	public async onChange(handler: ChangeHandler): Promise<void> {
 		// Filter out keys that are not related to this plugin
-		eventManager.on(EventName.SettingsChange, (event: ChangeEvent) => {
+		const listener = (event: ChangeEvent) => {
 			const keys = event.keys
 				.filter(k => k.indexOf(getPluginSettingKeyPrefix(this.plugin_.id)) === 0)
 				.map(k => k.substr(getPluginSettingKeyPrefix(this.plugin_.id).length));
 			if (!keys.length) return;
 			handler({ keys });
-		});
+		};
+		makeListener(this.plugin_, eventManager, EventName.SettingsChange, listener);
 	}
 }

packages/lib/services/plugins/api/JoplinWorkspace.ts

@@ -1,5 +1,6 @@
 /* eslint-disable multiline-comment-style */
 
+import Plugin from '../Plugin';
 import { ModelType } from '../../../BaseModel';
 import eventManager, { EventName } from '../../../eventManager';
 import Setting from '../../../models/Setting';
@@ -76,14 +77,14 @@ type ResourceChangeHandler = WorkspaceEventHandler<ResourceChangeEvent>;
  * [View the demo plugin](https://github.com/laurent22/joplin/tree/dev/packages/app-cli/tests/support/plugins)
  */
 export default class JoplinWorkspace {
-	// TODO: unregister events when plugin is closed or disabled
-
 	// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
 	private store: any;
+	private plugin: Plugin;
 
 	// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
-	public constructor(store: any) {
+	public constructor(plugin: Plugin, store: any) {
 		this.store = store;
+		this.plugin = plugin;
 	}
 
 	/**
@@ -92,6 +93,10 @@ export default class JoplinWorkspace {
 	// eslint-disable-next-line @typescript-eslint/ban-types -- Old code before rule was applied
 	public async onNoteSelectionChange(callback: WorkspaceEventHandler<NoteSelectionChangeEvent>): Promise<Disposable> {
 		eventManager.appStateOn('selectedNoteIds', callback);
+		const dispose = () => {
+			eventManager.appStateOff('selectedNoteIds', callback);
+		};
+		this.plugin.addOnUnloadListener(dispose);
 
 		return {};
 
@@ -108,6 +113,10 @@ export default class JoplinWorkspace {
 	 */
 	public async onNoteContentChange(callback: WorkspaceEventHandler<NoteContentChangeEvent>) {
 		eventManager.on(EventName.NoteContentChange, callback);
+		const dispose = () => {
+			eventManager.off(EventName.NoteContentChange, callback);
+		};
+		this.plugin.addOnUnloadListener(dispose);
 	}
 
 	/**
@@ -125,7 +134,7 @@ export default class JoplinWorkspace {
 			});
 		};
 
-		return makeListener(eventManager, EventName.ItemChange, wrapperHandler);
+		return makeListener(this.plugin, eventManager, EventName.ItemChange, wrapperHandler);
 	}
 
 	/**
@@ -133,28 +142,28 @@ export default class JoplinWorkspace {
 	 * called when a resource is added or deleted.
 	 */
 	public async onResourceChange(handler: ResourceChangeHandler): Promise<void> {
-		makeListener(eventManager, EventName.ResourceChange, handler);
+		makeListener(this.plugin, eventManager, EventName.ResourceChange, handler);
 	}
 
 	/**
 	 * Called when an alarm associated with a to-do is triggered.
 	 */
 	public async onNoteAlarmTrigger(handler: WorkspaceEventHandler<NoteAlarmTriggerEvent>): Promise<Disposable> {
-		return makeListener(eventManager, EventName.NoteAlarmTrigger, handler);
+		return makeListener(this.plugin, eventManager, EventName.NoteAlarmTrigger, handler);
 	}
 
 	/**
 	 * Called when the synchronisation process is starting.
 	 */
 	public async onSyncStart(handler: SyncStartHandler): Promise<Disposable> {
-		return makeListener(eventManager, EventName.SyncStart, handler);
+		return makeListener(this.plugin, eventManager, EventName.SyncStart, handler);
 	}
 
 	/**
 	 * Called when the synchronisation process has finished.
 	 */
 	public async onSyncComplete(callback: WorkspaceEventHandler<SyncCompleteEvent>): Promise<Disposable> {
-		return makeListener(eventManager, EventName.SyncComplete, callback);
+		return makeListener(this.plugin, eventManager, EventName.SyncComplete, callback);
 	}
 
 	/**
@@ -165,6 +174,9 @@ export default class JoplinWorkspace {
 	 */
 	public filterEditorContextMenu(handler: FilterHandler<EditContextMenuFilterObject>) {
 		eventManager.filterOn('editorContextMenu', handler);
+		this.plugin.addOnUnloadListener(() => {
+			eventManager.filterOff('editorContextMenu', handler);
+		});
 	}
 
 	/**

packages/lib/services/plugins/utils/makeListener.ts

@@ -1,8 +1,15 @@
+import Plugin from '../Plugin';
 import { EventListenerCallback, EventManager, EventName } from '../../../eventManager';
 import { Disposable } from '../api/types';
 
-export default function(eventManager: EventManager, eventName: EventName, callback: EventListenerCallback): Disposable {
+export default function(
+	plugin: Plugin, eventManager: EventManager, eventName: EventName, callback: EventListenerCallback,
+): Disposable {
 	eventManager.on(eventName, callback);
+	const dispose = () => {
+		eventManager.off(eventName, callback);
+	};
+	plugin.addOnUnloadListener(dispose);
 
 	return {};
 
@@ -15,8 +22,6 @@ export default function(eventManager: EventManager, eventName: EventName, callba
 	// await joplin.workspace.removeListener(listenerId);
 
 	// return {
-	// 	dispose: () => {
-	// 		eventManager.off(eventName, callback);
-	// 	}
+	// 	dispose,
 	// };
 }