From 8e7b63f4e5fb0601bb7034d84dfeb1e0610c807f Mon Sep 17 00:00:00 2001 From: Etienne Lescot Date: Tue, 11 Aug 2026 11:41:11 +0200 Subject: [PATCH] refactor(wgc): guard the WinRT calls that can kill the helper silently Every projected call on the capture path reports failure by throwing, and not one of them was caught: get_activation_factory, .as, 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. --- .../native/wgc-capture/src/wgc_session.cpp | 234 ++++++++++++------ electron/native/wgc-capture/src/wgc_session.h | 9 + 2 files changed, 172 insertions(+), 71 deletions(-) diff --git a/electron/native/wgc-capture/src/wgc_session.cpp b/electron/native/wgc-capture/src/wgc_session.cpp index 76649a99..ab9f3b14 100644 --- a/electron/native/wgc-capture/src/wgc_session.cpp +++ b/electron/native/wgc-capture/src/wgc_session.cpp @@ -7,6 +7,7 @@ #include #include +#include #include #include @@ -31,6 +32,57 @@ bool succeeded(HRESULT hr, const char* label) { return false; } +// Turns a C++/WinRT throw into a logged `false`. +// +// The projected calls on the setup path -- get_activation_factory, .as<>, +// item_.Size(), CreateFreeThreaded, CreateCaptureSession, FrameArrived, +// StartCapture -- report failure by throwing, and none of them was caught. +// initialize() therefore could not return false: an exception from any of them +// unwound past it into std::terminate and the process ended with no message at +// all, leaving Electron to report "the helper exited before recording started" +// and nothing else. The HRESULT was in the exception the whole time. +// +// No such failure has actually been observed. This is written from reading the +// calls, not from a reproduction -- see the PR for the crash that prompted it +// and turned out to be an unrelated stack-buffer overrun, which is a fastfail +// rather than an exception and is not catchable here or anywhere. +// +// The label is the diagnostic, so a region never spans two calls a reader would +// want told apart: the frame pool and the capture session get one each. Where a +// region does cover several calls it is because they are one step under one +// name -- "GraphicsCaptureItem for a monitor" is the activation factory, the +// interop cast and Size(), and knowing which of those three threw would not +// change what you do next. `succeeded()` above stays as it is for the calls +// that return an HRESULT rather than throwing; this is its counterpart, not its +// replacement. +template +bool guardWinrt(const char* what, Body&& body) { + try { + return body(); + } catch (winrt::hresult_error const& error) { + std::cerr << "ERROR: " << what << " threw (hr=0x" << std::hex + << static_cast(error.code()) << std::dec << "): " + << winrt::to_string(error.message()) << std::endl; + return false; + } catch (std::exception const& error) { + std::cerr << "ERROR: " << what << " threw (" << error.what() << ")" << std::endl; + return false; + } catch (...) { + std::cerr << "ERROR: " << what << " threw a non-standard exception" << std::endl; + return false; + } +} + +// Deliberately no GraphicsCaptureSession::IsSupported() pre-flight here, though +// it would give a nicer message than an HRESULT on some later call. It is the +// one thing that could *refuse* a recording that works today: a machine where +// IsSupported() answers false but capture would 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 the exact shape of the #336 regression -- a new +// gate in front of a path that was working -- and a better error message is not +// worth carrying it. Everything below only adds a branch that did not exist, so +// at worst it never runs. + int64_t timeSpanToHns(wf::TimeSpan const& value) { return value.count(); } @@ -132,43 +184,77 @@ bool WgcSession::createD3DDevice() { } bool WgcSession::createCaptureItem(HMONITOR monitor) { - auto factory = winrt::get_activation_factory(); - auto interop = factory.as(); - - wgcap::GraphicsCaptureItem item{nullptr}; - HRESULT hr = interop->CreateForMonitor( - monitor, - winrt::guid_of(), - reinterpret_cast(winrt::put_abi(item))); - if (!succeeded(hr, "CreateForMonitor")) { - return false; - } + return guardWinrt("GraphicsCaptureItem for a monitor", [&] { + auto factory = winrt::get_activation_factory(); + auto interop = factory.as(); + + wgcap::GraphicsCaptureItem item{nullptr}; + HRESULT hr = interop->CreateForMonitor( + monitor, + winrt::guid_of(), + reinterpret_cast(winrt::put_abi(item))); + if (!succeeded(hr, "CreateForMonitor")) { + return false; + } - item_ = item; - const auto size = item_.Size(); - width_ = static_cast(size.Width); - height_ = static_cast(size.Height); - return width_ > 0 && height_ > 0; + item_ = item; + const auto size = item_.Size(); + width_ = static_cast(size.Width); + height_ = static_cast(size.Height); + return width_ > 0 && height_ > 0; + }); } bool WgcSession::createCaptureItem(HWND window) { - auto factory = winrt::get_activation_factory(); - auto interop = factory.as(); - - wgcap::GraphicsCaptureItem item{nullptr}; - HRESULT hr = interop->CreateForWindow( - window, - winrt::guid_of(), - reinterpret_cast(winrt::put_abi(item))); - if (!succeeded(hr, "CreateForWindow")) { + return guardWinrt("GraphicsCaptureItem for a window", [&] { + auto factory = winrt::get_activation_factory(); + auto interop = factory.as(); + + wgcap::GraphicsCaptureItem item{nullptr}; + HRESULT hr = interop->CreateForWindow( + window, + winrt::guid_of(), + reinterpret_cast(winrt::put_abi(item))); + if (!succeeded(hr, "CreateForWindow")) { + return false; + } + + item_ = item; + const auto size = item_.Size(); + width_ = roundUpToEven(static_cast(size.Width)); + height_ = roundUpToEven(static_cast(size.Height)); + return width_ > 0 && height_ > 0; + }); +} + +// Two guards, not one around both: they are separate projected calls, and a +// single region would have reported a CreateCaptureSession throw under the +// CreateFreeThreaded label -- naming the wrong call, which is worse than naming +// none. +bool WgcSession::createFramePoolAndSession() { + const bool pooled = guardWinrt("Direct3D11CaptureFramePool::CreateFreeThreaded", [&] { + framePool_ = wgcap::Direct3D11CaptureFramePool::CreateFreeThreaded( + winrtDevice_, + wgdx::DirectXPixelFormat::B8G8R8A8UIntNormalized, + 2, + winrt::Windows::Graphics::SizeInt32{width_, height_}); + return true; + }); + if (!pooled) { return false; } - item_ = item; - const auto size = item_.Size(); - width_ = roundUpToEven(static_cast(size.Width)); - height_ = roundUpToEven(static_cast(size.Height)); - return width_ > 0 && height_ > 0; + return guardWinrt("Direct3D11CaptureFramePool::CreateCaptureSession", [&] { + session_ = framePool_.CreateCaptureSession(item_); + return true; + }); +} + +bool WgcSession::registerFrameArrived() { + return guardWinrt("Direct3D11CaptureFramePool::FrameArrived", [&] { + frameArrivedToken_ = framePool_.FrameArrived({this, &WgcSession::onFrameArrived}); + return true; + }); } bool WgcSession::applySessionOptions(bool captureCursor) { @@ -217,52 +303,26 @@ bool WgcSession::applySessionOptions(bool captureCursor) { return true; } +// Every step reports its own failure, so the two overloads are the step list and +// nothing else. Each returns false rather than throwing past its caller, which +// is what main.cpp's "Failed to initialize WGC display session" has always +// assumed and, until now, was not true of any of them. bool WgcSession::initialize(HMONITOR monitor, int fps, bool captureCursor) { fps_ = fps > 0 ? fps : 60; - if (!createD3DDevice()) { - return false; - } - if (!createCaptureItem(monitor)) { - return false; - } - - framePool_ = wgcap::Direct3D11CaptureFramePool::CreateFreeThreaded( - winrtDevice_, - wgdx::DirectXPixelFormat::B8G8R8A8UIntNormalized, - 2, - winrt::Windows::Graphics::SizeInt32{width_, height_}); - session_ = framePool_.CreateCaptureSession(item_); - - if (!applySessionOptions(captureCursor)) { - return false; - } - - frameArrivedToken_ = framePool_.FrameArrived({this, &WgcSession::onFrameArrived}); - return true; + return createD3DDevice() && + createCaptureItem(monitor) && + createFramePoolAndSession() && + applySessionOptions(captureCursor) && + registerFrameArrived(); } bool WgcSession::initialize(HWND window, int fps, bool captureCursor) { fps_ = fps > 0 ? fps : 60; - if (!createD3DDevice()) { - return false; - } - if (!createCaptureItem(window)) { - return false; - } - - framePool_ = wgcap::Direct3D11CaptureFramePool::CreateFreeThreaded( - winrtDevice_, - wgdx::DirectXPixelFormat::B8G8R8A8UIntNormalized, - 2, - winrt::Windows::Graphics::SizeInt32{width_, height_}); - session_ = framePool_.CreateCaptureSession(item_); - - if (!applySessionOptions(captureCursor)) { - return false; - } - - frameArrivedToken_ = framePool_.FrameArrived({this, &WgcSession::onFrameArrived}); - return true; + return createD3DDevice() && + createCaptureItem(window) && + createFramePoolAndSession() && + applySessionOptions(captureCursor) && + registerFrameArrived(); } void WgcSession::setFrameCallback(FrameCallback callback) { @@ -277,7 +337,12 @@ bool WgcSession::start() { if (!applySessionOptions(captureCursor_)) { return false; } - session_.StartCapture(); + if (!guardWinrt("GraphicsCaptureSession::StartCapture", [&] { + session_.StartCapture(); + return true; + })) { + return false; + } started_ = true; return true; } @@ -359,9 +424,36 @@ void WgcSession::stop() { d3dDevice_.Reset(); } +// The same defect as the setup path, on the hot path. TryGetNextFrame, +// Surface(), the interop cast and SystemRelativeTime() are all projections that +// throw, and a throw leaving a WinRT delegate goes straight to std::terminate -- +// a recording that ends with the process disappearing mid-capture, no stderr, +// and the partial file as the only evidence. +// +// Dropping the frame is the only useful response: one bad frame is not a reason +// to end a recording, and a surface that has gone bad usually stays bad. So it +// is logged once and not at frame rate, which at 60 fps is the difference +// between a diagnostic and a denial of service on the log. void WgcSession::onFrameArrived( wgcap::Direct3D11CaptureFramePool const& sender, wf::IInspectable const&) { + try { + deliverFrame(sender); + } catch (winrt::hresult_error const& error) { + if (!frameErrorLogged_.exchange(true)) { + std::cerr << "WARNING: Dropped a WGC frame (hr=0x" << std::hex + << static_cast(error.code()) << std::dec + << "). Further frame errors are not repeated." << std::endl; + } + } catch (...) { + if (!frameErrorLogged_.exchange(true)) { + std::cerr << "WARNING: Dropped a WGC frame. " + << "Further frame errors are not repeated." << std::endl; + } + } +} + +void WgcSession::deliverFrame(wgcap::Direct3D11CaptureFramePool const& sender) { auto frame = sender.TryGetNextFrame(); if (!frame) { return; diff --git a/electron/native/wgc-capture/src/wgc_session.h b/electron/native/wgc-capture/src/wgc_session.h index 33aba29b..c4fe681c 100644 --- a/electron/native/wgc-capture/src/wgc_session.h +++ b/electron/native/wgc-capture/src/wgc_session.h @@ -46,10 +46,17 @@ class WgcSession { bool createD3DDevice(); bool createCaptureItem(HMONITOR monitor); bool createCaptureItem(HWND window); + bool createFramePoolAndSession(); + bool registerFrameArrived(); bool applySessionOptions(bool captureCursor); void onFrameArrived( winrt::Windows::Graphics::Capture::Direct3D11CaptureFramePool const& sender, winrt::Windows::Foundation::IInspectable const&); + // The body of onFrameArrived, split out so the handler itself is nothing but + // the try/catch that keeps a throwing projection from reaching the WinRT + // delegate and taking the process down with it. + void deliverFrame( + winrt::Windows::Graphics::Capture::Direct3D11CaptureFramePool const& sender); Microsoft::WRL::ComPtr d3dDevice_; Microsoft::WRL::ComPtr d3dContext_; @@ -61,6 +68,8 @@ class WgcSession { FrameCallback frameCallback_; std::mutex callbackMutex_; std::atomic callbacksInFlight_ = 0; + // One line per recording, not one per bad frame. + std::atomic frameErrorLogged_ = false; bool quiesced_ = false; int width_ = 0; int height_ = 0;