Fix ownership of WTS plugin references - #189
Merged
Marc-André Moreau (mamoreau-devolutions) merged 1 commit intoSep 24, 2026
Merged
Marc-André Moreau (mamoreau-devolutions) merged 1 commit into
Marc-André Moreau (mamoreau-devolutions) merged 1 commit into
Conversation
Copilot started reviewing on behalf of
Richard Markiewicz (thenextman)
September 24, 2026 01:05
View session
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The ownership correction is targeted, ABI-compatible, and covered by focused lifecycle regression tests.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes WTS plugin COM reference ownership while preserving the existing ABI.
Changes:
- Releases plugin references on replacement, clearing, and destruction.
- Documents borrowed getter and ownership-transfer setter semantics.
- Adds four native regression scenarios using a test-only factory.
| File | Description |
|---|---|
dll/RdpInstance.cpp |
Releases transferred plugin references correctly. |
include/MsRdpEx/RdpInstance.h |
Documents ownership semantics. |
tests/logging/PluginReferenceTest.cpp |
Tests plugin reference lifecycles. |
tests/logging/PluginReferenceFixture.cpp |
Creates headless test instances. |
tests/logging/GatewayShutdownFixture.def |
Exports the test-only factory. |
tests/logging/CMakeLists.txt |
Builds and registers the new tests. |
tests/logging/README.md |
Documents the expanded regression suite. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Richard Markiewicz (thenextman)
marked this pull request as ready for review
September 24, 2026 01:07
Marc-André Moreau (mamoreau-devolutions)
approved these changes
Sep 24, 2026
Marc-André Moreau (mamoreau-devolutions)
merged commit Sep 24, 2026
d7fec7f
into
master
1 check passed
Marc-André Moreau (mamoreau-devolutions)
added a commit
that referenced
this pull request
Sep 24, 2026
## Summary - Keep the managed `IUnknown` reference owned until `SetWTSPluginObject` returns successfully; release it in `finally` if the COM call throws. - Preserve the native ownership-transfer ABI and success path from #189 without changing the native setter. - Add headless managed tests that verify exception cleanup and successful ownership transfer via a fake `IMsRdpExInstance`. ## Validation - `dotnet build dotnet\Devolutions.MsRdpEx\Devolutions.MsRdpEx.csproj --framework net48 --no-restore --verbosity quiet -p:WarningLevel=0` - `dotnet build dotnet\Devolutions.MsRdpEx\Devolutions.MsRdpEx.csproj --framework net8.0-windows --no-restore --verbosity quiet -p:WarningLevel=0` - `dotnet test dotnet\MsRdpEx_Test\MsRdpEx_Test.csproj --no-build --filter "FullyQualifiedName~MsRdpEx.Tests.RdpInstancePluginTests" --verbosity quiet` — 2 passed, 0 failed. No native COM registration or RDP client is required by the tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Marc-André Moreau (mamoreau-devolutions)
added a commit
that referenced
this pull request
Sep 24, 2026
## Summary
This closes the **primary root cause** of RDMW-24263 / the forum report
("Embedded RDP sessions leak the ActiveX control after the tab is
closed"): the wrapped `mstscax` control instance itself was never
released, so every closed embedded session permanently leaked 2 native
threads (`mstscax!CSND::SND_Main`, `mstscax!CRCV::RCVMain`) and ~100
handles.
It also fixes a refcount-imbalance bug that is a strong suspect for the
intermittent native heap-corruption crash (`ntdll.dll 0xC0000374`)
observed during teardown/forced-GC testing on the RDM side.
## Bugs fixed
1. **`CClassFactory::CreateInstance` leaked the real control's reference
(primary leak).**
`IClassFactory::CreateInstance` returns the real `mstscax` control
object with refcount 1, owned by the caller. `CMsRdpClient`'s
constructor takes its *own* `AddRef()` on that pointer, but the factory
never released its copy after wrapping — so the real control's refcount
never reached 0 when the `CMsRdpClient` wrapper was later destroyed.
Nothing ever drove the control's core shutdown, matching the reported
symptom exactly (send/receive threads parked forever in
`CTSThread::ThreadMsgLoop`).
2. **`CMsRdpClient` destructor released its wrapper objects after the
control interfaces.**
`m_pMsRdpExtendedSettings` and `m_pMsRdpExInstance` hold references into
control-internal objects whose lifetime is owned by the control.
Reordered so wrappers are released first, before the control interfaces.
3. **`CMsRdpPropertySet` released a reference it never took.**
It stored the internal `ITSPropertySet` pointer (for core/base/transport
TS property sets) without an `AddRef`, but its destructor
unconditionally called `Release()` on it anyway — decrementing a
refcount it never incremented, which could prematurely free a
control-internal object still referenced elsewhere. Added the missing
`AddRef()` in the constructor to balance the destructor's `Release()`.
## Verification
- Full x64 build (Ninja/MSVC).
- `ctest` — **31/31 passing**, including all existing DVC factory /
plugin-reference / detached-instance regression tests from #189–#192.
## How to verify the primary leak fix manually
Open and close N embedded RDP sessions, take a full-memory dump, count
threads whose stack contains `mstscax!CSND::SND_Main`. Before this fix:
N threads remain. After: 0.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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
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.
Summary
IUnknownwhen the plugin is replaced, cleared, or its RDP instance is destroyed, while preserving the existing borrowed getter and ownership-transfer setter ABI.IUnknownregression fixture covering replacement, same-pointer replacement with a separately owned reference, clearing, and destruction; keep the test factory out of the production DLL.Validation