Event and object leakage in MenuTracker

[dodexahedron]

Discovered some issues with MenuTracker while working on MenuBarTests.cs.

I was trying to add some pre-conditions to the TestMenuOperations test, to be sure it had a clean slate to start from, but it fails that check unless it is run in isolation, because the other tests change things.

Upon investigation, I found the root(s) of the problem:

If Register is called on MenuTracker, the MenuBar is subscribed to three events.

However, there is nothing that can un-register them from those events, nor is there anything that can remove them from the bars HashSet in MenuTracker.

Why are these issues problematic?

Well, for a couple reasons, unfortunately.

For one, it means that, if a menubar is deleted in the UI, it still exists with a path back to root, via the MenuTracker, and won't be collected by the GC.
That also means it is still alive and, if the events are raised, they are being called on the objects that should be gone.

Also, now that View is IDisposable, in v2, it exposes a potential runtime ObjectDisposedException or other potential issues, if anything happens once a MenuBar has been disposed, through any means.

The .Contains call in Register also checks if the menubar being registered is in the collection, but that's purely a reference equality, since no equality operator currently exists on MenuBar or any of its ancestor types. That has the potential to cause other weird problems in conjunction with the above problems, and would continue to compound and get weirder and weirder, as code is re-generated and re-compiled.

Proposed solution:

A few things, to address each part of the problem...

• Terminal.Gui really should expose a Disposing event on View, so that dependent objects can properly handle themselves when a View is disposed.
  • This should likely be done before working on the rest of the fixes for this.
• We should provide an explicit UnRegister method to do the inverse of the Register method, at minimum.
• We should be sure to call that method when deleting a MenuBar.
  • This may be un-fun, depending on how Redo and Undo are handled, which I haven't yet explored in the context of MenuBars.
• We should probably define a more formal means of determining equality between MenuBar items, at least in the bars collection, which can either be done in Terminal.Gui or, the probably better and less-impacting way, by having an appropriate IComparer in TGD to handle the HashSet.
• Some other thoughts, but I need to let it simmer a bit to determine if they're relevant, especially once the above are handled.

[dodexahedron]

As I've been working on these tests, I made some changes to the MenuTracker that improved static analysis without changing behavior, which then allowed minor improvements at the call sites for MenuTracker.Instance.GetParent.

Also, there was a spot in some code that replaces a menuitem that found the index of an existing item, converted the whole Children collection to a list, removed the old item at the found index, and then inserted a replacement at the same index.

That was condensed down to finding the index and then just directly replacing the array element. I see a few more places some similar improvements can be made in that class (RemoveMenuItemOperation), as well as a little more that can be done to MenuTracker, that I'm opportunistically taking care of while I'm doing the tests for it. I'm making each change in individual commits so it's easy to see what's been done.

All tests continue to pass.

[tznind]

Interesting, yes you are right we should definetly handle unregistering. I think we probably never dispose of views currently. We add/remove them but because of undo we are never calling Dispose. Unless for some reason the parent library is doing it (probably not as in Editor we never close the root except when exiting).

Probably something that we can add to the base class internal class Tests. There are various other managers that get cleared/setup there.

This may move us closer to being able to open/new without having to restart app. One of the things I did to make life easier on state management was to have the program lifecycle involve only editing a single view (Open or New) and have no way to change to another view during execution (i.e. without restarting the process). It made things easy because nothing ever gets closed or disposed but in future it would be nice if that were possible.

Also, there was a spot in some code that replaces a menuitem that found the index of an existing item, converted the whole Children collection to a list, removed the old item at the found index, and then inserted a replacement at the same index.

Cool. As long as it still correctly handles removing the last element e.g. when it has to convert from a menu item with sub items to a regular one (they have different classes from what I remember MenuItem vs MenuBarItem

I have started looking at supporting Slider<T> (at least with value types).

[dodexahedron]

Yeah there's a test for that last case you already wrote. 👍

As for disposal, no we don't really Dispose anything. V1 didn't even really have that for most things (anything?), so that's to be expected anyway.

V2 is IDisposable starting from the very bottom, at that Responder class that View inherits from.

I've added calls to Dispose in some tests, occasionally, but haven't been paying that close attention yet.

In the application, yes, undo/redo are my main concerns with both event unsubscription and disposal, since we need to be sure to un/re-register at the appropriate times and only Dispose when things really are gone.

Although, if Redo really does just Do again, plus stack manipulation, then eager disposal may be doable without too much work. 🤔

Without a Disposing event on View, though, the fact that parents and children of some types have references to each other makes everything kinda dangerous because we have no way of knowing if something got disposed before we try to touch it (that's a TG problem and impacts more than just TGD). That I think really is a critical component before any real work is done on anything related to disposal here.

[dodexahedron]

Oh and a place where not having a Disposing event has already come up (and one of the things that prompted raising the issue in the first place) was when trying to Dispose something at the end of a test. Because there was no way for anything else to be aware of what happened, and because that object was referenced in multiple other places, Disposing one object in one test caused numerous other tests to fail with various exceptions - mostly NullReferenceExceptions. I don't remember if any were ObjectDisposedExceptions, but almost all of them should have been.

[dodexahedron]

@BDisp has already put in a PR to add this event. :)

I filed the Exception issue over there a few minutes ago: gui-cs/Terminal.Gui#3105

That's not a blocker for this work, of course.