#3330 broke Dialog Dispose

[tig]

I broke something in

• Fixes #3313. Application.End leaves Top invalid #3330

Not sure how I missed this in my testing.... and clearly more/better unit tests are needed.

Repo:

• Run UI Catalog
• Hit Ctrl-A to show About box
• Close About box
• Exit UI Catalog

You'll get an assert that the Dialog for the about box was not disposed.

[BDisp]

Well, it seems that you need to dispose the toplevel on the End method.

        // Do NOT dispose .Toplevel here. It was not created by
        // Run, but Init or someone else.
        runState.Toplevel?.Dispose ();
        runState.Toplevel = null;
        runState.Dispose ();

[tig]

No. That's not correct.

The creator of the object needs to be responsible for its lifetime.

With my fix, anywhere someone creates a toplevel to pass to Run must dispose the object after Run returns.

I've got a pr in profess that will fix all instances in the solution.

[BDisp]

My thinking about this isn't the disposing on the End method that cause the issue, but the disposing that is done on the Begin method if the Top isn't the same passed by the parameter, which normally is the one created by the Init method and is this one that need to be disposed on the Shutdown method, because it may not been called by the Begin method. So, on the End method the runstate.Toplevel must be disposed because they was created by the Begin method.

[BDisp]

But I think that disposing the Top, the one created by the Init method and not added to the toplevel's stack, on the Begin method is the right procedure, because it will never be used and we to set the Top by the one passed on the parameter. What issue do you have before you done the #3313?

[tig]

What issue do you have before you done the #3313?

Window w = new ();
w.Ready += (s,e) { Application.RequestStop() }; // Causes `End` to be called
Application.Run(w);
if (w.SomeProperty) {} // Invalid - w has been disposed
Applicaton.Run(w); // Invalid - w has been disposed

See this new unit test:

[Fact]
    public void End_Should_Not_Dispose_ApplicationTop_Shutdown_Should ()
    {
        Init ();

        RunState rs = Application.Begin (Application.Top);
        Application.End (rs);

#if DEBUG_IDISPOSABLE
        Assert.True (rs.WasDisposed);
        Assert.False (Application.Top.WasDisposed);
#endif

        Assert.Null (rs.Toplevel);

        var top = Application.Top;

        Shutdown ();

#if DEBUG_IDISPOSABLE
        Assert.True (top.WasDisposed);
#endif

        Assert.Null (Application.Top);
    }

If the old End behavior is reverted this test fails.

Tons of unit tests re-use Toplevels passed to Run. It's only luck that they work. Adding [TestRespondersDisposed] to them causes them to fail.

If Init creates a toplevel, Shutdown should clean it up, not End. (Init does and Shutdown does, in ResetState).

If Begin creates a toplevel, Stop should clean it up, not End. (Begin doesn't).

If Run (Toplevel) creates a toplevel, End should clean it up (Run (toplevel) doesn't, End doesn't and doesn't need to).

If Run<T> creates a toplevel, End should clean it up (Run<T> does, End doesn't - this is technically incorrect right now).

If a Toplevel is created and passed to Run, the code that created it should clean it up.

[BDisp]

Window w = new ();
w.Ready += (s,e) { Application.RequestStop() }; // Causes `End` to be called
Application.Run(w);
if (w.SomeProperty) {} // Invalid - w has been disposed
Applicaton.Run(w); // Invalid - w has been disposed

But isn't normal that referencing a toplevel (w) after the call Application.Run(w) it already been disposed by the End method due the call to the Application.RequestStop() method?

We need to have sure what is the right behavior before continue.

[tig]

But isn't normal that referencing a toplevel (w) after the call Application.Run(w) it already been disposed by the End method due the call to the Application.RequestStop() method?

We need to have sure what is the right behavior before continue.

Before #3330 Application.End did this:

        runState.Toplevel?.Dispose ();
        runState.Toplevel = null;
        runState.Dispose ();

        // BUGBUG: Application.Top is now invalid?!?!

This is incorrect behavior:

a) Application.Top often points to the same object as runState.Top. Disposing that object is Bad.
b) Application.Current also oftent points to that same object.

An example of Scenario code that SHOULD fail (it would if we threw exceptions on property access for disposed objects) is Notepad:

        var open = new OpenDialog { Title = "Open", AllowsMultipleSelection = true };

        Application.Run (open);

        if (!open.Canceled)
        {
            foreach (string path in open.FilePaths)
            {
                if (string.IsNullOrEmpty (path) || !File.Exists (path))
                {
                    return;
                }

                // TODO should open in focused TabView
                Open (new FileInfo (path), Path.GetFileName (path));
            }
        }

If this code is changed to the below, the bug would be illustrated:

        var open = new OpenDialog { Title = "Open", AllowsMultipleSelection = true };

        Application.Run (open);

        Assert.True (open.WasDisposed);

It's only luck that the check for open.Cancelled works because open has been Disposed by End.

My fix does this:

        // Do NOT dispose .Toplevel here. It was not created by
        // Run, but Init or someone else.
        runState.Toplevel = null;
        runState.Dispose ();

(and changes RunState.Dispose to not thow an exception).

I believe the new code is correct. Run does not create runState.Toplevel, whoever called Run did. So End should not dispose it.

The downside of the "Fix" is that it means anyone who creates a Toplevel to pass to Run must be smart about managing the lifecycle of that instance, calling Dispose as needed. So, for example, the Notepad scenario needs to look like this:

        var open = new OpenDialog { Title = "Open", AllowsMultipleSelection = true };

        Application.Run (open);

        bool canceled = open.Canceled;

        if (!canceled)
        {
            foreach (string path in open.FilePaths)
            {
                if (string.IsNullOrEmpty (path) || !File.Exists (path))
                {
                    break;
                }

                // TODO should open in focused TabView
                Open (new FileInfo (path), Path.GetFileName (path));
            }
        }

        open.Dispose ();

[BDisp]

This is incorrect behavior:

a) Application.Top often points to the same object as runState.Top. Disposing that object is Bad. b) Application.Current also oftent points to that same object.

It's true that Application.Top often points to the same object as runState.Top when it's also the same object as the Application.Current.
The Application.Current always point to the same as runState.Top if it's still running. On overlapped toplevel's if the stack is changed during a RunLoop, the runState.Top is checked against the Current and if it isn't the same object then the runState.Top is set to the Current.

[tig]

Application.Currentalways point to the same asrunState.Topif it's still running. On overlapped toplevel's if the stack is changed during a RunLoop, therunState.Topis checked against theCurrentand if it isn't the same object then therunState.Topis set to theCurrent`.

We really need some tight-focused unit tests for all this. It's crazy complicated and the only code that really exercises it are the "BackgroundWorker" Scenarios which are NOT good test cases as they are convoluted and do many things.

Since you have a good idea in your head of how this is supposed to work, could you please engineer some unit tests that prove #3399 does/does not break the logic?

[BDisp]

A possible fix on this PR tig#24.

[tig]

Well THAT was fun. ;-)