mirror of
https://github.com/penpot/penpot.git
synced 2026-04-05 10:52:57 +02:00
🐛 Handle plugin errors gracefully without crashing the UI (#8810)
* 🐛 Handle plugin errors gracefully without crashing the UI Plugin errors (like 'Set is not a constructor') were propagating to the global error handler and showing the exception page. This fix: - Uses a WeakMap to track plugin errors (works in SES hardened environment) - Wraps setTimeout/setInterval handlers to mark errors and re-throw them - Frontend global handler checks isPluginError and logs to console Plugin errors are now logged to console with 'Plugin Error' prefix but don't crash the main application or show the exception page. Signed-off-by: AI Agent <agent@penpot.app> * ✨ Improved handling of plugin errors on initialization * ✨ Fix test and linter --------- Signed-off-by: AI Agent <agent@penpot.app> Co-authored-by: alonso.torres <alonso.torres@kaleidos.net>
This commit is contained in:
@@ -7,12 +7,14 @@
|
||||
(ns app.main.data.plugins
|
||||
(:require
|
||||
[app.common.data.macros :as dm]
|
||||
[app.common.exceptions :as ex]
|
||||
[app.common.files.changes-builder :as pcb]
|
||||
[app.common.time :as ct]
|
||||
[app.main.data.changes :as dch]
|
||||
[app.main.data.event :as ev]
|
||||
[app.main.data.modal :as modal]
|
||||
[app.main.data.notifications :as ntf]
|
||||
[app.main.errors :as errors]
|
||||
[app.main.store :as st]
|
||||
[app.plugins.flags :as pflag]
|
||||
[app.plugins.register :as preg]
|
||||
@@ -20,7 +22,8 @@
|
||||
[app.util.http :as http]
|
||||
[app.util.i18n :as i18n :refer [tr]]
|
||||
[beicon.v2.core :as rx]
|
||||
[potok.v2.core :as ptk]))
|
||||
[potok.v2.core :as ptk]
|
||||
[promesa.core :as p]))
|
||||
|
||||
(defn save-plugin-permissions-peek
|
||||
[id permissions]
|
||||
@@ -54,40 +57,45 @@
|
||||
|
||||
(defn start-plugin!
|
||||
[{:keys [plugin-id name version description host code permissions allow-background]} ^js extensions]
|
||||
(.ɵloadPlugin
|
||||
^js ug/global
|
||||
#js {:pluginId plugin-id
|
||||
:name name
|
||||
:version version
|
||||
:description description
|
||||
:host host
|
||||
:code code
|
||||
:allowBackground (boolean allow-background)
|
||||
:permissions (apply array permissions)}
|
||||
nil
|
||||
extensions))
|
||||
(-> (.ɵloadPlugin
|
||||
^js ug/global
|
||||
#js {:pluginId plugin-id
|
||||
:name name
|
||||
:version version
|
||||
:description description
|
||||
:host host
|
||||
:code code
|
||||
:allowBackground (boolean allow-background)
|
||||
:permissions (apply array permissions)}
|
||||
nil
|
||||
extensions)
|
||||
|
||||
(p/catch (fn [cause]
|
||||
(ex/print-throwable cause :prefix "Plugin Error")
|
||||
(errors/flash :cause cause :type :handled)))))
|
||||
|
||||
(defn- load-plugin!
|
||||
[{:keys [plugin-id name version description host code icon permissions]}]
|
||||
(try
|
||||
(st/emit! (pflag/clear plugin-id)
|
||||
(save-current-plugin plugin-id))
|
||||
(st/emit! (pflag/clear plugin-id)
|
||||
(save-current-plugin plugin-id))
|
||||
|
||||
(.ɵloadPlugin ^js ug/global
|
||||
#js {:pluginId plugin-id
|
||||
:name name
|
||||
:description description
|
||||
:version version
|
||||
:host host
|
||||
:code code
|
||||
:icon icon
|
||||
:permissions (apply array permissions)}
|
||||
(fn []
|
||||
(st/emit! (remove-current-plugin plugin-id))))
|
||||
(-> (.ɵloadPlugin
|
||||
^js ug/global
|
||||
#js {:pluginId plugin-id
|
||||
:name name
|
||||
:description description
|
||||
:version version
|
||||
:host host
|
||||
:code code
|
||||
:icon icon
|
||||
:permissions (apply array permissions)}
|
||||
(fn []
|
||||
(st/emit! (remove-current-plugin plugin-id))))
|
||||
|
||||
(catch :default e
|
||||
(st/emit! (remove-current-plugin plugin-id))
|
||||
(.error js/console "Error" e))))
|
||||
(p/catch (fn [cause]
|
||||
(st/emit! (remove-current-plugin plugin-id))
|
||||
(ex/print-throwable cause :prefix "Plugin Error")
|
||||
(errors/flash :cause cause :type :handled)))))
|
||||
|
||||
(defn open-plugin!
|
||||
[{:keys [url] :as manifest} user-can-edit?]
|
||||
|
||||
@@ -33,6 +33,16 @@
|
||||
;; Will contain last uncaught exception
|
||||
(def last-exception nil)
|
||||
|
||||
(defn is-plugin-error?
|
||||
"This is a placeholder that always return false. It will be
|
||||
overwritten when plugin system is initialized. This works this way
|
||||
because we can't import plugins here because plugins requries full
|
||||
DOM.
|
||||
|
||||
This placeholder is set on app.plugins/initialize event"
|
||||
[_]
|
||||
false)
|
||||
|
||||
;; --- Stale-asset error detection and auto-reload
|
||||
;;
|
||||
;; When the browser loads JS modules from different builds (e.g. shared.js from
|
||||
@@ -387,6 +397,15 @@
|
||||
(and (string? stack)
|
||||
(str/includes? stack "posthog"))))
|
||||
|
||||
;; Check if the error is marked as originating from plugin code.
|
||||
;; The plugin runtime tracks plugin errors in a WeakMap, which works
|
||||
;; even in SES hardened environments where error objects may be frozen.
|
||||
(from-plugin? [cause]
|
||||
(try
|
||||
(is-plugin-error? cause)
|
||||
(catch :default _
|
||||
false)))
|
||||
|
||||
(is-ignorable-exception? [cause]
|
||||
(let [message (ex-message cause)]
|
||||
(or (from-extension? cause)
|
||||
@@ -405,32 +424,56 @@
|
||||
(on-unhandled-error [event]
|
||||
(.preventDefault ^js event)
|
||||
(when-let [cause (unchecked-get event "error")]
|
||||
(when-not (is-ignorable-exception? cause)
|
||||
(if (stale-asset-error? cause)
|
||||
(cf/throttled-reload :reason (ex-message cause))
|
||||
(let [data (ex-data cause)
|
||||
type (get data :type)]
|
||||
(set! last-exception cause)
|
||||
(if (= :wasm-error type)
|
||||
(on-error cause)
|
||||
(do
|
||||
(ex/print-throwable cause :prefix "Uncaught Exception")
|
||||
(ts/asap #(flash :cause cause :type :unhandled)))))))))
|
||||
(cond
|
||||
(stale-asset-error? cause)
|
||||
(cf/throttled-reload :reason (ex-message cause))
|
||||
|
||||
;; Plugin errors: log to console and ignore
|
||||
(from-plugin? cause)
|
||||
(ex/print-throwable cause :prefix "Plugin Error")
|
||||
|
||||
;; Other ignorable exceptions: ignore silently
|
||||
(is-ignorable-exception? cause)
|
||||
nil
|
||||
|
||||
;; All other errors: show exception page
|
||||
:else
|
||||
|
||||
(let [data (ex-data cause)
|
||||
type (get data :type)]
|
||||
(set! last-exception cause)
|
||||
(if (= :wasm-error type)
|
||||
(on-error cause)
|
||||
(do
|
||||
(ex/print-throwable cause :prefix "Uncaught Exception")
|
||||
(ts/asap #(flash :cause cause :type :unhandled))))))))
|
||||
|
||||
(on-unhandled-rejection [event]
|
||||
(.preventDefault ^js event)
|
||||
(when-let [cause (unchecked-get event "reason")]
|
||||
(when-not (is-ignorable-exception? cause)
|
||||
(if (stale-asset-error? cause)
|
||||
(cf/throttled-reload :reason (ex-message cause))
|
||||
(let [data (ex-data cause)
|
||||
type (get data :type)]
|
||||
(set! last-exception cause)
|
||||
(if (= :wasm-error type)
|
||||
(on-error cause)
|
||||
(do
|
||||
(ex/print-throwable cause :prefix "Uncaught Rejection")
|
||||
(ts/asap #(flash :cause cause :type :unhandled)))))))))]
|
||||
(cond
|
||||
(stale-asset-error? cause)
|
||||
(cf/throttled-reload :reason (ex-message cause))
|
||||
|
||||
;; Plugin errors: log to console and ignore
|
||||
(from-plugin? cause)
|
||||
(ex/print-throwable cause :prefix "Plugin Error")
|
||||
|
||||
;; Other ignorable exceptions: ignore silently
|
||||
(is-ignorable-exception? cause)
|
||||
nil
|
||||
|
||||
;; All other errors: show exception page
|
||||
:else
|
||||
(let [data (ex-data cause)
|
||||
type (get data :type)]
|
||||
(set! last-exception cause)
|
||||
(if (= :wasm-error type)
|
||||
(on-error cause)
|
||||
(do
|
||||
(ex/print-throwable cause :prefix "Uncaught Rejection")
|
||||
(ts/asap #(flash :cause cause :type :unhandled))))))))]
|
||||
|
||||
|
||||
(.addEventListener g/window "error" on-unhandled-error)
|
||||
(.addEventListener g/window "unhandledrejection" on-unhandled-rejection)
|
||||
|
||||
@@ -8,6 +8,7 @@
|
||||
"RPC for plugins runtime."
|
||||
(:require
|
||||
["@penpot/plugins-runtime" :as runtime]
|
||||
[app.main.errors :as errors]
|
||||
[app.main.features :as features]
|
||||
[app.main.store :as st]
|
||||
[app.plugins.api :as api]
|
||||
@@ -30,6 +31,8 @@
|
||||
(ptk/reify ::initialize
|
||||
ptk/WatchEvent
|
||||
(watch [_ _ stream]
|
||||
(set! errors/is-plugin-error? runtime/isPluginError)
|
||||
|
||||
(->> stream
|
||||
(rx/filter (ptk/type? ::features/initialize))
|
||||
(rx/observe-on :async)
|
||||
|
||||
@@ -393,7 +393,7 @@ export class AppComponent {
|
||||
}
|
||||
|
||||
#startDownload(name: string, data: Uint8Array) {
|
||||
const blob = new Blob([data], { type: 'application/octet-stream' });
|
||||
const blob = new Blob([data as any], { type: 'application/octet-stream' });
|
||||
|
||||
// We need to start a download with this URL
|
||||
const downloadURL = URL.createObjectURL(blob);
|
||||
|
||||
@@ -8,6 +8,9 @@ import {
|
||||
ɵunloadPlugin,
|
||||
} from './lib/load-plugin.js';
|
||||
|
||||
// Export the plugin error checker so the frontend can identify plugin errors
|
||||
export { isPluginError } from './lib/create-sandbox.js';
|
||||
|
||||
import type { Context } from '@penpot/plugin-types';
|
||||
|
||||
console.log('%c[PLUGINS] Loading plugin system', 'color: #008d7c');
|
||||
|
||||
@@ -11,6 +11,7 @@ vi.mock('./plugin-manager.js', () => ({
|
||||
|
||||
vi.mock('./create-sandbox.js', () => ({
|
||||
createSandbox: vi.fn(),
|
||||
markPluginError: vi.fn(),
|
||||
}));
|
||||
|
||||
describe('createPlugin', () => {
|
||||
@@ -116,7 +117,11 @@ describe('createPlugin', () => {
|
||||
throw new Error('Evaluation error');
|
||||
});
|
||||
|
||||
await createPlugin(mockContext, manifest, onCloseCallback);
|
||||
try {
|
||||
await createPlugin(mockContext, manifest, onCloseCallback);
|
||||
} catch (err) {
|
||||
expect.assert(err);
|
||||
}
|
||||
|
||||
expect(mockPluginManager.close).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
import type { Context } from '@penpot/plugin-types';
|
||||
import type { Manifest } from './models/manifest.model.js';
|
||||
import { createPluginManager } from './plugin-manager.js';
|
||||
import { createSandbox } from './create-sandbox.js';
|
||||
import { createSandbox, markPluginError } from './create-sandbox.js';
|
||||
|
||||
export async function createPlugin(
|
||||
context: Context,
|
||||
@@ -13,9 +13,9 @@ export async function createPlugin(
|
||||
try {
|
||||
sandbox.evaluate();
|
||||
} catch (error) {
|
||||
console.error(error);
|
||||
|
||||
markPluginError(error);
|
||||
plugin.close();
|
||||
throw error;
|
||||
}
|
||||
};
|
||||
|
||||
@@ -33,7 +33,7 @@ export async function createPlugin(
|
||||
|
||||
const sandbox = createSandbox(plugin, apiExtensions);
|
||||
|
||||
evaluateSandbox();
|
||||
await evaluateSandbox();
|
||||
|
||||
return {
|
||||
plugin,
|
||||
|
||||
@@ -3,6 +3,61 @@ import type { createPluginManager } from './plugin-manager';
|
||||
import { createApi } from './api';
|
||||
import { ses } from './ses.js';
|
||||
|
||||
/**
|
||||
* WeakMap used to track errors originating from plugin code.
|
||||
* Using a WeakMap is safer than extending error objects because:
|
||||
* 1. It works even if the error object is frozen (SES hardened environment)
|
||||
* 2. It doesn't require modifying the error object
|
||||
* 3. It allows garbage collection of error objects when no longer referenced
|
||||
*/
|
||||
const pluginErrors = new WeakMap<object, true>();
|
||||
|
||||
/**
|
||||
* Checks if an error originated from plugin code.
|
||||
*/
|
||||
export function isPluginError(error: unknown): boolean {
|
||||
if (error !== null && typeof error === 'object') {
|
||||
return pluginErrors.has(error as object);
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
/**
|
||||
* Marks an error as originating from plugin code.
|
||||
* Uses a WeakMap so it works even if the error object is frozen.
|
||||
*/
|
||||
export function markPluginError(error: unknown): void {
|
||||
if (error !== null && typeof error === 'object') {
|
||||
pluginErrors.set(error as object, true);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Wraps a handler function to mark any thrown errors as plugin errors.
|
||||
* Errors are marked and re-thrown so they propagate to the global error handler,
|
||||
* where they can be identified and handled appropriately.
|
||||
*/
|
||||
function wrapHandler<T extends (...args: unknown[]) => unknown>(
|
||||
handler: T,
|
||||
): (...args: Parameters<T>) => ReturnType<T> {
|
||||
return function (...args: Parameters<T>) {
|
||||
try {
|
||||
const result = handler(...args);
|
||||
// Handle async functions - mark errors in the returned promise
|
||||
if (result instanceof Promise) {
|
||||
return result.catch((error: unknown) => {
|
||||
markPluginError(error);
|
||||
throw error;
|
||||
}) as ReturnType<T>;
|
||||
}
|
||||
return result as ReturnType<T>;
|
||||
} catch (error) {
|
||||
markPluginError(error);
|
||||
throw error;
|
||||
}
|
||||
};
|
||||
}
|
||||
|
||||
export function createSandbox(
|
||||
plugin: Awaited<ReturnType<typeof createPluginManager>>,
|
||||
apiExtensions?: object,
|
||||
@@ -58,9 +113,10 @@ export function createSandbox(
|
||||
fetch: ses.harden(safeFetch),
|
||||
setTimeout: ses.harden(
|
||||
(...[handler, timeout]: Parameters<typeof setTimeout>) => {
|
||||
const timeoutId = setTimeout(() => {
|
||||
handler();
|
||||
}, timeout);
|
||||
const wrappedHandler = wrapHandler(
|
||||
typeof handler === 'function' ? handler : () => {},
|
||||
);
|
||||
const timeoutId = setTimeout(wrappedHandler, timeout);
|
||||
|
||||
plugin.timeouts.add(timeoutId);
|
||||
|
||||
@@ -72,6 +128,23 @@ export function createSandbox(
|
||||
|
||||
plugin.timeouts.delete(id);
|
||||
}),
|
||||
setInterval: ses.harden(
|
||||
(...[handler, interval]: Parameters<typeof setInterval>) => {
|
||||
const wrappedHandler = wrapHandler(
|
||||
typeof handler === 'function' ? handler : () => {},
|
||||
);
|
||||
const intervalId = setInterval(wrappedHandler, interval);
|
||||
|
||||
plugin.intervals.add(intervalId);
|
||||
|
||||
return ses.safeReturn(intervalId);
|
||||
},
|
||||
) as typeof setInterval,
|
||||
clearInterval: ses.harden((id: ReturnType<typeof setInterval>) => {
|
||||
clearInterval(id);
|
||||
|
||||
plugin.intervals.delete(id);
|
||||
}),
|
||||
|
||||
/**
|
||||
* GLOBAL FUNCTIONS ACCESIBLE TO PLUGINS
|
||||
|
||||
@@ -19,6 +19,10 @@ vi.mock('./create-plugin', () => ({
|
||||
createPlugin: vi.fn(),
|
||||
}));
|
||||
|
||||
vi.mock('./create-sandbox.js', () => ({
|
||||
markPluginError: vi.fn(),
|
||||
}));
|
||||
|
||||
vi.mock('./ses.js', () => ({
|
||||
ses: {
|
||||
harden: vi.fn().mockImplementation((obj) => obj),
|
||||
@@ -102,16 +106,17 @@ describe('plugin-loader', () => {
|
||||
});
|
||||
|
||||
it('should handle errors and close all plugins', async () => {
|
||||
const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
|
||||
|
||||
vi.mocked(createPlugin).mockRejectedValue(
|
||||
new Error('Plugin creation failed'),
|
||||
);
|
||||
|
||||
await loadPlugin(manifest);
|
||||
try {
|
||||
await loadPlugin(manifest);
|
||||
} catch (err) {
|
||||
expect.assert(err);
|
||||
}
|
||||
|
||||
expect(getPlugins()).toHaveLength(0);
|
||||
expect(consoleSpy).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should handle messages sent to plugins', async () => {
|
||||
|
||||
@@ -64,11 +64,10 @@ export const loadPlugin = async function (
|
||||
},
|
||||
apiExtensions,
|
||||
);
|
||||
|
||||
plugins.push(plugin);
|
||||
} catch (error) {
|
||||
closeAllPlugins();
|
||||
console.error(error);
|
||||
throw error;
|
||||
}
|
||||
};
|
||||
|
||||
@@ -77,12 +76,12 @@ export const ɵloadPlugin = async function (
|
||||
closeCallback?: () => void,
|
||||
apiExtensions?: object,
|
||||
) {
|
||||
loadPlugin(manifest, closeCallback, apiExtensions);
|
||||
await loadPlugin(manifest, closeCallback, apiExtensions);
|
||||
};
|
||||
|
||||
export const ɵloadPluginByUrl = async function (manifestUrl: string) {
|
||||
const manifest = await loadManifest(manifestUrl);
|
||||
ɵloadPlugin(manifest);
|
||||
await ɵloadPlugin(manifest);
|
||||
};
|
||||
|
||||
export const ɵunloadPlugin = function (id: Manifest['pluginId']) {
|
||||
|
||||
@@ -21,6 +21,7 @@ export async function createPluginManager(
|
||||
let modal: PluginModalElement | null = null;
|
||||
let uiMessagesCallbacks: ((message: unknown) => void)[] = [];
|
||||
const timeouts = new Set<ReturnType<typeof setTimeout>>();
|
||||
const intervals = new Set<ReturnType<typeof setInterval>>();
|
||||
|
||||
const allowDownloads = !!manifest.permissions.find(
|
||||
(s) => s === 'allow:downloads',
|
||||
@@ -55,6 +56,9 @@ export async function createPluginManager(
|
||||
timeouts.forEach(clearTimeout);
|
||||
timeouts.clear();
|
||||
|
||||
intervals.forEach(clearInterval);
|
||||
intervals.clear();
|
||||
|
||||
if (modal) {
|
||||
modal.removeEventListener('close', closePlugin);
|
||||
modal.remove();
|
||||
@@ -151,6 +155,9 @@ export async function createPluginManager(
|
||||
get timeouts() {
|
||||
return timeouts;
|
||||
},
|
||||
get intervals() {
|
||||
return intervals;
|
||||
},
|
||||
get code() {
|
||||
return code;
|
||||
},
|
||||
|
||||
Reference in New Issue
Block a user