Skip to content

Fix a crash when opening a new tab with args#3901

Merged
1 commit merged intomasterfrom
dev/migrie/b/3897-fix-logging-crash
Dec 11, 2019
Merged

Fix a crash when opening a new tab with args#3901
1 commit merged intomasterfrom
dev/migrie/b/3897-fix-logging-crash

Conversation

@zadjii-msft
Copy link
Member

Summary of the Pull Request

I accidentally did the wrong check here to see if the value exists. For an IReference, you need to do variable != nullptr. I did variable.Value().

References

Introduced in #3825

PR Checklist

Detailed Description of the Pull Request / Additional comments

This includes a maybe unrelated fix to make TerminalPage's ShortcutActionDispatch a com_ptr. While I was messing with the tests for this, I caught that we're not supposed to direct allocate winrt types like that. Ofc, the TerminalAppLib project doesn't catch this.

Validation Steps Performed

Ran the terminal manually, instead of just running the tests

(cherry picked from commit 2e522f4)
@zadjii-msft zadjii-msft added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. Severity-Crash Crashes are real bad news. labels Dec 10, 2019
std::optional<int> _rearrangeTo;

ShortcutActionDispatch _actionDispatch{};
winrt::com_ptr<ShortcutActionDispatch> _actionDispatch{ winrt::make_self<ShortcutActionDispatch>() };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this perturbs me a bit. something about the complex initializer statement in the header, idk

@DHowett-MSFT DHowett-MSFT added the Needs-Second It's a PR that needs another sign-off label Dec 10, 2019
@DHowett-MSFT DHowett-MSFT added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 11, 2019
@ghost
Copy link

ghost commented Dec 11, 2019

Hello @DHowett-MSFT!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 8 hours, a condition that will be fulfilled in about 1 hour 14 minutes. No worries though, I will be back when the time is right! 😉

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit a5c397c into master Dec 11, 2019
@ghost ghost deleted the dev/migrie/b/3897-fix-logging-crash branch December 11, 2019 02:04
This pull request was closed.
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 AutoMerge Marked for automatic merge by the bot when requirements are met Needs-Second It's a PR that needs another sign-off Product-Terminal The new Windows Terminal. Severity-Crash Crashes are real bad news.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A {"action": "newTab", "profile": "NameOfProfile"} binding causes a crash

3 participants