mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-17 16:37:43 +00:00
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
This commit is contained in:
parent
45298edfa6
commit
5e63eeb294
16 changed files with 411 additions and 28 deletions
26
.github/workflows/build.yml
vendored
26
.github/workflows/build.yml
vendored
|
|
@ -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}"
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -541,6 +541,17 @@ export interface PluginAPI {
|
|||
// plugin API typings remain assignable; the host always provides it.
|
||||
onReady?(fn: () => void | Promise<void>): 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>): void;
|
||||
|
||||
// cross-process communication
|
||||
onMessage?(handler: (message: unknown) => Promise<unknown> | unknown): void;
|
||||
|
||||
|
|
|
|||
|
|
@ -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) {
|
||||
|
|
|
|||
|
|
@ -16,4 +16,5 @@ export {
|
|||
TooManyRequestsAPIError,
|
||||
UploadRevToMatchMismatchAPIError,
|
||||
WebDavNativeRequestError,
|
||||
WebDavSyncFolderUnusableSPError,
|
||||
} from './errors/index';
|
||||
|
|
|
|||
|
|
@ -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.',
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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', () => {
|
||||
|
|
|
|||
|
|
@ -217,29 +217,24 @@ describe('PluginAPI', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('onReady()', () => {
|
||||
it('should register a callback via the onReadyRegister function', async () => {
|
||||
let registeredFn: (() => void | Promise<void>) | undefined;
|
||||
const mockBridge2 = jasmine.createSpyObj('PluginBridgeService', [
|
||||
'createBoundMethods',
|
||||
]);
|
||||
mockBridge2.createBoundMethods.and.returnValue({
|
||||
describe('lifecycle registration', () => {
|
||||
type LifecycleRegisters = NonNullable<ConstructorParameters<typeof PluginAPI>[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<void>) | 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<void>) | 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();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -52,6 +52,7 @@ export class PluginAPI implements PluginAPIInterface {
|
|||
#pluginI18nService: PluginI18nService;
|
||||
#manifest?: PluginManifest;
|
||||
#onReadyRegister?: (fn: () => void | Promise<void>) => void;
|
||||
#onUnloadRegister?: (fn: () => void | Promise<void>) => void;
|
||||
#hookHandlers = new Map<string, Map<Hooks, Array<PluginHookHandler<Hooks>>>>();
|
||||
#messageHandler?: (message: unknown) => Promise<unknown>;
|
||||
#boundMethods: ReturnType<typeof PluginBridgeService.prototype.createBoundMethods>;
|
||||
|
|
@ -72,13 +73,17 @@ export class PluginAPI implements PluginAPIInterface {
|
|||
pluginBridge: PluginBridgeService,
|
||||
pluginI18nService: PluginI18nService,
|
||||
manifest?: PluginManifest,
|
||||
onReadyRegister?: (fn: () => void | Promise<void>) => void,
|
||||
lifecycleRegisters?: {
|
||||
onReady?: (fn: () => void | Promise<void>) => void;
|
||||
onUnload?: (fn: () => void | Promise<void>) => 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>): void {
|
||||
this.#onUnloadRegister?.(fn);
|
||||
}
|
||||
|
||||
/**
|
||||
* Register a message handler for the plugin
|
||||
* This allows the plugin's iframe to communicate with the plugin code
|
||||
|
|
|
|||
|
|
@ -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<string, jasmine.Spy> =>
|
||||
(globalThis as unknown as Record<string, Record<string, jasmine.Spy>>)[
|
||||
UNLOAD_GLOBAL
|
||||
];
|
||||
|
||||
beforeEach(() => {
|
||||
(globalThis as unknown as Record<string, Record<string, jasmine.Spy>>)[
|
||||
UNLOAD_GLOBAL
|
||||
] = {};
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
delete (globalThis as unknown as Record<string, unknown>)[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()', () => {
|
||||
|
|
|
|||
|
|
@ -25,6 +25,7 @@ export class PluginRunner {
|
|||
private _loadedPlugins = new Map<string, PluginInstance>();
|
||||
private _pluginApis = new Map<string, PluginAPI>();
|
||||
private _readyCallbacks = new Map<string, () => void | Promise<void>>();
|
||||
private _unloadCallbacks = new Map<string, () => void | Promise<void>>();
|
||||
|
||||
/**
|
||||
* 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.
|
||||
|
|
|
|||
|
|
@ -60,6 +60,7 @@ describe('PluginService loadPluginFromZip iframe-only plugins', () => {
|
|||
'loadPlugin',
|
||||
'triggerReady',
|
||||
'unloadPlugin',
|
||||
'triggerUnload',
|
||||
'pingNodeBridge',
|
||||
]);
|
||||
pluginRunner.loadPlugin.and.callFake(
|
||||
|
|
|
|||
|
|
@ -78,6 +78,7 @@ describe('PluginService', () => {
|
|||
pluginRunner = jasmine.createSpyObj<PluginRunner>('PluginRunner', [
|
||||
'loadPlugin',
|
||||
'unloadPlugin',
|
||||
'triggerUnload',
|
||||
'getLoadedPlugin',
|
||||
'triggerReady',
|
||||
'pingNodeBridge',
|
||||
|
|
|
|||
|
|
@ -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),
|
||||
);
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue