Skip to content

A throwing EffectiveViewportChanged handler leaves a null entry in the queue and wedges layout #24917

Description

@MartinZikmund

Current behavior

If an EffectiveViewportChanged handler throws, layout is permanently wedged. Every later layout pass throws a NullReferenceException from EventManager.RaiseEffectiveViewportChangedEvents, so new content never gets Loaded and the window stops updating.

From the log of the repro below:

System.InvalidOperationException: boom
   at ...EventManager.RaiseEffectiveViewportChangedEvents() in EventManager.cs:line 44
   at Microsoft.UI.Xaml.UIElement.UpdateLayout()
   at Uno.UI.Xaml.Core.CoreServices.OnTick()
System.NullReferenceException: Object reference not set to an instance of an object.
   at ...EventManager.RaiseEffectiveViewportChangedEvents() in EventManager.cs:line 44
   (repeated on every following tick)

The first exception is the handler's own. The NREs are the bug: they come from Uno's queue handling, not from user code, and never stop.

Expected behavior

A throwing handler should surface its own exception once, and layout should carry on. WinUI's CLayoutManager::RaiseEffectiveViewportChangedEvents (src/dxaml/xcp/core/layout/LayoutManager.cpp) walks the queue without modifying it and clears it afterwards, so it never leaves the queue half consumed.

This is based on reading the WinUI C++ source. I did not run a native WinUI repro.

How to reproduce it

Runtime test, run on Skia Desktop (Uno.UI.RuntimeTests):

[TestClass]
[RunsOnUIThread]
public class Given_EffectiveViewport_ThrowingHandler
{
	[TestMethod]
	[RequiresFullWindow]
	public async Task When_Handler_Throws_Once_Layout_Keeps_Working()
	{
		var thrower = new Border { Width = 50, Height = 50 };
		var throwCount = 0;
		thrower.EffectiveViewportChanged += (s, e) =>
		{
			throwCount++;
			throw new InvalidOperationException("boom");
		};

		WindowContent = new ScrollViewer { Content = thrower };
		await WaitForIdle();

		// New content must still load.
		var next = new Border { Width = 50, Height = 50 };
		WindowContent = next;
		await WaitForLoaded(next);
		Assert.IsTrue(next.IsLoaded);
		Assert.IsTrue(throwCount > 0, "The throwing handler should have been raised.");
	}
}
  1. Add the test above to src/Uno.UI.RuntimeTests.
  2. Build and run it with the /runtime-tests skill (Skia Desktop).
  3. The test fails on next.IsLoaded, and the log fills with NullReferenceException at EventManager.RaiseEffectiveViewportChangedEvents.

With the fix sketched below, the same test passes and the log has no NREs.

In a real app, any handler that throws once does the same thing. The same cascade also happens in the runtime-test host: one throwing handler makes every following test time out on "Waiting for loaded event".

Workaround

Never let an EffectiveViewportChanged handler throw (wrap the handler body in try/catch).

Works on UWP/WinUI

Yes (based on the C++ source, see above).

Environment

Uno.WinUI / Uno.SDK

NuGet package version(s)

master @ 7a22141 (net11.0-desktop)

Affected platforms

Skia (Desktop, and the other Skia heads, since the code is in Uno.UI core)

IDE

Other

IDE version

N/A

Relevant plugins

No response

Anything else we need to know?

Root cause, in EventManager.RaiseEffectiveViewportChangedEvents:

for (int i = 0; i < _effectiveViewportChangedQueue.Count; i++)
{
	var (fe, args) = _effectiveViewportChangedQueue[i];
	_effectiveViewportChangedQueue[i] = default;   // slot nulled BEFORE raising
	fe.RaiseEffectiveViewportChanged(args);        // throws -> Clear() below never runs
}
_effectiveViewportChangedQueue.Clear();

When a handler throws, the loop exits with default entries still in the list and Clear() never runs. On the next tick the first slot yields fe == null, so fe.RaiseEffectiveViewportChanged throws an NRE before reaching any real entry. UpdateLayout aborts every time, before the Loaded pass.

Suggested fix: swap the queue out before raising, so the field is never left half consumed. I checked that this makes the test above pass:

var queue = _effectiveViewportChangedQueue;
_effectiveViewportChangedQueue = new(32);

foreach (var (fe, args) in queue)
{
	fe.RaiseEffectiveViewportChanged(args);
}

Note that with this version the remaining entries in that batch are dropped when one handler throws. If that matters, wrap each call in try/catch and report the exception. That is a design call for whoever picks this up.

Related, not the same bug: EffectiveViewportChangedEventArgs.BringIntoViewDistanceX/Y are still [NotImplemented] and throw. That is what triggered this in practice, because a MUX ScrollPresenter test handler reads them. Implementing them is a separate task.

Related: #24901 (EffectiveViewportChanged reports a zero-sized viewport after re-adding an element). Different symptom, same event pipeline.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area/skia ✏️Categorizes an issue or PR as relevant to Skiadifficulty/starter 🚀Categorizes an issue for which the difficulty level is reachable by newcomersgood first issueDenotes an issue ready for a new contributor, according to the "help wanted" guidelines.kind/bugSomething isn't workingplatform/allCategorizes an issue or PR as relevant to the all platformsproject/layout 🧱Categorizes an issue or PR as relevant to layouting and containers (Measure/Arrange, Collections,..)triage/untriagedIndicates an issue requires triaging or verification

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions