From d573efc90f1efde3093cdbcfe7e10798a3b8dead Mon Sep 17 00:00:00 2001 From: m4air Date: Thu, 10 Sep 2026 22:12:11 -0700 Subject: [PATCH] fix(crash-reporting): let a late sibling amend withdraw the attribution it disproves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A renderer crash is labelled `crashAttribution: concurrent-process-deaths` from the sibling set known at record time. A child that dies afterwards is folded in by `attachDetails`, which merges — so when the wider set turns out to be a crash-looping child, `siblingProcessDeathDetails` merely OMITS the key and the stale label survives. Report 11a9d459 shipped `concurrent-process-deaths` next to `siblingProcessDeathRepeats: 2`, a pair `isOneIncident()` never emits. `attachDetails` now treats a null value as a withdrawal, and the late attribution emits `crashAttribution: null` once the report has been given a label the new evidence disproves. The flag is sticky, so a dropped fire-and-forget write is retried by the next late sibling. --- .../crash-reporting/crash-report-store.ts | 31 ++- ...one-sibling-attribution-withdrawal.test.ts | 229 ++++++++++++++++++ .../process-gone-sibling-correlation.test.ts | 3 +- .../process-gone-sibling-correlation.ts | 33 ++- 4 files changed, 285 insertions(+), 11 deletions(-) create mode 100644 src/main/crash-reporting/process-gone-sibling-attribution-withdrawal.test.ts diff --git a/src/main/crash-reporting/crash-report-store.ts b/src/main/crash-reporting/crash-report-store.ts index 143210418ce..da5afa8339d 100644 --- a/src/main/crash-reporting/crash-report-store.ts +++ b/src/main/crash-reporting/crash-report-store.ts @@ -9,6 +9,7 @@ import { sanitizeCrashReportBreadcrumbs, sanitizeCrashReportDetails, type CrashReportCreateInput, + type CrashReportDetailValue, type CrashReportRecord, type CrashReportStatus } from '../../shared/crash-reporting' @@ -39,6 +40,26 @@ function isRelatedCrashEvent(anchor: CrashReportRecord, candidate: CrashReportRe ) } +/** + * Applies an amend. A `null` value WITHDRAWS the key rather than storing it — see + * `attachDetails`. Nothing emits a null detail as data, and `record` never routes + * through here, so the initial write can still store one if a producer ever needs it. + */ +function mergeCrashReportDetails( + details: Record, + extraDetails: Record +): Record { + const merged = { ...details } + for (const [key, value] of Object.entries(sanitizeCrashReportDetails(extraDetails))) { + if (value === null) { + delete merged[key] + } else { + merged[key] = value + } + } + return merged +} + function isRetryableWindowsFileOperationError(error: unknown): boolean { const code = (error as NodeJS.ErrnoException).code return code === 'EPERM' || code === 'EACCES' || code === 'EBUSY' @@ -111,6 +132,11 @@ export class CrashReportStore { * Merges late-arriving details into an existing report. Crashpad finishes * writing the minidump after the process-gone event that created the record, * so the signature can only be folded in afterwards. + * + * A `null` value withdraws the key instead of storing it: an amend can also disprove a + * label the first write made on partial evidence, and a merge alone could never take one + * back (report 11a9d459 kept a one-incident sibling attribution beside the repeat count + * that contradicted it). */ async attachDetails( id: string, @@ -122,10 +148,7 @@ export class CrashReportStore { if (report.id !== id) { return report } - result = { - ...report, - details: { ...report.details, ...sanitizeCrashReportDetails(extraDetails) } - } + result = { ...report, details: mergeCrashReportDetails(report.details, extraDetails) } return result }) return { reports: nextReports, result } diff --git a/src/main/crash-reporting/process-gone-sibling-attribution-withdrawal.test.ts b/src/main/crash-reporting/process-gone-sibling-attribution-withdrawal.test.ts new file mode 100644 index 00000000000..a703057c771 --- /dev/null +++ b/src/main/crash-reporting/process-gone-sibling-attribution-withdrawal.test.ts @@ -0,0 +1,229 @@ +import fs from 'node:fs/promises' +import os from 'node:os' +import path from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +vi.mock('electron', () => ({ + app: { + getVersion: () => '1.4.199-test', + getAppMetrics: () => [], + getPath: () => os.tmpdir() + } +})) + +import { CrashReportStore } from './crash-report-store' +import { clearCrashBreadcrumbsForTest } from './crash-breadcrumb-store' +import { ProcessGoneDedupe } from './process-gone-dedupe' +import { recordProcessGoneCrash, type ProcessGoneCrashEvent } from './process-gone-recorder' +import { + collectLateSiblingAttributions, + resetProcessGoneSiblingCorrelationForTest, + trackRendererCrashReport, + type ChildProcessDeath +} from './process-gone-sibling-correlation' +import { _resetTracerForTests } from '../observability/tracer' + +// Field shape: crash report 11a9d459 (v1.4.199, win32 10.0.19045), renderer +// crashed/-2147483645 (0x80000003) with a GPU process crash-looping at -17ms / +14ms / +61ms. +const BREAKPOINT_EXIT = -2_147_483_645 +const GONE_AT = 1_789_021_889_062 + +const noMinidump = async () => null +const originalPlatform = process.platform + +function gpuChildCrash(): ProcessGoneCrashEvent { + return { + source: 'child', + processType: 'GPU', + reason: 'crashed', + exitCode: BREAKPOINT_EXIT, + expectedTeardown: 'none', + details: { serviceName: 'GPU', type: 'GPU' } + } +} + +function networkServiceCrash(): ProcessGoneCrashEvent { + return { + source: 'child', + processType: 'Utility', + reason: 'crashed', + exitCode: BREAKPOINT_EXIT, + expectedTeardown: 'none', + details: { serviceName: 'network.mojom.NetworkService', type: 'Utility' } + } +} + +function rendererCrash(): ProcessGoneCrashEvent { + return { + source: 'renderer', + processType: 'renderer', + reason: 'crashed', + exitCode: BREAKPOINT_EXIT, + expectedTeardown: 'none', + details: { processType: 'renderer' }, + webContentsId: 1 + } +} + +/** The amend is fire-and-forget behind two real file writes, so poll the stored document. */ +async function rendererDetails( + store: CrashReportStore, + expectedSiblingCount: number +): Promise> { + return vi.waitFor(async () => { + const [report] = await store.listRecent() + expect(report?.details.siblingProcessDeathCount).toBe(expectedSiblingCount) + return report.details + }) +} + +describe('sibling attribution withdrawal', () => { + let directory: string + let store: CrashReportStore + + beforeEach(async () => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + directory = await fs.mkdtemp(path.join(os.tmpdir(), 'orca-sibling-attr-')) + store = new CrashReportStore(path.join(directory, 'crash-reports.json')) + resetProcessGoneSiblingCorrelationForTest() + clearCrashBreadcrumbsForTest() + _resetTracerForTests() + }) + + afterEach(async () => { + Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) + vi.restoreAllMocks() + resetProcessGoneSiblingCorrelationForTest() + clearCrashBreadcrumbsForTest() + _resetTracerForTests() + await fs.rm(directory, { recursive: true, force: true }) + }) + + it('drops crashAttribution once a repeating sibling identity disqualifies it', async () => { + const dedupe = new ProcessGoneDedupe() + const now = vi.spyOn(Date, 'now') + + now.mockReturnValue(GONE_AT - 17) + recordProcessGoneCrash(store, gpuChildCrash(), dedupe, noMinidump) + + now.mockReturnValue(GONE_AT) + recordProcessGoneCrash(store, rendererCrash(), dedupe, noMinidump) + + // Same GPU identity dies twice more: a crash-looping child, which + // isOneIncident() explicitly refuses to call concurrent-process-deaths. + now.mockReturnValue(GONE_AT + 14) + recordProcessGoneCrash(store, gpuChildCrash(), dedupe, noMinidump) + now.mockReturnValue(GONE_AT + 61) + recordProcessGoneCrash(store, gpuChildCrash(), dedupe, noMinidump) + + const details = await rendererDetails(store, 3) + expect(details.siblingProcessDeathRepeats).toBe(2) + expect(details.siblingProcessDeaths).toBe('GPU +14ms, GPU -17ms, GPU +61ms') + expect(details).not.toHaveProperty('crashAttribution') + }) + + it('keeps the label when a second distinct sibling leaves the incident intact', async () => { + const dedupe = new ProcessGoneDedupe() + const now = vi.spyOn(Date, 'now') + + now.mockReturnValue(GONE_AT - 17) + recordProcessGoneCrash(store, gpuChildCrash(), dedupe, noMinidump) + now.mockReturnValue(GONE_AT) + recordProcessGoneCrash(store, rendererCrash(), dedupe, noMinidump) + now.mockReturnValue(GONE_AT + 14) + recordProcessGoneCrash(store, networkServiceCrash(), dedupe, noMinidump) + + const details = await rendererDetails(store, 2) + expect(details.siblingProcessDeathRepeats).toBeUndefined() + expect(details.crashAttribution).toBe('concurrent-process-deaths') + }) + + it('withdraws only the disproved key and leaves the rest of the report intact', async () => { + const recorded = await store.record({ + source: 'renderer', + processType: 'renderer', + reason: 'crashed', + exitCode: BREAKPOINT_EXIT, + appVersion: '1.4.199-test', + platform: 'win32', + osRelease: '10.0.19045', + arch: 'x64', + electronVersion: '38.0.0', + chromeVersion: '140.0.0.0', + details: { crashAttribution: 'concurrent-process-deaths', minidumpStatus: 'captured' } + }) + + const amended = await store.attachDetails(recorded.id, { + crashAttribution: null, + siblingProcessDeathRepeats: 2 + }) + + expect(amended?.details).toEqual({ + minidumpStatus: 'captured', + siblingProcessDeathRepeats: 2 + }) + // Withdrawal must survive the round trip, not just the in-memory result. + const [persisted] = await store.listRecent() + expect(persisted.details).not.toHaveProperty('crashAttribution') + }) +}) + +/** + * The amend is fire-and-forget: `attachAttribution` logs a failed write and moves on. A + * withdrawal that never reached the file has to stay owed, or the Windows retry ladder + * exhausting once (EPERM/EBUSY) ships the stale label the amend existed to take back. + */ +describe('a withdrawal amend whose write never lands', () => { + const gpuDeath = (at: number): ChildProcessDeath => ({ + at, + processType: 'GPU', + serviceName: 'GPU', + reason: 'crashed', + exitCode: BREAKPOINT_EXIT + }) + + beforeEach(() => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + resetProcessGoneSiblingCorrelationForTest() + }) + + afterEach(() => { + Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) + resetProcessGoneSiblingCorrelationForTest() + }) + + it('re-emits it on the next late sibling instead of reporting it withdrawn', () => { + trackRendererCrashReport( + { + at: GONE_AT, + reason: 'crashed', + exitCode: BREAKPOINT_EXIT, + attachAttribution: () => { + throw new Error('the amend is fire-and-forget; this write is dropped') + } + }, + [gpuDeath(GONE_AT - 17)] + ) + + const [dropped] = collectLateSiblingAttributions(gpuDeath(GONE_AT + 14)) + expect(dropped.attribution.crashAttribution).toBeNull() + const [retried] = collectLateSiblingAttributions(gpuDeath(GONE_AT + 61)) + expect(retried.attribution.crashAttribution).toBeNull() + }) + + it('never withdraws a label the report was never given', () => { + trackRendererCrashReport( + { + at: GONE_AT, + reason: 'crashed', + exitCode: BREAKPOINT_EXIT, + attachAttribution: () => {} + }, + // -900ms is in-window but too loose to attribute, so nothing was ever written. + [gpuDeath(GONE_AT - 900)] + ) + + const [attribution] = collectLateSiblingAttributions(gpuDeath(GONE_AT + 14)) + expect(attribution.attribution).not.toHaveProperty('crashAttribution') + }) +}) diff --git a/src/main/crash-reporting/process-gone-sibling-correlation.test.ts b/src/main/crash-reporting/process-gone-sibling-correlation.test.ts index 585253cd4fc..40c39af6be7 100644 --- a/src/main/crash-reporting/process-gone-sibling-correlation.test.ts +++ b/src/main/crash-reporting/process-gone-sibling-correlation.test.ts @@ -215,7 +215,8 @@ describe('sibling process-death attribution', () => { await vi.waitFor(() => expect(siblingAttaches(store)).toHaveLength(2)) expect(siblingAttaches(store)[0]).toMatchObject({ crashAttribution: CONCURRENT }) expect(siblingAttaches(store)[1]).toMatchObject({ siblingProcessDeathRepeats: 1 }) - expect(siblingAttaches(store)[1]).not.toHaveProperty('crashAttribution') + // Null withdraws the label the first attach wrote: a merge alone can never take it back. + expect(siblingAttaches(store)[1]).toMatchObject({ crashAttribution: null }) }) it('does not erase recorded sibling evidence during unrelated child churn', async () => { diff --git a/src/main/crash-reporting/process-gone-sibling-correlation.ts b/src/main/crash-reporting/process-gone-sibling-correlation.ts index b2036d8aa9a..7c876af710b 100644 --- a/src/main/crash-reporting/process-gone-sibling-correlation.ts +++ b/src/main/crash-reporting/process-gone-sibling-correlation.ts @@ -69,6 +69,12 @@ export type PendingRendererCrashReport = { type TrackedRendererCrashReport = PendingRendererCrashReport & { siblingDeaths: ChildProcessDeath[] lateAttaches: number + /** + * Whether the one-incident label may still be on the persisted report. Sticky once set: + * the amend that withdraws it is fire-and-forget, so a failed write must leave the next + * amend still trying rather than treating the label as already gone. + */ + attributedOneIncident: boolean } export type LateSiblingAttribution = { @@ -135,7 +141,12 @@ export function trackRendererCrashReport( .slice(-(MAX_PENDING_RENDERER_REPORTS - 1)) pendingRendererReports = [ ...liveReports, - { ...pending, siblingDeaths: [...siblingDeaths], lateAttaches: 0 } + { + ...pending, + siblingDeaths: [...siblingDeaths], + lateAttaches: 0, + attributedOneIncident: isOneIncident(siblingDeaths, pending.at) + } ] } @@ -181,13 +192,22 @@ function isOneIncident(siblings: ChildProcessDeath[], rendererAt: number): boole ) } +/** + * `withdrawStaleAttribution` is for an amend onto a report that already carries the label: + * attachDetails merges, so evidence that disproves one incident (report 11a9d459 shipped + * `crashAttribution=concurrent-process-deaths` beside `siblingProcessDeathRepeats=2`) has + * to say so explicitly. A null detail value withdraws the key. + */ export function siblingProcessDeathDetails( siblings: ChildProcessDeath[], - rendererAt: number + rendererAt: number, + { withdrawStaleAttribution = false }: { withdrawStaleAttribution?: boolean } = {} ): Record { const repeats = repeatedIdentityCount(siblings) + const oneIncident = isOneIncident(siblings, rendererAt) return { - ...(isOneIncident(siblings, rendererAt) ? { crashAttribution: CONCURRENT_PROCESS_DEATHS } : {}), + ...(oneIncident ? { crashAttribution: CONCURRENT_PROCESS_DEATHS } : {}), + ...(!oneIncident && withdrawStaleAttribution ? { crashAttribution: null } : {}), siblingProcessDeathCount: siblings.length, ...(repeats > 0 ? { siblingProcessDeathRepeats: repeats } : {}), siblingProcessDeaths: describeSiblingDeaths(siblings, rendererAt) @@ -212,10 +232,11 @@ export function collectLateSiblingAttributions(death: ChildProcessDeath): LateSi } pending.siblingDeaths.push(death) pending.lateAttaches += 1 - attributions.push({ - pending, - attribution: siblingProcessDeathDetails(pending.siblingDeaths, pending.at) + const attribution = siblingProcessDeathDetails(pending.siblingDeaths, pending.at, { + withdrawStaleAttribution: pending.attributedOneIncident }) + pending.attributedOneIncident ||= attribution.crashAttribution === CONCURRENT_PROCESS_DEATHS + attributions.push({ pending, attribution }) } return attributions }