Allow for a way to disable or avoid the fields for delegates and other objects in Xamarin.Mac

[Therzok]

Steps to Reproduce

Due to the general design of Xamarin.Mac wanting to assist users so they don't have weak properties collected behind their backs, it can actually cause problems down the line.

1. The fields saved in the __mt_ variables can become invalid.

var win1 = new CustomWindow1();
var win2 = new NSWindow();
win2.AppearanceSource = win1;
win1.Close(); // ReleasedWhenClosed=true, so implicit Dispose is called.
// win2.AppearanceSource is now disposed, instead of being null.

class CustomWindow1()
{
  NSButton button = new NSButton();
  public CustomWindow1()
  {
    ContentView = button = new NSButton();
  }
}

2. The fields can cause ref cycles which are invisible to the user.

Expected Behavior

There should be a way to avoid those fields holding onto objects in the toolkit.

Actual Behavior

The current options we have is using reflection to clear the field or avoiding Xamarin.Mac alltogether for all the properties that expose these fields.

The fields hold onto objects and extend their lifetimes quite a bit, until win2 is Dispose'd. That sounds like a serious lifetime bug.

In native, it would just get collected and all would be good.

Solution

Rolf suggested something like WeakAppearanceSource which would avoid the fields.

Note: the Weak prefix can't be used since WeakDelegate already means weakly typed delegate.

[Therzok]

I've managed to find leaks caused these fields from Delegate, AccessibilityParent, AppearanceSource.

Manually disposing the managed wrappers for objects is a sure-fire way of getting crashed due to us not knowing object lifetimes that exist in AppKit.

[Therzok]

@rolfbjarne any thoughts on this?

[rolfbjarne]

Rolf suggested something like WeakAppearanceSource which would avoid the fields.

Note: the Weak prefix can't be used since WeakDelegate already means weakly typed delegate.

One idea might be to use something Direct somewhere: DirectAppearanceSource.

@rolfbjarne any thoughts on this?

I'm quite hesitant to add this to our product assemblies (Xamarin.Mac.dll), because seems like it'll be a lot of bloat for little gain for most people (even if it could be useful, once we add it, there's no way back).

One idea would be to generate these Direct methods as extension methods in a separate assembly using Mono.Cecil. That way we could gauge the usefulness of it before we make it something we'll have to support.

[Therzok]

I'm quite hesitant to add this to our product assemblies (Xamarin.Mac.dll), because seems like it'll be a lot of bloat for little gain for most people (even if it could be useful, once we add it, there's no way back).

These are real issues that we've experienced in VSMac, they cause indirect leaks or data integrity, just because the bindings are not exposed as WeakReference when weakly referenced in native.

On the other hand, the bloat part a matter of perspective on the issue here. It's going to be hard to enforce using DirectAppearanceSource instead of AppearanceSource when someone writes an extension for VSMac. It's going to be near impossible with an extra cecil-generated assembly.

Why not offer a toggle to disable the fields? A change that says Delegate is actually not held by anything in managed, even though it's a property (aka WeakReference), would force the user to hold onto the object and not rely on the framework creating references implicitly.

Everything becomes much simpler when things become directly mapped to AppKit's design, such as top-down reference graph.

[Therzok]

Just realized something, if the proposed Direct variants of the API are added, the leaks introduced via those fields are going to happen as soon as something uses the non-direct version.

Also, the fact that these fields exist cause leaks in scenarios where NSWindow.Delegate = controller. This causes a ref-cycle because of the managed strong ref.

[Therzok]

Can we revisit this? The problem is increasingly visible in scenarios like the below by causing managed ref cycles:

class Controller : NSWindowController, INSWindowDelegate
{
    public override LoadWindow()
    {
        Window = new NSWindow { Delegate = this, };
    }
}

It applies to all the code that uses NS*Delegate and has generated fields.

[rolfbjarne]

Everything becomes much simpler when things become directly mapped to AppKit's design, such as top-down reference graph.

If you get it wrong, you'll get crashes which are dependent upon when the GC executed.

Why not offer a toggle to disable the fields?

Would that be a global toggle for the entire process, or a per-instance toggle, so you could decide on a per-instance basis?

[Therzok]

If you get it wrong, you'll get crashes which are dependent upon when the GC executed.

These kinds of issues pop up with the static registrar anyway, so just because they work in debug builds, doesn't mean they would work in release builds, right?

Binding the weak fields as WeakReferences in managed code or something similar would force the user to care about it.

Would that be a global toggle for the entire process, or a per-instance toggle, so you could decide on a per-instance basis?

Honestly speaking, a per-instance toggle would just make things more confusing and add another possible outcome for everything - and it's harder to ensure it's added. A process-wide one would at least make it easier to reason about.

[rolfbjarne]

One consequence is that you won't be able to use events, they'll be inherently racy.

var window = new NSWindow ();
window.DidMove += () => { Console.WriteLine ("DidMove"); }
window.DidResize += () = > { Console.WriteLine ("DidResize"); }

In this particular case you might only get the DidResize event, no matter how much you moved the window, because the underlying delegate got collected by the GC between the assignment of the two event handlers.

So we'd have to disable (throw an exception probably) all event usage like this as well, you'd have to use the Delegate pattern everywhere instead.

[Therzok]

We don't use events (by convention), unless it's in a ViewController and we have managed events that we subscribe/unsubscribe in ViewDidAppear/ViewDidDisappear.

Whenever we need delegate handling, we use delegate objects ourselves.

[rolfbjarne]

Here's a POC of a fix: https://github.com/xamarin/xamarin-macios/commit/1afde63475a71b8936a127938be94d544f1efc0e#commitcomment-76096206

You'll have to opt-in by setting ObjCRuntime.Runtime.DisableWeakPropertyStorage = true; as early as possible in the process startup.

It would be great if you could try this and see if it it's actually useful, or if it causes more grief than anything else.

[Therzok]

Thank you very much! I will give it a test run and see how much we can actually simplify by using Delegate = this in controllers!