diff --git a/plugins/CHANGELOG.md b/plugins/CHANGELOG.md index 94cd3f268c..433f43e8c2 100644 --- a/plugins/CHANGELOG.md +++ b/plugins/CHANGELOG.md @@ -9,6 +9,7 @@ ### 🩹 Fixes +- **plugin-runtime:** Cancel plugin loads before they execute after unloading or logout, and allow reopening plugins that close during startup. - **plugins-runtime**: An interaction obtained from `Shape.interactions` now keeps addressing that interaction instead of the position it held when the array was read. Removing every interaction of a shape from a single read removes all of them rather than leaving some behind, and writing through a held interaction after an earlier one is removed no longer lands on a different interaction. - **plugins-runtime**: `Shape.removeInteraction()` now rejects an interaction belonging to a different shape with a validation error, instead of removing whichever interaction sat at the same position on the target shape. - **plugins-runtime**: Writing `trigger`, `delay` or `action` on an interaction the shape no longer has now raises a validation error. The write used to be sent to the workspace with no position to apply it at, where it failed out of the plugin's reach: nothing was written and nothing was reported. diff --git a/plugins/libs/plugins-runtime/src/lib/create-plugin.spec.ts b/plugins/libs/plugins-runtime/src/lib/create-plugin.spec.ts index 29d0b6d691..7960f77ec2 100644 --- a/plugins/libs/plugins-runtime/src/lib/create-plugin.spec.ts +++ b/plugins/libs/plugins-runtime/src/lib/create-plugin.spec.ts @@ -83,6 +83,7 @@ describe('createPlugin', () => { manifest, expect.any(Function), expect.any(Function), + undefined, ); expect(createSandbox).toHaveBeenCalledWith(mockPluginManager, undefined); expect(mockSandbox.evaluate).toHaveBeenCalled(); diff --git a/plugins/libs/plugins-runtime/src/lib/create-plugin.ts b/plugins/libs/plugins-runtime/src/lib/create-plugin.ts index fc4ca233f3..c2b4a2f548 100644 --- a/plugins/libs/plugins-runtime/src/lib/create-plugin.ts +++ b/plugins/libs/plugins-runtime/src/lib/create-plugin.ts @@ -8,8 +8,13 @@ export async function createPlugin( manifest: Manifest, onCloseCallback: () => void, apiExtensions?: object, + signal?: AbortSignal, ) { + // Closing can happen while the manager is still fetching code. + let sandbox: ReturnType | undefined = undefined; + const evaluateSandbox = async () => { + if (plugin.destroyed || !sandbox) return; try { sandbox.evaluate(); } catch (error) { @@ -23,15 +28,18 @@ export async function createPlugin( context, manifest, function onClose() { - sandbox.cleanGlobalThis(); + sandbox?.cleanGlobalThis(); onCloseCallback(); }, function onReloadModal() { evaluateSandbox(); }, + signal, ); - const sandbox = createSandbox(plugin, apiExtensions); + if (plugin.destroyed) return; + + sandbox = createSandbox(plugin, apiExtensions); await evaluateSandbox(); diff --git a/plugins/libs/plugins-runtime/src/lib/load-plugin-context.spec.ts b/plugins/libs/plugins-runtime/src/lib/load-plugin-context.spec.ts index 52a0267db0..9305878b3f 100644 --- a/plugins/libs/plugins-runtime/src/lib/load-plugin-context.spec.ts +++ b/plugins/libs/plugins-runtime/src/lib/load-plugin-context.spec.ts @@ -30,7 +30,7 @@ describe('loadPlugin host context boundary (regression for #11001)', () => { close: vi.fn(), sendMessage: vi.fn(), }, - } as unknown as Awaited>); + } as unknown as NonNullable>>); }); it('does not freeze host-owned functions reachable through the context', async () => { @@ -60,6 +60,7 @@ describe('loadPlugin host context boundary (regression for #11001)', () => { manifest, expect.any(Function), undefined, + expect.any(AbortSignal), ); // Host-owned functions must remain extensible: page navigation and diff --git a/plugins/libs/plugins-runtime/src/lib/load-plugin-real-path.spec.ts b/plugins/libs/plugins-runtime/src/lib/load-plugin-real-path.spec.ts index 2200f434b1..f921455fbe 100644 --- a/plugins/libs/plugins-runtime/src/lib/load-plugin-real-path.spec.ts +++ b/plugins/libs/plugins-runtime/src/lib/load-plugin-real-path.spec.ts @@ -52,15 +52,23 @@ function makeManifest( function makeHostFixture() { const listenerTypes: string[] = []; - const listeners = new Map(); + const listeners = new Map< + symbol, + { type: string; callback: (...args: unknown[]) => unknown } + >(); + const shapes: object[] = []; // Inline code (empty host + non-URL code) resolves without network, so no // fetch mock is needed. UI/modal APIs are never touched by the probe code. - const createRectangle = vi.fn(() => ({ type: 'rectangle-marker' })); + const createRectangle = vi.fn(() => { + const shape = { type: 'rectangle-marker' }; + shapes.push(shape); + return shape; + }); const selection: object[] = [{ id: 'shape-1' }]; const context = { - addListener: (type: string, _callback: (...args: unknown[]) => unknown) => { + addListener: (type: string, callback: (...args: unknown[]) => unknown) => { const id = Symbol(type); - listeners.set(id, type); + listeners.set(id, { type, callback }); listenerTypes.push(type); return id; }, @@ -68,13 +76,21 @@ function makeHostFixture() { listeners.delete(id); }, theme: 'dark', + management: { workspace: { status: 'ready' } }, createRectangle, selection, // Host-only member: present on the raw context but NOT part of the // public penpot API. Plugin code must never see it (see B-2 below). __internalSecret: 'host-internal', } as unknown as Context; - return { context, listeners, listenerTypes, createRectangle, selection }; + return { + context, + listeners, + listenerTypes, + createRectangle, + selection, + shapes, + }; } function lastCompartmentGlobalThis(): Record { @@ -182,7 +198,7 @@ describe('loadPlugin real initialization path (regression for #11001)', () => { const fixture = makeHostFixture(); setContextBuilder(() => fixture.context); const manifest = { - ...makeManifest('plugin.js', []), + ...makeManifest('plugin.js', ['content:read', 'content:write']), host: 'https://plugins.test/', scope: 'global' as const, }; @@ -207,9 +223,10 @@ describe('loadPlugin real initialization path (regression for #11001)', () => { if (order === 'after') await replacement(); finishFetch({ ok: true, - text: async () => 'globalThis.marker = "cancelled";', + text: async () => 'penpot.createRectangle();', }); await cancelled; + expect(fixture.shapes).toEqual([]); if (order === 'before') { expect(getPlugins()).toHaveLength(0); expect(fixture.listeners.size).toBe(0); @@ -225,6 +242,74 @@ describe('loadPlugin real initialization path (regression for #11001)', () => { }, ); + it('cancels a workspace load when another load replaces the same plugin', async () => { + const fixture = makeHostFixture(); + setContextBuilder(() => fixture.context); + const manifest = { + ...makeManifest('plugin.js', ['content:read', 'content:write']), + host: 'https://plugins.test/', + }; + let finishFetch!: (response: object) => void; + vi.stubGlobal( + 'fetch', + () => + new Promise((resolve) => { + finishFetch = resolve; + }), + ); + try { + const cancelled = loadPlugin(manifest); + await loadPlugin({ + ...manifest, + host: '', + code: 'globalThis.marker = "replacement";', + }); + finishFetch({ ok: true, text: async () => 'penpot.createRectangle();' }); + await cancelled; + expect(fixture.shapes).toEqual([]); + expect(getPlugins()).toHaveLength(1); + expect(lastCompartmentGlobalThis()['marker']).toBe('replacement'); + } finally { + for (const plugin of [...getPlugins()]) plugin.plugin.close(); + vi.unstubAllGlobals(); + } + }); + + it('ignores a fetch failure after a load has been cancelled', async () => { + const fixture = makeHostFixture(); + setContextBuilder(() => fixture.context); + const manifest = { + ...makeManifest('plugin.js', []), + host: 'https://plugins.test/', + scope: 'global' as const, + }; + let failFetch!: (error: Error) => void; + vi.stubGlobal( + 'fetch', + () => + new Promise((_resolve, reject) => { + failFetch = reject; + }), + ); + try { + const cancelled = loadPlugin(manifest); + ɵunloadPlugin(manifest.pluginId); + await loadPlugin({ + ...manifest, + host: '', + code: 'globalThis.marker = "replacement";', + }); + failFetch(new Error('Cancelled fetch failed')); + await cancelled; + expect(getPlugins()).toHaveLength(1); + expect(lastCompartmentGlobalThis()['marker']).toBe('replacement'); + expect(fixture.listeners.size).toBe(3); + } finally { + ɵunloadPlugin(manifest.pluginId); + vi.unstubAllGlobals(); + } + }); + it('allows retrying a global plugin after a failed load', async () => { const fixture = makeHostFixture(); setContextBuilder(() => fixture.context); @@ -315,4 +400,94 @@ describe('loadPlugin real initialization path (regression for #11001)', () => { expect(globals['penpotMgmt']).toBeUndefined(); expect(listeners.size).toBe(0); }); + + it('cancels a global load at logout before it can execute in another session', async () => { + const fixture = makeHostFixture(); + setContextBuilder(() => fixture.context); + const manifest = { + ...makeManifest('plugin.js', ['content:read', 'content:write']), + pluginId: 'logout-plugin', + host: 'https://plugins.test/', + scope: 'global' as const, + }; + let finishFetch!: (response: object) => void; + vi.stubGlobal( + 'fetch', + () => + new Promise((resolve) => { + finishFetch = resolve; + }), + ); + try { + const cancelled = loadPlugin(manifest); + for (const listener of [...fixture.listeners.values()]) { + if (listener.type === 'logout') listener.callback(); + } + await loadPlugin({ + ...manifest, + host: '', + code: 'globalThis.marker = "new-session";', + }); + finishFetch({ ok: true, text: async () => 'penpot.createRectangle();' }); + await cancelled; + expect(fixture.shapes).toEqual([]); + expect(getPlugins()).toHaveLength(1); + expect(lastCompartmentGlobalThis()['marker']).toBe('new-session'); + expect(fixture.listeners.size).toBe(3); + } finally { + ɵunloadPlugin(manifest.pluginId); + vi.unstubAllGlobals(); + } + }); + + it('cleans up a failed fetch and allows loading the plugin again', async () => { + const fixture = makeHostFixture(); + setContextBuilder(() => fixture.context); + const manifest = { + ...makeManifest('plugin.js', []), + pluginId: 'failed-fetch-plugin', + host: 'https://plugins.test/', + scope: 'global' as const, + }; + vi.stubGlobal('fetch', () => Promise.reject(new Error('Fetch failed'))); + try { + await expect(loadPlugin(manifest)).rejects.toThrow('Fetch failed'); + expect(getPlugins()).toHaveLength(0); + expect(fixture.listeners.size).toBe(0); + await loadPlugin({ + ...manifest, + host: '', + code: 'globalThis.marker = "retry";', + }); + expect(lastCompartmentGlobalThis()['marker']).toBe('retry'); + } finally { + ɵunloadPlugin(manifest.pluginId); + vi.unstubAllGlobals(); + } + }); + + it('does not register a global plugin that closes during startup and permits reopening', async () => { + const fixture = makeHostFixture(); + setContextBuilder(() => fixture.context); + const manifest = { + ...makeManifest('penpot.closePlugin();', [ + 'content:read', + 'content:write', + ]), + pluginId: 'self-closing-plugin', + scope: 'global' as const, + }; + try { + await loadPlugin(manifest); + expect(getPlugins()).toHaveLength(0); + expect(fixture.listeners.size).toBe(0); + await loadPlugin({ ...manifest, code: 'penpot.createRectangle();' }); + expect(fixture.shapes).toEqual([{ type: 'rectangle-marker' }]); + expect(getPlugins()).toHaveLength(1); + } finally { + ɵunloadPlugin(manifest.pluginId); + } + expect(getPlugins()).toHaveLength(0); + expect(fixture.listeners.size).toBe(0); + }); }); diff --git a/plugins/libs/plugins-runtime/src/lib/load-plugin.spec.ts b/plugins/libs/plugins-runtime/src/lib/load-plugin.spec.ts index 779619279c..eeffb3d236 100644 --- a/plugins/libs/plugins-runtime/src/lib/load-plugin.spec.ts +++ b/plugins/libs/plugins-runtime/src/lib/load-plugin.spec.ts @@ -33,7 +33,7 @@ vi.mock('./ses.js', () => ({ describe('plugin-loader', () => { let mockContext: Context; let manifest: Manifest; - let mockPluginApi: Awaited>; + let mockPluginApi: NonNullable>>; let mockClose: ReturnType; beforeEach(() => { @@ -61,7 +61,7 @@ describe('plugin-loader', () => { close: mockClose, sendMessage: vi.fn(), }, - } as unknown as Awaited>; + } as unknown as NonNullable>>; mockContext = { addListener: vi.fn(), @@ -84,6 +84,7 @@ describe('plugin-loader', () => { manifest, expect.any(Function), undefined, + expect.any(AbortSignal), ); expect(mockPluginApi.plugin.close).not.toHaveBeenCalled(); expect(getPlugins()).toHaveLength(1); @@ -129,7 +130,7 @@ describe('plugin-loader', () => { }, iframeWindow: mockIframeWindow, manifest: { ...manifest, host: 'http://localhost:4202' }, - } as unknown as Awaited>; + } as unknown as NonNullable>>; vi.mocked(createPlugin).mockResolvedValue(mockPluginWithIframe); @@ -173,7 +174,7 @@ describe('plugin-loader', () => { }, iframeWindow: mockIframeWindow1, manifest: { ...manifest, host: 'http://localhost:4202' }, - } as unknown as Awaited>; + } as unknown as NonNullable>>; const mockPluginApi2 = { plugin: { @@ -182,7 +183,7 @@ describe('plugin-loader', () => { }, iframeWindow: mockIframeWindow2, manifest: { ...manifest, host: 'http://localhost:4203' }, - } as unknown as Awaited>; + } as unknown as NonNullable>>; vi.mocked(createPlugin).mockResolvedValue(mockPluginApi1); await loadPlugin(manifest); @@ -250,6 +251,7 @@ describe('plugin-loader', () => { manifest, expect.any(Function), undefined, + expect.any(AbortSignal), ); }); @@ -265,6 +267,7 @@ describe('plugin-loader', () => { manifest, expect.any(Function), undefined, + expect.any(AbortSignal), ); }); }); diff --git a/plugins/libs/plugins-runtime/src/lib/load-plugin.ts b/plugins/libs/plugins-runtime/src/lib/load-plugin.ts index bad0d4fd63..8da32c4d64 100644 --- a/plugins/libs/plugins-runtime/src/lib/load-plugin.ts +++ b/plugins/libs/plugins-runtime/src/lib/load-plugin.ts @@ -4,8 +4,10 @@ import { loadManifest } from './parse-manifest.js'; import { Manifest } from './models/manifest.model.js'; import { createPlugin } from './create-plugin.js'; -let plugins: Awaited>[] = []; -const pendingPlugins = new Map(); +type Plugin = NonNullable>>; + +let plugins: Plugin[] = []; +const pendingPlugins = new Map(); export type ContextBuilder = (id: string) => Context; @@ -50,7 +52,7 @@ export const loadPlugin = async function ( closeCallback?: () => void, apiExtensions?: object, ) { - const loadId = Symbol(); + const load = new AbortController(); try { const context = contextBuilder && contextBuilder(manifest.pluginId); @@ -86,13 +88,17 @@ export const loadPlugin = async function ( // `createSandbox`'s proxy handler applies `ses.safeReturn` to values // crossing into the sandbox. Compartment isolation and intrinsics // hardening are performed by createSandbox, not here. - pendingPlugins.set(manifest.pluginId, loadId); - let plugin: Awaited> | undefined = - undefined; + pendingPlugins.get(manifest.pluginId)?.abort(); + pendingPlugins.set(manifest.pluginId, load); + let plugin: Plugin | undefined = undefined; plugin = await createPlugin( context, manifest, () => { + load.abort(); + if (pendingPlugins.get(manifest.pluginId) === load) { + pendingPlugins.delete(manifest.pluginId); + } plugins = plugins.filter((api) => api !== plugin); if (closeCallback) { @@ -100,9 +106,10 @@ export const loadPlugin = async function ( } }, apiExtensions, + load.signal, ); - if (pendingPlugins.get(manifest.pluginId) !== loadId) { - plugin.plugin.close(); + if (!plugin || load.signal.aborted) { + plugin?.plugin.close(); return; } plugins.push(plugin); @@ -110,7 +117,7 @@ export const loadPlugin = async function ( if (manifest.scope !== 'global') closeAllPlugins(); throw error; } finally { - if (pendingPlugins.get(manifest.pluginId) === loadId) { + if (pendingPlugins.get(manifest.pluginId) === load) { pendingPlugins.delete(manifest.pluginId); } } @@ -130,7 +137,7 @@ export const ɵloadPluginByUrl = async function (manifestUrl: string) { }; export const ɵunloadPlugin = function (id: Manifest['pluginId']) { - pendingPlugins.delete(id); + pendingPlugins.get(id)?.abort(); const plugin = plugins.find((plugin) => plugin.manifest.pluginId === id); if (plugin) { diff --git a/plugins/libs/plugins-runtime/src/lib/plugin-manager.ts b/plugins/libs/plugins-runtime/src/lib/plugin-manager.ts index f430321210..3cb6a9c261 100644 --- a/plugins/libs/plugins-runtime/src/lib/plugin-manager.ts +++ b/plugins/libs/plugins-runtime/src/lib/plugin-manager.ts @@ -14,8 +14,9 @@ export async function createPluginManager( manifest: Manifest, onCloseCallback: () => void, onReloadModal: (code: string) => void, + signal?: AbortSignal, ) { - let code = await loadManifestCode(manifest); + let code = ''; let loaded = false; let destroyed = false; @@ -61,6 +62,7 @@ export async function createPluginManager( const closePlugin = () => { if (destroyed) return; destroyed = true; + signal?.removeEventListener('abort', closePlugin); removeAllEventListeners(); destroyListener(listenerId); destroyListener(logoutId); @@ -148,7 +150,24 @@ export async function createPluginManager( context.removeListener(listenerId); }; + signal?.addEventListener('abort', closePlugin, { once: true }); + try { + if (signal?.aborted) { + closePlugin(); + } else { + code = await loadManifestCode(manifest); + } + } catch (error) { + if (!destroyed) { + closePlugin(); + throw error; + } + } + return { + get destroyed() { + return destroyed; + }, close: closePlugin, destroyListener, openModal,