From 91dfdf3d3f790875bf005c6da3e1fecd1857fd1c Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 25 Aug 2026 02:23:27 -0700 Subject: [PATCH] fix(crash-reporting): stop filing recoverable Chromium utility teardown as app crashes Chromium spawns utility services on demand and tears them down as routine churn, but only three of them (audio, network, video capture) were on the recoverable allowlist. Everything else surfaced to the user as an Orca crash and landed in triage as a real defect: the 1.4.188 batch filed the PAC proxy resolver and the print compositor that way, one of them carrying a bug note about opening a 14MB JSON file. An allowlist of Chromium's service list can only ever be behind it, so the default inverts: any well-formed Mojo service name is recoverable churn, and the enumeration now covers the two utilities whose death is ours to answer for -- NodeService (Orca's own forked code) and StorageService (possible user data loss). Two consequences of widening suppression are fixed with it: - A burst across the whole service population no longer wipes the pre-crash trail. Coalescing is per key, so 30 different services meant 30 breadcrumb ring slots and the 30-entry ring evicted every user action leading up to the crash. Suppressed churn now holds at most a fifth of the ring, with the rest folded into one overflow slot that keeps counting them. - A suppressed crash no longer donates its minidump to an unrelated report. Nothing claims the dump Crashpad writes for a suppressed exit, so the next reportable crash of the same process type adopted it and attached another service's CHECK file, line and stack -- sending a triager after the wrong crash. Each suppressed exit now fences off one same-type dump. Suppression stays breadcrumb-only: the exit is still in the durable trail as process_gone_suppressed, but no report, no minidump signature, no dump claim. That is why the denylist is the only place to keep a service's post-mortem detail, and why it is documented on the predicate itself. --- ...ium-utility-service-recoverability.test.ts | 39 +++++ ...chromium-utility-service-recoverability.ts | 35 +++++ src/main/crash-reporting/crashpad-capture.ts | 57 +++++++ .../crash-reporting/crashpad-process-type.ts | 14 ++ .../process-gone-classification.test.ts | 57 +++++++ .../process-gone-classification.ts | 17 +-- .../process-gone-killed-one-ordering.test.ts | 3 + .../process-gone-recorder.test.ts | 5 +- .../crash-reporting/process-gone-recorder.ts | 58 ++----- .../suppressed-process-gone-breadcrumb.ts | 75 +++++++++ ...uppressed-process-gone-ring-budget.test.ts | 87 +++++++++++ .../suppressed-process-gone-ring-budget.ts | 49 ++++++ ...essed-utility-crash-diagnosability.test.ts | 98 ++++++++++++ ...uppressed-utility-dump-attribution.test.ts | 142 ++++++++++++++++++ 14 files changed, 673 insertions(+), 63 deletions(-) create mode 100644 src/main/crash-reporting/chromium-utility-service-recoverability.test.ts create mode 100644 src/main/crash-reporting/chromium-utility-service-recoverability.ts create mode 100644 src/main/crash-reporting/crashpad-process-type.ts create mode 100644 src/main/crash-reporting/suppressed-process-gone-ring-budget.test.ts create mode 100644 src/main/crash-reporting/suppressed-process-gone-ring-budget.ts create mode 100644 src/main/crash-reporting/suppressed-utility-crash-diagnosability.test.ts create mode 100644 src/main/crash-reporting/suppressed-utility-dump-attribution.test.ts diff --git a/src/main/crash-reporting/chromium-utility-service-recoverability.test.ts b/src/main/crash-reporting/chromium-utility-service-recoverability.test.ts new file mode 100644 index 00000000000..84dabbaca70 --- /dev/null +++ b/src/main/crash-reporting/chromium-utility-service-recoverability.test.ts @@ -0,0 +1,39 @@ +import { describe, expect, it } from 'vitest' +import { isRecoverableChromiumUtilityService } from './chromium-utility-service-recoverability' + +describe('isRecoverableChromiumUtilityService', () => { + it('treats the on-demand services from the 1.4.188 crash batch as recoverable', () => { + expect(isRecoverableChromiumUtilityService('proxy_resolver.mojom.ProxyResolverFactory')).toBe( + true + ) + expect(isRecoverableChromiumUtilityService('printing.mojom.PrintCompositor')).toBe(true) + }) + + it('keeps the previously enumerated services recoverable', () => { + expect(isRecoverableChromiumUtilityService('audio.mojom.AudioService')).toBe(true) + expect(isRecoverableChromiumUtilityService('network.mojom.NetworkService')).toBe(true) + expect(isRecoverableChromiumUtilityService('video_capture.mojom.VideoCaptureService')).toBe( + true + ) + }) + + it('covers Chromium services the allowlist never enumerated', () => { + expect(isRecoverableChromiumUtilityService('data_decoder.mojom.DataDecoderService')).toBe(true) + expect(isRecoverableChromiumUtilityService('media.mojom.MediaFoundationService')).toBe(true) + expect(isRecoverableChromiumUtilityService('tracing.mojom.TracedProcess')).toBe(true) + }) + + it('reports the utility that hosts Orca code and the one holding user data', () => { + expect(isRecoverableChromiumUtilityService('node.mojom.NodeService')).toBe(false) + expect(isRecoverableChromiumUtilityService('storage.mojom.StorageService')).toBe(false) + }) + + it('reports anything that is not a Chromium Mojo service name', () => { + expect(isRecoverableChromiumUtilityService(undefined)).toBe(false) + expect(isRecoverableChromiumUtilityService('')).toBe(false) + expect(isRecoverableChromiumUtilityService('com.orca.unexpected')).toBe(false) + expect(isRecoverableChromiumUtilityService('Orca Helper')).toBe(false) + expect(isRecoverableChromiumUtilityService('mojom.Thing')).toBe(false) + expect(isRecoverableChromiumUtilityService('printing.mojom.')).toBe(false) + }) +}) diff --git a/src/main/crash-reporting/chromium-utility-service-recoverability.ts b/src/main/crash-reporting/chromium-utility-service-recoverability.ts new file mode 100644 index 00000000000..78204b39c42 --- /dev/null +++ b/src/main/crash-reporting/chromium-utility-service-recoverability.ts @@ -0,0 +1,35 @@ +// Chromium spawns utility services on demand and tears them down as routine +// churn, so an allowlist of them can only ever be behind: 1.4.188 filed the PAC +// evaluator and the print compositor as user-facing crashes because neither was +// enumerated. We cannot enumerate Chromium's service list, but we can enumerate +// the services whose death is ours to answer for, so the default inverts and the +// exceptions below carry the enumeration. +const NON_RECOVERABLE_UTILITY_SERVICE_NAMES = new Set([ + // Electron's utilityProcess.fork() host: this is Orca's own code dying. + 'node.mojom.NodeService', + // Backs durable profile storage, so an exit can mean real user data loss. + 'storage.mojom.StorageService' +]) + +// Mojo service names are `.mojom.`; anything else is not a +// Chromium-owned service and has to keep reporting. +const CHROMIUM_MOJO_SERVICE_NAME = /^[a-z0-9_]+(?:\.[a-z0-9_]+)*\.mojom\.[A-Za-z0-9_]+$/ + +/** + * Whether a `child-process-gone` utility exit is routine Chromium churn. + * + * Suppression is breadcrumb-only. The exit lands in the durable trail as + * `process_gone_suppressed` carrying {source, processType, serviceName, reason, + * exitCode} and nothing more: no crash report, no minidump signature, no + * `electron.minidump_signature` span in the diagnostic bundle, and the dump is + * never claimed, so pruning can reclaim it. A suppressed CHECK failure stays an + * anonymous 0x80000003 -- no name, file, line or stack survives anywhere. + * So deny a service below whenever we would need that post-mortem detail to act + * on its death, not merely when the service sounds important. + */ +export function isRecoverableChromiumUtilityService(serviceName: string | undefined): boolean { + if (serviceName === undefined || NON_RECOVERABLE_UTILITY_SERVICE_NAMES.has(serviceName)) { + return false + } + return CHROMIUM_MOJO_SERVICE_NAME.test(serviceName) +} diff --git a/src/main/crash-reporting/crashpad-capture.ts b/src/main/crash-reporting/crashpad-capture.ts index 1f628fe9c24..7aed896b0c3 100644 --- a/src/main/crash-reporting/crashpad-capture.ts +++ b/src/main/crash-reporting/crashpad-capture.ts @@ -32,6 +32,15 @@ const MAX_STORED_DUMP_BYTES = 128 * 1024 * 1024 const MAX_STORED_DUMPS = 64 const DUMP_PRUNE_DELAY_MS = 2_000 +type SuppressedDumpBarrier = { + readonly processType: string + readonly armedAtMs: number +} + +// Bounds the list across a crash loop; a barrier older than the recency window +// can no longer match any capture anyway. +const MAX_SUPPRESSED_DUMP_BARRIERS = 32 + type DumpCandidate = { readonly filePath: string readonly mtimeMs: number @@ -46,6 +55,9 @@ let captureStarted = false let captureStartedAtMs: number | null = null const claimedDumpPaths = new Map() const reservedDumpPaths = new Set() +// Suppressed crashes still leave a dump behind and never claim one, so each is +// fenced off here for the next same-type capture to consume instead of adopt. +let suppressedDumpBarriers: SuppressedDumpBarrier[] = [] let dumpPruneTimer: NodeJS.Timeout | null = null export type CrashpadCaptureOptions = { @@ -110,6 +122,7 @@ export function _setCrashpadCaptureStateForTest( captureStartedAtMs = state?.started ? (state.startedAtMs ?? Number.NEGATIVE_INFINITY) : null claimedDumpPaths.clear() reservedDumpPaths.clear() + suppressedDumpBarriers = [] if (dumpPruneTimer) { clearTimeout(dumpPruneTimer) dumpPruneTimer = null @@ -269,6 +282,45 @@ export async function waitForCrashMinidump( return pollDumpCandidates(crashedAtMs, options, async (candidate) => candidate) } +/** + * Records that a crash we deliberately did not report may have left a dump. + * + * Why: the dump is on disk with nothing pointing at it, and the next reportable + * crash of the same process type polls for "a recent dump of my type" — it would + * otherwise attach the suppressed service's CHECK file, line and stack to an + * unrelated report, sending a triager after the wrong crash. + */ +export function noteSuppressedCrashDump(processType: string, crashedAtMs: number): void { + suppressedDumpBarriers.push({ processType, armedAtMs: crashedAtMs }) + if (suppressedDumpBarriers.length > MAX_SUPPRESSED_DUMP_BARRIERS) { + suppressedDumpBarriers.shift() + } +} + +/** One barrier absorbs one dump: consume it so a genuine later crash of the + * same type still pairs with the dump Crashpad writes for it. */ +function consumeSuppressedDumpBarrier( + dump: DumpCandidate, + processType: string | undefined, + crashedAtMs: number +): boolean { + if (processType === undefined) { + return false + } + const index = suppressedDumpBarriers.findIndex( + (barrier) => + barrier.processType === processType && + barrier.armedAtMs <= crashedAtMs && + barrier.armedAtMs >= crashedAtMs - DUMP_RECENCY_WINDOW_MS && + dump.mtimeMs >= barrier.armedAtMs + ) + if (index === -1) { + return false + } + suppressedDumpBarriers.splice(index, 1) + return true +} + export type CapturedMinidump = { readonly filePath: string readonly sizeBytes: number @@ -303,6 +355,11 @@ export async function captureMinidumpSignature( rejectedDumpPaths.add(dump.filePath) return null } + if (consumeSuppressedDumpBarrier(dump, options.expectedProcessType, crashedAtMs)) { + // Deliberately not claimed: a suppressed crash's dump stays prunable. + rejectedDumpPaths.add(dump.filePath) + return null + } claimedDumpPaths.set(dump.filePath, dump.mtimeMs) return { filePath: dump.filePath, sizeBytes: dump.size, signature } } finally { diff --git a/src/main/crash-reporting/crashpad-process-type.ts b/src/main/crash-reporting/crashpad-process-type.ts new file mode 100644 index 00000000000..9dc474dcd4d --- /dev/null +++ b/src/main/crash-reporting/crashpad-process-type.ts @@ -0,0 +1,14 @@ +// Electron's process-gone `processType` vocabulary is not Crashpad's dump +// annotation vocabulary; map once so both the report and the suppression path +// fence dumps by the same name. +const CHILD_CRASHPAD_PROCESS_TYPES: Readonly> = { + gpu: 'gpu-process', + utility: 'utility', + zygote: 'zygote' +} + +export function crashpadProcessTypeFor(source: string, processType: string): string | null { + return source === 'renderer' + ? 'renderer' + : (CHILD_CRASHPAD_PROCESS_TYPES[processType.trim().toLowerCase()] ?? null) +} diff --git a/src/main/crash-reporting/process-gone-classification.test.ts b/src/main/crash-reporting/process-gone-classification.test.ts index 78522b76617..a4e81a85c46 100644 --- a/src/main/crash-reporting/process-gone-classification.test.ts +++ b/src/main/crash-reporting/process-gone-classification.test.ts @@ -226,6 +226,63 @@ describe('shouldRecordProcessGoneCrash', () => { ).toBe(true) }) + // 1.4.188 batch: both were filed as user-facing crashes. The PAC evaluator report + // even carried a bug note about opening a 14MB JSON file. + it('suppresses on-demand Chromium utility teardown outside the old allowlist', () => { + expect( + shouldRecordProcessGoneCrash({ + source: 'child', + processType: 'Utility', + serviceName: 'proxy_resolver.mojom.ProxyResolverFactory', + reason: 'killed', + exitCode: 1, + expectedTeardown: 'none' + }) + ).toBe(false) + expect( + shouldRecordProcessGoneCrash({ + source: 'child', + processType: 'Utility', + serviceName: 'printing.mojom.PrintCompositor', + reason: 'crashed', + exitCode: -1, + expectedTeardown: 'none' + }) + ).toBe(false) + }) + + it('still records utility exits Chromium does not own', () => { + expect( + shouldRecordProcessGoneCrash({ + source: 'child', + processType: 'Utility', + serviceName: 'node.mojom.NodeService', + reason: 'crashed', + exitCode: -1, + expectedTeardown: 'none' + }) + ).toBe(true) + expect( + shouldRecordProcessGoneCrash({ + source: 'child', + processType: 'Utility', + serviceName: 'storage.mojom.StorageService', + reason: 'killed', + exitCode: 1, + expectedTeardown: 'none' + }) + ).toBe(true) + expect( + shouldRecordProcessGoneCrash({ + source: 'child', + processType: 'Utility', + reason: 'crashed', + exitCode: -1, + expectedTeardown: 'none' + }) + ).toBe(true) + }) + it('still records severe Chromium child process failures', () => { expect( shouldRecordProcessGoneCrash({ diff --git a/src/main/crash-reporting/process-gone-classification.ts b/src/main/crash-reporting/process-gone-classification.ts index d3f363d1332..835b2ccf6ef 100644 --- a/src/main/crash-reporting/process-gone-classification.ts +++ b/src/main/crash-reporting/process-gone-classification.ts @@ -1,15 +1,10 @@ +import { isRecoverableChromiumUtilityService } from './chromium-utility-service-recoverability' + export type ProcessGoneSource = 'renderer' | 'child' export type ExpectedTeardownScope = 'none' | 'renderer-reload' | 'app-shutdown' const WINDOWS_CONTROL_TERMINATION_EXIT_CODES = new Set([0xc000013a, 0x40010004]) const RECOVERABLE_CHILD_PROCESS_TYPES = new Set(['gpu']) -const RECOVERABLE_UTILITY_SERVICE_NAMES = new Set([ - 'audio.mojom.AudioService', - 'network.mojom.NetworkService', - // Why: Windows media/screen capture can churn this Chromium utility without - // taking down Orca; prompting users for those child exits is noise. - 'video_capture.mojom.VideoCaptureService' -]) const RECOVERABLE_CHILD_PROCESS_REASONS = new Set(['abnormal-exit', 'crashed', 'killed']) const NON_RECOVERABLE_RENDERER_REASONS = new Set(['integrity-failure']) @@ -41,11 +36,7 @@ function isRecoverableChromiumChildProcess({ if (normalizedProcessType && RECOVERABLE_CHILD_PROCESS_TYPES.has(normalizedProcessType)) { return true } - return ( - normalizedProcessType === 'utility' && - serviceName !== undefined && - RECOVERABLE_UTILITY_SERVICE_NAMES.has(serviceName) - ) + return normalizedProcessType === 'utility' && isRecoverableChromiumUtilityService(serviceName) } export function shouldRecordProcessGoneCrash({ @@ -63,7 +54,7 @@ export function shouldRecordProcessGoneCrash({ exitCode: number | null expectedTeardown: ExpectedTeardownScope }): boolean { - // Why: GPU, Network Service, and Audio Service exits are recoverable Chromium + // Why: GPU exits and on-demand Chromium utility-service exits are recoverable // child-process churn; treating them as app crashes creates noisy user prompts. if (isRecoverableChromiumChildProcess({ source, processType, serviceName, reason })) { return false diff --git a/src/main/crash-reporting/process-gone-killed-one-ordering.test.ts b/src/main/crash-reporting/process-gone-killed-one-ordering.test.ts index fffd36f1016..ebb8d90131f 100644 --- a/src/main/crash-reporting/process-gone-killed-one-ordering.test.ts +++ b/src/main/crash-reporting/process-gone-killed-one-ordering.test.ts @@ -17,6 +17,7 @@ import { import { ProcessGoneDedupe } from './process-gone-dedupe' import { recordProcessGoneCrash, type ProcessGoneCrashEvent } from './process-gone-recorder' import { resetProcessGoneSiblingCorrelationForTest } from './process-gone-sibling-correlation' +import { resetSuppressedProcessGoneRingBudgetForTest } from './suppressed-process-gone-ring-budget' const noMinidump = async () => null const attachDetails = async () => null @@ -63,6 +64,7 @@ beforeEach(() => { now = 1_000 resetExpectedTeardownStateForTest(() => now) clearCrashBreadcrumbsForTest() + resetSuppressedProcessGoneRingBudgetForTest() resetProcessGoneSiblingCorrelationForTest() }) @@ -70,6 +72,7 @@ afterEach(() => { vi.restoreAllMocks() resetExpectedTeardownStateForTest() clearCrashBreadcrumbsForTest() + resetSuppressedProcessGoneRingBudgetForTest() resetProcessGoneSiblingCorrelationForTest() }) diff --git a/src/main/crash-reporting/process-gone-recorder.test.ts b/src/main/crash-reporting/process-gone-recorder.test.ts index 95dbd992401..4428b7a4ab6 100644 --- a/src/main/crash-reporting/process-gone-recorder.test.ts +++ b/src/main/crash-reporting/process-gone-recorder.test.ts @@ -20,6 +20,7 @@ import { ProcessGoneDedupe } from './process-gone-dedupe' import { recordProcessGoneCrash, type ProcessGoneCrashEvent } from './process-gone-recorder' import { resetProcessGoneSiblingCorrelationForTest } from './process-gone-sibling-correlation' import { _resetTracerForTests, setActiveSink, type TracerSink } from '../observability/tracer' +import { resetSuppressedProcessGoneRingBudgetForTest } from './suppressed-process-gone-ring-budget' type CapturingSink = TracerSink & { records: unknown[]; flushMock: ReturnType } @@ -57,6 +58,7 @@ beforeEach(() => { sink = capturingSink() setActiveSink(sink) clearCrashBreadcrumbsForTest() + resetSuppressedProcessGoneRingBudgetForTest() resetProcessGoneSiblingCorrelationForTest() }) @@ -65,6 +67,7 @@ afterEach(() => { vi.restoreAllMocks() _resetTracerForTests() clearCrashBreadcrumbsForTest() + resetSuppressedProcessGoneRingBudgetForTest() resetProcessGoneSiblingCorrelationForTest() }) @@ -595,7 +598,7 @@ describe('minidump signature attachment', () => { event({ source: 'child', processType: 'Utility', - // A utility outside the recoverable-service allowlist still reports. + // A utility Chromium churn does not own still reports. details: { type: 'Utility', serviceName: 'storage.mojom.StorageService' } }), new ProcessGoneDedupe(), diff --git a/src/main/crash-reporting/process-gone-recorder.ts b/src/main/crash-reporting/process-gone-recorder.ts index 47ac561c4a7..1a8c775380f 100644 --- a/src/main/crash-reporting/process-gone-recorder.ts +++ b/src/main/crash-reporting/process-gone-recorder.ts @@ -3,17 +3,13 @@ import { app } from 'electron' import { isCrashReportReason, sanitizeCrashReportDetails, - sanitizeCrashReportString, - type CrashReportBreadcrumbData + sanitizeCrashReportString } from '../../shared/crash-reporting' import { decodePosixWaitStatus, describePosixWaitStatus } from '../../shared/posix-wait-status' import { rendererCrashBreadcrumbOrigin } from '../../shared/crash-breadcrumb-origin' import type { CrashReportStore } from './crash-report-store' import { getCrashBreadcrumbSnapshot } from './crash-breadcrumb-store' -import { - recordCoalescedDurableCrashBreadcrumb, - recordDurableCrashBreadcrumb -} from './durable-crash-breadcrumb' +import { recordDurableCrashBreadcrumb } from './durable-crash-breadcrumb' import { correlateChildProcessDeath, trackRendererSiblingAttribution @@ -24,7 +20,10 @@ import { type ProcessGoneSource } from './process-gone-classification' import { buildProcessGoneCrashDetails } from './process-gone-diagnostics' -import { buildSuppressedProcessGoneBreadcrumbData } from './suppressed-process-gone-breadcrumb' +import { + buildSuppressedProcessGoneBreadcrumbData, + recordSuppressedProcessGone +} from './suppressed-process-gone-breadcrumb' import { getProcessGoneDedupeKey, processGoneDedupe, @@ -40,6 +39,7 @@ import { scheduleCrashpadDumpPrune, type CapturedMinidump } from './crashpad-capture' +import { crashpadProcessTypeFor } from './crashpad-process-type' import { minidumpSignatureDetails } from './minidump-crash-signature' import { flushActiveSink, startSpan } from '../observability/tracer' @@ -61,25 +61,13 @@ export type MinidumpCapture = ( expectedProcessType: string ) => Promise -const CHILD_CRASHPAD_PROCESS_TYPES: Readonly> = { - gpu: 'gpu-process', - utility: 'utility', - zygote: 'zygote' -} - function expectedCrashpadProcessType(event: ProcessGoneCrashEvent): string | null { - return event.source === 'renderer' - ? 'renderer' - : (CHILD_CRASHPAD_PROCESS_TYPES[event.processType.trim().toLowerCase()] ?? null) + return crashpadProcessTypeFor(event.source, event.processType) } const captureProcessMinidump: MinidumpCapture = (crashedAtMs, expectedProcessType) => captureMinidumpSignature(crashedAtMs, { expectedProcessType }) -// Why: the coalesce map prunes every key against the calling window, so a shorter -// one here would weaken the other 30s coalescers. Stay uniform with them. -const SUPPRESSED_PROCESS_GONE_COALESCE_MS = 30_000 - function processGoneBreadcrumbData(event: ProcessGoneCrashEvent) { return buildSuppressedProcessGoneBreadcrumbData(event) } @@ -90,21 +78,6 @@ function processGoneRendererOrigin(event: ProcessGoneCrashEvent): string | undef : rendererCrashBreadcrumbOrigin(event.webContentsId) } -// Why: key off the emitted breadcrumb, not the crash-report dedupe key, so two -// different recoverable services can never suppress each other's evidence. -function suppressedProcessGoneCoalesceKey(data: CrashReportBreadcrumbData): string { - return JSON.stringify([ - data.source, - data.processType, - data.reason, - data.exitCode, - data.expectedTeardown, - data.serviceName ?? null, - data.name ?? null, - data.type ?? null - ]) -} - // Why: POSIX exit codes arrive as raw wait statuses (61696 = exit 241); name the // meaning on the span so bundles read without manual decoding. Display-only — // the recorded exitCode stays raw. launch-failed codes are not wait statuses. @@ -202,20 +175,7 @@ export function recordProcessGoneCrash( expectedTeardown: event.expectedTeardown }) ) { - // Why: Chromium can crash-loop a recoverable child (network service seen at - // 1459/min) and each suppressed event costs a span plus a forced disk flush, - // which both floods the 30-entry ring and evicts the real pre-crash trail. - const suppressedData = processGoneBreadcrumbData(event) - const origin = processGoneRendererOrigin(event) - recordCoalescedDurableCrashBreadcrumb({ - name: 'process_gone_suppressed', - data: suppressedData, - coalesceKey: origin - ? `${origin}\u0000${suppressedProcessGoneCoalesceKey(suppressedData)}` - : suppressedProcessGoneCoalesceKey(suppressedData), - minIntervalMs: SUPPRESSED_PROCESS_GONE_COALESCE_MS, - ...(origin ? { origin } : {}) - }) + recordSuppressedProcessGone(event, goneAt, processGoneRendererOrigin(event)) return } if (!store) { diff --git a/src/main/crash-reporting/suppressed-process-gone-breadcrumb.ts b/src/main/crash-reporting/suppressed-process-gone-breadcrumb.ts index 97b6c1c72f0..ebe0bb73279 100644 --- a/src/main/crash-reporting/suppressed-process-gone-breadcrumb.ts +++ b/src/main/crash-reporting/suppressed-process-gone-breadcrumb.ts @@ -1,5 +1,28 @@ import type { CrashReportBreadcrumbData } from '../../shared/crash-reporting' +import { crashpadProcessTypeFor } from './crashpad-process-type' +import { noteSuppressedCrashDump } from './crashpad-capture' +import { recordCoalescedDurableCrashBreadcrumb } from './durable-crash-breadcrumb' import type { ExpectedTeardownScope } from './process-gone-classification' +import { + budgetedSuppressedCoalesceKey, + isSuppressedOverflowCoalesceKey +} from './suppressed-process-gone-ring-budget' + +type SuppressedProcessGone = { + source: 'renderer' | 'child' + processType: string + reason: string + exitCode: number | null + expectedTeardown: ExpectedTeardownScope + details: Record +} + +// Why: the coalesce map prunes every key against the calling window, so a shorter +// one here would weaken the other 30s coalescers. Stay uniform with them. +const SUPPRESSED_PROCESS_GONE_COALESCE_MS = 30_000 + +// Reasons where Crashpad has actually written a dump for the dead process. +const DUMP_PRODUCING_REASONS = new Set(['crashed', 'abnormal-exit', 'integrity-failure']) function safeString(value: unknown): string | undefined { return typeof value === 'string' && value.length > 0 ? value : undefined @@ -41,3 +64,55 @@ export function buildSuppressedProcessGoneBreadcrumbData({ } return breadcrumb } + +// Why: key off the emitted breadcrumb, not the crash-report dedupe key, so two +// different recoverable services can never suppress each other's evidence. +function suppressedProcessGoneCoalesceKey(data: CrashReportBreadcrumbData): string { + return JSON.stringify([ + data.source, + data.processType, + data.reason, + data.exitCode, + data.expectedTeardown, + data.serviceName ?? null, + data.name ?? null, + data.type ?? null + ]) +} + +/** + * Records a process exit we deliberately do not report. + * + * Why: Chromium can crash-loop a recoverable child (network service seen at + * 1459/min) and each suppressed event costs a span plus a forced disk flush, + * which both floods the 30-entry ring and evicts the real pre-crash trail. + */ +export function recordSuppressedProcessGone( + event: SuppressedProcessGone, + goneAtMs: number, + origin?: string +): void { + const data = buildSuppressedProcessGoneBreadcrumbData(event) + // Nothing reports this exit, so nothing claims the dump Crashpad wrote for + // it; fence it off before an unrelated report adopts its CHECK signature. + const dumpProcessType = crashpadProcessTypeFor(event.source, event.processType) + if (dumpProcessType && DUMP_PRODUCING_REASONS.has(event.reason)) { + noteSuppressedCrashDump(dumpProcessType, goneAtMs) + } + const coalesceKey = budgetedSuppressedCoalesceKey( + origin + ? `${origin}\u0000${suppressedProcessGoneCoalesceKey(data)}` + : suppressedProcessGoneCoalesceKey(data), + SUPPRESSED_PROCESS_GONE_COALESCE_MS + ) + // Flag the shared slot so a triager reads its count as "many services + // churned", not "this one service died N times". + const overflow = isSuppressedOverflowCoalesceKey(coalesceKey) + recordCoalescedDurableCrashBreadcrumb({ + name: 'process_gone_suppressed', + data: overflow ? { ...data, coalescedAcrossServices: true } : data, + coalesceKey, + minIntervalMs: SUPPRESSED_PROCESS_GONE_COALESCE_MS, + ...(origin && !overflow ? { origin } : {}) + }) +} diff --git a/src/main/crash-reporting/suppressed-process-gone-ring-budget.test.ts b/src/main/crash-reporting/suppressed-process-gone-ring-budget.test.ts new file mode 100644 index 00000000000..e4bc1ce63f9 --- /dev/null +++ b/src/main/crash-reporting/suppressed-process-gone-ring-budget.test.ts @@ -0,0 +1,87 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +vi.mock('electron', () => ({ + app: { getVersion: () => '1.2.3-test', getAppMetrics: () => [] } +})) + +import { + clearCrashBreadcrumbsForTest, + getCrashBreadcrumbSnapshot, + recordCrashBreadcrumb +} from './crash-breadcrumb-store' +import { ProcessGoneDedupe } from './process-gone-dedupe' +import { recordProcessGoneCrash, type ProcessGoneCrashEvent } from './process-gone-recorder' +import { resetProcessGoneSiblingCorrelationForTest } from './process-gone-sibling-correlation' +import { _resetTracerForTests, setActiveSink } from '../observability/tracer' +import { resetSuppressedProcessGoneRingBudgetForTest } from './suppressed-process-gone-ring-budget' + +// Chromium services that all suppress by default, so each one is its own coalesce +// key: an OOM sweep or shutdown race can burst the whole set inside one window. +const CHROMIUM_SERVICES = [ + 'proxy_resolver.mojom.ProxyResolverFactory', + 'printing.mojom.PrintCompositor', + 'data_decoder.mojom.DataDecoderService', + 'tracing.mojom.TracedProcess', + 'media.mojom.MediaFoundationService', + 'device.mojom.DeviceService', + 'shape_detection.mojom.ShapeDetectionService', + 'unzip.mojom.Unzipper', + 'patch.mojom.FilePatcher', + 'quarantine.mojom.Quarantine', + 'screen_ai.mojom.ScreenAIService', + 'speech_recognition.mojom.SpeechRecognitionService', + 'paint_preview.mojom.PaintPreviewCompositorCollection', + 'video_effects.mojom.VideoEffectsService', + 'file_util.mojom.SafeArchiveAnalyzer' +] + +function utilityCrash(serviceName: string, exitCode: number): ProcessGoneCrashEvent { + return { + source: 'child', + processType: 'Utility', + reason: 'crashed', + exitCode, + expectedTeardown: 'none', + details: { type: 'Utility', serviceName } + } +} + +beforeEach(() => { + setActiveSink({ push: vi.fn(), flush: vi.fn(), close: vi.fn() }) + clearCrashBreadcrumbsForTest() + resetSuppressedProcessGoneRingBudgetForTest() + resetProcessGoneSiblingCorrelationForTest() +}) + +afterEach(() => { + vi.restoreAllMocks() + _resetTracerForTests() + clearCrashBreadcrumbsForTest() + resetSuppressedProcessGoneRingBudgetForTest() + resetProcessGoneSiblingCorrelationForTest() +}) + +describe('suppressed process-gone breadcrumb ring budget', () => { + it('leaves the pre-crash trail intact when a whole service population churns', () => { + const dedupe = new ProcessGoneDedupe() + for (let index = 0; index < 20; index += 1) { + recordCrashBreadcrumb('user_action', { step: index }) + } + + for (const serviceName of CHROMIUM_SERVICES) { + for (const exitCode of [139, 11]) { + recordProcessGoneCrash( + { record: vi.fn() } as never, + utilityCrash(serviceName, exitCode), + dedupe + ) + } + } + + const names = getCrashBreadcrumbSnapshot().map((breadcrumb) => breadcrumb.name) + expect({ + userAction: names.filter((name) => name === 'user_action').length, + suppressed: names.filter((name) => name === 'process_gone_suppressed').length + }).toEqual({ userAction: 20, suppressed: 6 }) + }) +}) diff --git a/src/main/crash-reporting/suppressed-process-gone-ring-budget.ts b/src/main/crash-reporting/suppressed-process-gone-ring-budget.ts new file mode 100644 index 00000000000..47ba6ec54c4 --- /dev/null +++ b/src/main/crash-reporting/suppressed-process-gone-ring-budget.ts @@ -0,0 +1,49 @@ +// Coalescing bounds a *repeating* suppressed exit to one breadcrumb, but it is +// per-key: once every Chromium utility service suppresses by default, one OOM +// sweep or shutdown race churns a whole population of distinct services and each +// one claims its own ring slot. Thirty such exits fill the 30-entry ring and +// evict the entire pre-crash trail -- the exact blind spot coalescing exists to +// prevent. So cap the slots suppressed churn may hold and fold the rest into a +// single overflow slot, which keeps counting them via `suppressedSinceLast`. + +// A fifth of the 30-entry ring; the last of the six is the overflow slot. +const SUPPRESSED_RING_SLOT_BUDGET = 6 +// Leading space: coalesce keys are JSON arrays, so this can never collide. +const OVERFLOW_COALESCE_KEY = ' process-gone-suppressed-overflow' + +// Why monotonic: wall-clock corrections must not stretch or collapse the window. +const monotonicNow = (): number => performance.now() + +let windowStartedAtMs = Number.NEGATIVE_INFINITY +let slotKeys = new Set() + +/** + * Maps a suppressed process-gone coalesce key onto the key it may actually use. + * Returns the key unchanged while the budget has room, and the shared overflow + * key once it does not, so a service-population burst costs a bounded number of + * ring slots instead of one per service. + */ +export function budgetedSuppressedCoalesceKey(coalesceKey: string, windowMs: number): string { + const now = monotonicNow() + if (now - windowStartedAtMs >= windowMs) { + windowStartedAtMs = now + slotKeys = new Set() + } + if (slotKeys.has(coalesceKey)) { + return coalesceKey + } + if (slotKeys.size < SUPPRESSED_RING_SLOT_BUDGET - 1) { + slotKeys.add(coalesceKey) + return coalesceKey + } + return OVERFLOW_COALESCE_KEY +} + +export function isSuppressedOverflowCoalesceKey(coalesceKey: string): boolean { + return coalesceKey === OVERFLOW_COALESCE_KEY +} + +export function resetSuppressedProcessGoneRingBudgetForTest(): void { + windowStartedAtMs = Number.NEGATIVE_INFINITY + slotKeys = new Set() +} diff --git a/src/main/crash-reporting/suppressed-utility-crash-diagnosability.test.ts b/src/main/crash-reporting/suppressed-utility-crash-diagnosability.test.ts new file mode 100644 index 00000000000..15c37332cd3 --- /dev/null +++ b/src/main/crash-reporting/suppressed-utility-crash-diagnosability.test.ts @@ -0,0 +1,98 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +vi.mock('electron', () => ({ + app: { getVersion: () => '1.2.3-test', getAppMetrics: () => [] } +})) + +import { clearCrashBreadcrumbsForTest, getCrashBreadcrumbSnapshot } from './crash-breadcrumb-store' +import { ProcessGoneDedupe } from './process-gone-dedupe' +import { recordProcessGoneCrash, type ProcessGoneCrashEvent } from './process-gone-recorder' +import { resetProcessGoneSiblingCorrelationForTest } from './process-gone-sibling-correlation' +import { _resetTracerForTests, setActiveSink, type TracerSink } from '../observability/tracer' +import { resetSuppressedProcessGoneRingBudgetForTest } from './suppressed-process-gone-ring-budget' + +const records: unknown[] = [] +const sink: TracerSink = { + push: (record) => records.push(record), + flush: vi.fn(), + close: vi.fn() +} + +/** A repeatable CHECK failure inside an on-demand Chromium utility service. */ +function printCompositorCheckFailure(): ProcessGoneCrashEvent { + return { + source: 'child', + processType: 'utility', + reason: 'crashed', + exitCode: 0x80000003, + expectedTeardown: 'none', + details: { processType: 'utility', serviceName: 'printing.mojom.PrintCompositor' } + } +} + +function suppress(capture: unknown): { record: ReturnType } { + const store = { record: vi.fn(async () => 'report-1'), attachDetails: vi.fn(async () => null) } + recordProcessGoneCrash( + store as never, + printCompositorCheckFailure(), + new ProcessGoneDedupe(), + capture as never + ) + return store +} + +beforeEach(() => { + records.length = 0 + setActiveSink(sink) + clearCrashBreadcrumbsForTest() + resetSuppressedProcessGoneRingBudgetForTest() + resetProcessGoneSiblingCorrelationForTest() +}) + +afterEach(() => { + vi.restoreAllMocks() + _resetTracerForTests() + clearCrashBreadcrumbsForTest() + resetSuppressedProcessGoneRingBudgetForTest() + resetProcessGoneSiblingCorrelationForTest() +}) + +// Pins the contract stated on isRecoverableChromiumUtilityService: suppression is +// breadcrumb-only, so widening the denylist is the ONLY way to keep a service's +// post-mortem detail. A suppressed exit must never pay for a minidump poll either +// -- Chromium can crash-loop these at 1459/min. +describe('suppressed Chromium utility crash diagnosability', () => { + it('never reaches the minidump signature path', async () => { + const capture = vi.fn(async () => null) + + const store = suppress(capture) + await Promise.resolve() + + expect(store.record).not.toHaveBeenCalled() + expect(capture).not.toHaveBeenCalled() + expect(records).not.toContainEqual( + expect.objectContaining({ name: 'electron.minidump_signature' }) + ) + }) + + it('leaves the CHECK anonymous in the durable breadcrumb trail', () => { + suppress(async () => null) + + const [breadcrumb] = getCrashBreadcrumbSnapshot() + expect(breadcrumb).toEqual( + expect.objectContaining({ + name: 'process_gone_suppressed', + data: expect.objectContaining({ + source: 'child', + processType: 'utility', + serviceName: 'printing.mojom.PrintCompositor', + reason: 'crashed', + exitCode: 0x80000003 + }) + }) + ) + expect(Object.keys(breadcrumb.data ?? {})).toEqual( + expect.not.arrayContaining(['minidumpStatus', 'minidumpPath', 'crashSignature']) + ) + }) +}) diff --git a/src/main/crash-reporting/suppressed-utility-dump-attribution.test.ts b/src/main/crash-reporting/suppressed-utility-dump-attribution.test.ts new file mode 100644 index 00000000000..b99259d0d0d --- /dev/null +++ b/src/main/crash-reporting/suppressed-utility-dump-attribution.test.ts @@ -0,0 +1,142 @@ +import { mkdtemp, mkdir, rm, utimes, writeFile } from 'node:fs/promises' +import os from 'node:os' +import path from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const parseMinidumpCrashSignatureMock = vi.hoisted(() => vi.fn()) + +vi.mock('electron', () => ({ + app: { + getVersion: () => '1.2.3-test', + getAppMetrics: () => [], + getPath: () => '/unused-in-tests' + }, + crashReporter: { start: vi.fn() } +})) +vi.mock('./minidump-crash-signature', async (importOriginal) => ({ + ...(await importOriginal>()), + parseMinidumpCrashSignature: parseMinidumpCrashSignatureMock +})) + +import { _setCrashpadCaptureStateForTest, captureMinidumpSignature } from './crashpad-capture' +import { clearCrashBreadcrumbsForTest } from './crash-breadcrumb-store' +import { ProcessGoneDedupe } from './process-gone-dedupe' +import { recordProcessGoneCrash, type ProcessGoneCrashEvent } from './process-gone-recorder' +import { resetProcessGoneSiblingCorrelationForTest } from './process-gone-sibling-correlation' +import { _resetTracerForTests, setActiveSink } from '../observability/tracer' +import { resetSuppressedProcessGoneRingBudgetForTest } from './suppressed-process-gone-ring-budget' + +let dumpDir: string + +/** Minimal but valid minidump header; the signature parser is mocked. */ +function emptyDump(): Buffer { + const buf = Buffer.alloc(32) + buf.writeUInt32LE(0x504d444d, 0) + buf.writeUInt32LE(0xa793, 4) + buf.writeUInt32LE(0, 8) + buf.writeUInt32LE(32, 12) + return buf +} + +async function writeDump(relativePath: string, mtimeMs: number): Promise { + const filePath = path.join(dumpDir, relativePath) + await mkdir(path.dirname(filePath), { recursive: true }) + await writeFile(filePath, emptyDump()) + await utimes(filePath, mtimeMs / 1000, mtimeMs / 1000) + return filePath +} + +/** Suppressed by the Chromium-utility default; leaves an unreported dump behind. */ +function printCompositorCheckFailure(): ProcessGoneCrashEvent { + return { + source: 'child', + processType: 'utility', + reason: 'crashed', + exitCode: 0x80000003, + expectedTeardown: 'none', + details: { type: 'Utility', serviceName: 'printing.mojom.PrintCompositor' } + } +} + +/** Reportable: `launch-failed` is not recoverable churn, and it never ran, so it + * has no dump of its own to pair with. */ +function videoCaptureLaunchFailure(): ProcessGoneCrashEvent { + return { + source: 'child', + processType: 'utility', + reason: 'launch-failed', + exitCode: null, + expectedTeardown: 'none', + details: { type: 'Utility', serviceName: 'video_capture.mojom.VideoCaptureService' } + } +} + +function recorderStore() { + return { + record: vi.fn(async () => ({ id: 'report-1' })), + attachDetails: vi.fn(async () => null) + } +} + +// Bounded poll: the recorder's real capture would spin for 8s when no dump matches. +const capture = (crashedAtMs: number, expectedProcessType: string) => + captureMinidumpSignature(crashedAtMs, { expectedProcessType, timeoutMs: 0 }) + +beforeEach(async () => { + parseMinidumpCrashSignatureMock.mockReset() + parseMinidumpCrashSignatureMock.mockReturnValue({ + processType: 'utility', + checkMessage: '[0125/000000.000:FATAL:print_compositor.cc(42)] Check failed: alive.', + checkFile: 'print_compositor.cc', + checkLine: 42, + annotations: {} + }) + dumpDir = await mkdtemp(path.join(os.tmpdir(), 'orca-suppressed-dump-')) + _setCrashpadCaptureStateForTest({ dumpDirectory: dumpDir, started: true }) + setActiveSink({ push: vi.fn(), flush: vi.fn(), close: vi.fn() }) + clearCrashBreadcrumbsForTest() + resetSuppressedProcessGoneRingBudgetForTest() + resetProcessGoneSiblingCorrelationForTest() +}) + +afterEach(async () => { + _setCrashpadCaptureStateForTest(null) + vi.restoreAllMocks() + _resetTracerForTests() + clearCrashBreadcrumbsForTest() + resetSuppressedProcessGoneRingBudgetForTest() + resetProcessGoneSiblingCorrelationForTest() + await rm(dumpDir, { recursive: true, force: true }) +}) + +describe('minidump attribution after a suppressed utility crash', () => { + it('never hands a suppressed service dump to an unrelated utility report', async () => { + const store = recorderStore() + const dedupe = new ProcessGoneDedupe() + + recordProcessGoneCrash(store as never, printCompositorCheckFailure(), dedupe, capture) + await writeDump(path.join('reports', 'print-compositor.dmp'), Date.now() + 100) + recordProcessGoneCrash(store as never, videoCaptureLaunchFailure(), dedupe, capture) + + await vi.waitFor(() => expect(store.attachDetails).toHaveBeenCalled()) + expect(store.attachDetails).toHaveBeenCalledWith('report-1', { minidumpStatus: 'absent' }) + }) + + it('still pairs a reportable utility crash with its own dump', async () => { + const store = recorderStore() + + await writeDump(path.join('reports', 'video-capture.dmp'), Date.now() + 100) + recordProcessGoneCrash( + store as never, + videoCaptureLaunchFailure(), + new ProcessGoneDedupe(), + capture + ) + + await vi.waitFor(() => expect(store.attachDetails).toHaveBeenCalled()) + expect(store.attachDetails).toHaveBeenCalledWith( + 'report-1', + expect.objectContaining({ minidumpStatus: 'captured' }) + ) + }) +})