Skip to content

fix(desktop): never request the 'large' file icon that kills packaged macOS builds - #3455

Open
UncertaintyDeterminesYou4ndMe wants to merge 1 commit into
apache:mainfrom
UncertaintyDeterminesYou4ndMe:fix/packaged-permission-guide-icon-crash
Open

fix(desktop): never request the 'large' file icon that kills packaged macOS builds#3455
UncertaintyDeterminesYou4ndMe wants to merge 1 commit into
apache:mainfrom
UncertaintyDeterminesYou4ndMe:fix/packaged-permission-guide-icon-crash

Conversation

@UncertaintyDeterminesYou4ndMe

Copy link
Copy Markdown
Contributor

Summary

Packaged macOS builds crash with EXC_BREAKPOINT (SIGTRAP) in Chromium's IconLoader::ReadIcon() the moment the drag-to-grant permission guide loads the app icon. The trigger is app.getFileIcon(bundlePath, { size: 'large' }): Electron documents 'large' as unsupported on macOS, and in practice it hits a fatal NOTREACHED in native code — the process dies before the promise settles, so the try/catch inside loadNativeBundleIcon never sees an error to catch. Unpackaged builds were spared only because #1920 gates native icon loading on app.isPackaged (the #1919 dev-mode crash carried this exact signature: SIGTRAP on ThreadPoolForegroundWorker while reading a bundle icon).

The fix requests { size: 'normal' } instead, hoisted into a single exported constant (BUNDLE_ICON_OPTIONS in app-bundle.ts) shared by both call sites, so the size choice and the reason it must never be 'large' live in one place. Both call sites are only reachable on macOS (resolveAppBundle returns not_darwin elsewhere, and the drag IPC handler early-returns off-darwin), and 'normal' is supported on every platform regardless. The 32×32 result is upscaled to the same 64×64 the code already produced on the platforms where 'large' silently meant 32×32 (Windows), so no downstream contract changes.

Fixes #3352

Verification

Empirical reproduction with this repo's Electron 43.2.0 on an Apple silicon Mac (the exact version and hardware class from the report):

  • app.getFileIcon(<.app>, { size: 'large' }) → process exits 133 (SIGTRAP), no JavaScript error thrown — matching the crash report's IconLoader::ReadIcon() frame.
  • app.getFileIcon(<.app>, { size: 'normal' }) → resolves a 32×32 image.
  • The full fixed path (loadNativeBundleIcon(true, …)getFileIcon normalresize({64,64})toDataURL()) → produces a valid 7 KB PNG data URL, exit 0.

Checks run locally (Node 24.18):

  • npm --workspace @maka/desktop run typecheck — pass
  • npm --workspace @maka/desktop run test — 1038 pass / 0 fail (includes the new regression tripwire)
  • biome check on the changed files — clean; knip desktop workspace — clean
  • Regression-test validity: reverting BUNDLE_ICON_OPTIONS to 'large' makes the new test fail; restoring the fix makes it pass.

Not run: a full packaged DMG build. The crash and the fix were both demonstrated against the real Electron binary with the production code path mirrored 1:1; the packaged-only aspect is fully explained by the app.isPackaged gate in loadNativeBundleIcon.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code diagnosed the crash, wrote the fix and test, and ran the reproduction and verification; an independent adversarial review pass (also Claude) probed the fix with fault-injection experiments before submission. I reviewed and verified the result.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

… macOS builds

app.getFileIcon(path, { size: 'large' }) is unsupported on macOS and hits
a fatal NOTREACHED inside Chromium's IconLoader — the process dies with
SIGTRAP before the promise settles, so loadNativeBundleIcon's try/catch
never runs. Packaged builds crashed the moment the drag-to-grant
permission guide loaded the app icon; dev builds were spared only by the
isPackaged gate from apache#1920.

Hoist the size choice into one shared BUNDLE_ICON_OPTIONS constant
requesting 'normal' (32x32, supported everywhere), which the existing
resize step upscales to the same 64x64 the code already produced.

Fixes apache#3352

Generated-by: Claude Code
@UncertaintyDeterminesYou4ndMe

Copy link
Copy Markdown
Contributor Author

Adversarial review record (pre-submission)

An independent review pass probed this fix with fault-injection experiments before submission. All six attack vectors came back safe; evidence was experimental, not read-only:

  1. No other crash sites. Repo-wide sweep: getFileIcon exists at exactly the two call sites in permission-overlay-main.ts, both now on BUNDLE_ICON_OPTIONS. No other size: 'large' icon requests anywhere.
  2. Downstream contract holds with a 32px source. Measured on Electron 43.2.0: 'normal' returns logical 32×32 with scaleFactors [1,2] (a real 64px rep exists); after resize({64,64}) → real 64×64 PNG that round-trips through createFromDataURL. The overlay renders the icon at 28×28 CSS and guards only on naturalWidth > 0 — no minimum-size assumption. No fidelity regression is possible: on macOS 'large' never returned an image, it killed the process.
  3. Type contract. FileIconOptions.size accepts the readonly const with no cast; desktop typecheck clean.
  4. The regression test fails without the fix (verified by injection): reverting the constant to 'large' and rebuilding → 1 fail; restoring → full desktop suite 1038/1038.
  5. Non-darwin unreachability confirmed by executing resolveAppBundle for win32/linux (not_darwin → loader invoked 0 times) plus the drag handler's platform early-return. 'normal' would be valid there anyway.
  6. fix(desktop): make macOS dev permission grants reliable #1920's isPackaged gate untouched; its regression test still passes.

Extra: the crash and fix were both reproduced against the real Electron binary — { size: 'large' } exits 133 (SIGTRAP) with the catch block provably never executing; the fixed path (packaged gate → getFileIcon → resize → data URL) exits 0 with a valid PNG.

One out-of-scope observation for the record: start() awaits the decorative icon fetch before opening the card with no timeout (permission-overlay-main.ts:195). A hung macOS icon service would delay the card indefinitely. Pre-existing behavior, untouched here; happy to file a follow-up issue if maintainers think it's worth hardening.

@M4n5ter M4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

English

Reviewed exact head 6a18d8376065f7c7989512e1c1efb36607bcb897.

The diagnosis of the native crash is correct: Electron documents large as unsupported on macOS, and the Chromium implementation reaches a fatal NOTREACHED before JavaScript can catch anything. I found one remaining issue with the proposed fix.

[Important] Use the shipped Maka icon instead of the file-type icon

Changing the two calls at permission-overlay-main.ts:80,277 from large to normal prevents the crash, but it does not load the icon belonging to Maka.app.

Electron 43.2.0 forwards app.getFileIcon() to Chromium's IconLoader. On macOS, that implementation discards the individual application identity, reduces the path to its UTType, and calls NSWorkspace.iconForContentType:. Chromium's own contract explicitly notes that applications have unique icons but this loader does not handle them. An Electron maintainer has also confirmed that getFileIcon returns a file-type icon rather than the icon belonging to the individual file:

Consequently, the permission card and drag replica will show the generic application-bundle icon rather than the Maka icon. The current real-device evidence—32×32, non-empty PNG, successful resize and round-trip—only verifies the image's shape, not its identity. This also contradicts the existing comment that the result is the same image Finder and the TCC list display for the bundle.

There is now a smaller root fix available. This PR's exact base is #3451, which already:

  • ships the canonical 1024×1024 assets/icon.png in packaged resources;
  • provides desktopAssetPath() for packaged and development layouts;
  • uses that same PNG as electron-builder's macOS bundle-icon source.

Please load that known asset with nativeImage.createFromPath(desktopAssetPath(..., 'assets', 'icon.png')) and resize it as needed. resolveAppBundle() should remain the authority for the file being dragged, but it no longer needs to be the icon source.

This also gives the simplify-audit result for free:

  • delete both app.getFileIcon() calls;
  • delete BUNDLE_ICON_OPTIONS;
  • delete loadNativeBundleIcon() and its packaged-only state;
  • delete the implementation-level “not large” test;
  • delete the second native fallback during drag—the renderer normally supplies the complete row image, and the existing code already permits an empty decorative drag icon when decoding fails;
  • remove the unbounded native icon await from start().

Since the PNG becomes a load-bearing packaged resource, add assets/icon.png to assertPackagedResources; the existing desktopAssetPath tests can continue to cover the dev/packaged path split.

No other material findings. The crash-fix direction is correct, but the current implementation retains the wrong icon authority and returns the wrong representation for every supported packaged-macOS invocation.

Verdict: request changes.

简体中文

已审查精确 head 6a18d8376065f7c7989512e1c1efb36607bcb897

对 native crash 的诊断是正确的:Electron 明确说明 macOS 不支持 large,Chromium 实现会在 JavaScript 有机会捕获错误前进入 fatal NOTREACHED。不过,当前修复仍有一个问题。

[Important] 应使用随包发布的 Maka 图标,而不是文件类型图标

permission-overlay-main.ts:80,277 的两个调用从 large 改为 normal 可以避免崩溃,但不会读取属于 Maka.app 的图标。

Electron 43.2.0 会把 app.getFileIcon() 转发给 Chromium 的 IconLoader。macOS 实现会丢弃具体应用身份,把路径归约为 UTType,然后调用 NSWorkspace.iconForContentType:。Chromium 自己的 contract 也明确说明:macOS 应用虽然有独有图标,但这个 loader 不处理它们。Electron maintainer 同样确认,getFileIcon 返回的是文件类型图标,而不是具体文件自身的图标:

因此,权限卡片和拖拽图会显示通用 application-bundle 图标,而不是 Maka 图标。目前的真机证据——32×32、PNG 非空、resize 和 round-trip 成功——只能证明图片结构有效,不能证明图标身份正确。这也与现有注释中“和 Finder/TCC 列表显示同一 bundle 图标”的说法冲突。

当前已经有更小的根因修复。本 PR 的精确 base 就是 #3451,它已经:

  • 将 canonical 1024×1024 assets/icon.png 放入 packaged resources;
  • 提供 desktopAssetPath() 处理 packaged/dev 两种目录布局;
  • 使用同一 PNG 作为 electron-builder 的 macOS bundle icon 来源。

请通过 nativeImage.createFromPath(desktopAssetPath(..., 'assets', 'icon.png')) 读取这个已知资源,再按需 resize。resolveAppBundle() 继续作为实际拖拽文件的 authority,但不再兼任图标来源。

这也能直接落实 simplify audit:

  • 删除两处 app.getFileIcon()
  • 删除 BUNDLE_ICON_OPTIONS
  • 删除 loadNativeBundleIcon() 及其 packaged-only state;
  • 删除只检查 “not large” 的实现型测试;
  • 删除拖拽时的第二次 native fallback——正常情况下 renderer 会提供完整的 row image,而现有代码已经允许解码失败时使用空的装饰性 drag icon;
  • 移除 start() 中无时限的 native icon await。

由于该 PNG 会成为正式的 packaged runtime dependency,还应将 assets/icon.png 加入 assertPackagedResources;现有 desktopAssetPath 测试可以继续验证 dev/packaged 路径分支。

没有发现其他 material finding。止崩方向正确,但当前实现仍保留了错误的 icon authority,并会在每次受支持的 packaged macOS 调用中返回错误 representation。

结论:request changes。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(desktop): packaged macOS permission guide crashes while loading the app icon

2 participants