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

Crash on exit #932

Closed
andy-yx-chen opened this issue May 22, 2019 · 6 comments
Closed

Crash on exit #932

andy-yx-chen opened this issue May 22, 2019 · 6 comments
Labels
Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing. Severity-Crash Crashes are real bad news.

Comments

@andy-yx-chen
Copy link

Environment

Windows build number: [run "ver" at a command prompt]
Microsoft Windows [Version 10.0.18362.53]   
Windows Terminal version (if applicable):
build from master branch
Any other software?
none

Steps to reproduce

close the terminal and you will get it, double Release call

Expected behavior

Actual behavior

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels May 22, 2019
@zadjii-msft zadjii-msft added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something Product-Terminal The new Windows Terminal. Severity-Crash Crashes are real bad news. labels May 22, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label May 22, 2019
@zadjii-msft
Copy link
Member

I would have thought that this was #627, which should have been fixed by #746. Can you post a stack of the double release you're seeing?

@andy-yx-chen
Copy link
Author

Microsoft.UI.Xaml.Markup.dll!winrt::impl::root_implementswinrt::Microsoft::UI::Xaml::Markup::implementation::XamlApplication,winrt::Microsoft::UI::Xaml::Markup::XamlApplication,winrt::Microsoft::UI::Xaml::Markup::IXamlMetadataProviderContainer,winrt::Windows::Foundation::IClosable,winrt::composable,winrt::composing,winrt::Windows::UI::Xaml::IApplicationOverrides,winrt::Windows::UI::Xaml::IApplicationOverrides2,winrt::Windows::UI::Xaml::Markup::IXamlMetadataProvider::Release() Line 6413 C++
Microsoft.UI.Xaml.Markup.dll!winrt::implementswinrt::Microsoft::UI::Xaml::Markup::implementation::XamlApplication,winrt::Microsoft::UI::Xaml::Markup::XamlApplication,winrt::Microsoft::UI::Xaml::Markup::IXamlMetadataProviderContainer,winrt::Windows::Foundation::IClosable,winrt::composable,winrt::composing,winrt::Windows::UI::Xaml::IApplicationOverrides,winrt::Windows::UI::Xaml::IApplicationOverrides2,winrt::Windows::UI::Xaml::Markup::IXamlMetadataProvider::Release() Line 6954 C++
Microsoft.UI.Xaml.Markup.dll!winrt::impl::produce_basewinrt::Microsoft::UI::Xaml::Markup::implementation::XamlApplication,winrt::Microsoft::UI::Xaml::Markup::IXamlApplication,void::Release() Line 6085 C++

The problem could be solved by Increase the reference for TerminalApp, but that does not make sense to me though

@andy-yx-chen
Copy link
Author

namespace winrt::TerminalApp::implementation
{

    App::App() :
        App(winrt::TerminalApp::XamlMetaDataProvider())
    {
    }

    App::App(Windows::UI::Xaml::Markup::IXamlMetadataProvider const& parentProvider) :
        base_type(parentProvider),
        _settings{  },
        _tabs{  },
        _loadedInitialSettings{ false }
    {
        // For your own sanity, it's better to do setup outside the ctor.
        // If you do any setup in the ctor that ends up throwing an exception,
        // then it might look like App just failed to activate, which will
        // cause you to chase down the rabbit hole of "why is App not
        // registered?" when it definitely is.
        **this->AddRef();**
    }

@zadjii-msft
Copy link
Member

That's almost certainly not the correct solution to this bug, even if it might work.

I am seeing some other problems on exit, so we can use this to track following up on this issue.

I got some RoOriginateError in App::_RemoveTabViewItem on this line:

_tabs.erase(_tabs.begin() + tabIndexFromControl);

@zadjii-msft zadjii-msft added Help Wanted We encourage anyone to jump in on these. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels May 22, 2019
@ghost ghost added the Needs-Attention The core contributors need to come back around and look at this ASAP. label May 22, 2019
@metathinker
Copy link
Contributor

Could this be the same issue as #457? @DHowett-MSFT said about that bug:

This is also the cause of a crash when closeOnExit is true and you exit the last shell.

And since PR #599 was merged, closeOnExit is indeed the default behavior.

@zadjii-msft
Copy link
Member

Oh yea, this probably is a dupe of that one. Good catch!

@zadjii-msft zadjii-msft added Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing. and removed Help Wanted We encourage anyone to jump in on these. Needs-Attention The core contributors need to come back around and look at this ASAP. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels May 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing. Severity-Crash Crashes are real bad news.
Projects
None yet
Development

No branches or pull requests

3 participants