Skip to content

Implement Default Terminal#7489

Merged
DHowett merged 89 commits intomainfrom
dev/miniksa/default
Mar 26, 2021
Merged

Implement Default Terminal#7489
DHowett merged 89 commits intomainfrom
dev/miniksa/default

Conversation

@miniksa
Copy link
Member

@miniksa miniksa commented Aug 31, 2020

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

  • Implements the default application behavior and handoff mechanisms between console and terminal. The inbox portion is done already. This adds the ability for our OpenConsole.exe to accept the incoming server connection from the Windows OS, stand up a PTY session, start the Windows Terminal as a listener for an incoming connection, and then send it the incoming PTY connection for it to launch a tab.
  • The tab is launched with default settings at the moment.
  • You must configure the default application using the conhost.exe propsheet or with the registry keys. Finishing the setting inside Windows Terminal will be a todo after this is complete. The OS Settings panel work to surface this setting is a dependency delivered by another team and you will not see it here.

Validation Steps Performed

  • - Manual adjust of registry keys to the delegation conhost/terminal behavior
  • - Adjustment of the delegation options with the propsheet
  • - Launching things from the run box manually and watching them show in Terminal
  • - Launching things from shortcuts and watching them show in the Terminal

miniksa and others added 24 commits August 4, 2020 14:56
Includes cherry-pick of Mike's changes to let intellisense work
Also do a whole bunch of stuff to COMify the console handoff.
…. it finally works with the correct ndr version.... still need to manually patch that. also need to dump dummy call to interface. also fixed arg parser sort of.
…tabs are made for embedding... and it totally works. Except Terminal can't read from the pipes it was given. So gotta figure that one out.
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Don't forget to put the copyright headers on some of these (figured this would be easier than putting the same comment over and over again).

Overall, looks good though.

}

[[nodiscard]] HRESULT ConsoleCreateIoThreadLegacy(_In_ HANDLE Server, const ConsoleArguments* const args)
HRESULT ConsoleCreateIoThread(_In_ HANDLE Server,
Copy link
Member

Choose a reason for hiding this comment

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

Adding method comments would be nice for this file.

Copy link
Member

Choose a reason for hiding this comment

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

out of scope for this PR.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, I know the discussion is like 6 months gone by at this point, but doc comments really would help now that we're gonna be checking this in for real

… out of proc com server handling a bit for the exe server delegation conhost.
@microsoft-github-updates microsoft-github-updates bot changed the base branch from master to main October 22, 2020 00:27
@github-actions

This comment has been minimized.

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

42/50, i've reviewed the terminal side. Blocking over the discussion about HANDLE on the internal interface w/ or w/out the casting

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 25, 2021
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

50/50. I'll sign off after the earlier comments!

@@ -0,0 +1,334 @@
// Copyright (c) Microsoft Corporation.
Copy link
Member

Choose a reason for hiding this comment

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

ugh, nobody can tell that this file was renamed so it's going to become a real pain to un-merge-conflict when Dragos' changes come back out. It's also hard to review.

This is not a complaint about your work. This is the mechanics of git!

Copy link
Member

Choose a reason for hiding this comment

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

for easier reviewing, check out this branch and...

git diff origin/main:src/host/exemain.cpp HEAD:src/host/exe/exemain.cpp

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 26, 2021
@miniksa miniksa closed this Mar 26, 2021
@miniksa miniksa reopened this Mar 26, 2021
@DHowett DHowett merged commit 906edf7 into main Mar 26, 2021
@DHowett DHowett deleted the dev/miniksa/default branch March 26, 2021 22:09
@zadjii-msft
Copy link
Member

🎉

@dddominikk
Copy link

For my records here...

The weird crash I was experiencing in the middle OpenConsole.exe while testing off the handoffs from the operating system, especially from background tasks running out of things like Visual Studio, VS Code, and on login, were because there was TOO MUCH handoff occuring.

I had a torn state. My inbox console was doing the very aggressive handoff strategy, including in the case of connect of a second client. That's no bueno. The handoff happens just fine, but the OpenConsole.exe accepting the handoff doesn't have the process list table or other records of the already connected clients. Therefore when it eventually receives a disconnect message for them... it tries to look them up in its table and horks because it was the conhost.exe that accepted the initial connection, not the OpenConsole.exe.

I already solved this problem in 1cf4fb1 by blocking handoff for attaches and for consoles that are already initialized the first time.

I'm putting my foot down that there's no half way state here right now. Either the inbox one, when it initially starts up, hands everything off and is done with the matter OR something goes wrong and the inbox one remains the console server for the remainder of that session. Otherwise there's this state and countless other state that would have to pass, and that's beyond the scope of what I'm trying to accomplish right now.

Also, technically, we probably should be having some opaque value stored in CONSOLE_API_MSG.Descriptor.Process instead of a (ConsoleProcessHandle*) so this would fail in a table-lookup sort of way not an access violation from a weird pointer dereference off of a foreign heap value. But I'm just going to put this over here...
image

Stories like these are why I browse open source, good luck with the rest of the fire.😅

@miniksa miniksa mentioned this pull request Mar 29, 2021
3 tasks
DHowett pushed a commit that referenced this pull request Mar 29, 2021
By default from ARM64 architecture projects, `WIN32` is not defined. It
is supposed to be for this proxy stub to work. So I've set it with the
preprocessor for this project.

## PR Checklist
* [x] Closes release build failure after #7489 
* [x] I work here.
* [x] Built on my machine.

## Detailed Description of the Pull Request / Additional comments

`WIN32` appears to convey two meanings depending on who you are:
- To most of Windows, `WIN32` appears to mean the Win32 API surface and
  sometimes the major OS version that goes with it. (Specifically in
  contrast to 16-bit Windows.)
- To others, `WIN32` appears to mean a 32-bit processor or a synonym of
  `x86`.

This is generally not a problem for a few reasons:
- VS defines `WIN32` in the default targets/props only for the `x86`
  processor type. **BUT**
- Windows defines `WIN32` if it's not already defined in both
  `minwinbase.h` and `ole2.h` which generally speaking manage to get
  compiled into practically everything especially since `minwinbase.h`
  tends to sneak itself in somehow through `windows.h` and that's
  **THE** include to use the Windows API surface.
- Windows also defines `WIN32` for itself unconditionally and relatively
  globally when building itself.

However, it's a problem here because: 
- `rpcproxy.h` is the only header included in `dlldata.c`, a file
  generated automatically by `midl.exe` in the SDK when making a proxy
  stub.
- `rpcproxy.h` only defines its contents for a proxy when `WIN32` or
  `_M_AMD64` are found.
- Therefore, it's defined pretty naturally for x86 and AMD64 targets
  from VS, but not for ARM64.
- ARM64 support is pretty new and those who are attempting to build for
  ARM64 and against the public SDK with Visual Studio for a classic COM
  proxy... seems like a relatively unlikely combination.

I will follow up with the Visual Studio, Windows SDK, and MIDL/COM teams
to try to remove this pitfall from the public tooling. But for now, this
is the fix.
zadjii-msft added a commit that referenced this pull request Mar 30, 2021
  Broadly, the tests were broken by #7489 because there were no _startupActions. They relied on the removed codepath that assumed wt.exe always set actions, or `AppCommandlineArgs::ValidateStartupCommands` created one by default.

  These tests are still broken:
  * `TerminalAppLocalTests::TabTests::TryZoomPane [Failed]`
  * `TerminalAppLocalTests::TabTests::MoveFocusFromZoomedPane [Failed]`
  * `TerminalAppLocalTests::TabTests::CloseZoomedPane [Failed]`

  Re:#9659
ghost pushed a commit that referenced this pull request Mar 30, 2021
Broadly, the tests were broken by #7489 because there were no `_startupActions`. They relied on the removed codepath that assumed `wt.exe` always set actions, or `AppCommandlineArgs::ValidateStartupCommands` created one by default.

* [x] fixes #9659
* [x] I work here
* [x] the tests pass again
ghost pushed a commit that referenced this pull request Jan 27, 2022
There are a couple places where we now bail immediately on startup, if we think the window is going to get created without any tabs. We do that to prevent a blank window from flashing on the screen when launching auto-elevate profiles. Unfortunately, those broke defterm in a particularly hard to debug way. In the defterm invocation, there actually aren't any tabs when the app completes initialization. We use the initialization to actually accept the defterm handoff. So what would happen is that the window would immediately close itself gracefully, never accepting the handoff.

In my defense, #8514, the original auto-elevated PR, predates defterm merging (906edf7) by a few months, so I totally forgot to test this when rolling it into the subsequent iterations of that PR.

* Related to: 
  * #7489
  * #12137 
  * #12205 
* [x] Closes #12267 
* [x] I work here
* [ ] No tests on this code unfortunately
* [x] Tested manually

Includes a semi-related code fix to #10922 to make that quieter. That is perpetually noisy, and when trying to debug defterm, you've only got about 30s to do that before it bails, so the `sxe eh` breaks in there are quite annoying.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Change Windows OS to support default terminal [defterm]

6 participants