Skip to content

Commit a37a59f

Browse files
perf: Unload iframe before removing (#3796)
To drastically reduce the amount of memory leaked if the iframe cannot be garbage collected properly, this PR modifies `terminateJob` to navigate the iframe away from the Snaps execution environment before removing it, effectively unloading it. Before: ``` Explicit Allocations 15.36 MB (100.0%) -- explicit └──15.36 MB (100.0%) -- window-objects/top(none)/detached ├──15.26 MB (99.40%) -- window(http://localhost:6363/) │ ├───7.67 MB (49.97%) ++ js-realm(moz-nullprincipal:{45eed705-e0da-4e2b-b0bf-e7ac336c93f4}?http://localhost:6363, http://localhost:6363/) │ ├───5.76 MB (37.49%) ++ js-realm(moz-nullprincipal:{8cb3b86e-774f-4a1a-bf6a-eeac2636fe9f}?http://localhost:6363, http://localhost:6363/) │ ├───1.83 MB (11.94%) ++ dom │ └───0.00 MB (00.00%) ++ (2 tiny) └───0.09 MB (00.60%) ++ window(chrome://devtools/content/shared/webextension-fallback.html#webextension-flask@metamask.io) ``` After: ``` Explicit Allocations 0.14 MB (100.0%) -- explicit └──0.14 MB (100.0%) -- window-objects/top(none)/detached/window(about:blank) ├──0.06 MB (46.17%) ++ js-realm(moz-nullprincipal:{0114dc8e-3f5e-4a2c-8008-c4219c84dcc0}?moz-extension://1465affa-73bf-4b77-9103-8afb21b2fa9d, about:blank) ├──0.06 MB (46.17%) ++ js-realm(moz-nullprincipal:{7e2fd14e-3040-493d-862a-9c207eae5213}?moz-extension://1465affa-73bf-4b77-9103-8afb21b2fa9d, about:blank) ├──0.01 MB (07.16%) ++ dom └──0.00 MB (00.50%) ++ (2 tiny) ``` <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Improves termination flow and reduces iframe memory retention by making job termination asynchronous and explicitly unloading iframes before removal. > > - Changes `AbstractExecutionService.terminateJob` to return a `Promise<void>` and awaits it in `terminateSnap` and when init times out in `#initStreams` > - In `IframeExecutionService`, `terminateJob` now navigates the iframe to `about:blank` (with a short `withTimeout`-guarded wait for `load`) before removing it > - Updates `NodeProcessExecutionService` and `WebViewExecutionService` `terminateJob` implementations to async (no functional change beyond signature) > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit c5ec278. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent 2b60768 commit a37a59f

4 files changed

Lines changed: 34 additions & 7 deletions

File tree

packages/snaps-controllers/src/services/AbstractExecutionService.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,9 @@ export abstract class AbstractExecutionService<WorkerType>
153153
*
154154
* @param job - The object corresponding to the job to be terminated.
155155
*/
156-
protected abstract terminateJob(job: TerminateJobArgs<WorkerType>): void;
156+
protected abstract terminateJob(
157+
job: TerminateJobArgs<WorkerType>,
158+
): Promise<void>;
157159

158160
/**
159161
* Terminates the Snap with the specified ID and deletes all its associated
@@ -197,7 +199,7 @@ export abstract class AbstractExecutionService<WorkerType>
197199
}
198200
});
199201

200-
this.terminateJob(job);
202+
await this.terminateJob(job);
201203

202204
this.#jobs.delete(snapId);
203205
this.#status.delete(snapId);
@@ -258,7 +260,7 @@ export abstract class AbstractExecutionService<WorkerType>
258260

259261
if (result === hasTimedOut) {
260262
// For certain environments, such as the iframe we may have already created the worker and wish to terminate it.
261-
this.terminateJob({ id: snapId });
263+
await this.terminateJob({ id: snapId });
262264

263265
const status = this.#status.get(snapId);
264266
if (status === 'created') {

packages/snaps-controllers/src/services/iframe/IframeExecutionService.ts

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import type { BasePostMessageStream } from '@metamask/post-message-stream';
22
import { WindowPostMessageStream } from '@metamask/post-message-stream';
33
import { createWindow } from '@metamask/snaps-utils';
44

5+
import { withTimeout } from '../../utils';
56
import type {
67
ExecutionServiceArgs,
78
TerminateJobArgs,
@@ -29,8 +30,28 @@ export class IframeExecutionService extends AbstractExecutionService<Window> {
2930
this.iframeUrl = iframeUrl;
3031
}
3132

32-
protected terminateJob(jobWrapper: TerminateJobArgs<Window>): void {
33-
document.getElementById(jobWrapper.id)?.remove();
33+
protected async terminateJob(
34+
jobWrapper: TerminateJobArgs<Window>,
35+
): Promise<void> {
36+
const iframe = document.getElementById(
37+
jobWrapper.id,
38+
) as HTMLIFrameElement | null;
39+
40+
if (!iframe) {
41+
return;
42+
}
43+
44+
iframe.id = '';
45+
46+
await withTimeout(
47+
new Promise<void>((resolve) => {
48+
iframe.addEventListener('load', () => resolve(), { once: true });
49+
iframe.src = 'about:blank';
50+
}),
51+
10,
52+
);
53+
54+
iframe.remove();
3455
}
3556

3657
protected async initEnvStream(snapId: string): Promise<{

packages/snaps-controllers/src/services/node-js/NodeProcessExecutionService.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,9 @@ export class NodeProcessExecutionService extends AbstractExecutionService<ChildP
3636
return Promise.resolve({ worker, stream });
3737
}
3838

39-
protected terminateJob(jobWrapper: TerminateJobArgs<ChildProcess>): void {
39+
protected async terminateJob(
40+
jobWrapper: TerminateJobArgs<ChildProcess>,
41+
): Promise<void> {
4042
jobWrapper.worker?.kill();
4143
}
4244
}

packages/snaps-controllers/src/services/webview/WebViewExecutionService.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,9 @@ export class WebViewExecutionService extends AbstractExecutionService<WebViewInt
5353
return { worker: webView, stream };
5454
}
5555

56-
protected terminateJob(jobWrapper: TerminateJobArgs<WebViewInterface>): void {
56+
protected async terminateJob(
57+
jobWrapper: TerminateJobArgs<WebViewInterface>,
58+
): Promise<void> {
5759
this.#removeWebView(jobWrapper.id);
5860
}
5961
}

0 commit comments

Comments
 (0)