ContextMenu accesses disposed MenuBar

[tig]

To repo, change Application.UngrabMouse():

    /// <summary>Releases the mouse grab, so mouse events will be routed to the view on which the mouse is.</summary>
    public static void UngrabMouse ()
    {
        if (MouseGrabView is null)
        {
            return;
        }

#if DEBUG_IDISPOSABLE
        ObjectDisposedException.ThrowIf (MouseGrabView.WasDisposed, typeof (View));
#endif

        if (!OnUnGrabbingMouse (MouseGrabView))
        {
            View view = MouseGrabView;
            MouseGrabView = null;
            OnUnGrabbedMouse (view);
        }
    }

Run unit tests.

My suggestion is to refactor it to create _menuBar as a static and use _menuBar.Visible and/or _menuBar.Enabled to manage.

[BDisp]

My suggestion is to refactor it to create _menuBar as a static and use _menuBar.Visible and/or _menuBar.Enabled to manage.

It's already static:

Terminal.Gui/Terminal.Gui/Views/Menu/ContextMenu.cs

Line 25 in e95f821

private static MenuBar _menuBar;

and it's disposing here:

Terminal.Gui/Terminal.Gui/Views/Menu/ContextMenu.cs

Lines 106 to 121 in e95f821

	public void Dispose ()
	{
	if (IsShow)
	{
	_menuBar.MenuAllClosed -= MenuBar_MenuAllClosed;
	_menuBar.Dispose ();
	_menuBar = null;
	IsShow = false;
	}
	if (_container is { })
	{
	_container.Closing -= Container_Closing;
	_container.Deactivate -= Container_Deactivate;
	}
	}

My suggestion is calling UngrabMouse method before the _menuBar.Dispose and thus the assertion won't fail. The _menuBar will always be recreated on a new popup.

[dodexahedron]

I have a lot of questions and concerns regarding that static menubar reference. 🫤

I'll need to pull down the branch in its latest state to get a good look at it all, though, in case I'm missing key context.

[dodexahedron]

At a high level, the concern is based on the following rules of thumb regarding holding a reference to another object.

Unless an object holding a reference to another object in any member field or property:

1. Is THE owner of the other object it has a reference to, or

2. The object does not modify the other object AND guards itself against any potential changes to that other object which could affect its read-only use of it, or

3. The object may write to the other object AND there is a way for that object holding that reference to notify the actual owner of things it does to that other object

4. And, in addition to at least one of the above, if the object holding the reference to the other object cannot guarantee thread-safety on read and write accesses to it (which it can't, without cooperation from that object or using an intermediary without possibility of other access paths to synchronize access)...

That is, unless (1 || 2 || 3) && 4 is true, then that reference should almost definitely not be static, especially in a library where that object and related types are in the visible API surface, via one or more code paths.

[BDisp]

The reason for the _menuBar be static is because it's possible to have only one ContextMenu at a time. The reason for the his disposing it's because even not added to a superview, any new instance that aren't disposed will throw an assertion failure. The only doubt is if TGD really need this object. ContextMenu has a read-only MenuBar property that return the static _menuBar. But when a ContextMenu is closed _menuBar is disposed and set to null. This is a way for the ContextMenu control any instance in use. It's much more easy to consult the ContextMenu.cs itself and see how it iterate with the MenuBar class.

[dodexahedron]

Hm. Thanks for the added context.

That sounds like there's a more fundamental problem that we might not be managing the lifetime of the object(s?) properly.

Making it static just addresses a symptom without fixing the underlying issue, though, while bringing in caveats that can be more difficult to address than tracking down and addressing the root problem, anyway.

But again, I haven't pulled it yet since I'm in the middle of a few other things, at the moment, but I'll take a peek at whatever you have later, when I get to it. Turning a static member back into an instance member isn't usually that hard, so the risk of wasted effort is pretty low even if tweaks are needed. It may even end up not warranting a change at all, so certainly not worth waiting up for me if you're working on it right now.

[dodexahedron]

Some follow-up thoughts (if these have already been addressed, don't mind me):

Do we really need to create and dispose the thing all the time, then? Can't we create one instance and just show it and hide it, instead, or similar.

Seems perfectly reasonable to me that the context menu could be added to the view the application was started with, so that it fits into the current expected flow for views, no? Or it could be added to the view that had focus at the time. Something.

And there's also the option key on most keyboards, or other programmatic methods by which someone may want to be able to open a context menu - not just the mouse. Those really should all share the same root code path, which would suggest to me that unconditionally operating on the state of the mouse is at least not ideal.

[tig]

ContextMenu is a hack. It works but it's a hack.

In order to not have to have a hack like this, we need to get rid of Toplevel and make it so any View can host an overlapped/modal.

So lets not worry about ContextMenu for now.

Plus, the whole reason I invested in creating Shortcut and Bar was to replace StatusBar (done), Menu, MenuBar, and ContextMenu with something that had a clean design and was composible.

See

• Modernize Menu - Base on Bar and address litany of issues #2975

I think the first step here is to actually do ContextMenu based on Bar first.

But it won't be possible until this is complete:

• Get rid of Toplevel - Introduce Runnable and Overlapped instead #2491

[dodexahedron]

Ahhh ok that makes things fall into place much better. Thank you.

I was just about to get busy with tracing through it line by line, after shooting over a quick initial cleanup first (@BDisp - I sent you this PR for your convenience).

I'll just mainly focus on the disposal, then, which should end up being a pretty small change overall, and of course any other of the usual little opportunistic cleanup bits that don't dirty up the PR too much, if the urge strikes in the process. 😅

[dodexahedron]

And, side note, looks like y'all have fixed whatever had broken the tests from this, by the point I branched from, so 🥳

[dodexahedron]

Actually... Know what?

I'm getting tired and less effective, so should probably cease with code work for the night.

Besides, the change to the disposal pattern, while technically "breaking" because it changes the type of one or more thrown exceptions, isn't a blocker that needs to hold up this PR being finished for merge. It can be dealt with in later cleanup and formalizing passes, as far as I'm concerned, now that my main worry about the way the rest of it operates is largely allayed.

@BDisp, what I was trying to get across is that the application isn't the one who should be throwing that ODE. The view that has been disposed is the thing that is supposed to throw the exception, upon an attempt to call a method on it that won't have the expected effect due to the object being disposed.

In the application class code, you would then follow the numbered steps in my earlier comment describing the ODE handling procedure.

Ideally, that means hooking the application up to receive the Disposed event so it can just null the view out and then avoid the exception altogether with a simple null check. But you'd also want to make access to it thread-safe (need to anyway, but that's an additional reason why).

[dodexahedron]

Short version:

IDisposables throw ODE about themselves.
Owners of IDisposables don't, and try to avoid it ever happening.

[BDisp]

@BDisp, what I was trying to get across is that the application isn't the one who should be throwing that ODE. The view that has been disposed is the thing that is supposed to throw the exception, upon an attempt to call a method on it that won't have the expected effect due to the object being disposed.

I know that not the only place that should be throwing the ODE. That why @tig is checking in some crucial places (properties and methods) if an object that was already disposed is being accessed. In ContextMenu was the one where @tig catch the failure and opened a separate issue for that. In my PR #3681 I only fix this particular issue and not the whole thing that @tig is doing on the PR #3676.