fix(win): stop depending on the Visual C++ Redistributable - #321
Merged
Conversation
Store certification rejected 1.9.1 with recording completely unusable:
Error: Native Windows capture exited before recording started
(code=3221225781)
3221225781 is 0xC0000135, STATUS_DLL_NOT_FOUND. wgc-capture.exe imported
VCRUNTIME140.dll, VCRUNTIME140_1.dll and MSVCP140.dll, which are not part of
Windows — they come from the Visual C++ Redistributable. The loader killed the
helper before main(), so the parent only ever saw an exit code. The app itself
started fine because Electron already links the CRT statically.
The import tables of the shipped payload say the helper was not alone:
wgc-capture.exe MSVCP140 VCRUNTIME140 VCRUNTIME140_1
cursor-sampler.exe MSVCP140 VCRUNTIME140 VCRUNTIME140_1
compositor_view.node VCRUNTIME140
That last one is the same visible symptom as the MSIX/PATH bug fixed in 1.9.1 —
editor open, audio playing, preview permanently blank — from an unrelated
cause that colocation does nothing about. Fixing only the helper would have
failed certification a second time, on the preview, and read as a regression of
a fix that was in fact correct.
So: static CRT. /MT for the two CMake helpers, which are standalone processes
sharing no CRT state with anything, and +crt-static for the napi addon, where
only opaque napi_values cross the boundary and Buffers handed to Node carry a
finalizer that frees in the addon what the addon allocated. That removes the
dependency rather than obliging us to redistribute Microsoft's DLLs beside our
own. Verified on the rebuilt binaries: every msvcp/vcruntime import is gone,
recording still produces 5s of h264 with a non-black first frame, cursor
capture still returns typed sprites with hotspots, and probeBackend() still
reports "hardware".
No amount of local testing could have caught this. Every machine that can build
this repo has the redistributable in System32 and always will, so the dependency
is invisible here, in CI, and in every packaging format. Only the import table
tells the truth, which is why before-pack.cjs now reads it — for every
.exe/.dll/.node in the shipped directory — instead of running anything. It
rejects msvcp*/vcruntime*/concrt* and deliberately allows api-ms-win-crt-*,
which is the UCRT and does ship with Windows. It also fails when a binary yields
no imports at all, because every native binary imports at least kernel32: a
parser that quietly stops working would otherwise report "clean" forever, which
is a worse outcome than having no guard.
The .gitattributes line is unrelated housekeeping from writing this: an edit
from Windows rewrote CMakeLists.txt as CRLF and turned 22 changed lines into a
156-line diff.
This was referenced Aug 9, 2026
Two Windows releases in a row shipped a dependency that could not be resolved on
the target machine, and both times someone outside the project found it. 1.9.0's
compositor addon was reached through PATH, which MSIX ignores, and the editor
opened with a blank preview while audio played. 1.9.1's capture helper needed the
Visual C++ Redistributable, which is not part of Windows, and Store certification
rejected it with recording unusable.
Each fix arrived with a guard aimed at the failure already understood. The
colocation check would never have caught the redistributable; the import-table
check would never have caught the PATH bug. Both are worth keeping and neither
generalises, which is the actual problem: a static check only ever knows about
the mistakes already made.
So ask the Windows loader instead. verify-appx-native.ps1 registers a built appx
and, from inside the package, calls LoadLibraryEx with LOAD_WITH_ALTERED_SEARCH_PATH
on every .dll/.node — the same call Node makes for an addon — and starts each
helper executable with no arguments. Whatever the next unresolvable dependency
turns out to be, this fails on it. The Windows Store job runs it on every build,
enabling Developer Mode on the runner it is about to discard.
It deliberately records nothing. A real capture needs a GPU and a desktop session
that a runner does not usefully have, and a flaky gate gets switched off. The
loader is what broke both times and it can be tested with neither: a helper the
loader rejects produces NO output and exits 0xC0000135, while one that reaches
main() prints its usage. That distinction needs no hardware.
Verified both ways on the 1.9.1 package. All 17 binaries load and all three
helpers reach main() under package identity. With avutil-60.dll removed from the
package, 7 of them fail with ERROR_MOD_NOT_FOUND, compositor_view.node among
them — the negative case matters more than the positive one here, and it caught a
real defect in the first version of this script: `@(... | ConvertFrom-Json)` looks
like it produces an array and does not, because Windows PowerShell writes the
whole deserialized array to the pipeline as one object. `Where-Object { -not
$_.ok }` was evaluating `-not` against an array of booleans, which is always
false. It reported "All 1 native binaries load" for seventeen of them and would
have reported success no matter what the probe found.
The documentation gains the part none of this covers. Every failure here shares
one shape — the machines we test on have more installed than the machines we ship
to — so "it works here" is not evidence about anything. Three layers, in order of
preference: remove the dependency at the source, prove what remains automatically,
and keep a virtual machine whose whole value is what is not installed in it. That
last one is the only thing that catches a failure nobody has thought of yet, and
this change does not replace it.
Contributor
|
Warning Review limit reached
Next review available in: 10 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughWindows native builds now use static MSVC runtimes. Packaging rejects Visual C++ runtime imports. AppX packages undergo native load checks under package identity before upload. ChangesWindows native runtime validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WindowsStoreWorkflow
participant VerifyAppxNative
participant MSIXPackage
participant InPackageProbe
participant NativePayload
WindowsStoreWorkflow->>VerifyAppxNative: invoke AppX verification
VerifyAppxNative->>MSIXPackage: extract and register package
VerifyAppxNative->>InPackageProbe: launch probe under package identity
InPackageProbe->>NativePayload: load DLL and NODE files
InPackageProbe->>NativePayload: start helper executables
InPackageProbe-->>VerifyAppxNative: write JSON results
VerifyAppxNative-->>WindowsStoreWorkflow: report success or failure
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
scripts/before-pack.cjs (2)
257-291: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe parser reads only the standard import directory, not delay-loaded imports.
Data directory index 1 covers the normal import table. A binary that delay-loads a runtime DLL records it in directory index 13 (
IMAGE_DIRECTORY_ENTRY_DELAY_IMPORT) and passes this guard unnoticed. The delay-load descriptor has its own layout, but the DLL name field is also an RVA, so the existingfileOffsethelper covers it.This is not a current defect: the payload today is built by your own toolchains and does not delay-load the CRT. It matters if a third-party DLL enters
electron/native/bin/win32-x64/later, which the comment at line 351 explicitly anticipates.♻️ Optional extension to also walk the delay-import directory
const importRva = b.readUInt32LE(dirs + 8); // directory 1 = imports - if (!importRva) return []; + const delayRva = b.readUInt32LE(dirs + 13 * 8); // directory 13 = delay imports + if (!importRva && !delayRva) return []; @@ const names = []; - // Array of 20-byte descriptors, terminated by an all-zero one. - for (let entry = fileOffset(importRva); ; entry += 20) { - const nameRva = b.readUInt32LE(entry + 12); - if (!nameRva) return names; - const at = fileOffset(nameRva); - names.push(b.subarray(at, b.indexOf(0, at)).toString("latin1")); - } + // Both directories are arrays of fixed-size descriptors terminated by an + // all-zero one, and both carry the DLL name as an RVA at a known offset. + const walk = (dirRva, stride, nameOffset) => { + if (!dirRva) return; + for (let entry = fileOffset(dirRva); ; entry += stride) { + const nameRva = b.readUInt32LE(entry + nameOffset); + if (!nameRva) return; + const at = fileOffset(nameRva); + names.push(b.subarray(at, b.indexOf(0, at)).toString("latin1")); + } + }; + walk(importRva, 20, 12); // IMAGE_IMPORT_DESCRIPTOR.Name + walk(delayRva, 32, 4); // ImgDelayDescr.rvaDLLName + return names; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/before-pack.cjs` around lines 257 - 291, Extend importedDlls to inspect both the normal import directory at index 1 and the delay-import directory at index 13, resolving each descriptor’s DLL name through the existing fileOffset helper. Preserve the current empty-result behavior when neither directory exists, and collect names from both tables without changing standard import parsing.
259-266: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard the header reads so a truncated file reports the intended message.
Line 259 reads
e_lfanewwithout checking the file length or theMZsignature. Line 265 reads data directory 1 without checkingNumberOfRvaAndSizes. A truncated or non-PE file therefore fails with aRangeErrorfromreadUInt32LE, not with theis not a PE binaryerror on line 260.The build still fails, so this is not a correctness hole. It only affects the diagnostic quality of a rare failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/before-pack.cjs` around lines 259 - 266, Update the PE parsing logic around the e_lfanew read and import-directory lookup to validate the MZ signature, ensure each header read is within the buffer, and verify NumberOfRvaAndSizes includes directory 1 before reading it. Route truncated or non-PE inputs through the existing “is not a PE binary” error instead of allowing RangeError, while preserving normal importRva handling.scripts/verify-appx-native.ps1 (1)
95-97: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFail fast if the native directory is absent from the package.
Get-NativeDirbuilds a fixed path. If electron-builder ever changes the AppX resource layout,Get-ChildItem -Path $dirthrows under$ErrorActionPreference = "Stop". The child then exits without writing$ReportPath. The parent waits the full 180 s on line 199 and reports "the in-package probe never wrote its report", which does not name the real cause.An explicit check turns a 180 s vague timeout into an immediate, accurate message.
♻️ Proposed fix
$results = @() $dir = Get-NativeDir -Root $PackageRoot + if (-not (Test-Path $dir)) { + $results += [pscustomobject]@{ name = $dir; kind = "layout"; ok = $false; detail = "the native payload directory is not in the package at this path" } + $results | ConvertTo-Json -Depth 4 | Set-Content -Path $ReportPath -Encoding utf8 + return + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/verify-appx-native.ps1` around lines 95 - 97, Validate that the path returned by Get-NativeDir exists before the Get-ChildItem loop; if it is absent, fail immediately with a clear error identifying the missing native directory and package layout issue, ensuring the parent receives an actionable failure instead of waiting for the report timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/verify-appx-native.ps1`:
- Around line 120-132: Update the process execution flow around the
ProcessStartInfo instance and proc to drain StandardOutput and StandardError
concurrently, using asynchronous reads or equivalent concurrent tasks instead of
sequential ReadToEnd calls. Ensure both streams are fully captured before
evaluating the process result, while retaining the existing 20-second timeout
and termination behavior.
In `@technical-documentation/engineering/build-and-packaging.md`:
- Around line 73-75: Update the fenced code block containing the native Windows
capture error to specify the text language, changing its opening fence to use
text while preserving the error output unchanged.
---
Nitpick comments:
In `@scripts/before-pack.cjs`:
- Around line 257-291: Extend importedDlls to inspect both the normal import
directory at index 1 and the delay-import directory at index 13, resolving each
descriptor’s DLL name through the existing fileOffset helper. Preserve the
current empty-result behavior when neither directory exists, and collect names
from both tables without changing standard import parsing.
- Around line 259-266: Update the PE parsing logic around the e_lfanew read and
import-directory lookup to validate the MZ signature, ensure each header read is
within the buffer, and verify NumberOfRvaAndSizes includes directory 1 before
reading it. Route truncated or non-PE inputs through the existing “is not a PE
binary” error instead of allowing RangeError, while preserving normal importRva
handling.
In `@scripts/verify-appx-native.ps1`:
- Around line 95-97: Validate that the path returned by Get-NativeDir exists
before the Get-ChildItem loop; if it is absent, fail immediately with a clear
error identifying the missing native directory and package layout issue,
ensuring the parent receives an actionable failure instead of waiting for the
report timeout.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cd0fcda3-ae96-4e00-ab9c-679a9d176c17
📒 Files selected for processing (7)
.gitattributes.github/workflows/build.ymlcrates/.cargo/config.tomlelectron/native/wgc-capture/CMakeLists.txtscripts/before-pack.cjsscripts/verify-appx-native.ps1technical-documentation/engineering/build-and-packaging.md
Three review findings, all real, all in the machinery meant to make silent packaging failures loud. The import parser read data directory 1 and stopped. A binary that DELAY-loads a runtime records it in directory 13 instead and sailed past the guard untouched. Nothing we ship does that today — the payload comes from our own toolchains — but the guard exists precisely for the third-party binary that has not arrived yet, and it would have missed exactly the case it was written for. Both directories are arrays of fixed-size descriptors carrying the DLL name as an RVA, so one walker covers them at different strides. Verified against shell32.dll, whose delay-loaded tail (UIAutomationCore, dxgi, d2d1, gdiplus) the old parser could not see at all. Every header read is now bounds-checked and the MZ signature verified, so a truncated or non-PE file arrives at "is not a PE binary, or is truncated" instead of a RangeError from readUInt32LE. That is not only cosmetic: NumberOfRvaAndSizes was never consulted, so a binary declaring fewer directories than we index had unrelated header bytes read as an RVA. Confirmed with a four-byte file. And in the appx verifier, two failures could hide each other. The probe reports a missing payload directory as one finding, while the parent asserted a minimum count first — so an accurate diagnosis was replaced by "only checked 1 binaries". Results are now printed before anything is asserted, explicit failures are reported ahead of the count check, and the count check runs only where it is meaningful: on a run where everything passed, which is the only case where a probe that quietly stopped looking is indistinguishable from a healthy package. The verifier also fails immediately when the native directory is absent from the package, rather than letting the child die unwritten and the parent spend its full 180-second timeout to conclude only that no report arrived. Re-verified end to end: the clean payload still passes, the pre-fix binary is still rejected by name, and the truncated file now reports what it is.
The appx verifier redirected stdout and stderr, then read them in sequence: ReadToEnd on stdout, and only once that returned, stderr. A child that fills its stderr pipe buffer while the parent is still blocked on stdout stops writing, the parent never gets its end-of-stream, and neither moves again. WaitForExit(20000) does not break it — the block happens before that line is ever reached. The three helpers print one line each today, so the buffer never fills. The hang arrives the day one of them turns chatty, which is the day this check has the most to say, and it would not even look like a hang: the child dies without writing its report, the parent spends its full 180-second deadline, and the run ends on "the in-package probe never wrote its report" — accurate and pointing nowhere near the cause. Both reads now start before anything blocks, and the process is killed only if it outlives the timeout. Re-verified on the 1.9.1 package: 17 of 17 binaries load under package identity, and each helper's message still comes back intact, which is what proves the asynchronous path is reading the streams rather than silently returning empty. Also labels the fenced block markdownlint flagged under MD040. The other bare fences in that file predate this branch and are left alone.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Store certification rejected 1.9.1: screen recording was completely unusable on the test device.
3221225781is0xC0000135,STATUS_DLL_NOT_FOUND.wgc-capture.exeimportsVCRUNTIME140.dll,VCRUNTIME140_1.dllandMSVCP140.dll— the Visual C++ Redistributable, which is not part of Windows. The loader kills the helper beforemain(), so the parent only ever sees an exit code. The app itself starts fine because Electron already links the CRT statically, which is why "1- Open app" worked and only the record button failed.The helper was not alone
Import tables of the shipped 1.9.1 payload:
wgc-capture.exeVCRUNTIME140,VCRUNTIME140_1,MSVCP140cursor-sampler.exeVCRUNTIME140,VCRUNTIME140_1,MSVCP140compositor_view.nodeVCRUNTIME140require()fails — blank preview, audio keeps playingThe third row is the same visible symptom as the MSIX/
PATHbug fixed in 1.9.1, from an unrelated cause that colocation does nothing about. Fixing only the helper would have failed certification a second time, on the preview, and read as a regression of a fix that was actually correct.The ffmpeg, ggml and whisper DLLs are clean — they import only the UCRT, which does ship with Windows.
Fix
Static CRT, which removes the dependency instead of obliging us to redistribute Microsoft's DLLs beside our own:
CMAKE_MSVC_RUNTIME_LIBRARY MultiThreaded. Standalone processes sharing no CRT state with anything, so/MTcosts ~100 KB each and nothing else.-C target-feature=+crt-static. Safe for the napi cdylib: only opaquenapi_values cross the boundary, and Buffers handed to Node carry a finalizer that frees, in the addon, what the addon allocated.Verified
On the rebuilt binaries:
msvcp/vcruntimeimport is gone; what remains is Media Foundation, D3D11, GDI+, theapi-ms-win-core-*api-sets and the colocated ffmpeg DLLs — all present on a stock Windowsscripts/test-windows-wgc-helper.mjs→ 5 s of h264, 1.27 MB, first frame non-black (luma 241)scripts/test-windows-native-cursor.mjs→ typed cursor sprites with hotspots, 9 screen framesnodewith all 13 exports;probeBackend()→"hardware"Local testing cannot confirm the other half. Every machine that can build this repo has the redistributable in
System32and always will, so a green run here proves the build is not broken and says nothing about a clean machine. The import table is the only evidence for that, which is why the guard reads it rather than running anything.Guard
scripts/before-pack.cjsnow reads the import table of every.exe/.dll/.nodeinelectron/native/bin/win32-x64/and refuses to package if any importsmsvcp*/vcruntime*/concrt*.api-ms-win-crt-*is deliberately allowed — that is the UCRT.Proven to fail on the pre-fix binary rather than merely passing on the fixed one:
It also fails when a binary yields no imports at all — every native binary imports at least
kernel32, so an empty result means the parser broke. A guard that silently stops looking reports "clean" for the rest of the project's life, which is worse than no guard.Import parsing is ~35 lines of PE header walking rather than
dumpbin, so it needs no Visual Studio on the runner.Note
The
.gitattributesline is housekeeping from writing this: an edit from Windows rewroteCMakeLists.txtas CRLF and turned 22 changed lines into a 156-line diff.Summary by CodeRabbit
Bug Fixes
Documentation