Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error: Cannot set Visibility to Visible or call Show, ShowDialog, Close, or WindowInteropHelper.EnsureHandle while a Window is closing. #3535

Closed
RickStrahl opened this issue Jun 18, 2019 · 11 comments · Fixed by #3536

Comments

@RickStrahl
Copy link

commented Jun 18, 2019

Describe the bug

Disclaimer: this is a bit vague but thought I'd bring this up as I see this error frequently in my logs initiated off a Window button close.

I haven't been able to track this one down myself for debugging in Markdown Monster, but I see this error quite frequently in my logs. I believe this is caused when the close box is clicked on the window perhaps when another shut down operation is already in place, or when a forced shutdown occurs due to an unhandled exception and app shutdown dialog.

Here's the stack trace:

image

To Reproduce
Can't reproduce reliably - that's where the vagueness comes in.

Environment(please complete the following information):

  • MahApps.Metro 1.6.5
  • OS: Win10 1903
  • Visual Studio 2019
  • .NET Framework 4.8

other:
I'm guessing this is due to calling Close() when the window is already closing, so perhaps an additional check can be performed before calling Window.Close() closing the window.

@punker76 punker76 added the Bug label Jun 18, 2019

@timunie

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

@RickStrahl Do you have any code in the closing event of the window?

I have the same problem from time to time, but i found a way to bypass it, so i didn't make a bug report for this.

image

@punker76 @batzen : If you want to reproduce this you could change the demo app like this:
MainWindow.xaml.cs Line 368

        private async void MetroWindow_Closing(object sender, System.ComponentModel.CancelEventArgs e)
        {
            if (e.Cancel)
            {
                return;
            }

            e.Cancel = !_shutdown;
            if (!e.Cancel)
            {
                return;
            }

            if (_viewModel.QuitConfirmationEnabled)
            {
                var mySettings = new MetroDialogSettings()
                {
                    AffirmativeButtonText = "Quit",
                    NegativeButtonText = "Cancel",
                    AnimateShow = true,
                    AnimateHide = false
                };

                var result = await this.ShowMessageAsync("Quit application?",
                                                         "Sure you want to quit application?",
                                                         MessageDialogStyle.AffirmativeAndNegative, mySettings);

                _shutdown = result == MessageDialogResult.Affirmative;
            }
            else
            {
                _shutdown = true;
            }
            if (_shutdown)
            {
                Close();
            }
        }

@batzen : May this error come from ControlzEx?

@RickStrahl

This comment has been minimized.

Copy link
Author

commented Jun 20, 2019

@timunie Yes - quite a bit and a few different code paths. I think the problem is aggravated by the fact that there are closing abort scenarios. But I think those all work as expected. You can see in the stack trace, no user code is actually firing from my app, so I suspect this is triggered from an exception that's effectively killing the app. Not sure how the CloseButtonClick fits into that scenario, but I can't see how else this would get fired out of band like this.

I've seen the errors in my personal logs as well, but I can't duplicate - even with the exception scenario. I see about 5 of these errors a day in Markdown Monster's log (out of ~1000 users a day).

@timunie

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

@RickStrahl I noticed that this error is thrown when you have a close of a window with any async code running (ShowMessageAsync for example). But I don't know the root cause.

@batzen

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

I don't think this is caused by ControlzEx but by async instead.
Calling Close in a Closing handler does cause this issue in cases where the code does not run async, which might happen and explains why this only happens rarely.

@punker76 Should i fix this issue in the showcase application and create a PR?

@punker76

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

@batzen 👍

@timunie

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

@batzen I manipulated the ShowCaseApp like the code I posted to produce this error. So I think there is no need to edit anything there. But maybe one should mention this in the documentation.

@batzen

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

@timunie Ah, thought the code posted here was the code contained in the showcase application.

@RickStrahl I can fully reproduce your issue and you can too. Just double click on the close window button in MarkdownMonster.
It's possible to cause this crash in MarkdownMonster because you are doing way too much in your closing handler and even issue DoEvents in there. DoEvents breaks the control flow. Even showing dialogs might cause these kinds of issues.
The way i solved this was to check for a flag in Closing and always set e.Cancel if that flag isn't set. To set the flag i call the code for setting the flag via Dispatcher.BeginInvoke and also check for re-entrancy to prevent the code to set the flag from running multiple times before one call completely finished.
As you are doing quite a lot in closing i'd also add some kind of busy adorner/marker.

@RickStrahl

This comment has been minimized.

Copy link
Author

commented Jun 22, 2019

@batzen Yeah no doubt there's a lot happening in Closing in MM. I've looked at that a long time ago, but couldn't find a different way to handle that as Closing is the only way to detect the app shutdown. This is further complicated by the fact that there are a couple of things that can stop the close operation (version check, changed files and cancelling out or the registration reminder) so there are definite challenges to the normal control flow.

I've removed the DoEvents and replaced with a Dispatcher call which should make the flow more WPF friendly. I suspect the real issue is that there might be another hard exit that forces the app to close. So per your suggestion also added IsClosing to prevent double execution (although I couldn't duplicate that with a double click as you suggested - I get no failure with that, but that could be because of the changes :-))

I'll start with this and see what effect that has in the logs that show up before pulling the entire closing operation out and just always cancel until explicitly telling it to exit (good idea BTW!) which would require a bit more work (and some code-reducing refactoring). Unfortunately I still can't reproduce this reliably on my end - in fact looking at my logs I haven't seen this error in many months in my local logs so it's very likely a very specific set of steps that leads to this exception.

Thanks for your help!

@batzen

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2019

@RickStrahl Essentially you'd just have to move most of the code in closing to a separate method and call that with dispatcher.begininvoke. I updated the showcase of MahApps.Metro to just do that.

@RickStrahl

This comment has been minimized.

Copy link
Author

commented Jun 28, 2019

Looks like with the simple changes preventing re entrancy the errors in the log have stopped... I haven’t seen any repeats in recent version logs which is great.

@RickStrahl RickStrahl closed this Jun 28, 2019

@RickStrahl RickStrahl reopened this Aug 30, 2019

@RickStrahl

This comment has been minimized.

Copy link
Author

commented Aug 30, 2019

So looks like this issue has come back to bite me and I'm looking at this again as I'm now seeing large numbers of errors in regards to this.

@batzen your suggestion to move off into a separate method via Dispatcher sounds good, but I do have cancel scenarios - it took a bit of experimentation but I now ended up with a forced closing scenario that looks like this:

bool ForceClose = false;

protected override void OnClosing(CancelEventArgs e)
{
    // force method to abort - we'll force a close explicitly
    e.Cancel = true;

    if (ForceClose)
    { 
        // cleanup code already ran
        e.Cancel = false;
        return;
    }

    // execute shutdown logic - set ForceClose and call Close() to
    // actually close the window
    Dispatcher.InvokeAsync(ShutdownApplication, DispatcherPriority.Normal);
}

private void ShutdownApplication()
{
    try
    {
        // have to do this here to capture open windows etc. in SaveSettings()
        mmApp.Configuration.ApplicationUpdates.AccessCount++;      
        SaveSettings();

        if (!CloseAllTabs())
        {
            // tab closing was cancelled - no forced close
            mmApp.Configuration.ApplicationUpdates.AccessCount--;
            return;
        }

        // hide the window quickly
        Top -= 10000;

        FolderBrowser?.ReleaseFileWatcher();
        bool isNewVersion = CheckForNewVersion(false, false);

        var displayCount = 6;
        if (mmApp.Configuration.ApplicationUpdates.AccessCount > 250)
            displayCount = 1;
        else if (mmApp.Configuration.ApplicationUpdates.AccessCount > 100)
            displayCount = 2;
        else if (mmApp.Configuration.ApplicationUpdates.AccessCount > 50)
            displayCount = 5;

        if (!isNewVersion && mmApp.Configuration.ApplicationUpdates.AccessCount % displayCount == 0 && !UnlockKey.IsRegistered())
        {
            var rd = new RegisterDialog(true);
            rd.Owner = this;
            rd.ShowDialog();
        }

        PipeManager?.StopServer();

        AddinManager.Current.RaiseOnApplicationShutdown();
        AddinManager.Current.UnloadAddins();
        
        App.Mutex?.Dispose();

        PipeManager?.WaitForThreadShutDown(5000);
        mmApp.Shutdown();

        ForceClose = true;
        Close();
    }
    catch(Exception ex)
    {
        mmApp.Log("Shutdown: " + ex.Message, ex, logLevel: LogLevels.Error);
        ForceClose = true;
        Close();
    }
}

It's pretty ugly but it works. Whether this addresses the bug scenario I'm not sure until I see the logs, but it seems this should handle both duplicate closing scnearios as well as closing only when I know explicitly that the form should be closed 'quickly'.

We'll see how this goes, but I hope it addresses the number of issues showing up in my logs which are 90% of all critical errors that occur :-)

image

@RickStrahl RickStrahl closed this Aug 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.