Proposal: Replace Window.Visible with cached/proxy value

[mqudsi]

Proposal: Replace Window.Visible with cached/proxy value

Instead of directly exposing the WinRT IWindow.Visible as a property, use a C# property instead that is updated on IWindow.Closed and IWindow.VisibilityChanged to protect against exceptions during teardown.

Summary

Currently, accessing Window.Visible directly maps to a WinRT/COM operation that throws a COMException if the Window has been closed. Since UI code may continue to be called until the state quiesces and operations that may query Window.Visible may already be taking place mid-flight, it would be safer for this property to provide cached/proxied window visibility state and for reading it to be a simple read operation that does not cross ABI boundaries and does not depend on the window handle still being valid.

Rationale

• As stated, operations may be mid-flight when the window is closed and generally one would not want to block the closing of the Window (in an event handler) for non-critical UI update code to complete. As such, it is "normal" for regular UI code to access properties of the window.
• Window.Visible is defined as a property and not a function; it is not the expectation that its execution will be crossing ABI boundaries and that it would be performing "meaningful" work;
• Generally C# class properties are expected to be non-throwing in idiomatic, well-designed code
• Developers coming from other UI frameworks (including WPF and SWF, if I'm not mistaken) expect that as long as the reference to the window itself remains live (i.e. the Window has not been cleaned up by the GC), access to its properties would be valid (though other operations might - of course - throw after the window has closed)
• The Window facade around IWindow should make every reasonable (non-expensive and straightforward) effort to make it seem like you are interacting with a native C# object and accessing a property leading to a COMException being thrown flies in the face of that.
• If a developer is accessing Window.Visible repeatedly in their UI update code, it is not an unreasonable expectation that this is a quick and fast operation that does not incur much cost. Crossing ABI boundaries to query a value that rarely changes contradicts that, especially when there exists an easy fix for this.

Important Notes

This is what I have in my own project to protect against this:

public sealed partial class MainWindow : Window
{
    private TypedEventHandler<object, WindowVisibilityChangedEventArgs> _visibilityHandler
        = static (sender, args) => ((MainWindow)sender).IsVisible = args.Visible;

    /// <summary>
    /// Caches the window visibility and updates on the <see cref="Window.VisibilityChanged"/> event;
    /// should be used in lieu of <see cref="Visible"/> to protect against COM exceptions when the
    /// window has been destroyed.
    /// </summary>
    public bool IsVisible { get; private set; }
    [Obsolete("Use " + nameof(IsVisible) + " instead to protect against COM exceptions")]
    public new bool Visible => IsVisible;

    public MainWindow()
    {
        this.InitializeComponent();

        IsVisible = true;
        VisibilityChanged += _visibilityHandler;
        Closed += static (sender, _args) =>
        {
            var @this = (MainWindow)sender;
            @this.VisibilityChanged -= @this._visibilityHandler;
            @this.IsVisible = false;
        };

        // Remainder of initialization code
    }
}

Obviously this is not a sufficient solution in that it doesn't protect against Window.Current.Visible being called and must be manually copied to each Window class in a project.

Open Questions

• Is this technically a breaking change? I don't think so, as I can't see any previously working code no longer compiling after this change, nor compiling but no longer working as expected. There is a slight concern about the reactivity latency here (the time it takes for Window.IsVisible in the sample code above to reflect a change if the call races with the execution of the VisibiltyChanged callback handler) but since the IWindow.Visible property is non-synchronized and non-atomic (the actual state of the window's visibility may change immediately after the call determines the return value, resulting in stale information being returned), this does not seem materially different.

[Scottj1s]

@mqudsi Thanks for your analysis and suggestion.

Unfortunately, this is not something we can support for several reasons:

1. Caching a value for Visible is misleading if there is no window, particularly if the cached value is True. But a cached value of False is also problematic. Attempting to map a third state (no Window) to the boolean Visible property is lossy and would likely break existing code.
2. It would be a runtime breaking change to no longer raise a COMException. Client code could be relying on that signal, such as for cleanup logic. Whether that's a good design choice is beside the point - exceptions are commonly considered part of an API contract.
3. All types in WinUI 3 (and Windows App SDK) are based on WinRT, and that does carry some overhead for frequent calls across the ABI. But it would be confusing to provide caching for a single property of a single class, and not for other Window properties (like Content, Bounds, Title, etc), much less for all WinRT types (including Windows.UI.Xaml.Window).
4. Other UI frameworks, such as WPF, have very different designs and implementations. It's not always possible to map behaviors or performance characteristics between them.

I appreciate the added complexity to client code, but I think your approach of handling Window.VisibilityChanged (or Window.Closed) to know when any window property is safe to access, is best.

[mqudsi]

Thanks for evaluating the proposal and providing your thoughts.

I realize in hindsight that I omitted an important part of why I wanted this for Window.Visible specifically: I also use (my safe version of it) to test if it's OK to access the other properties you mentioned like Title, Bounds, etc.

In light of your reasons, can I amend the proposal slightly? Would you be amenable to a different (completely new) property, let's say (and defer bikeshedding over the name to later) called Created that gets init to false, set to true after the window has been fully constructed and is safe to access, then is changed to false on the Closed event (so like the sample code above but minus the VisibilityChanged handler). This would address your points one and two?

It loses some of the value (for me, personally) but if the expectation (as you laid out) is that consumers of the API would need to track and cache creation status to know when it's safe to access properties (barring race conditions, unfortunately) then providing this out of the box would certainly reduce the boilerplate required.

(Actually, a better thought: if you change the proposed Created to Disposed with the inverse semantics, I presume you could just expose an already extant internal property/field and it would fit with the idiomatic C# pattern of checking if a disposable object is disposed or not prior to performing operations that require passing a liveness check, though Window would be a bit of an odd duck since it doesn't actually implement IDisposable, though it does have (unmanaged) resources it needs to relinquish at teardown.)

[Scottj1s]

That's a tempting proposal, but it suffers from the time-of-check to time-of-use (TOCTOU) problem. I.e., a Windows.Closed event could fire asynchronously at any time, so client code on a non-UI thread that sees a Disposed=false property value and proceeds to access a Window property could still crash. Yes, it's a narrow window, but we'd be reluctant to provide a pattern that's reliable most of the time.

/cc: @JJBrychell for opinions

[mqudsi]

Yup, that was my “race condition” reference above. But as I understand it, there is no solution (whether provided by the framework or implemented by the user) that can actually avoid this due to the current underlying architecture, and all code would have to handle COMException everywhere?

[Scottj1s]

Yes, that's my understanding - the only truly reliable technique is try/catch at the point of use