Skip to content

[Windows] Fix COMException when restoring a ScrollView as ContentPage.Content after swapping it out #35360

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kubaflo merged 1 commit into from
May 9, 2026
+88 −7
Merged

[Windows] Fix COMException when restoring a ScrollView as ContentPage.Content after swapping it out #35360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions src/Controls/tests/TestCases.HostApp/Issues/Issue35277.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
namespace Maui.Controls.Sample.Issues;

[Issue(IssueTracker.Github, 35277, "COMException when restoring a page content after swapping it out", PlatformAffected.UWP)]
public class Issue35277 : ContentPage
{
ScrollView _originalScrollView;

@MauiBot MauiBot May 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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).


public Issue35277()
{
var swapButton = new Button
{
Text = "Swap and Restore",
HorizontalOptions = LayoutOptions.Center,
AutomationId = "SwapAndRestoreButton"
};
swapButton.Clicked += OnSwapAndRestoreClicked;

_originalScrollView = new ScrollView
{
Content = new VerticalStackLayout
{
Spacing = 20,
VerticalOptions = LayoutOptions.Center,
Children =
{
swapButton,
new Label
{
Text = "Original Content",
HorizontalOptions = LayoutOptions.Center,
AutomationId = "OriginalLabel"
}
}
}
};

Content = _originalScrollView;
}

void OnSwapAndRestoreClicked(object sender, EventArgs e)
{
var savedScrollView = _originalScrollView;
Content = new Label
{
Text = "Temporary Content",
HorizontalOptions = LayoutOptions.Center,
VerticalOptions = LayoutOptions.Center,
AutomationId = "TemporaryLabel"
};

Microsoft.Maui.ApplicationModel.MainThread.BeginInvokeOnMainThread(() =>
{
Content = savedScrollView;

@MauiBot MauiBot May 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});
}
}
22 changes: 22 additions & 0 deletions src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue35277.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
using NUnit.Framework;
using UITest.Appium;
using UITest.Core;

namespace Microsoft.Maui.TestCases.Tests.Issues;

public class Issue35277 : _IssuesUITest
{
public Issue35277(TestDevice device) : base(device) { }

public override string Issue => "COMException when restoring a page content after swapping it out";

[Test]
[Category(UITestCategories.ScrollView)]
public void ScrollViewContentShouldRestoreWithoutCOMException()
{
App.WaitForElement("SwapAndRestoreButton");
App.Tap("SwapAndRestoreButton");

@MauiBot MauiBot May 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

// If no COMException is thrown, the original ScrollView content (with the button) restores successfully

@MauiBot MauiBot May 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

App.WaitForElement("SwapAndRestoreButton");
}
}
17 changes: 10 additions & 7 deletions src/Core/src/Handlers/ScrollView/ScrollViewHandler.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,20 +149,23 @@ static void UpdateContentPanel(IScrollView scrollView, IScrollViewHandler handle
{
currentPaddingLayer.CachedChildren.Clear();
}

return;
}

if (currentPaddingLayer is not null)
{
currentPaddingLayer.CachedChildren.Clear();

@MauiBot MauiBot May 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// Detach the old handler if it exists (prevents WinUI COM exception on reuse)

@MauiBot MauiBot May 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

scrollView.PresentedContent.Handler?.DisconnectHandler();

@MauiBot MauiBot May 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@MauiBot MauiBot May 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


var nativeContent = scrollView.PresentedContent.ToPlatform(handler.MauiContext);

if (currentPaddingLayer is not null)
{
// Only update if content has changed or is missing
if (currentPaddingLayer.CachedChildren.Count == 0 || currentPaddingLayer.CachedChildren[0] != nativeContent)
{
currentPaddingLayer.CachedChildren.Clear();
currentPaddingLayer.CachedChildren.Add(nativeContent);
}
currentPaddingLayer.CachedChildren.Add(nativeContent);

@MauiBot MauiBot May 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

}
else
{
Expand Down
Loading