From 5e63eeb2947c1e95252573801c716bb5ca22bbb5 Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Fri, 12 Jun 2026 17:09:28 +0200 Subject: [PATCH] fix(plugins): stop leaked timers on plugin disable via onUnload hook (#8286) * feat(sync): show actionable error on persistent WebDAV 409 When a WebDAV PUT keeps returning 409 Conflict even after the parent collection is created, the Base URL / Sync Folder Path is misconfigured (the classic Synology / raw-WebDAV setup mistake). Previously the raw "HTTP 409 Conflict" (or a bare "Unknown error") reached the user with no hint at the cause. Throw a dedicated WebDavSyncFolderUnusableSPError with a privacy-safe, actionable message (no path, no response body) at the spot that already detects this condition. Mirrors NetworkUnavailableSPError: a fixed user-facing message matched by instanceof. * ci(release): revive contributors section in release notes * fix(plugins): stop leaked timers on plugin disable via onUnload hook * fix(plugins): harden onUnload teardown hook after review * refactor(plugins): group lifecycle registers into options object * fix(plugins): close onReady stale-guard gap and timer interleave race --- .github/workflows/build.yml | 26 +++ docs/plugin-development.md | 31 ++++ packages/plugin-api/src/types.ts | 11 ++ packages/plugin-dev/voice-reminder/plugin.js | 22 ++- packages/sync-providers/src/errors.ts | 1 + packages/sync-providers/src/errors/index.ts | 22 +++ .../src/file-based/webdav/webdav-api.ts | 9 +- .../file-based/webdav/webdav-api.spec.ts | 29 ++++ src/app/plugins/plugin-api.spec.ts | 48 +++--- src/app/plugins/plugin-api.ts | 20 ++- src/app/plugins/plugin-runner.spec.ts | 153 ++++++++++++++++++ src/app/plugins/plugin-runner.ts | 53 +++++- .../plugin.service.load-from-zip.spec.ts | 1 + src/app/plugins/plugin.service.spec.ts | 1 + src/app/plugins/plugin.service.ts | 7 + src/app/plugins/util/plugin-iframe.util.ts | 5 + 16 files changed, 411 insertions(+), 28 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 0e572b83ae..0d2b795033 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -21,6 +21,7 @@ jobs: env: GH_REPOSITORY: ${{ github.repository }} GH_REF_NAME: ${{ github.ref_name }} + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | if echo "$GH_REF_NAME" | grep -Eq '^v[0-9]+\.[0-9]+\.[0-9]+$'; then PREV_TAG=$(git tag --merged HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+$' | grep -v "^${GH_REF_NAME}$" | sed -n '1p') @@ -28,8 +29,33 @@ jobs: PREV_TAG=$(git tag --merged HEAD --sort=-v:refname | grep '^v' | grep -v "^${GH_REF_NAME}$" | sed -n '1p') fi + # Contributor attribution was lost when curated notes replaced GitHub's + # auto-generated ones; pull the generated notes only to extract it. + GENERATED_NOTES="" + if [ -n "$PREV_TAG" ]; then + GENERATED_NOTES=$(gh api "repos/${GH_REPOSITORY}/releases/generate-notes" \ + -f tag_name="$GH_REF_NAME" -f previous_tag_name="$PREV_TAG" --jq .body) \ + || { GENERATED_NOTES=""; echo "Could not fetch generated notes for contributor extraction"; } + fi + CONTRIBUTORS=$(printf '%s\n' "$GENERATED_NOTES" \ + | grep -oE ' by @[[:alnum:]-]+(\[bot\])? in https://[^ ]+/pull/[0-9]+$' \ + | sed 's/^ by //;s/ in .*//' | grep -v '\[bot\]$' | sort -fu | paste -sd ' ' -) || true + NEW_CONTRIBUTORS=$(printf '%s\n' "$GENERATED_NOTES" \ + | awk '/^## New Contributors$/{f=1} /^\*\*Full Changelog/{f=0} f' \ + | sed 's/^## /### /') || true + { cat build/release-notes.md + if [ -n "$CONTRIBUTORS" ]; then + echo "" + echo "### Contributors" + echo "" + echo "Thanks to everyone who contributed to this release: $CONTRIBUTORS" + fi + if [ -n "$NEW_CONTRIBUTORS" ]; then + echo "" + echo "$NEW_CONTRIBUTORS" + fi echo "" if [ -n "$PREV_TAG" ]; then echo "**Full Changelog**: https://github.com/${GH_REPOSITORY}/compare/${PREV_TAG}...${GH_REF_NAME}" diff --git a/docs/plugin-development.md b/docs/plugin-development.md index 154120aba3..6c76127006 100644 --- a/docs/plugin-development.md +++ b/docs/plugin-development.md @@ -599,6 +599,37 @@ fine in practice because iframe plugins are rendered on user navigation (well af startup). Iframe API calls still go through the host bridge when they are made; cold-boot bridge pings are only performed for host-side plugin code. +**Clean up with `plugin.onUnload()`:** + +Code-based plugins (`plugin.js`) run directly in the app's renderer, so timers and +listeners they create are **not** cleaned up automatically when the plugin is disabled, +reloaded, or uninstalled — a `setInterval` started by your plugin keeps firing until the +app is fully reloaded. Register a teardown callback to clear them yourself: + +```javascript +const intervalId = setInterval(doWork, 60000); + +plugin.onUnload(() => { + clearInterval(intervalId); + // also: removeEventListener, speechSynthesis.cancel(), close connections, … +}); +``` + +The host invokes the callback at the start of plugin teardown, while the Plugin API is +still usable for calls like persisting data — but don't register new hooks or listeners +from inside it (the plugin is going away; re-registering `onUnload` there is ignored). +The returned promise is **not awaited** — do synchronous cleanup (`clearInterval` etc.) +before any `await`, since teardown continues immediately. Registering again replaces the +previous callback, so register once and do all cleanup there. Errors thrown by the +callback are logged and do not block teardown. + +Plugins distributed independently of the app should feature-detect it +(`if (plugin.onUnload) { ... }`) — hosts predating the hook don't provide it. + +**Iframe plugins:** `onUnload` exists but is a no-op — the host unmounts the iframe on +unload, which takes its timers and listeners with it. Don't rely on it for unload-time +persistence in iframes; persist when the data changes instead. + ### 4. Don't spam the logs `console.logs` should be kept to a minimum. diff --git a/packages/plugin-api/src/types.ts b/packages/plugin-api/src/types.ts index 824be955da..3dcc26f7f4 100644 --- a/packages/plugin-api/src/types.ts +++ b/packages/plugin-api/src/types.ts @@ -541,6 +541,17 @@ export interface PluginAPI { // plugin API typings remain assignable; the host always provides it. onReady?(fn: () => void | Promise): void; + // teardown signal — register a callback the host invokes when the plugin is + // disabled, reloaded, or uninstalled. Code-based plugins run directly in the + // renderer, so timers/listeners they create survive unload unless cleared + // here (clearInterval, removeEventListener, speechSynthesis.cancel, …). + // The returned promise is NOT awaited — do synchronous cleanup before any + // await. In iframe plugins this is a no-op: the iframe is unmounted on + // unload and takes its timers with it. Registering again replaces the + // previous callback. Optional so older plugin API typings remain assignable; + // the host always provides it. + onUnload?(fn: () => void | Promise): void; + // cross-process communication onMessage?(handler: (message: unknown) => Promise | unknown): void; diff --git a/packages/plugin-dev/voice-reminder/plugin.js b/packages/plugin-dev/voice-reminder/plugin.js index 0f3312931a..719a0cdadb 100644 --- a/packages/plugin-dev/voice-reminder/plugin.js +++ b/packages/plugin-dev/voice-reminder/plugin.js @@ -8,6 +8,8 @@ if (!window.speechSynthesis) { var _vrInterval = null; var _vrCurrentTask = null; var _vrDefaultTtsRate = 0.7; + var _vrUnloaded = false; + var _vrStartGen = 0; var _vrDefaults = { isEnabled: false, @@ -36,6 +38,8 @@ if (!window.speechSynthesis) { } function _vrSpeak(text, volume, voiceName) { + // the settings dialog can outlive the plugin (Test button) — stay silent + if (_vrUnloaded) return; var synth = window.speechSynthesis; if (!synth) { console.error('[voice-reminder] No window.speechSynthesis available.'); @@ -70,9 +74,15 @@ if (!window.speechSynthesis) { } async function _vrStartTimer() { + var myGen = ++_vrStartGen; _vrStopTimer(); var cfg = await _vrLoadConfig(); - if (!cfg.isEnabled) return; + // overlapping calls (e.g. Save racing the initial load) resume with stale + // config — only the latest call may touch the timer, and only while the + // plugin is still loaded (#8281) + if (myGen !== _vrStartGen) return; + _vrStopTimer(); + if (!cfg.isEnabled || _vrUnloaded) return; var intervalMs = Math.max(cfg.interval || 300000, 5000); _vrInterval = setInterval(function () { @@ -90,6 +100,16 @@ if (!window.speechSynthesis) { _vrCurrentTask = payload && payload.current ? payload.current : null; }); + // Stop the reminder timer and any in-flight speech when the plugin is + // disabled/reloaded — without this the interval survives unload (#8281). + // shortcut: cancel() clears ALL renderer TTS, not just ours — fine while + // this is the only plugin using speechSynthesis + PluginAPI.onUnload(function () { + _vrUnloaded = true; + _vrStopTimer(); + window.speechSynthesis.cancel(); + }); + // Initial load and start _vrLoadConfig().then(function (cfg) { if (cfg.isEnabled) { diff --git a/packages/sync-providers/src/errors.ts b/packages/sync-providers/src/errors.ts index e6157adb67..d29daf848e 100644 --- a/packages/sync-providers/src/errors.ts +++ b/packages/sync-providers/src/errors.ts @@ -16,4 +16,5 @@ export { TooManyRequestsAPIError, UploadRevToMatchMismatchAPIError, WebDavNativeRequestError, + WebDavSyncFolderUnusableSPError, } from './errors/index'; diff --git a/packages/sync-providers/src/errors/index.ts b/packages/sync-providers/src/errors/index.ts index f9782c8437..db7009e163 100644 --- a/packages/sync-providers/src/errors/index.ts +++ b/packages/sync-providers/src/errors/index.ts @@ -247,3 +247,25 @@ export class EmptyRemoteBodySPError extends InvalidDataSPError { export class RemoteFileChangedUnexpectedly extends AdditionalLogErrorBase { override name = 'RemoteFileChangedUnexpectedly'; } + +/** + * Raised when a WebDAV PUT keeps returning 409 Conflict even after we + * created the parent collection — i.e. the configured sync folder cannot + * be resolved/written relative to the server root (a misconfigured Base + * URL or Sync Folder Path, the classic Synology/raw-WebDAV setup mistake). + * + * Without this, the raw `HTTP 409 Conflict` (or a bare "Unknown error") + * reaches the user, which gives no hint at the actual cause. The message + * here is privacy-safe (no path, no response body) and actionable, so UI + * surfaces can show it verbatim. Mirrors `NetworkUnavailableSPError`: a + * fixed user-facing message matched by `instanceof`, not a string round-trip. + */ +export class WebDavSyncFolderUnusableSPError extends Error { + override name = 'WebDavSyncFolderUnusableSPError'; + constructor() { + super( + 'WebDAV server returned 409 (Conflict) and the sync folder could not be created. ' + + 'Check that the Base URL points to your WebDAV root and that the Sync Folder Path is correct and writable.', + ); + } +} diff --git a/packages/sync-providers/src/file-based/webdav/webdav-api.ts b/packages/sync-providers/src/file-based/webdav/webdav-api.ts index ae7685a68a..0c2ad11152 100644 --- a/packages/sync-providers/src/file-based/webdav/webdav-api.ts +++ b/packages/sync-providers/src/file-based/webdav/webdav-api.ts @@ -6,6 +6,7 @@ import { MissingCredentialsSPError, RemoteFileChangedUnexpectedly, RemoteFileNotFoundAPIError, + WebDavSyncFolderUnusableSPError, } from '../../errors'; import { errorMeta } from '../../log/error-meta'; import { computeContentRev } from '../content-rev'; @@ -257,13 +258,17 @@ export class WebdavApi { retryError.response.status === WebDavHttpStatus.CONFLICT ) { // Demoted from `critical` to `normal`: this is a config-debug - // hint, not an exceptional / unrecoverable condition. The - // caller still gets the thrown error to surface in the UI. + // hint, not an exceptional / unrecoverable condition. this._deps.logger.normal( `${WebdavApi.L}.upload() 409 Conflict persists after creating parent. ` + `Verify syncFolderPath is relative to the WebDAV server root.`, { path }, ); + // Re-throw as an actionable, privacy-safe error so the user + // sees *why* sync fails (misconfigured Base URL / Sync Folder + // Path) instead of a bare "HTTP 409 Conflict". The raw 409 is + // already captured by the logger.normal() call above. + throw new WebDavSyncFolderUnusableSPError(); } throw retryError; } diff --git a/packages/sync-providers/tests/file-based/webdav/webdav-api.spec.ts b/packages/sync-providers/tests/file-based/webdav/webdav-api.spec.ts index eb63812a17..5d3fa8b936 100644 --- a/packages/sync-providers/tests/file-based/webdav/webdav-api.spec.ts +++ b/packages/sync-providers/tests/file-based/webdav/webdav-api.spec.ts @@ -16,6 +16,7 @@ import { RemoteFileChangedUnexpectedly, RemoteFileNotFoundAPIError, WebDavNativeRequestError, + WebDavSyncFolderUnusableSPError, } from '../../../src/errors'; const cfg: WebdavPrivateCfg = { @@ -225,6 +226,34 @@ describe('WebdavApi', () => { // PUT + MKCOL + PUT + GET expect(adapter.request).toHaveBeenCalledTimes(4); }); + + // A 409 that persists after we create the parent dir means the + // Base URL / Sync Folder Path is misconfigured (classic Synology / + // raw-WebDAV setup mistake). Surface an actionable error instead of a + // bare `HTTP 409 Conflict` so the user knows what to fix. + it('throws an actionable WebDavSyncFolderUnusableSPError when 409 persists after creating parent', async () => { + const adapter = makeAdapter(); + const data = 'fresh'; + // First PUT → 409 + adapter.request.mockRejectedValueOnce( + new HttpNotOkAPIError(new Response('', { status: 409 })), + ); + // MKCOL → success + adapter.request.mockResolvedValueOnce(okResponse('', 201)); + // Retry PUT → 409 again (folder path still unresolvable) + adapter.request.mockRejectedValueOnce( + new HttpNotOkAPIError(new Response('', { status: 409 })), + ); + + const err = await makeApi(adapter) + .upload({ path: 'sp/op-1.json', data }) + .catch((e) => e); + expect(err).toBeInstanceOf(WebDavSyncFolderUnusableSPError); + // Privacy-safe + actionable message, no path or response body. + expect(err.message).toContain('Base URL'); + expect(err.message).toContain('Sync Folder Path'); + expect(err.message).not.toContain('op-1.json'); + }); }); describe('remove', () => { diff --git a/src/app/plugins/plugin-api.spec.ts b/src/app/plugins/plugin-api.spec.ts index e2247bc593..0c05fbfdc8 100644 --- a/src/app/plugins/plugin-api.spec.ts +++ b/src/app/plugins/plugin-api.spec.ts @@ -217,29 +217,24 @@ describe('PluginAPI', () => { }); }); - describe('onReady()', () => { - it('should register a callback via the onReadyRegister function', async () => { - let registeredFn: (() => void | Promise) | undefined; - const mockBridge2 = jasmine.createSpyObj('PluginBridgeService', [ - 'createBoundMethods', - ]); - mockBridge2.createBoundMethods.and.returnValue({ + describe('lifecycle registration', () => { + type LifecycleRegisters = NonNullable[5]>; + + const buildApiWithLifecycle = (lifecycle: LifecycleRegisters): PluginAPI => { + const bridge = jasmine.createSpyObj('PluginBridgeService', ['createBoundMethods']); + bridge.createBoundMethods.and.returnValue({ log: jasmine.createSpyObj('log', ['log', 'err', 'info', 'warn', 'debug']), }); - const mockI18n2 = jasmine.createSpyObj('PluginI18nService', [ + const i18n = jasmine.createSpyObj('PluginI18nService', [ 'translate', 'getCurrentLanguage', ]); - const api = new PluginAPI( - baseCfg, - 'test-plugin-2', - mockBridge2, - mockI18n2, - undefined, - (fn) => { - registeredFn = fn; - }, - ); + return new PluginAPI(baseCfg, 'test-plugin-2', bridge, i18n, undefined, lifecycle); + }; + + it('should register an onReady callback via the lifecycle register', async () => { + let registeredFn: (() => void | Promise) | undefined; + const api = buildApiWithLifecycle({ onReady: (fn) => (registeredFn = fn) }); const readySpy = jasmine.createSpy('readyFn').and.resolveTo(); api.onReady(readySpy); @@ -249,9 +244,22 @@ describe('PluginAPI', () => { expect(readySpy).toHaveBeenCalledTimes(1); }); - it('should be a no-op when no onReadyRegister is provided', () => { - // pluginAPI was constructed without onReadyRegister — should not throw + it('should register an onUnload callback via the lifecycle register', async () => { + let registeredFn: (() => void | Promise) | undefined; + const api = buildApiWithLifecycle({ onUnload: (fn) => (registeredFn = fn) }); + + const unloadSpy = jasmine.createSpy('unloadFn').and.resolveTo(); + api.onUnload(unloadSpy); + expect(registeredFn).toBeDefined(); + + await registeredFn!(); + expect(unloadSpy).toHaveBeenCalledTimes(1); + }); + + it('should be a no-op when no lifecycle registers are provided', () => { + // pluginAPI was constructed without lifecycle registers — should not throw expect(() => pluginAPI.onReady(() => {})).not.toThrow(); + expect(() => pluginAPI.onUnload(() => {})).not.toThrow(); }); }); }); diff --git a/src/app/plugins/plugin-api.ts b/src/app/plugins/plugin-api.ts index c6ec9e8315..c0c8c3e2e0 100644 --- a/src/app/plugins/plugin-api.ts +++ b/src/app/plugins/plugin-api.ts @@ -52,6 +52,7 @@ export class PluginAPI implements PluginAPIInterface { #pluginI18nService: PluginI18nService; #manifest?: PluginManifest; #onReadyRegister?: (fn: () => void | Promise) => void; + #onUnloadRegister?: (fn: () => void | Promise) => void; #hookHandlers = new Map>>>(); #messageHandler?: (message: unknown) => Promise; #boundMethods: ReturnType; @@ -72,13 +73,17 @@ export class PluginAPI implements PluginAPIInterface { pluginBridge: PluginBridgeService, pluginI18nService: PluginI18nService, manifest?: PluginManifest, - onReadyRegister?: (fn: () => void | Promise) => void, + lifecycleRegisters?: { + onReady?: (fn: () => void | Promise) => void; + onUnload?: (fn: () => void | Promise) => void; + }, ) { this.#pluginId = pluginId; this.#pluginBridge = pluginBridge; this.#pluginI18nService = pluginI18nService; this.#manifest = manifest; - this.#onReadyRegister = onReadyRegister; + this.#onReadyRegister = lifecycleRegisters?.onReady; + this.#onUnloadRegister = lifecycleRegisters?.onUnload; // Get bound methods for this plugin this.#boundMethods = this.#pluginBridge.createBoundMethods( @@ -343,6 +348,17 @@ export class PluginAPI implements PluginAPIInterface { this.#onReadyRegister?.(fn); } + /** + * Register a callback the host invokes when the plugin is disabled, reloaded, + * or uninstalled. Code-based plugins must clear timers/listeners they created + * here, since they run directly in the renderer and outlive their unload + * otherwise. The returned promise is not awaited — do synchronous cleanup + * before any await. Registering again replaces the previous callback. + */ + onUnload(fn: () => void | Promise): void { + this.#onUnloadRegister?.(fn); + } + /** * Register a message handler for the plugin * This allows the plugin's iframe to communicate with the plugin code diff --git a/src/app/plugins/plugin-runner.spec.ts b/src/app/plugins/plugin-runner.spec.ts index f8af9e2025..b69cc531fd 100644 --- a/src/app/plugins/plugin-runner.spec.ts +++ b/src/app/plugins/plugin-runner.spec.ts @@ -1,5 +1,6 @@ import { TestBed } from '@angular/core/testing'; import { PluginRunner } from './plugin-runner'; +import { PluginAPI } from './plugin-api'; import { PluginBridgeService } from './plugin-bridge.service'; import { PluginSecurityService } from './plugin-security'; import { SnackService } from '../core/snack/snack.service'; @@ -279,6 +280,158 @@ describe('PluginRunner', () => { it('should resolve silently for unknown plugin id', async () => { await expectAsync(service.triggerReady('does-not-exist')).toBeResolved(); }); + + it('should ignore onReady registrations from a stale API instance', async () => { + const staleSpy = jasmine.createSpy('staleReady'); + // plugin leaks its API object so the test can register after unload + const code = `globalThis['${READY_GLOBAL}']['leakedApi'] = plugin;`; + await service.loadPlugin(mockManifest, code, mockBaseCfg); + service.unloadPlugin(mockManifest.id); + + const leakedApi = getGlobal()['leakedApi'] as unknown as PluginAPI; + leakedApi.onReady(staleSpy); + + // reload the plugin: the stale registration must not run in its activation + await service.loadPlugin(mockManifest, `/* no-op */`, mockBaseCfg); + await service.triggerReady(mockManifest.id); + expect(staleSpy).not.toHaveBeenCalled(); + }); + }); + + describe('onUnload', () => { + // Same globalThis-spy pattern as the triggerReady() tests above. + const UNLOAD_GLOBAL = '__pluginRunnerSpec_onUnload__'; + const getGlobal = (): Record => + (globalThis as unknown as Record>)[ + UNLOAD_GLOBAL + ]; + + beforeEach(() => { + (globalThis as unknown as Record>)[ + UNLOAD_GLOBAL + ] = {}; + }); + + afterEach(() => { + delete (globalThis as unknown as Record)[UNLOAD_GLOBAL]; + }); + + it('should call the registered onUnload callback when unloading', async () => { + const unloadSpy = jasmine.createSpy('unload'); + getGlobal()[mockManifest.id] = unloadSpy; + + const code = `plugin.onUnload(() => globalThis['${UNLOAD_GLOBAL}']['${mockManifest.id}']());`; + await service.loadPlugin(mockManifest, code, mockBaseCfg); + + expect(unloadSpy).not.toHaveBeenCalled(); + const result = service.unloadPlugin(mockManifest.id); + + expect(result).toBe(true); + expect(unloadSpy).toHaveBeenCalledTimes(1); + }); + + it('should invoke the callback before hooks are unregistered', async () => { + const callOrder: string[] = []; + getGlobal()[mockManifest.id] = jasmine + .createSpy('unload') + .and.callFake(() => callOrder.push('onUnload')); + mockPluginBridge.unregisterPluginHooks.and.callFake(() => { + callOrder.push('unregisterPluginHooks'); + }); + + const code = `plugin.onUnload(() => globalThis['${UNLOAD_GLOBAL}']['${mockManifest.id}']());`; + await service.loadPlugin(mockManifest, code, mockBaseCfg); + service.unloadPlugin(mockManifest.id); + + expect(callOrder).toEqual(['onUnload', 'unregisterPluginHooks']); + }); + + it('should not block teardown when the callback throws', async () => { + getGlobal()[mockManifest.id] = jasmine.createSpy('unload').and.throwError('boom'); + + const code = `plugin.onUnload(() => globalThis['${UNLOAD_GLOBAL}']['${mockManifest.id}']());`; + await service.loadPlugin(mockManifest, code, mockBaseCfg); + + const result = service.unloadPlugin(mockManifest.id); + + expect(result).toBe(true); + expect(mockCleanupService.cleanupPlugin).toHaveBeenCalledWith(mockManifest.id); + expect(mockPluginBridge.unregisterPluginHooks).toHaveBeenCalledWith( + mockManifest.id, + ); + }); + + it('should not block teardown when the callback rejects asynchronously', async () => { + getGlobal()[mockManifest.id] = jasmine + .createSpy('unload') + .and.rejectWith(new Error('async boom')); + + const code = `plugin.onUnload(() => globalThis['${UNLOAD_GLOBAL}']['${mockManifest.id}']());`; + await service.loadPlugin(mockManifest, code, mockBaseCfg); + + const result = service.unloadPlugin(mockManifest.id); + + expect(result).toBe(true); + expect(mockCleanupService.cleanupPlugin).toHaveBeenCalledWith(mockManifest.id); + // let the rejected promise settle so it doesn't leak into other specs + await new Promise((r) => setTimeout(r, 0)); + }); + + it('should fire the callback at most once across triggerUnload and unloadPlugin', async () => { + const unloadSpy = jasmine.createSpy('unload'); + getGlobal()[mockManifest.id] = unloadSpy; + + const code = `plugin.onUnload(() => globalThis['${UNLOAD_GLOBAL}']['${mockManifest.id}']());`; + await service.loadPlugin(mockManifest, code, mockBaseCfg); + + // plugin.service fires triggerUnload at the start of teardown, then + // unloadPlugin runs as part of the same teardown — must not double-fire + service.triggerUnload(mockManifest.id); + expect(unloadSpy).toHaveBeenCalledTimes(1); + + const result = service.unloadPlugin(mockManifest.id); + expect(result).toBe(true); + expect(unloadSpy).toHaveBeenCalledTimes(1); + }); + + it('should ignore onUnload registrations from a stale API instance', async () => { + const staleSpy = jasmine.createSpy('staleUnload'); + // plugin leaks its API object so the test can register after unload + const code = `globalThis['${UNLOAD_GLOBAL}']['leakedApi'] = plugin;`; + await service.loadPlugin(mockManifest, code, mockBaseCfg); + service.unloadPlugin(mockManifest.id); + + const leakedApi = getGlobal()['leakedApi'] as unknown as PluginAPI; + leakedApi.onUnload(staleSpy); + + // reload the plugin: the stale registration must not fire on its unload + await service.loadPlugin(mockManifest, `/* no-op */`, mockBaseCfg); + service.unloadPlugin(mockManifest.id); + expect(staleSpy).not.toHaveBeenCalled(); + }); + + it('should only fire the callback of the unloaded plugin', async () => { + const manifestB = { ...mockManifest, id: 'plugin-b', name: 'Plugin B' }; + const aSpy = jasmine.createSpy('aUnload'); + const bSpy = jasmine.createSpy('bUnload'); + getGlobal()[mockManifest.id] = aSpy; + getGlobal()[manifestB.id] = bSpy; + + await service.loadPlugin( + mockManifest, + `plugin.onUnload(() => globalThis['${UNLOAD_GLOBAL}']['${mockManifest.id}']());`, + mockBaseCfg, + ); + await service.loadPlugin( + manifestB, + `plugin.onUnload(() => globalThis['${UNLOAD_GLOBAL}']['${manifestB.id}']());`, + mockBaseCfg, + ); + + service.unloadPlugin(mockManifest.id); + expect(aSpy).toHaveBeenCalledTimes(1); + expect(bSpy).not.toHaveBeenCalled(); + }); }); describe('pingNodeBridge()', () => { diff --git a/src/app/plugins/plugin-runner.ts b/src/app/plugins/plugin-runner.ts index 18a9fc4307..fdda6e810a 100644 --- a/src/app/plugins/plugin-runner.ts +++ b/src/app/plugins/plugin-runner.ts @@ -25,6 +25,7 @@ export class PluginRunner { private _loadedPlugins = new Map(); private _pluginApis = new Map(); private _readyCallbacks = new Map void | Promise>(); + private _unloadCallbacks = new Map void | Promise>(); /** * Load and execute a plugin @@ -43,7 +44,22 @@ export class PluginRunner { this._pluginBridge, this._pluginI18nService, manifest, - (fn) => this._readyCallbacks.set(manifest.id, fn), + { + // both registers ignore calls from a stale API instance — leaked + // plugin code can run after its own unload (the failure class the + // onUnload hook fixes) and must not clobber a reloaded instance's + // callbacks + onReady: (fn) => { + if (this._pluginApis.get(manifest.id) === pluginAPI) { + this._readyCallbacks.set(manifest.id, fn); + } + }, + onUnload: (fn) => { + if (this._pluginApis.get(manifest.id) === pluginAPI) { + this._unloadCallbacks.set(manifest.id, fn); + } + }, + }, ); // executeNodeScript is now automatically bound if permitted via createBoundMethods @@ -182,10 +198,14 @@ export class PluginRunner { * Unload a plugin and clean up resources */ unloadPlugin(pluginId: string): boolean { + // Fallback for teardown routes that bypass PluginService's + // _teardownPluginRuntime (activation-error cleanup) — no-op when the + // service already fired it. Outside the loaded-check so a plugin whose + // loadPlugin threw after API creation still gets cleaned up. + this.triggerUnload(pluginId); + const plugin = this._loadedPlugins.get(pluginId); if (plugin) { - // Clean up API reference - this._pluginApis.delete(pluginId); this._readyCallbacks.delete(pluginId); // Clean up all resources @@ -210,6 +230,33 @@ export class PluginRunner { return this._loadedPlugins.get(pluginId); } + /** + * Fire the plugin's registered onUnload callback so it can clear timers and + * listeners it created in the renderer — code-based plugins outlive their + * unload otherwise (see #8281). Idempotent: the callback fires at most once. + * Fire-and-forget: teardown is sync and a buggy callback must not block it; + * the returned promise of an async callback is not awaited (unlike + * triggerReady, which is awaited and may throw). + * + * Side effect: also drops the plugin's API reference, which disables + * sendMessageToPlugin and further lifecycle registrations for this instance. + * Callers must follow up with unloadPlugin() — it is only called separately + * by plugin.service.ts at the start of teardown, while hooks and + * translations are still registered. + */ + triggerUnload(pluginId: string): void { + const unloadFn = this._unloadCallbacks.get(pluginId); + this._unloadCallbacks.delete(pluginId); + // drop the API reference first so re-registration from inside the callback + // (or any later stale call) is ignored by the registration guard + this._pluginApis.delete(pluginId); + if (unloadFn) { + void (async () => unloadFn())().catch((e) => + PluginLog.err(`Plugin ${pluginId} onUnload callback failed:`, e), + ); + } + } + /** * Fire the onReady callback for a plugin. * Called by plugin.service.ts after the IPC bridge is confirmed available. diff --git a/src/app/plugins/plugin.service.load-from-zip.spec.ts b/src/app/plugins/plugin.service.load-from-zip.spec.ts index 5b1c625a5a..5a6afef541 100644 --- a/src/app/plugins/plugin.service.load-from-zip.spec.ts +++ b/src/app/plugins/plugin.service.load-from-zip.spec.ts @@ -60,6 +60,7 @@ describe('PluginService loadPluginFromZip iframe-only plugins', () => { 'loadPlugin', 'triggerReady', 'unloadPlugin', + 'triggerUnload', 'pingNodeBridge', ]); pluginRunner.loadPlugin.and.callFake( diff --git a/src/app/plugins/plugin.service.spec.ts b/src/app/plugins/plugin.service.spec.ts index fed1202c2f..ad3c191275 100644 --- a/src/app/plugins/plugin.service.spec.ts +++ b/src/app/plugins/plugin.service.spec.ts @@ -78,6 +78,7 @@ describe('PluginService', () => { pluginRunner = jasmine.createSpyObj('PluginRunner', [ 'loadPlugin', 'unloadPlugin', + 'triggerUnload', 'getLoadedPlugin', 'triggerReady', 'pingNodeBridge', diff --git a/src/app/plugins/plugin.service.ts b/src/app/plugins/plugin.service.ts index 576d083334..31e5c4bd75 100644 --- a/src/app/plugins/plugin.service.ts +++ b/src/app/plugins/plugin.service.ts @@ -1599,6 +1599,10 @@ export class PluginService implements OnDestroy { * without changing isEnabled or _pluginStates. Used for re-upload and reload. */ private _teardownPluginRuntime(pluginId: string): void { + // Let the plugin clear its renderer-side timers/listeners first, while its + // hooks and translations are still registered (#8281) + this._pluginRunner.triggerUnload(pluginId); + this._bumpPluginIframeGeneration(pluginId); // Close the side panel if this plugin is active @@ -1628,6 +1632,9 @@ export class PluginService implements OnDestroy { // SECURITY: revoke the main-process nodeExecution token on teardown, so a // disabled/uninstalled plugin cannot run Node for the rest of the session. // Best-effort and fire-and-forget because teardown is synchronous. + // Deliberately AFTER triggerUnload above: the onUnload callback may make + // one final node call for cleanup — same capability the plugin held while + // enabled, just at a guaranteed point. void this._revokeNodeExecutionGrant(pluginId).catch((e) => PluginLog.err(`Failed to revoke nodeExecution grant for ${pluginId}`, e), ); diff --git a/src/app/plugins/util/plugin-iframe.util.ts b/src/app/plugins/util/plugin-iframe.util.ts index 3f6aea3b98..e5bec6833f 100644 --- a/src/app/plugins/util/plugin-iframe.util.ts +++ b/src/app/plugins/util/plugin-iframe.util.ts @@ -493,6 +493,11 @@ export const createPluginApiScript = (config: PluginIframeConfig): string => { }); }, + // Teardown signal — no-op in iframes: the host unmounts the iframe on + // unload, which takes its timers/listeners with it. Provided so plugin + // code can call onUnload unconditionally on both execution paths. + onUnload: (fn) => {}, + // Message handling onMessage: (handler) => { // Store the handler and set up message listener