[ios] update MemoryAnalyzers and cleanup

[jonathanpeppers]

MemoryAnalyzers 0.1.0-beta.5 now warns about interfaces like:

public class MyView : UIView
{
    IFoo foo;
}

Since another UIView subclass could be assigned to foo at runtime, creating a circular reference -- it should be good to warn about this going forward. This is what allowed some of the leaks to slip through in a589b12.

A few new warnings appeared, but I don't think they are real issues except for src\Core\src\Platform\iOS\MauiUIContextMenuInteraction.cs, which I think we can just remove the field.

I also changed the warning code from MA* to MEM*, as there were already error codes in the MA* range.

Lastly, I renamed Microsoft.Extensions.targets to NuGetVersions.targets as we have many packages in there beyond Microsoft.Extensions. We can also start tracking the MemoryAnalyzers version here as we start adding it to more projects.

[rmarinho]

Looking at the comment, isn't this here for a reason? @Eilon @PureWeen do you remember?

[jonathanpeppers]

No, I showed this to the iOS team in a meeting and they say we should delete it. I planned to have @mandel-macaque review when this is green.

[Eilon]

Well, yeah there's a reason, it says right there 😁

It's from quite some time ago, but my recollection is that without it (which was my first attempt when doing context menus), the delegate would get GC'ed and then you get no context menus after a short amount of time (because it got GC'ed). Or maybe the menu is there but you can't click or something. I recall it took me quite a lot of debugging to figure that out, and that's what I ended up with.

So, please carefully test context menu scenarios on Mac/iOS before deleting this. My also vague recollection is that it wasn't too difficult to repro: just run a context menu sample (like in the Controls Sample project) and after a short while of playing with context menus, something stops working (either they don't show up at all (?), or they show up but clicking does nothing (?)).

[jonathanpeppers]

@Eilon can we put that in the commit message next time? 😄

I think the iOS team should review why you need to do this either way.

[Eilon]

I tried finding more history on this but I couldn't. It's certainly possible that this code isn't strictly needed, perhaps because it was never needed, or perhaps because enough other things changed so it's no longer needed. But I definitely added it because at the time I wrote it the code did not work without it.

[jonathanpeppers]

The problem with saving delegates in a field, either:

1. Customers won't know to do this
2. It actually leaks (possible if two objects have strong references to each other)
3. It isn't needed

So, I mainly want to understand which one applies here. We might wait for @rolfbjarne to return from OOF, too.

[Eilon]

Indeed, perfectly fair points. Unfortunately, I don't remember that far back.

[rolfbjarne]

It looks like the field is solving an actual problem, because otherwise the delegate is collected.

Apple says it's a weak property: https://developer.apple.com/documentation/uikit/uicontextmenuinteraction/3295925-delegate?language=objc, so unless managed code keeps a reference to it, it'll be collected.

It doesn't look like there should be a leak here though, because the delegate instance doesn't have any references to anything else.

However, there's a race condition, the managed FlyoutUIContextMenuInteractionDelegate instance is created in the call to the base constructor, then fetched in the next line. If the GC runs between those two lines, the delegate can still be collected.

Ideally it would be possible to fix this by making MauiUIContextMenuInteraction implement IUIContextMenuInteractionDelegate and the corresponding interface method:

class MauiUIContextMenuInteraction : UIContextMenuInteraction, IUIContextMenuInteractionDelegate
{
	public MauiUIContextMenuInteraction (IElementHandler handler)
		: base (this)
	{ /* ... */ }
	
	UIContextMenuConfiguration? IUIContextMenuInteractionDelegate.GetConfigurationForMenu(UIContextMenuInteraction interaction, CGPoint location)
	{ /* ... */ }
}

but that runs into:

error CS0027: Keyword 'this' is not available in the current context

But I believe his should fix the race condition and keep the delegate alive:

class MauiUIContextMenuInteraction : UIContextMenuInteraction, IUIContextMenuInteractionDelegate
{
	IUIContextMenuInteractionDelegate? _uiContextMenuInteractionDelegate;
	public MauiUIContextMenuInteraction (IElementHandler handler)
		: base (CreateDelegate (out var del)
	{
		_uiContextMenuInteractionDelegate = del;
	}

	static IUIContextMenuInteractionDelegate CreateDelegate (out IUIContextMenuInteractionDelegate del)
	{
		return del = new FlyoutUIContextMenuInteractionDelegate();
	}
}

[jonathanpeppers]

@rolfbjarne I'll update the code for this one, but that brings up two questions:

1. Is there a <summary> comment or something that would tell developers this delegate is weak? And then maybe link to a docs page that explains more?

2. Could the UIContextMenuInteraction automatically save the instance in a field for you somehow? Would be a change to binding generation, I guess. And then any manual bindings are a problem...

[rolfbjarne]

This class is a bit of a weird case (passing the delegate as a parameter to the constructor is not common, it's usually a property setter), so I've created this fix: xamarin/xamarin-macios#19386

[Eilon]

I'm not quite sure what this does, but assuming it's sorta similar to what was there before (that is, holding onto the delegate for as long as required), then that sounds good to me.

[jonathanpeppers]

It's the same as before, we just made it readonly and [UnconditionalSuppressMessage("Memory", "MEM0002"] is the thing that will silence this analyzer:

https://github.com/jonathanpeppers/memory-analyzers

There might be better designs/ideas for this analyzer.

[mattleibow]

All good. Just linked to all the changes for future me and reviewers.