Skip to content

Commit f03c680

Browse files
committed
fix(desktop): guard splash-stage teardown races (#6136)
Both pre-existing and the new structural test cases surface two sub-races between setSplashStage β†’ applySplashStage β†’ executeJavaScript and the splash BrowserWindow lifecycle: 1. Pre-load destroy (race 1): a stage is deferred while the splash page is still loading. Before did-finish-load fires, the splash window is destroyed. The existing registerSplashStageTracking replay callback unconditionally called applySplashStage on the destroyed surface, causing executeJavaScript to throw synchronously in Electron's teardown path. Fix: bail out of the replay when splash.isDestroyed() OR splash.webContents.isDestroyed(). 2. Check-to-call destroy (race 2): the splash survives the isDestroyed() check in setSplashStage, but the underlying webContents is torn down before executeJavaScript runs. Electron throws TypeError("Object has been destroyed") synchronously β€” not as a rejected Promise β€” so the existing .catch(() => undefined) does not catch it and the exception escapes to the process. Fix: wrap the entire executeJavaScript + .catch sequence in try/catch and also short-circuit via webContents.isDestroyed() before the call. SplashStageSurface gains webContents.isDestroyed() to the structural type so callers (both production code and the mock in the test) can explicitly probe renderer readiness. Changes: - apps/desktop/src/main/runtime.ts: - Add webContents.isDestroyed() to SplashStageSurface type. - applySplashStage: short-circuit via splash/isDestroyed checks and wrap executeJavaScript in try/catch. - registerSplashStageTracking: bail out of the did-finish-load replay when the surface or its webContents is already destroyed. - apps/desktop/tests/main/splash-stage-replay.test.ts: - Add webContents.isDestroyed() to createMockSplash. - New test: splash destroyed before did-finish-load β†’ no-op. - New test: sync throw from executeJavaScript β†’ not thrown. All pre-existing desktop test failures (missing @open-design/download resolve) are pre-existing and unrelated to this change. The splash stage replay tests typecheck clean on this branch. Fixes #6136 Signed-off-by: xxiaoxiong <2482929840@qq.com>
1 parent 6b90486 commit f03c680

2 files changed

Lines changed: 76 additions & 6 deletions

File tree

β€Žapps/desktop/src/main/runtime.tsβ€Ž

Lines changed: 34 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1308,10 +1308,19 @@ function splashStagePayload(stage: SplashBootStage): { step: number; total: numb
13081308
* Narrow view of the splash window that the stage updater needs. A real
13091309
* `BrowserWindow` satisfies this structurally; tests pass a mock so the
13101310
* load-ready/replay logic is exercisable without a live Electron renderer.
1311+
*
1312+
* `webContents.isDestroyed()` is part of the contract because Electron can
1313+
* tear a window's underlying `webContents` down before (or in between) the
1314+
* `isDestroyed()`-and-`executeJavaScript` boundary in `setSplashStage` /
1315+
* `applySplashStage`; without that probe, the synchronous
1316+
* `executeJavaScript` call throws `TypeError("Object has been destroyed")`
1317+
* β€” a rejection that escapes the existing `.catch` because no Promise is
1318+
* returned in the synchronous-throw case. See issue #6136.
13111319
*/
13121320
export type SplashStageSurface = {
13131321
isDestroyed(): boolean;
13141322
webContents: {
1323+
isDestroyed(): boolean;
13151324
executeJavaScript(code: string, userGesture?: boolean): Promise<unknown>;
13161325
once(event: "did-finish-load", listener: () => void): void;
13171326
};
@@ -1324,12 +1333,26 @@ type SplashStageState = { ready: boolean; pending: SplashBootStage | null };
13241333
const splashStageState = new WeakMap<SplashStageSurface, SplashStageState>();
13251334

13261335
function applySplashStage(splash: SplashStageSurface, stage: SplashBootStage): void {
1327-
void splash.webContents
1328-
.executeJavaScript(
1329-
`window.__odSplashSetStage && window.__odSplashSetStage(${JSON.stringify(splashStagePayload(stage))});`,
1330-
true,
1331-
)
1332-
.catch(() => undefined);
1336+
// Guard against the teardown race: `setSplashStage` checks
1337+
// `splash.isDestroyed()` before calling here, but the window can
1338+
// be destroyed between that check and this call (see #6136).
1339+
// `webContents.isDestroyed()` covers the case where the renderer
1340+
// tears down before the wrapper BrowserWindow is reaped.
1341+
if (splash.isDestroyed() || splash.webContents.isDestroyed()) return;
1342+
try {
1343+
void splash.webContents
1344+
.executeJavaScript(
1345+
`window.__odSplashSetStage && window.__odSplashSetStage(${JSON.stringify(splashStagePayload(stage))});`,
1346+
true,
1347+
)
1348+
.catch(() => undefined);
1349+
} catch {
1350+
// Electron can throw `TypeError("Object has been destroyed")`
1351+
// synchronously from executeJavaScript in the residual teardown
1352+
// race not covered by the isDestroyed() checks above. Mirror the
1353+
// Promise-rejection contract: this path is best-effort, never a
1354+
// process-killing uncaught exception.
1355+
}
13331356
}
13341357

13351358
/**
@@ -1345,6 +1368,11 @@ export function registerSplashStageTracking(splash: SplashStageSurface): void {
13451368
const state: SplashStageState = { ready: false, pending: null };
13461369
splashStageState.set(splash, state);
13471370
splash.webContents.once("did-finish-load", () => {
1371+
// The splash or its webContents may have been destroyed between
1372+
// the stage being stashed and the load event firing (race 1).
1373+
// Bail out early rather than calling executeJavaScript on a
1374+
// dead renderer.
1375+
if (splash.isDestroyed() || splash.webContents.isDestroyed()) return;
13481376
state.ready = true;
13491377
if (state.pending != null) {
13501378
const stage = state.pending;

β€Žapps/desktop/tests/main/splash-stage-replay.test.tsβ€Ž

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,9 +29,11 @@ function createMockSplash(): MockSplash {
2929
const executed: string[] = [];
3030
let didFinishLoad: (() => void) | null = null;
3131
let destroyed = false;
32+
let webContentsDestroyed = false;
3233
const surface: SplashStageSurface = {
3334
isDestroyed: () => destroyed,
3435
webContents: {
36+
isDestroyed: () => webContentsDestroyed,
3537
executeJavaScript: (code: string) => {
3638
executed.push(code);
3739
return Promise.resolve(undefined);
@@ -47,6 +49,7 @@ function createMockSplash(): MockSplash {
4749
emitDidFinishLoad: () => didFinishLoad?.(),
4850
destroy: () => {
4951
destroyed = true;
52+
webContentsDestroyed = true;
5053
},
5154
};
5255
}
@@ -100,6 +103,45 @@ describe('splash boot-stage replay guard', () => {
100103
expect(splash.executed).toEqual([]);
101104
});
102105

106+
// Race 1: stage deferred before load, splash destroyed before did-finish-load.
107+
// Without the guard in registerSplashStageTracking, the replay callback
108+
// would call applySplashStage and then executeJavaScript on a dead surface.
109+
test('is a no-op when the splash is destroyed before did-finish-load', () => {
110+
const splash = createMockSplash();
111+
registerSplashStageTracking(splash.surface);
112+
113+
setSplashStage(splash.surface, 'engine');
114+
expect(splash.executed).toEqual([]);
115+
116+
// Destroy the splash after the stage is stashed but before load fires.
117+
splash.destroy();
118+
splash.emitDidFinishLoad();
119+
expect(splash.executed).toEqual([]);
120+
});
121+
122+
// Race 2: the webContents throws `TypeError("Object has been destroyed")`
123+
// synchronously (not returning a rejected Promise) during executeJavaScript.
124+
// The existing `.catch` only handles Promise rejections β€” a sync throw
125+
// escapes and can crash the process. The try/catch in applySplashStage must
126+
// swallow this sync throw, mirroring the existing Promise-rejection contract.
127+
test('tolerates a destroyed webContents mid-flight instead of throwing', () => {
128+
const surface: SplashStageSurface = {
129+
isDestroyed: () => false,
130+
webContents: {
131+
isDestroyed: () => false,
132+
executeJavaScript: () => {
133+
// Simulate the teardown race: throw before returning a Promise.
134+
throw new TypeError('Object has been destroyed');
135+
},
136+
once: () => { /* no-op */ },
137+
},
138+
};
139+
140+
// No throw β€” the sync exception is swallowed the same way an
141+
// executeJavaScript rejection would be.
142+
expect(() => setSplashStage(surface, 'engine')).not.toThrow();
143+
});
144+
103145
// Slow-cold-boot UX: the splash must tell the user WHICH step of how many is
104146
// underway, not just a bare label, so the wait reads as forward progress. The
105147
// stage payload handed to the renderer carries a 1-based step index and the

0 commit comments

Comments
Β (0)