Mobile: Fix quickly enabling/disabling multiple plugins can lead to errors and missing plugins (#10380)

packages/app-cli/app/services/plugins/PluginRunner.ts

@@ -5,6 +5,8 @@ import executeSandboxCall from '@joplin/lib/services/plugins/utils/executeSandbo
 import Global from '@joplin/lib/services/plugins/api/Global';
 import mapEventHandlersToIds, { EventHandlers } from '@joplin/lib/services/plugins/utils/mapEventHandlersToIds';
 import uuid from '@joplin/lib/uuid';
+import Joplin from '@joplin/lib/services/plugins/api/Joplin';
+import { Console } from 'console';
 const sandboxProxy = require('@joplin/lib/services/plugins/sandboxProxy');
 
 function createConsoleWrapper(pluginId: string) {
@@ -33,11 +35,18 @@ function createConsoleWrapper(pluginId: string) {
 // For example, all plugin calls go through a proxy, however they could made directly since
 // the plugin script is running within the same process as the main app.
 
+interface SandboxProxy {
+	joplin: Joplin;
+	console: typeof Console;
+	stop: ()=> void;
+}
+
 export default class PluginRunner extends BasePluginRunner {
 
 	private eventHandlers_: EventHandlers = {};
 	// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
 	private activeSandboxCalls_: any = {};
+	private sandboxProxies: Map<string, SandboxProxy> = new Map();
 
 	public constructor() {
 		super();
@@ -52,8 +61,14 @@ export default class PluginRunner extends BasePluginRunner {
 	}
 
 	private newSandboxProxy(pluginId: string, sandbox: Global) {
+		let stopped = false;
+
 		// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
 		const target = async (path: string, args: any[]) => {
+			if (stopped) {
+				throw new Error(`Plugin with ID ${pluginId} has been stopped. Cannot execute sandbox call.`);
+			}
+
 			const callId = `${pluginId}::${path}::${uuid.createNano()}`;
 			this.activeSandboxCalls_[callId] = true;
 			const promise = executeSandboxCall(pluginId, sandbox, `joplin.${path}`, mapEventHandlersToIds(args, this.eventHandlers_), this.eventHandler);
@@ -64,10 +79,15 @@ export default class PluginRunner extends BasePluginRunner {
 			return promise;
 		};
 
-		return {
+		const proxy = {
 			joplin: sandboxProxy(target),
 			console: createConsoleWrapper(pluginId),
+			stop: () => {
+				stopped = true;
+			},
 		};
+		this.sandboxProxies.set(pluginId, proxy);
+		return proxy;
 	}
 
 	public async run(plugin: Plugin, sandbox: Global): Promise<void> {
@@ -90,6 +110,13 @@ export default class PluginRunner extends BasePluginRunner {
 		});
 	}
 
+	public async stop(plugin: Plugin): Promise<void> {
+		// TODO: Node VM doesn't support stopping plugins without running them in a child process.
+		// For now, we stop the plugin by making interactions with the Joplin API throw Errors.
+		const proxy = this.sandboxProxies.get(plugin.id);
+		proxy?.stop();
+	}
+
 	public async waitForSandboxCalls(): Promise<void> {
 		const startTime = Date.now();
 		// eslint-disable-next-line @typescript-eslint/ban-types -- Old code before rule was applied

packages/app-cli/tests/services/plugins/PluginService.ts

@@ -1,5 +1,5 @@
 import PluginRunner from '../../../app/services/plugins/PluginRunner';
-import PluginService, { PluginSettings } from '@joplin/lib/services/plugins/PluginService';
+import PluginService, { PluginSettings, defaultPluginSetting } from '@joplin/lib/services/plugins/PluginService';
 import { ContentScriptType } from '@joplin/lib/services/plugins/api/types';
 import MdToHtml from '@joplin/renderer/MdToHtml';
 import shim from '@joplin/lib/shim';
@@ -9,6 +9,7 @@ import Note from '@joplin/lib/models/Note';
 import Folder from '@joplin/lib/models/Folder';
 import { expectNotThrow, setupDatabaseAndSynchronizer, switchClient, expectThrow, createTempDir, supportDir, mockMobilePlatform } from '@joplin/lib/testing/test-utils';
 import { newPluginScript } from '../../testUtils';
+import { join } from 'path';
 
 const testPluginDir = `${supportDir}/plugins`;
 
@@ -397,4 +398,78 @@ describe('services_PluginService', () => {
 		expect(newPluginSettings[pluginId1]).toBe(undefined);
 		expect(newPluginSettings[pluginId2]).toBe(undefined);
 	});
+
+	it('re-running loadAndRunPlugins should reload plugins that have changed but keep unchanged plugins running', async () => {
+		const testDir = await createTempDir();
+		try {
+			const loadCounterNote = await Note.save({ title: 'Log of plugin loads' });
+			const readLoadCounterNote = async () => {
+				return (await Note.load(loadCounterNote.id)).body;
+			};
+			expect(await readLoadCounterNote()).toBe('');
+
+			const writePluginScript = async (version: string, id: string) => {
+				const script = `
+					/* joplin-manifest:
+					{
+						"id": ${JSON.stringify(id)},
+						"manifest_version": 1,
+						"app_min_version": "1.0.0",
+						"name": "JS Bundle test",
+						"version": ${JSON.stringify(version)}
+					}
+					*/
+
+					joplin.plugins.register({
+						onStart: async function() {
+							const noteId = ${JSON.stringify(loadCounterNote.id)};
+							const pluginId = ${JSON.stringify(id)};
+							const note = await joplin.data.get(['notes', noteId], { fields: ['body'] });
+							const newBody = note.body + '\\n' + pluginId;
+							await joplin.data.put(['notes', noteId], null, { body: newBody.trim() });
+						},
+					});
+				`;
+				await fs.writeFile(join(testDir, `${id}.bundle.js`), script);
+			};
+
+			const service = newPluginService();
+			const pluginId1 = 'org.joplinapp.testPlugin1';
+			await writePluginScript('0.0.1', pluginId1);
+			const pluginId2 = 'org.joplinapp.testPlugin2';
+			await writePluginScript('0.0.1', pluginId2);
+
+			let pluginSettings: PluginSettings = {
+				[pluginId1]: defaultPluginSetting(),
+				[pluginId2]: defaultPluginSetting(),
+			};
+			await service.loadAndRunPlugins(testDir, pluginSettings);
+
+			// Plugins should initially load once
+			expect(service.pluginIds).toHaveLength(2);
+			expect(service.pluginById(pluginId1).running).toBe(true);
+			expect(service.pluginById(pluginId2).running).toBe(true);
+			expect(await readLoadCounterNote()).toBe(`${pluginId1}\n${pluginId2}`);
+
+			// Updating just plugin 1 reload just plugin 1.
+			await writePluginScript('0.0.2', pluginId1);
+			await service.loadAndRunPlugins(testDir, pluginSettings);
+
+			expect(service.pluginById(pluginId1).running).toBe(true);
+			expect(service.pluginById(pluginId2).running).toBe(true);
+			expect(await readLoadCounterNote()).toBe(`${pluginId1}\n${pluginId2}\n${pluginId1}`);
+
+			// Disabling plugin 1 should not reload plugin 2
+			pluginSettings = { ...pluginSettings, [pluginId1]: { ...defaultPluginSetting(), enabled: false } };
+			await service.loadAndRunPlugins(testDir, pluginSettings);
+
+			expect(service.pluginById(pluginId1).running).toBe(false);
+			expect(service.pluginById(pluginId2).running).toBe(true);
+			expect(await readLoadCounterNote()).toBe(`${pluginId1}\n${pluginId2}\n${pluginId1}`);
+
+			await service.destroy();
+		} finally {
+			await fs.remove(testDir);
+		}
+	});
 });

packages/app-mobile/components/screens/ConfigScreen/plugins/PluginStates.test.tsx

@@ -24,13 +24,12 @@ const shouldShowBasedOnSettingSearchQuery = ()=>true;
 const PluginStatesWrapper = (props: WrapperProps) => {
 	const styles = configScreenStyles(Setting.THEME_LIGHT);
 
-	const [pluginStates, setPluginStates] = useState(() => {
-		return PluginService.instance().serializePluginSettings(props.initialPluginSettings ?? {});
+	const [pluginSettings, setPluginSettings] = useState(() => {
+		return props.initialPluginSettings ?? {};
 	});
 
 	const updatePluginStates = useCallback((newStates: PluginSettings) => {
-		const serialized = PluginService.instance().serializePluginSettings(newStates);
-		setPluginStates(serialized);
+		setPluginSettings(newStates);
 	}, []);
 
 	return (
@@ -38,7 +37,7 @@ const PluginStatesWrapper = (props: WrapperProps) => {
 			themeId={Setting.THEME_LIGHT}
 			styles={styles}
 			updatePluginStates={updatePluginStates}
-			pluginSettings={pluginStates}
+			pluginSettings={pluginSettings}
 			shouldShowBasedOnSearchQuery={shouldShowBasedOnSettingSearchQuery}
 		/>
 	);

packages/app-mobile/components/screens/ConfigScreen/plugins/PluginStates.tsx

@@ -1,21 +1,23 @@
 import * as React from 'react';
-import { useCallback, useState } from 'react';
+import { useCallback, useMemo, useRef, useState } from 'react';
 import { ConfigScreenStyles } from '../configScreenStyles';
 import { View } from 'react-native';
 import { Banner, Button, Text } from 'react-native-paper';
 import { _ } from '@joplin/lib/locale';
-import PluginService, { PluginSettings } from '@joplin/lib/services/plugins/PluginService';
+import PluginService, { PluginSettings, SerializedPluginSettings } from '@joplin/lib/services/plugins/PluginService';
 import PluginToggle from './PluginToggle';
 import SearchPlugins from './SearchPlugins';
 import { ItemEvent } from '@joplin/lib/components/shared/config/plugins/types';
 import NavService from '@joplin/lib/services/NavService';
 import useRepoApi from './utils/useRepoApi';
 import RepositoryApi from '@joplin/lib/services/plugins/RepositoryApi';
+import shim from '@joplin/lib/shim';
+import Logger from '@joplin/utils/Logger';
 
 interface Props {
 	themeId: number;
 	styles: ConfigScreenStyles;
-	pluginSettings: string;
+	pluginSettings: SerializedPluginSettings;
 	settingsSearchQuery?: string;
 
 	updatePluginStates: (settingValue: PluginSettings)=> void;
@@ -35,6 +37,42 @@ export const getSearchText = () => {
 	return searchText;
 };
 
+const logger = Logger.create('PluginStates');
+
+// Loaded plugins: All plugins with available manifests.
+const useLoadedPluginIds = (pluginSettings: SerializedPluginSettings) => {
+	const allPluginIds = useMemo(() => {
+		return Object.keys(
+			PluginService.instance().unserializePluginSettings(pluginSettings),
+		);
+	}, [pluginSettings]);
+
+	const [pluginReloadCounter, setPluginReloadCounter] = useState(0);
+	const loadedPluginIds = useMemo(() => {
+		if (pluginReloadCounter > 0) {
+			logger.debug(`Not all plugins were loaded in the last render. Re-loading (try ${pluginReloadCounter})`);
+		}
+
+		const pluginService = PluginService.instance();
+		return allPluginIds.filter(id => !!pluginService.plugins[id]);
+	}, [allPluginIds, pluginReloadCounter]);
+	const hasLoadingPlugins = loadedPluginIds.length !== allPluginIds.length;
+
+	// Force a re-render if not all plugins have available metadata. This can happen
+	// if plugins are still loading.
+	const pluginReloadCounterRef = useRef(0);
+	pluginReloadCounterRef.current = pluginReloadCounter;
+	const timeoutRef = useRef(null);
+	if (hasLoadingPlugins && !timeoutRef.current) {
+		timeoutRef.current = shim.setTimeout(() => {
+			timeoutRef.current = null;
+			setPluginReloadCounter(pluginReloadCounterRef.current + 1);
+		}, 1000);
+	}
+
+	return loadedPluginIds;
+};
+
 const PluginStates: React.FC<Props> = props => {
 	const [repoApiError, setRepoApiError] = useState(null);
 	const [repoApiLoaded, setRepoApiLoaded] = useState(false);
@@ -91,15 +129,17 @@ const PluginStates: React.FC<Props> = props => {
 
 	const installedPluginCards = [];
 	const pluginService = PluginService.instance();
-	for (const key in pluginService.plugins) {
-		const plugin = pluginService.plugins[key];
+
+	const pluginIds = useLoadedPluginIds(props.pluginSettings);
+	for (const pluginId of pluginIds) {
+		const plugin = pluginService.plugins[pluginId];
 
 		if (!props.shouldShowBasedOnSearchQuery || props.shouldShowBasedOnSearchQuery(plugin.manifest.name)) {
 			installedPluginCards.push(
 				<PluginToggle
-					key={`plugin-${key}`}
+					key={`plugin-${pluginId}`}
 					themeId={props.themeId}
-					pluginId={plugin.id}
+					pluginId={pluginId}
 					styles={props.styles}
 					pluginSettings={props.pluginSettings}
 					updatablePluginIds={updatablePluginIds}

packages/app-mobile/components/screens/ConfigScreen/plugins/PluginToggle.tsx

@@ -1,7 +1,7 @@
 
 import * as React from 'react';
 import { ConfigScreenStyles } from '../configScreenStyles';
-import PluginService, { PluginSettings, defaultPluginSetting } from '@joplin/lib/services/plugins/PluginService';
+import PluginService, { PluginSettings, defaultPluginSetting, SerializedPluginSettings } from '@joplin/lib/services/plugins/PluginService';
 import { useCallback, useMemo, useState } from 'react';
 import PluginBox, { UpdateState } from './PluginBox';
 import useOnDeleteHandler from '@joplin/lib/components/shared/config/plugins/useOnDeleteHandler';
@@ -13,7 +13,7 @@ interface Props {
 	pluginId: string;
 	themeId: number;
 	styles: ConfigScreenStyles;
-	pluginSettings: string;
+	pluginSettings: SerializedPluginSettings;
 	updatablePluginIds: Record<string, boolean>;
 	repoApi: RepositoryApi;
 

packages/app-mobile/components/screens/ConfigScreen/plugins/PluginUploadButton.tsx

@@ -1,6 +1,6 @@
 
 import { _ } from '@joplin/lib/locale';
-import PluginService, { PluginSettings, defaultPluginSetting } from '@joplin/lib/services/plugins/PluginService';
+import PluginService, { PluginSettings, SerializedPluginSettings, defaultPluginSetting } from '@joplin/lib/services/plugins/PluginService';
 import * as React from 'react';
 import { useCallback, useState } from 'react';
 import { Button } from 'react-native-paper';
@@ -14,7 +14,7 @@ import Setting from '@joplin/lib/models/Setting';
 
 interface Props {
 	updatePluginStates: (settingValue: PluginSettings)=> void;
-	pluginSettings: string;
+	pluginSettings: SerializedPluginSettings;
 }
 
 const logger = Logger.create('PluginUploadButton');

packages/app-mobile/components/screens/ConfigScreen/plugins/SearchPlugins.test.tsx

@@ -7,8 +7,7 @@ import '@testing-library/react-native/extend-expect';
 
 import SearchPlugins from './SearchPlugins';
 import Setting from '@joplin/lib/models/Setting';
-import PluginService, { PluginSettings } from '@joplin/lib/services/plugins/PluginService';
-import { useMemo } from 'react';
+import { PluginSettings } from '@joplin/lib/services/plugins/PluginService';
 import pluginServiceSetup from './testUtils/pluginServiceSetup';
 import newRepoApi from './testUtils/newRepoApi';
 
@@ -22,14 +21,10 @@ interface WrapperProps {
 const noOpFunction = ()=>{};
 
 const SearchWrapper = (props: WrapperProps) => {
-	const serializedPluginSettings = useMemo(() => {
-		return PluginService.instance().serializePluginSettings(props.pluginSettings ?? {});
-	}, [props.pluginSettings]);
-
 	return (
 		<SearchPlugins
 			themeId={Setting.THEME_LIGHT}
-			pluginSettings={serializedPluginSettings}
+			pluginSettings={props.pluginSettings ?? {}}
 			repoApiInitialized={props.repoApiInitialized ?? true}
 			repoApi={props.repoApi}
 			onUpdatePluginStates={props.onUpdatePluginStates ?? noOpFunction}

packages/app-mobile/components/screens/ConfigScreen/plugins/SearchPlugins.tsx

@@ -7,7 +7,7 @@ import { useCallback, useMemo, useState } from 'react';
 import { FlatList, View } from 'react-native';
 import { Searchbar } from 'react-native-paper';
 import PluginBox, { InstallState } from './PluginBox';
-import PluginService, { PluginSettings } from '@joplin/lib/services/plugins/PluginService';
+import PluginService, { PluginSettings, SerializedPluginSettings } from '@joplin/lib/services/plugins/PluginService';
 import useInstallHandler from '@joplin/lib/components/shared/config/plugins/useOnInstallHandler';
 import { OnPluginSettingChangeEvent, PluginItem } from '@joplin/lib/components/shared/config/plugins/types';
 import RepositoryApi from '@joplin/lib/services/plugins/RepositoryApi';
@@ -15,7 +15,7 @@ import openWebsiteForPlugin from './utils/openWebsiteForPlugin';
 
 interface Props {
 	themeId: number;
-	pluginSettings: string;
+	pluginSettings: SerializedPluginSettings;
 	repoApiInitialized: boolean;
 	onUpdatePluginStates: (states: PluginSettings)=> void;
 	repoApi: RepositoryApi;

packages/app-mobile/plugins/PluginRunner/PluginRunnerWebView.tsx

@@ -8,7 +8,7 @@ import loadPlugins from '../loadPlugins';
 import { connect, useStore } from 'react-redux';
 import Logger from '@joplin/utils/Logger';
 import { View } from 'react-native';
-import PluginService, { PluginSettings } from '@joplin/lib/services/plugins/PluginService';
+import PluginService, { PluginSettings, SerializedPluginSettings } from '@joplin/lib/services/plugins/PluginService';
 import { PluginHtmlContents, PluginStates } from '@joplin/lib/services/plugins/reducer';
 import useAsyncEffect from '@joplin/lib/hooks/useAsyncEffect';
 import PluginDialogManager from './dialogs/PluginDialogManager';
@@ -16,7 +16,7 @@ import { AppState } from '../../utils/types';
 
 const logger = Logger.create('PluginRunnerWebView');
 
-const usePluginSettings = (serializedPluginSettings: string) => {
+const usePluginSettings = (serializedPluginSettings: SerializedPluginSettings) => {
 	return useMemo(() => {
 		const pluginService = PluginService.instance();
 		return pluginService.unserializePluginSettings(serializedPluginSettings);
@@ -41,7 +41,7 @@ const usePlugins = (
 
 
 interface Props {
-	serializedPluginSettings: string;
+	serializedPluginSettings: SerializedPluginSettings;
 	pluginStates: PluginStates;
 	pluginHtmlContents: PluginHtmlContents;
 	themeId: number;

packages/app-mobile/plugins/loadPlugins.ts

@@ -23,10 +23,11 @@ const loadPlugins = async (
 		);
 		pluginService.isSafeMode = Setting.value('isSafeMode');
 
-		// Unload any existing plugins (important for React Native's fast refresh)
-		logger.debug('Unloading plugins...');
-		for (const pluginId of pluginService.pluginIds) {
-			await pluginService.unloadPlugin(pluginId);
+		for (const pluginId of Object.keys(pluginService.plugins)) {
+			if (!pluginSettings[pluginId]?.enabled) {
+				logger.info('Unloading disabled plugin', pluginId);
+				await pluginService.unloadPlugin(pluginId);
+			}
 
 			if (cancel.cancelled) {
 				return;

packages/lib/services/plugins/Plugin.ts

@@ -43,6 +43,7 @@ export default class Plugin {
 	private dataDir_: string;
 	private dataDirCreated_ = false;
 	private hasErrors_ = false;
+	private running_ = false;
 	private onUnloadListeners_: OnUnloadListener[] = [];
 
 	// eslint-disable-next-line @typescript-eslint/ban-types -- Old code before rule was applied
@@ -87,6 +88,14 @@ export default class Plugin {
 		return this.baseDir_;
 	}
 
+	public get running(): boolean {
+		return this.running_;
+	}
+
+	public set running(running: boolean) {
+		this.running_ = running;
+	}
+
 	public async dataDir(): Promise<string> {
 		if (this.dataDirCreated_) return this.dataDir_;
 

packages/lib/services/plugins/PluginService.ts

@@ -68,6 +68,8 @@ export interface PluginSettings {
 	[pluginId: string]: PluginSetting;
 }
 
+export type SerializedPluginSettings = Record<string, Partial<PluginSetting>>;
+
 interface PluginLoadOptions {
 	devMode: boolean;
 	builtIn: boolean;
@@ -158,6 +160,7 @@ export default class PluginService extends BaseService {
 
 			plugin.onUnload();
 			await this.runner_.stop(plugin);
+			plugin.running = false;
 
 			this.deletePluginAt(pluginId);
 			this.startedPlugins_ = { ...this.startedPlugins_ };
@@ -177,8 +180,7 @@ export default class PluginService extends BaseService {
 		return this.plugins_[id];
 	}
 
-	// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
-	public unserializePluginSettings(settings: any): PluginSettings {
+	public unserializePluginSettings(settings: SerializedPluginSettings): PluginSettings {
 		const output = { ...settings };
 
 		for (const pluginId in output) {
@@ -188,7 +190,7 @@ export default class PluginService extends BaseService {
 			};
 		}
 
-		return output;
+		return output as PluginSettings;
 	}
 
 	public serializePluginSettings(settings: PluginSettings): string {
@@ -410,11 +412,32 @@ export default class PluginService extends BaseService {
 
 			try {
 				const plugin = await this.loadPluginFromPath(pluginPath);
+				const enabled = this.pluginEnabled(settings, plugin.id);
 
-				// After transforming the plugin path to an ID, multiple plugins might end up with the same ID. For
-				// example "MyPlugin" and "myplugin" would have the same ID. Technically it's possible to have two
-				// such folders but to keep things sane we disallow it.
-				if (this.plugins_[plugin.id]) throw new Error(`There is already a plugin with this ID: ${plugin.id}`);
+				const existingPlugin = this.plugins_[plugin.id];
+				if (existingPlugin) {
+					const isSamePlugin = existingPlugin.baseDir === plugin.baseDir;
+
+					// On mobile, plugins can reload without restarting the app. If a plugin is currently
+					// running and hasn't changed, it doesn't need to be reloaded.
+					if (isSamePlugin) {
+						const isSameVersion =
+							existingPlugin.manifest.version === plugin.manifest.version
+							&& existingPlugin.manifest._package_hash === plugin.manifest._package_hash;
+						if (isSameVersion && existingPlugin.running === enabled) {
+							logger.debug('Not reloading same-version plugin', plugin.id);
+							continue;
+						} else {
+							logger.info('Reloading plugin with ID', plugin.id);
+							await this.unloadPlugin(plugin.id);
+						}
+					} else {
+						// After transforming the plugin path to an ID, multiple plugins might end up with the same ID. For
+						// example "MyPlugin" and "myplugin" would have the same ID. Technically it's possible to have two
+						// such folders but to keep things sane we disallow it.
+						throw new Error(`There is already a plugin with this ID: ${plugin.id}`);
+					}
+				}
 
 				// We mark the plugin as built-in even if not enabled (being built-in affects
 				// update UI).
@@ -422,7 +445,7 @@ export default class PluginService extends BaseService {
 
 				this.setPluginAt(plugin.id, plugin);
 
-				if (!this.pluginEnabled(settings, plugin.id)) {
+				if (!enabled) {
 					logger.info(`Not running disabled plugin: "${plugin.id}"`);
 					continue;
 				}
@@ -507,6 +530,7 @@ export default class PluginService extends BaseService {
 
 		plugin.on('started', onStarted);
 
+		plugin.running = true;
 		const pluginApi = new Global(this.platformImplementation_, plugin, this.store_);
 		return this.runner_.run(plugin, pluginApi);
 	}