refactor(wgc): guard the WinRT calls that can kill the helper silently - #339
refactor(wgc): guard the WinRT calls that can kill the helper silently#339EtienneLescot wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesWGC exception safety
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
EtienneLescot
changed the title
57bd082 to
a3d6322
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@electron/native/wgc-capture/src/wgc_session.cpp`:
- Around line 35-44: Revise the comment above guardWinrt to remove claims that
the 0xC0000409 termination was caused by an uncaught WinRT exception or that an
HRESULT was available. Keep the comment limited to projected WinRT calls that
throw and are handled by guardWinrt, without attributing the MAX_PATH
stack-buffer overrun or __fastfail condition to that mechanism.
- Around line 223-232: Update WgcSession::createFramePoolAndSession so
Direct3D11CaptureFramePool::CreateFreeThreaded and
framePool_.CreateCaptureSession(item_) execute in separate guardWinrt regions,
using a distinct CreateCaptureSession label for the latter while preserving the
existing success return behavior.
🪄 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: 5c59d0ed-c205-44a5-9e56-e1344d4df75c
📒 Files selected for processing (1)
electron/native/wgc-capture/src/wgc_session.cpp
Every projected call on the capture path reports failure by throwing, and not one of them was caught: get_activation_factory, .as<IGraphicsCaptureItemInterop>, item_.Size(), CreateFreeThreaded, CreateCaptureSession, FrameArrived, StartCapture, and everything inside onFrameArrived. So the failure mode was std::terminate -- exit code 0xC0000409, no stderr, nothing. main.cpp has always had an "ERROR: Failed to initialize WGC display session" line ready for this and could never reach it, because initialize() did not return false on failure, it took the process with it. guardWinrt() turns a throw into a logged false, applied one or two calls at a time so its label alone names the call that threw -- no breadcrumb to thread through the way mf_encoder.cpp needs one. It is the counterpart of the existing succeeded(), for the calls that throw instead of returning an HRESULT, and it reuses the catch shape applySessionOptions already had. Both initialize() overloads become their step list and nothing else, which also drops the frame-pool block that was duplicated between them verbatim. onFrameArrived gets the same treatment for the same reason, on the hot path: a throw leaving a WinRT delegate is std::terminate, so mid-recording the process would simply vanish. A bad frame is now dropped instead, logged once rather than at frame rate. No GraphicsCaptureSession::IsSupported() pre-flight, deliberately, though an earlier draft of this had one and it read well. It is the only thing here that could refuse a recording that works today -- a machine where IsSupported() answers false but capture would have succeeded stops recording -- and there is no evidence either way about whether such a machine exists. That is the shape of #336: a new gate in front of a path that was working. A nicer error message does not buy that risk. Everything that remains only adds a branch that did not exist, so at worst it never runs. This is hardening, not a fix for an observed failure. The crash that prompted it turned out to be a MAX_PATH stack-buffer overrun rather than an uncaught throw: a build with these guards dies identically and logs nothing, because __fastfail is not an exception. No shipped install path is anywhere near that limit (measured: 140 chars for the Store build, threshold ~255), so it is a local testing hazard only.
a3d6322 to
8e7b63f
Compare
|
Both findings were valid, both fixed in 1. The fast-fail diagnosis in the 2. One thing the fix surfaced that neither comment mentioned: the corrected wording claimed every region wraps a single call, which is still false for Not done: the suggested CodeRabbit CLI install. Nothing was needed beyond reading the two comments against the code. |
1 participant
Summary
Every projected C++/WinRT call on the capture-setup path reports failure by throwing, and none of them is caught:
winrt::get_activation_factory<GraphicsCaptureItem>()factory.as<IGraphicsCaptureItemInterop>()item_.Size()Direct3D11CaptureFramePool::CreateFreeThreaded()framePool_.CreateCaptureSession()framePool_.FrameArrived()session_.StartCapture()onFrameArrivedIf any of them ever throws, the exception leaves
initialize()(or, worse, a WinRT delegate), reachesstd::terminate, and the process dies with no message. main.cpp has anERROR: Failed to initialize WGC display sessionline ready for exactly this and cannot reach it —initialize()does not returnfalseon failure, it takes the process with it.That is a real gap in a helper whose failures are already hard to diagnose from Electron's side. It is just not a gap anyone has been observed to fall into.
Related issue
No issue. Found while working on #338; see the correction above for why it is not evidence for #252 / #292 / #327, contrary to what this PR first claimed.
Type of change
Release impact
Desktop impact
What changed
guardWinrt()turns a throw into a loggedfalse, applied one or two calls at a time so its label alone names the call that threw — no breadcrumb to thread through the waymf_encoder.cppneeds one. It is the counterpart of the existingsucceeded()(for calls that return an HRESULT rather than throwing) and reuses the catch shapeapplySessionOptionsalready had.Both
initialize()overloads become their step list and nothing else:which also drops the frame-pool block that was duplicated verbatim between them.
No
GraphicsCaptureSession::IsSupported()pre-flight, deliberately — an earlier revision of this PR had one, and it has been removed. It was the only thing here that could refuse a recording that works today: a machine whereIsSupported()answers false but capture would in fact have succeeded records fine now and would stop doing so, and there is no evidence either way about whether such a machine exists. That is precisely the shape of #336 — a new gate in front of a path that was working — and a nicer error message does not buy that risk.Everything that remains only adds a branch that did not exist before, so at worst it never runs. That asymmetry is the whole argument for taking this without a reproduction.
onFrameArrivedgets the same treatment, on the hot path — this is the part I would keep even if the rest went.TryGetNextFrame,Surface(), the interop cast andSystemRelativeTime()all throw, and a throw leaving a WinRT delegate isstd::terminate: mid-recording, the process simply vanishes. A bad frame is now dropped instead, logged once rather than at frame rate (60 fps of identical warnings is not a diagnostic). The existingInFlightGuardalready unwound correctly on exception, soquiesceCapture()'s drain is unaffected.Not in scope:
quiesceCapture()andstop()were already guarded and are untouched.Testing
/W4)mainwith #338 in it[stop-timing]sequence, exit 0The repro that motivated this, and why it was not this
On Windows 11 Home
10.0.26200, the helper printed{"event":"ready"}and died ~230 ms later with0xC0000409and no stderr. I read that as an uncaught WinRT throw —0xC0000409is what__fastfailreports for an unhandled C++ exception, and there were seven uncaught candidates right where it died.It was the length of the helper's own
.exepath:0xC0000409@ ~290 ms0xC0000409@ ~319 ms — guards log nothingThe last row is the one that matters.
0xC0000409isSTATUS_STACK_BUFFER_OVERRUN, and here it is the literal meaning — a/GScookie check failing on aMAX_PATHbuffer somewhere in the WinRT/WGC stack — not__fastfail(FATAL_APP_EXIT)from an unhandled exception. Notry/catchcan intercept it. The unzipped diagnostic bundle simply landed in a deeply nested scratch directory.The helper is fine on this machine at a normal path, before and after this change.
One thing worth someone's attention: MSIX install paths under
C:\Program Files\WindowsApps\<publisher>.<package>_<version>_x64__<hash>\are long by construction. I have not measured how close the Store build gets to 259 characters, and if it is close, this is a real crash with no diagnostic and no way to catch it. That seems worth checking independently of this PR.What was verified
Recording still works end to end with this branch:
ready→cursor-capture→capture-adapter→encoder-selection→recording-started, 139 frames in 4.6 s, full[stop-timing]sequence, exit 0. The guards only add a path that did not exist, and on a healthy machine nothing takes it.What is not verified is the guards actually firing, because I cannot make
get_activation_factoryorCreateFreeThreadedthrow on demand. That is the honest state of this PR.Summary by CodeRabbit