[Windows] Fix COMException when restoring a ScrollView as ContentPage.Content after swapping it out#35360
Conversation
…d set it back later in mainthread on Windows
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35360Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35360" |
There was a problem hiding this comment.
Pull request overview
Fixes a Windows-specific WinUI COMException that occurs when a ScrollView is removed from ContentPage.Content and later restored (reusing the same ScrollView instance). The PR updates the Windows ScrollViewHandler content plumbing to detach stale handlers/native views before reattaching, and adds an issue reproduction UI test + HostApp page.
Changes:
- Windows
ScrollViewHandlernow clears the current content host and disconnectsPresentedContent’s existing handler before callingToPlatform(...)and re-adding the content. - Adds a HostApp issue page that swaps
ContentPage.Contentaway from and back to a savedScrollViewinstance. - Adds an Appium UI test for issue #35277 to validate the swap/restore scenario.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Core/src/Handlers/ScrollView/ScrollViewHandler.Windows.cs | Clears the ScrollView content host and disconnects stale PresentedContent handlers before reattaching content to prevent WinUI parentage/COM exceptions. |
| src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue35277.cs | Adds an Appium test covering swap/restore of page content involving a ScrollView. |
| src/Controls/tests/TestCases.HostApp/Issues/Issue35277.cs | Adds the HostApp reproduction page that swaps out and restores a ScrollView as ContentPage.Content. |
| public void ScrollViewContentShouldRestoreWithoutCOMException() | ||
| { | ||
| App.WaitForElement("SwapAndRestoreButton"); | ||
| App.Tap("SwapAndRestoreButton"); |
🤖 AI Summary
📊 Review Session —
|
| Category | Tests | Notes |
|---|---|---|
ios_ui_tests-controls-ScrollView |
83/158 (74 ❌) | from TRX |
ios_ui_tests-controls-ViewBaseTests |
98/270 (171 ❌) | from TRX |
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 5 findings
See inline comments for details.
| currentPaddingLayer.CachedChildren.Clear(); | ||
| } | ||
|
|
||
| // Detach the old handler if it exists (prevents WinUI COM exception on reuse) |
There was a problem hiding this comment.
[minor] Performance / Hot-Path Side Effects — Removing the CachedChildren[0] != nativeContent no-op fast path AND adding an unconditional PresentedContent.Handler?.DisconnectHandler() means every MapContent invocation now (a) tears down the existing handler, (b) forces ToPlatform to allocate a brand-new platform view, (c) discards transient platform-view state (focus, animations, attached behaviors). MapContent is normally only fired on Content change, so the regression is bounded — but worth verifying via a simple trace that no other code path (re-mapping, ModifyMapping consumer, Shell tab re-entry) triggers MapContent without an actual Content change. If it does, this becomes a perf regression. Consider gating the disconnect on nativeContent's parent already being non-null, e.g. only disconnect when reuse is actually happening.
| currentPaddingLayer.CachedChildren.Clear(); | ||
| currentPaddingLayer.CachedChildren.Add(nativeContent); | ||
| } | ||
| currentPaddingLayer.CachedChildren.Add(nativeContent); |
There was a problem hiding this comment.
[minor] Documentation / Traceability — The comment justifies the DisconnectHandler call but does not reference the issue number. Future readers (and git blame consumers) benefit from the link. Update to // Detach the old handler if it exists (prevents WinUI COM exception on reuse — see issue #35277) so the rationale survives even if the comment migrates.
| public void ScrollViewContentShouldRestoreWithoutCOMException() | ||
| { | ||
| App.WaitForElement("SwapAndRestoreButton"); | ||
| App.Tap("SwapAndRestoreButton"); |
There was a problem hiding this comment.
[moderate] Test Coverage — The test only re-finds SwapAndRestoreButton after the swap. That is also present in the temporary page, so this assertion would pass even if the restore reparented an empty ScrollView or if the button were the only thing that survived. Add App.WaitForElement("OriginalLabel") after the tap to prove the original content (button + inner label) actually came back. Also tap a second time to validate the path is idempotent — most CV/ScrollView regressions in this area surface only on the second cycle.
| [Issue(IssueTracker.Github, 35277, "COMException when restoring a page content after swapping it out", PlatformAffected.UWP)] | ||
| public class Issue35277 : ContentPage | ||
| { | ||
| ScrollView _originalScrollView; |
There was a problem hiding this comment.
[minor] Null Annotations — _originalScrollView is declared as ScrollView (non-nullable) without nullable annotation context but is assigned in the constructor. Fine as-is; flag only because the field could trivially be marked readonly since it is never reassigned (only its Content is via the parent page swap).
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 6 findings
See inline comments for details.
| } | ||
|
|
||
| // Detach the old handler if it exists (prevents WinUI COM exception on reuse) | ||
| scrollView.PresentedContent.Handler?.DisconnectHandler(); |
There was a problem hiding this comment.
Unconditional handler churn. Removing the CachedChildren[0] != nativeContent short-circuit and unconditionally calling DisconnectHandler() + ToPlatform() on every MapContent invocation rebuilds the inner native tree even when the content reference is identical (e.g., a property-change rebroadcast or re-entrant mapper). For a content with deep visual hierarchy this is significant work. Suggest gating the disconnect on whether the existing handler's MauiContext differs (i.e., it's a stale handler from a previous visual tree), or move the invariant fix to DisconnectHandler so MapContent keeps its idempotent fast path.
| } | ||
|
|
||
| // Detach the old handler if it exists (prevents WinUI COM exception on reuse) | ||
| scrollView.PresentedContent.Handler?.DisconnectHandler(); |
There was a problem hiding this comment.
Threading. DisconnectHandler and ToPlatform must run on the UI thread. MapContent is invoked through the handler mapper which is called from the dispatcher when bindings update. If a binding raises PropertyChanged from a worker thread, this code path will throw on the WinUI side. Confirm callers always dispatch (current behaviour is unchanged from the old code, so this is not a regression — flagging only).
|
|
||
| if (currentPaddingLayer is not null) | ||
| { | ||
| currentPaddingLayer.CachedChildren.Clear(); |
There was a problem hiding this comment.
Order is correct, but worth a comment. The Clear() must happen before DisconnectHandler() because the disconnect tears down the bridge that owns the native view; clearing first ensures Parent is unset while the handler still knows about the view. Add an inline comment to lock in this ordering for future maintainers.
|
|
||
| Microsoft.Maui.ApplicationModel.MainThread.BeginInvokeOnMainThread(() => | ||
| { | ||
| Content = savedScrollView; |
There was a problem hiding this comment.
Use Dispatcher.Dispatch(...) rather than MainThread.BeginInvokeOnMainThread to mirror the original repro from the issue more closely (issue #35277 step 1 uses Dispatcher.Dispatch). Either should reproduce, but matching the issue removes a degree of freedom if someone has to re-investigate later.
| { | ||
| App.WaitForElement("SwapAndRestoreButton"); | ||
| App.Tap("SwapAndRestoreButton"); | ||
| // If no COMException is thrown, the original ScrollView content (with the button) restores successfully |
There was a problem hiding this comment.
Weak assertion. The test only verifies the button is still findable after the swap+restore. On platforms where the bug never reproduces (Android/iOS/macOS), the test will pass trivially. Add [Category(UITestCategories.Compatibility)] or scope the test with if (Device != TestDevice.Windows) Assert.Ignore(...) so the green result is meaningful only on Windows. Also consider asserting the original "OriginalLabel" is visible after restore — currently the test would also pass if only the button rendered but the rest of the ScrollView didn't.
|
/backport to release/10.0.1xx-sr7 |
|
Started backporting to |
…rollView as ContentPage.Content after swapping it out (#35427) Backport of #35360 to release/10.0.1xx-sr7 /cc @kubaflo @Vignesh-SF3580 --------- Co-authored-by: Vignesh-SF3580 <102575140+Vignesh-SF3580@users.noreply.github.com>
….Content after swapping it out (#35360) > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ### Issue Details Restoring a previously used ScrollView as ContentPage.Content after swapping it out throws System.Runtime.InteropServices.COMException: 'No installed components were detected.' on Windows only. ### Regression PR #30047 ### How it regressed: **Before PR 30047:** Swapping away from a ScrollView left its handler intact. On restore, the same handler was reused, so no new ContentPanel was created and the inner content’s native parent remained valid. No crash. **After PR 30047:** Swapping away destroys the ScrollView’s handler. On restore, a new ScrollViewHandler is created, which calls ToPlatform() on the inner content without first disconnecting its stale handler. As a result, the inner content’s native view still has Parent = old ContentPanel, leading to a COMException. ### Root Cause When ContentPage.Content = savedScrollView restores a previously used ScrollView: - ContentViewHandler.UpdateContent (PR #30047) disconnects the ScrollView’s handler and calls ToPlatform(), creating a new ScrollViewHandler. - The new handler’s MapContent → UpdateContentPanel calls scrollView.PresentedContent.ToPlatform(). - PresentedContent (e.g., VerticalStackLayout) still holds its old handler, whose WinUI native element has its Parent set to the previous visual tree’s ContentPanel. - WinUI throws a COMException at paddingShim.CachedChildren.Add(nativeContent) because an element with an existing parent cannot be added to a new parent. ### Description of Changes - Added scrollView.PresentedContent.Handler?.DisconnectHandler() before calling ToPlatform() in ScrollViewHandler.UpdateContentPanel, aligning with the pattern used in ContentViewHandler.Windows.cs and BorderHandler.Windows.cs. - Added currentPaddingLayer.CachedChildren.Clear() before DisconnectHandler() to unparent the existing native view and ensure a clean state before handler teardown. - Removed the CachedChildren[0] != nativeContent optimization check, as ToPlatform() always returns a new native view instance after DisconnectHandler(), making the condition redundant. ### Why Is This ScrollView-Specific? Every other handler clears its inner content during disconnect, but ScrollViewHandler does not. When MAUI destroys a handler, the inner native view is expected to be detached so that Parent = null. LayoutHandler and ContentViewHandler clear CachedChildren, while RefreshViewHandler sets contentPanel.Content = null, all of which correctly unparent the native view. However, ScrollViewHandler only unsubscribes an event and leaves the inner view attached to the old paddingShim. Later, when the same view is added to a new ScrollViewHandler, WinUI detects that Parent != null and throws a COMException. ### Issues Fixed Fixes #35277 ### Screenshots | Before Issue Fix | After Issue Fix | |----------|----------| | <video width="300" height="600" src="https://github.com/user-attachments/assets/3d3ea98d-23f4-4216-b28f-01f16cdb178b"> | <video width="300" height="600" src="https://github.com/user-attachments/assets/eefdf29d-e34a-47cb-81b0-1a694f79e1d4"> |
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Issue Details
Restoring a previously used ScrollView as ContentPage.Content after swapping it out throws System.Runtime.InteropServices.COMException: 'No installed components were detected.' on Windows only.
Regression PR
#30047
How it regressed:
Before PR 30047: Swapping away from a ScrollView left its handler intact. On restore, the same handler was reused, so no new ContentPanel was created and the inner content’s native parent remained valid. No crash.
After PR 30047: Swapping away destroys the ScrollView’s handler. On restore, a new ScrollViewHandler is created, which calls ToPlatform() on the inner content without first disconnecting its stale handler. As a result, the inner content’s native view still has Parent = old ContentPanel, leading to a COMException.
Root Cause
When ContentPage.Content = savedScrollView restores a previously used ScrollView:
Description of Changes
Why Is This ScrollView-Specific?
Every other handler clears its inner content during disconnect, but ScrollViewHandler does not. When MAUI destroys a handler, the inner native view is expected to be detached so that Parent = null. LayoutHandler and ContentViewHandler clear CachedChildren, while RefreshViewHandler sets contentPanel.Content = null, all of which correctly unparent the native view. However, ScrollViewHandler only unsubscribes an event and leaves the inner view attached to the old paddingShim. Later, when the same view is added to a new ScrollViewHandler, WinUI detects that Parent != null and throws a COMException.
Issues Fixed
Fixes #35277
Screenshots
35277BeforeFix.mp4
35277AfterFix.mp4