fix(picker): pointer-width vtable offsets, COM apartment pairing, unconditional abort budget, and the full failure chain
Review round two on the in-process dialog:
- Vtable slots and out-pointers use koffi.sizeof('void *') instead of a
hardcoded 8 - win32-ia32 (which Node and koffi both ship) would have read
method pointers from the wrong address and crashed in-process before any
fallback could run.
- runFolderDialog pairs every successful (incl. S_FALSE) CoInitializeEx
with CoUninitialize in the outermost finally, releasing the dialog first;
a failed init is deliberately unpaired. Pinned across fake-bindings and
mocked-koffi suites.
- The abort close budget starts unconditionally: a worker hung before the
showing notice (koffi import or COM init) now ends in terminate instead
of a dangling promise; WM_CLOSE posting still waits for the thread id.
- A triple miss (dialog + pwsh + 5.1) surfaces an AggregateError carrying
all three causes - the in-process tier's reason was previously
unrecoverable from the final PowerShell error.
- The stray '=>{ ' formatter artifacts are normalized to real blocks.
Both stale note claims from the review are fixed: the DPI note's
Consequences no longer claims an ENOENT classification or zero new
dependencies, and the 2026-07-27 picker note's Windows bullet now names
the in-process primary and keeps the PowerShell chain as fallback (both
languages, pairings re-recorded).
This commit is contained in:
@@ -97,10 +97,16 @@ describe('native directory picker', () => {
|
||||
.mockResolvedValueOnce({ stdout: '', stderr: '' })
|
||||
await expect(pickNativeDirectory(signal(), { platform: 'win32', run: cancelled, pickWin32Dialog: noDialog })).resolves.toBeNull()
|
||||
|
||||
// Triple miss: the surfaced AggregateError carries all three causes,
|
||||
// including the otherwise-lost in-process dialog failure.
|
||||
const failed = vi.fn<DirectoryPickerRunner>()
|
||||
.mockRejectedValueOnce(failure('ENOENT'))
|
||||
.mockRejectedValueOnce(failure(2))
|
||||
await expect(pickNativeDirectory(signal(), { platform: 'win32', run: failed, pickWin32Dialog: noDialog })).rejects.toThrow('command failed')
|
||||
const tripleMiss = await pickNativeDirectory(signal(), { platform: 'win32', run: failed, pickWin32Dialog: noDialog })
|
||||
.then(() => { throw new Error('expected rejection') }, (error: unknown) => error as AggregateError)
|
||||
expect(tripleMiss.message).toContain('the in-process dialog and both PowerShell hosts failed')
|
||||
expect((tripleMiss.errors[0] as Error).message).toBe('dialog unavailable')
|
||||
expect((tripleMiss.errors[2] as Error).message).toContain('command failed')
|
||||
})
|
||||
|
||||
it('wires the real Win32 dialog as the default tier', async () => {
|
||||
@@ -153,7 +159,9 @@ describe('native directory picker', () => {
|
||||
execFileMock.mockImplementationOnce((_command, _args, _options, callback) => {
|
||||
callback(commandError, 'partial output', 'failure details')
|
||||
})
|
||||
await expect(pickNativeDirectory(signal(), { platform: 'win32', pickWin32Dialog: noDialog })).rejects.toMatchObject({
|
||||
const surfaced = await pickNativeDirectory(signal(), { platform: 'win32', pickWin32Dialog: noDialog })
|
||||
.then(() => { throw new Error('expected rejection') }, (error: unknown) => error as AggregateError)
|
||||
expect(surfaced.errors[2]).toMatchObject({
|
||||
message: 'powershell failed', cause: commandError, code: 7,
|
||||
stdout: 'partial output', stderr: 'failure details',
|
||||
})
|
||||
|
||||
@@ -31,6 +31,7 @@ interface ComWorld {
|
||||
posted: { hwnd: unknown; message: number }[]
|
||||
registered: number
|
||||
unregistered: number
|
||||
uninitialized: number
|
||||
}
|
||||
|
||||
function comWorld(overrides: Partial<ComWorld> = {}): ComWorld {
|
||||
@@ -39,7 +40,7 @@ function comWorld(overrides: Partial<ComWorld> = {}): ComWorld {
|
||||
hasThreadDpi: true, enumThrows: false,
|
||||
path: 'C:\\选中\\directory',
|
||||
titles: [], options: [], dpiContexts: [], freed: [], released: [], posted: [],
|
||||
registered: 0, unregistered: 0,
|
||||
registered: 0, unregistered: 0, uninitialized: 0,
|
||||
...overrides,
|
||||
}
|
||||
}
|
||||
@@ -85,6 +86,7 @@ function installFakeKoffi(world: ComWorld): void {
|
||||
func: (_convention: string, name: string, _result: string, _args: string[]) => {
|
||||
switch (name) {
|
||||
case 'CoInitializeEx': return () => world.coInitHr
|
||||
case 'CoUninitialize': return () => { world.uninitialized += 1 }
|
||||
case 'CoCreateInstance': return (...args: unknown[]) => {
|
||||
if (world.coCreateHr < 0) return world.coCreateHr
|
||||
outBuffers.set(args[4], dialogPtr)
|
||||
@@ -109,6 +111,7 @@ function installFakeKoffi(world: ComWorld): void {
|
||||
}),
|
||||
proto: (declaration: string) => ({ declaration }),
|
||||
pointer: (type: unknown) => type,
|
||||
sizeof: (type: string) => { void type; return 8 },
|
||||
register: (fn: (hwnd: unknown, lparam: unknown) => number) => { world.registered += 1; return { fn } },
|
||||
unregister: () => { world.unregistered += 1 },
|
||||
decode: (value: unknown, offsetOrType: unknown): unknown => {
|
||||
@@ -153,6 +156,7 @@ describe('loadWin32DialogBindings over the fake COM world', () => {
|
||||
expect(showing).toHaveBeenCalledWith(31337)
|
||||
expect(world.freed).toHaveLength(1)
|
||||
expect(world.released).toEqual(['item', 'dialog'])
|
||||
expect(world.uninitialized).toBe(1)
|
||||
})
|
||||
|
||||
it('maps dismissal, missing DPI support, and the S_FALSE CoInitializeEx', async () => {
|
||||
@@ -163,6 +167,7 @@ describe('loadWin32DialogBindings over the fake COM world', () => {
|
||||
expect(runFolderDialog(bindings, 'Pick', vi.fn())).toBeNull()
|
||||
expect(world.dpiContexts).toEqual([])
|
||||
expect(world.released).toEqual(['dialog'])
|
||||
expect(world.uninitialized).toBe(1)
|
||||
})
|
||||
|
||||
it('surfaces creation and extraction failures as HRESULT errors', async () => {
|
||||
@@ -225,6 +230,7 @@ describe('the worker entry over a mocked thread boundary', () => {
|
||||
loadWin32DialogBindings: async () => ({
|
||||
setThreadDpiAwareness: () => undefined,
|
||||
coInitializeSta: () => 0,
|
||||
coUninitialize: () => undefined,
|
||||
currentThreadId: () => 11,
|
||||
createFolderDialog: () => ({
|
||||
setOptions: () => 0,
|
||||
|
||||
@@ -16,6 +16,7 @@ interface FakeWorld {
|
||||
bindings: Win32DialogBindings
|
||||
dpi: ReturnType<typeof vi.fn>
|
||||
createDialog: ReturnType<typeof vi.fn>
|
||||
uninitialize: ReturnType<typeof vi.fn>
|
||||
dialog: {
|
||||
setOptions: ReturnType<typeof vi.fn>
|
||||
setTitle: ReturnType<typeof vi.fn>
|
||||
@@ -36,21 +37,25 @@ function world(overrides: Partial<Win32FolderDialog> = {}, coInit = 0): FakeWorl
|
||||
}
|
||||
const dpi = vi.fn()
|
||||
const createDialog = vi.fn(() => dialog)
|
||||
const uninitialize = vi.fn()
|
||||
const bindings: Win32DialogBindings = {
|
||||
setThreadDpiAwareness: dpi,
|
||||
coInitializeSta: vi.fn(() => coInit),
|
||||
coUninitialize: uninitialize,
|
||||
createFolderDialog: createDialog,
|
||||
currentThreadId: vi.fn(() => 4242),
|
||||
}
|
||||
return { bindings, dpi, createDialog, dialog: dialog as FakeWorld['dialog'] }
|
||||
return { bindings, dpi, createDialog, uninitialize, dialog: dialog as FakeWorld['dialog'] }
|
||||
}
|
||||
|
||||
describe('runFolderDialog', () => {
|
||||
it('sequences DPI, STA, options, title, show, and result extraction', () => {
|
||||
const { bindings, dpi, dialog } = world()
|
||||
it('sequences DPI, STA, options, title, show, result extraction, and apartment teardown', () => {
|
||||
const { bindings, dpi, dialog, uninitialize } = world()
|
||||
const showing = vi.fn()
|
||||
expect(runFolderDialog(bindings, 'Pick', showing)).toBe('C:\\picked\\目录')
|
||||
expect(dpi).toHaveBeenCalledOnce()
|
||||
expect(uninitialize).toHaveBeenCalledOnce()
|
||||
expect(dialog.release.mock.invocationCallOrder[0]).toBeLessThan(uninitialize.mock.invocationCallOrder[0] as number)
|
||||
expect(dialog.setOptions).toHaveBeenCalledWith(FOS_PICKFOLDERS | FOS_FORCEFILESYSTEM | FOS_NOCHANGEDIR)
|
||||
expect(dialog.setTitle).toHaveBeenCalledWith('Pick')
|
||||
expect(showing).toHaveBeenCalledWith(4242)
|
||||
@@ -58,11 +63,12 @@ describe('runFolderDialog', () => {
|
||||
expect(dialog.release).toHaveBeenCalledOnce()
|
||||
})
|
||||
|
||||
it('maps the cancelled HRESULT to null and still releases the dialog', () => {
|
||||
const { bindings, dialog } = world({ show: vi.fn(() => HRESULT_CANCELLED) })
|
||||
it('maps the cancelled HRESULT to null and still releases the dialog and apartment', () => {
|
||||
const { bindings, dialog, uninitialize } = world({ show: vi.fn(() => HRESULT_CANCELLED) })
|
||||
expect(runFolderDialog(bindings, 'Pick', vi.fn())).toBeNull()
|
||||
expect(dialog.resultPath).not.toHaveBeenCalled()
|
||||
expect(dialog.release).toHaveBeenCalledOnce()
|
||||
expect(uninitialize).toHaveBeenCalledOnce()
|
||||
})
|
||||
|
||||
it('accepts the S_FALSE re-entry HRESULT from CoInitializeEx', () => {
|
||||
@@ -70,10 +76,12 @@ describe('runFolderDialog', () => {
|
||||
expect(runFolderDialog(bindings, 'Pick', vi.fn())).toBe('C:\\picked\\目录')
|
||||
})
|
||||
|
||||
it('throws on a failing CoInitializeEx without creating a dialog', () => {
|
||||
const { bindings, createDialog } = world({}, E_FAIL)
|
||||
it('throws on a failing CoInitializeEx without creating a dialog or uninitializing', () => {
|
||||
const { bindings, createDialog, uninitialize } = world({}, E_FAIL)
|
||||
expect(() => runFolderDialog(bindings, 'Pick', vi.fn())).toThrow('CoInitializeEx failed: HRESULT 0x80004005')
|
||||
expect(createDialog).not.toHaveBeenCalled()
|
||||
// A failed CoInitializeEx must NOT be paired with CoUninitialize.
|
||||
expect(uninitialize).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it.each([
|
||||
@@ -81,10 +89,11 @@ describe('runFolderDialog', () => {
|
||||
['SetTitle', { setTitle: vi.fn(() => E_FAIL) }],
|
||||
['Show', { show: vi.fn(() => E_FAIL) }],
|
||||
['GetResult', { resultPath: vi.fn(() => ({ hr: E_FAIL })) }],
|
||||
] satisfies [string, Partial<Win32FolderDialog>][])('releases the dialog when %s fails', (what, overrides) => {
|
||||
const { bindings, dialog } = world(overrides)
|
||||
] satisfies [string, Partial<Win32FolderDialog>][])('releases the dialog and apartment when %s fails', (what, overrides) => {
|
||||
const { bindings, dialog, uninitialize } = world(overrides)
|
||||
expect(() => runFolderDialog(bindings, 'Pick', vi.fn())).toThrow(`${what} failed: HRESULT 0x80004005`)
|
||||
expect(dialog.release).toHaveBeenCalledOnce()
|
||||
expect(uninitialize).toHaveBeenCalledOnce()
|
||||
void bindings
|
||||
})
|
||||
})
|
||||
|
||||
@@ -94,7 +94,9 @@ describe('pickWin32Directory', () => {
|
||||
const picked = expect(pickWin32Directory(controller.signal, internals)).rejects.toThrow('native directory picker aborted')
|
||||
worker.post({ kind: 'showing', threadId: 99 })
|
||||
controller.abort()
|
||||
await vi.waitFor(() =>{ expect(close).toHaveBeenCalledWith(99) })
|
||||
await vi.waitFor(() => {
|
||||
expect(close).toHaveBeenCalledWith(99)
|
||||
})
|
||||
worker.post({ kind: 'done', path: null })
|
||||
await picked
|
||||
})
|
||||
@@ -108,11 +110,25 @@ describe('pickWin32Directory', () => {
|
||||
controller.abort()
|
||||
expect(closeFailures).not.toHaveBeenCalled()
|
||||
worker.post({ kind: 'showing', threadId: 12 })
|
||||
await vi.waitFor(() =>{ expect(closeFailures.mock.calls.length).toBeGreaterThan(1) })
|
||||
await vi.waitFor(() => {
|
||||
expect(closeFailures.mock.calls.length).toBeGreaterThan(1)
|
||||
})
|
||||
worker.post({ kind: 'done', path: null })
|
||||
await picked
|
||||
})
|
||||
|
||||
it('terminates a worker that never reports showing after an abort', async () => {
|
||||
// The budget runs without a thread id (nothing to WM_CLOSE yet), so a
|
||||
// worker hung before `showing` cannot dangle the pick.
|
||||
const { worker, internals, close } = harness()
|
||||
const controller = new AbortController()
|
||||
const picked = expect(pickWin32Directory(controller.signal, internals)).rejects.toThrow('dialog unresponsive; worker terminated')
|
||||
controller.abort()
|
||||
await picked
|
||||
expect(worker.terminate).toHaveBeenCalledOnce()
|
||||
expect(close).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('terminates an unresponsive worker after the close budget', async () => {
|
||||
const { worker, internals, close } = harness()
|
||||
const controller = new AbortController()
|
||||
@@ -134,7 +150,9 @@ describe('pickWin32Directory', () => {
|
||||
// and the abort service closes it (the same lever a disconnecting client pulls).
|
||||
it.skipIf(process.platform !== 'win32')('opens and abort-closes a real dialog', async () => {
|
||||
const controller = new AbortController()
|
||||
setTimeout(() =>{ controller.abort() }, 400)
|
||||
setTimeout(() => {
|
||||
controller.abort()
|
||||
}, 400)
|
||||
await expect(pickWin32Directory(controller.signal)).rejects.toThrow('native directory picker aborted')
|
||||
}, 30_000)
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user