From fe789c5d52e76fad654944cbd7c66da253aaa8b6 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Thu, 6 Aug 2026 15:36:21 -0700 Subject: [PATCH] fix(renderer): contain corrupt lazy chunks when the recovery reload never lands (#12950) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(renderer): contain corrupt lazy chunks when the recovery reload never lands 9 react-error-boundary crash reports across v1.4.171-1.4.175 (macOS, Linux, Windows) all end the same way: a corrupt lazy chunk fails to import, recovery requests a reload, the reload never lands, and loadLazyWithRetry re-throws the raw SyntaxError/TypeError. RecoverableRenderErrorBoundary only suppresses LazyChunkLoadError, so the raw error files a user-facing crash report. LazyChunkLoadError was unreachable in production. Its precondition is a guard written by a *different* document ('reload-landed'), but the finally block clears that guard before the throw, so the only path that could construct it never ran. Confirmed by the shipped bundles: 16/16 lazy_chunk_reload_vetoed breadcrumbs carry outcome=never-landed, zero carry any other outcome, and no bundle contains a boundary-degraded breadcrumb. Route every exhausted-recovery path through exhaustedRecoveryFailure() so an attempted-and-failed recovery yields a LazyChunkLoadError the boundary can contain, and record a lazy_chunk_recovery_exhausted breadcrumb carrying the call site, the real chunk error, and the outcome. Deliberately unchanged: when recovery is never *attempted* (no window/SSR, blocked sessionStorage, guard write failure) the raw error is still thrown so normal crash reporting is unaffected. Only isKnownDynamicImportFailure matches are contained, so module logic bugs keep reporting. * perf(renderer): trim redundant work on the lazy-chunk failure path Hoist the dynamic-import message patterns to module scope so classification stops allocating seven RegExp objects per call, thread the already-computed classification into exhaustedRecoveryFailure so the guard-not-landed path does not re-run it, and bound recordedExhaustionKeys the way the breadcrumb and renderer-error key stores are bounded, since error.name is library-controlled. Failure path only; the success path is unchanged. * refactor(renderer): remove a transposition trap on the lazy-chunk failure path exhaustedRecoveryFailure ended in two adjacent booleans with opposite consequences: transposing them would have returned the raw SyntaxError and silently restored the crash this branch fixes, with no test able to catch it (the only call site passed true for both). The isChunkFailure parameter saved one regex scan on a path that only runs after a 10s reload wait, so drop it. Also evict recordedExhaustionKeys oldest-first instead of clearing wholesale, matching the breadcrumb and renderer-error key stores the comment cites, so an overflow cannot re-open the entire set to a repeat burst. * test(renderer): cover the exhaustion dedupe bound The bound had no coverage, unlike the crash-breadcrumb store it mirrors, so a refactor could drop it or invert the comparison with every test still green. Drive 200 distinct error names through the contained path and assert the set stays capped. Also move MAX_RECORDED_EXHAUSTION_KEYS above the comment that describes the set, not between them. * test(renderer): pin the exhaustion eviction policy, not just the cap The bound test asserted only the size cap, so it stayed green under the old wholesale clear(): after 200 distinct keys a clear-on-overflow leaves 72, which still satisfies the cap. Replay a key that oldest-first eviction retains and assert it emits no second breadcrumb — that fails under clear(), which would otherwise silently re-open the whole set to a repeat burst and flush the 30-entry ring the dedupe exists to protect. * refactor(renderer): cut the breadcrumb machinery down to the actual fix The lazy_chunk_recovery_exhausted breadcrumb was an optional addition that paid for itself in complexity and nothing else: it needed a dedupe set to avoid flushing the 30-entry ring, the set needed a bound because error.name is library-controlled, the bound needed an oldest-first eviction policy, and that needed two more tests plus a boolean parameter that review flagged as a transposition trap. On the dominant never-landed path it did not even fire, because lazy_chunk_reload_vetoed already records the same reloadKey, message and outcome. Drop it. Observability on every path returns to the main baseline, and the fix is what it always was: name an exhausted recovery so the boundary can contain it. Also revert the unrelated regex hoist -- its only caller is the failure path, so the saved allocations are noise. * Verify ordinary errors bypass lazy chunk containment Add test ensuring module evaluation bugs still surface despite never-landed reload attempts. Clarify containment scope: recovery only applies to known dynamic-import failures, not ordinary errors. --- ...ErrorBoundary.never-landed-reload.test.tsx | 90 +++++++++++ .../RecoverableRenderErrorBoundary.tsx | 1 + ...azy-with-retry.never-landed-reload.test.ts | 144 ++++++++++++++++++ src/renderer/src/lib/lazy-with-retry.test.ts | 35 +++-- src/renderer/src/lib/lazy-with-retry.ts | 64 +++++--- 5 files changed, 299 insertions(+), 35 deletions(-) create mode 100644 src/renderer/src/components/error-boundaries/RecoverableRenderErrorBoundary.never-landed-reload.test.tsx create mode 100644 src/renderer/src/lib/lazy-with-retry.never-landed-reload.test.ts diff --git a/src/renderer/src/components/error-boundaries/RecoverableRenderErrorBoundary.never-landed-reload.test.tsx b/src/renderer/src/components/error-boundaries/RecoverableRenderErrorBoundary.never-landed-reload.test.tsx new file mode 100644 index 000000000..ea32cc997 --- /dev/null +++ b/src/renderer/src/components/error-boundaries/RecoverableRenderErrorBoundary.never-landed-reload.test.tsx @@ -0,0 +1,90 @@ +// @vitest-environment happy-dom + +// The end-to-end shape of the 9 shipped lazy-chunk crash reports: a corrupt chunk +// fails, recovery requests a reload, the reload never lands, and the boundary files +// a react-error-boundary crash report instead of containing the failure. + +import { Suspense, act, type ReactElement, type ReactNode } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +import { lazyWithRetry, resetLazyChunkReloadRequestsForTest } from '@/lib/lazy-with-retry' +import { RecoverableRenderErrorBoundary } from './RecoverableRenderErrorBoundary' + +const reportCrashMock = vi.hoisted(() => vi.fn()) + +vi.mock('@/lib/react-error-boundary-reporting', () => ({ + reportReactErrorBoundaryCrash: reportCrashMock +})) + +const RELOAD_SETTLE_GRACE_MS = 10_000 + +globalThis.IS_REACT_ACT_ENVIRONMENT = true + +function BoundaryHarness({ children }: { children: ReactNode }): ReactElement { + return ( + + Loading...}>{children} + + ) +} + +describe('RecoverableRenderErrorBoundary after a recovery reload never lands', () => { + let root: Root | null = null + let container: HTMLDivElement | null = null + let consoleError: ReturnType + + beforeEach(() => { + vi.useFakeTimers() + reportCrashMock.mockReset() + window.sessionStorage.clear() + resetLazyChunkReloadRequestsForTest() + vi.spyOn(window.location, 'reload').mockImplementation(() => undefined) + consoleError = vi.spyOn(console, 'error').mockImplementation(() => undefined) + }) + + afterEach(() => { + if (root) { + act(() => root?.unmount()) + } + container?.remove() + root = null + container = null + window.sessionStorage.clear() + resetLazyChunkReloadRequestsForTest() + vi.restoreAllMocks() + vi.useRealTimers() + consoleError.mockRestore() + }) + + it('shows the fallback without filing a crash report', async () => { + const LazyCorruptChunk = lazyWithRetry( + () => Promise.reject(new SyntaxError("Unexpected token '}'")), + { retries: 0, reloadKey: 'right-sidebar' } + ) + container = document.createElement('div') + document.body.appendChild(container) + root = createRoot(container) + + await act(async () => { + root?.render( + + + + ) + }) + + // Outlive the reload settle grace window, then let React commit the rejection. + await act(async () => { + await vi.advanceTimersByTimeAsync(RELOAD_SETTLE_GRACE_MS + 50) + }) + await act(async () => { + await vi.advanceTimersByTimeAsync(0) + }) + + expect(container?.querySelector('[role="alert"]')).not.toBeNull() + // Before the fix the boundary received the raw SyntaxError and filed a report; + // that is exactly what produced all 9 shipped crash reports. + expect(reportCrashMock).not.toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/components/error-boundaries/RecoverableRenderErrorBoundary.tsx b/src/renderer/src/components/error-boundaries/RecoverableRenderErrorBoundary.tsx index eddfbd142..e4c03e96a 100644 --- a/src/renderer/src/components/error-boundaries/RecoverableRenderErrorBoundary.tsx +++ b/src/renderer/src/components/error-boundaries/RecoverableRenderErrorBoundary.tsx @@ -50,6 +50,7 @@ export class RecoverableRenderErrorBoundary extends React.Component reload requested -> the reload never +// lands -> recovery gives up. 16/16 `lazy_chunk_reload_vetoed` breadcrumbs across +// the shipped bundles carry outcome=never-landed and no bundle contains a +// LazyChunkLoadError, so the boundary receives the raw SyntaxError and files a crash. + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +import { ORCA_RENDERER_UNLOAD_PREVENTED_EVENT } from '../../../shared/renderer-shutdown-events' +import { + isLazyChunkLoadError, + loadLazyWithRetry, + resetLazyChunkReloadRequestsForTest +} from './lazy-with-retry' + +const RELOAD_GUARD_KEY = 'orca:lazy-chunk-reload-attempted' +const RELOAD_SETTLE_GRACE_MS = 10_000 + +// The dominant crash-time message across the shipped bundles (7/9 reports). +const CORRUPT_CHUNK_ERROR = (): SyntaxError => new SyntaxError("Unexpected token '}'") + +type Breadcrumb = { name: string; data: Record } + +function installBreadcrumbSink(): Breadcrumb[] { + const breadcrumbs: Breadcrumb[] = [] + ;(window as unknown as { api: unknown }).api = { + crashReports: { + recordBreadcrumb: (crumb: Breadcrumb) => { + breadcrumbs.push(crumb) + } + } + } + return breadcrumbs +} + +describe('loadLazyWithRetry when the recovery reload never lands', () => { + beforeEach(() => { + vi.useFakeTimers() + window.sessionStorage.clear() + resetLazyChunkReloadRequestsForTest() + // Production truth: location.reload() produced zero navigations in all 10 + // bundles — no renderer_bootstrap_started follows any lazy_chunk_reload. + vi.spyOn(window.location, 'reload').mockImplementation(() => undefined) + }) + + afterEach(() => { + vi.useRealTimers() + vi.restoreAllMocks() + window.sessionStorage.clear() + resetLazyChunkReloadRequestsForTest() + delete (window as unknown as { api?: unknown }).api + }) + + it('surfaces a recognizable LazyChunkLoadError so the boundary can contain it', async () => { + const breadcrumbs = installBreadcrumbSink() + const settled = loadLazyWithRetry(() => Promise.reject(CORRUPT_CHUNK_ERROR()), { + retries: 0, + reloadKey: 'right-sidebar' + }).then( + () => ({ ok: true }) as const, + (error: unknown) => ({ ok: false, error }) as const + ) + + // Let the reload request run, then expire the settle grace window. + await vi.advanceTimersByTimeAsync(0) + await vi.advanceTimersByTimeAsync(RELOAD_SETTLE_GRACE_MS + 1) + + const result = await settled + expect(result.ok).toBe(false) + + const vetoed = breadcrumbs.find((crumb) => crumb.name === 'lazy_chunk_reload_vetoed') + expect(vetoed?.data.outcome).toBe('never-landed') + + // The boundary only suppresses LazyChunkLoadError; a raw SyntaxError files a crash report. + expect(isLazyChunkLoadError((result as { error: unknown }).error)).toBe(true) + }) + + it('does not strand a sibling lazy import that fails while a reload is pending', async () => { + installBreadcrumbSink() + const first = loadLazyWithRetry(() => Promise.reject(CORRUPT_CHUNK_ERROR()), { + retries: 0, + reloadKey: 'app.root' + }).catch((error: unknown) => error) + + await vi.advanceTimersByTimeAsync(0) + + // A second surface resolves its lazy chunk while the first reload is still in + // flight — the shape of 131d2ed2 and a7bc7be0, which filed raw crash reports + // 250 ms / 95 ms after the reload request with no explanatory breadcrumb. + const sibling = loadLazyWithRetry(() => Promise.reject(CORRUPT_CHUNK_ERROR()), { + retries: 0, + reloadKey: 'app.root.sibling' + }).catch((error: unknown) => error) + + await vi.advanceTimersByTimeAsync(RELOAD_SETTLE_GRACE_MS + 1) + + expect(isLazyChunkLoadError(await first)).toBe(true) + expect(isLazyChunkLoadError(await sibling)).toBe(true) + }) + + it('contains an unload-vetoed reload and records it as a distinct outcome', async () => { + const breadcrumbs = installBreadcrumbSink() + vi.spyOn(window.location, 'reload').mockImplementation(() => { + window.dispatchEvent(new Event(ORCA_RENDERER_UNLOAD_PREVENTED_EVENT)) + }) + + const settled = loadLazyWithRetry(() => Promise.reject(CORRUPT_CHUNK_ERROR()), { + retries: 0 + }).catch((error: unknown) => error) + await vi.advanceTimersByTimeAsync(0) + + const vetoed = breadcrumbs.find((crumb) => crumb.name === 'lazy_chunk_reload_vetoed') + expect(vetoed?.data.outcome).toBe('unload-vetoed') + // A veto is still an attempted-and-failed recovery, so it is contained too. + expect(isLazyChunkLoadError(await settled)).toBe(true) + }) + + it('leaves no guard behind that would block a later document from recovering', async () => { + installBreadcrumbSink() + const settled = loadLazyWithRetry(() => Promise.reject(CORRUPT_CHUNK_ERROR()), { + retries: 0 + }).catch((error: unknown) => error) + await vi.advanceTimersByTimeAsync(0) + await vi.advanceTimersByTimeAsync(RELOAD_SETTLE_GRACE_MS + 1) + + expect(isLazyChunkLoadError(await settled)).toBe(true) + expect(window.sessionStorage.getItem(RELOAD_GUARD_KEY)).toBeNull() + }) + + it('still surfaces ordinary evaluation bugs after a never-landed reload attempt', async () => { + const error = new Error('render bug from lazy module evaluation') + const settled = loadLazyWithRetry(() => Promise.reject(error), { retries: 0 }).catch( + (rejection: unknown) => rejection + ) + await vi.advanceTimersByTimeAsync(0) + await vi.advanceTimersByTimeAsync(RELOAD_SETTLE_GRACE_MS + 1) + + // Containment is only for known dynamic-import failures; real bugs must still report. + expect(await settled).toBe(error) + expect(isLazyChunkLoadError(error)).toBe(false) + }) +}) diff --git a/src/renderer/src/lib/lazy-with-retry.test.ts b/src/renderer/src/lib/lazy-with-retry.test.ts index b38386d2f..86b0bcef2 100644 --- a/src/renderer/src/lib/lazy-with-retry.test.ts +++ b/src/renderer/src/lib/lazy-with-retry.test.ts @@ -124,7 +124,7 @@ describe('loadLazyWithRetry', () => { expect(settled).toBe(false) }) - it('surfaces the original error when the guarded reload never tears the document down', async () => { + it('contains the failure when the guarded reload never tears the document down', async () => { const reload = spyOnReload() stubCrashReportsBreadcrumb() const error = chunkParseError() @@ -146,7 +146,9 @@ describe('loadLazyWithRetry', () => { expect(settled).toBe('pending') await vi.advanceTimersByTimeAsync(10_000) - expect(settled).toBe(error) + // The reload was the last recovery step, so the boundary gets a nameable error. + expect(isLazyChunkLoadError(settled)).toBe(true) + expect(settled).toMatchObject({ cause: error }) expect(reload).toHaveBeenCalledTimes(1) }) @@ -169,23 +171,26 @@ describe('loadLazyWithRetry', () => { expect(isLazyChunkLoadError(caught)).toBe(true) }) - it('reports the original error when the guard belongs to this same document (reload vetoed)', async () => { + it('contains the failure when the guard belongs to this same document (reload vetoed)', async () => { const reload = spyOnReload() window.sessionStorage.setItem(RELOAD_GUARD_KEY, String(performance.timeOrigin)) const error = chunkParseError() const factory = vi.fn(() => Promise.reject(error)) const loaded = loadLazyWithRetry(factory, { retries: 0 }) - const assertion = expect(loaded).rejects.toBe(error) + const assertion = expect(loaded).rejects.toMatchObject({ + name: 'LazyChunkLoadError', + cause: error + }) await vi.advanceTimersByTimeAsync(5000) await assertion expect(reload).not.toHaveBeenCalled() const caught = await loaded.catch((rejection) => rejection) - expect(isLazyChunkLoadError(caught)).toBe(false) + expect(isLazyChunkLoadError(caught)).toBe(true) }) - it('records a lazy_chunk_reload_vetoed breadcrumb in the tick that reports the crash', async () => { + it('records a lazy_chunk_reload_vetoed breadcrumb in the tick that contains the failure', async () => { spyOnReload() const recordBreadcrumb = stubCrashReportsBreadcrumb() window.sessionStorage.setItem(RELOAD_GUARD_KEY, String(performance.timeOrigin)) @@ -195,7 +200,7 @@ describe('loadLazyWithRetry', () => { retries: 0, reloadKey: 'rich-markdown-editor' }) - const assertion = expect(loaded).rejects.toBe(error) + const assertion = expect(loaded).rejects.toMatchObject({ name: 'LazyChunkLoadError' }) await vi.advanceTimersByTimeAsync(1) await assertion @@ -471,7 +476,8 @@ describe('loadLazyWithRetry recovery reload vs the dirty-editor-tab unload veto' await vi.advanceTimersByTimeAsync(5000) expect(harness.navigations).toEqual([]) - expect(settled).toBe(error) + expect(isLazyChunkLoadError(settled)).toBe(true) + expect(settled).toMatchObject({ cause: error }) expect(restartAborted).toHaveBeenCalled() expect(isIntentionalAppRestartInProgress()).toBe(false) expect(recordBreadcrumb).toHaveBeenCalledWith({ @@ -498,7 +504,8 @@ describe('loadLazyWithRetry recovery reload vs the dirty-editor-tab unload veto' reloadKey: 'rich-markdown-editor' }).catch((rejection: unknown) => rejection) - expect(settled).toBe(error) + expect(isLazyChunkLoadError(settled)).toBe(true) + expect(settled).toMatchObject({ cause: error }) expect(harness.hotExitBackups).toBe(1) expect(isIntentionalAppRestartInProgress()).toBe(false) expect(window.sessionStorage.getItem(RELOAD_GUARD_KEY)).toBeNull() @@ -531,7 +538,8 @@ describe('loadLazyWithRetry recovery reload vs the dirty-editor-tab unload veto' ) await vi.advanceTimersByTimeAsync(50) - expect(settled).toBe(error) + expect(isLazyChunkLoadError(settled)).toBe(true) + expect(settled).toMatchObject({ cause: error }) expect(vi.getTimerCount()).toBe(0) }) @@ -555,13 +563,14 @@ describe('loadLazyWithRetry recovery reload vs the dirty-editor-tab unload veto' return settled } - expect(await attempt()).toBe(error) + expect(isLazyChunkLoadError(await attempt())).toBe(true) expect(window.sessionStorage.getItem(RELOAD_GUARD_KEY)).toBeNull() - expect(await attempt()).toBe(error) + expect(isLazyChunkLoadError(await attempt())).toBe(true) expect(reload).toHaveBeenCalledTimes(2) - expect(await attempt()).toBe(error) + // Cap reached: still contained, but no third navigation. + expect(isLazyChunkLoadError(await attempt())).toBe(true) expect(reload).toHaveBeenCalledTimes(2) }) diff --git a/src/renderer/src/lib/lazy-with-retry.ts b/src/renderer/src/lib/lazy-with-retry.ts index b052fa8e0..77650b67d 100644 --- a/src/renderer/src/lib/lazy-with-retry.ts +++ b/src/renderer/src/lib/lazy-with-retry.ts @@ -130,6 +130,13 @@ function recordReloadBreadcrumb( const wait = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)) +/** Recovery is spent, so name the failure in the one way the boundary can contain. */ +function containedChunkFailure(lastError: unknown, reloadKey: string): unknown { + return isKnownDynamicImportFailure(lastError) + ? new LazyChunkLoadError(lastError, reloadKey) + : lastError +} + function isKnownDynamicImportFailure(error: unknown): boolean { if (!(error instanceof Error)) { return false @@ -140,11 +147,14 @@ function isKnownDynamicImportFailure(error: unknown): boolean { } // Why: a stale/truncated/corrupt chunk parses as invalid JS, so import() - // rejects with a native SyntaxError ("Unexpected token ')'", "Unexpected end - // of input", …). That reaches this catch only from the chunk's fetch+parse - // phase — a recoverable corrupt-chunk failure. Genuine module-evaluation - // logic bugs throw ordinary Errors (still surfaced raw) or fail later during - // React render (outside this load path), so they are unaffected. + // rejects with a native SyntaxError ("Unexpected token '}'", "Unexpected + // string", "Illegal return statement", … — all four observed in shipped + // reports). That reaches this catch only from the chunk's fetch+parse phase. + // Trade-off: a SyntaxError thrown while *evaluating* a lazily imported module + // (e.g. a top-level JSON.parse) is indistinguishable here by message, so it is + // contained too. Stack-frame discrimination was tried and rejected: parser + // errors carry no frames today, but that is not guaranteed across V8 versions + // and a false negative silently restores the crash this guards against. if (error.name === 'SyntaxError') { return true } @@ -190,6 +200,7 @@ export async function loadLazyWithRetry( reloadRequestsThisDocument < MAX_RELOAD_REQUESTS_PER_DOCUMENT ) { if (!markChunkReloadAttempted()) { + // No recovery was attempted, so keep normal reporting rather than containing. throw lastError } reloadRequestsThisDocument += 1 @@ -204,29 +215,38 @@ export async function loadLazyWithRetry( clearChunkReloadGuard() recordReloadBreadcrumb('lazy_chunk_reload_vetoed', reloadKey, failureMessage, outcome) } - throw lastError + // The reload was this document's last recovery step for this chunk, whether it + // was refused outright or simply never navigated. + throw containedChunkFailure(lastError, reloadKey) } - if (reloadGuardState === 'reload-landed' && isKnownDynamicImportFailure(lastError)) { - throw new LazyChunkLoadError(lastError, reloadKey) + if (reloadGuardState === 'reload-landed') { + throw containedChunkFailure(lastError, reloadKey) } - if ( - reloadGuardState === 'reload-not-landed' && - !reloadRequestInFlight && - isKnownDynamicImportFailure(lastError) - ) { - // Record the veto beside the resulting crash report before the ring can evict it. - clearChunkReloadGuard() - recordReloadBreadcrumb( - 'lazy_chunk_reload_vetoed', - reloadKey, - failureMessage, - 'guard-not-landed' - ) + if (reloadGuardState === 'reload-not-landed') { + // A sibling failing under a pending reload must not clear the guard, and an + // unrelated failure must not clear a guard it did not set. + if (!reloadRequestInFlight && isKnownDynamicImportFailure(lastError)) { + // Record the veto before the ring can evict it; the failure is then contained. + clearChunkReloadGuard() + recordReloadBreadcrumb( + 'lazy_chunk_reload_vetoed', + reloadKey, + failureMessage, + 'guard-not-landed' + ) + } + throw containedChunkFailure(lastError, reloadKey) } - // Without a proven reload, preserve normal error-reporting behavior. + if (reloadGuardState === 'not-attempted') { + // The per-document reload cap is spent; further failures cannot recover. + throw containedChunkFailure(lastError, reloadKey) + } + + // No window or no usable storage: recovery was never attempted, so preserve + // normal error reporting instead of containing a failure we never acted on. throw lastError }