Add support for running the terminal core as a content process #12938

zadjii-msft · 2022-04-19T21:00:19Z

Summary of the Pull Request

This is 1000% WIP work. This PR, however, can go in now before the rest of tear out is ready. It's gated behind a new velocity flag that only enables it for Dev builds. Terminal the app also provides no way to leverage this quite yet, but it is hooked up for the sample app.

Adds an undocumented --content parameter to wt which allows you to pass a content GUID on the commandline. When you do that, wt will start up as a content process, instead of as a window process. WT will serve a singular terminal control instance, which can be accessed with

winrt::create_instance<winrt::Microsoft::Terminal::Control::ContentProcess>(contentGuid, CLSCTX_LOCAL_SERVER);

This content process will hang around until the last outstanding reference to it is closed.

PR Checklist

See Scenario: Windows Terminal 2.0 Process Model Improvements #5000
I work here
Tests - probably should, yea.
[n/a] Requires documentation to be updated

Detailed Description of the Pull Request / Additional comments

Multiple different clients can access the same content process by simply communicating the GUID. That's obviously not recommended to do simultaneously, cause you'd have to keep the controls exactly the same size, but it's something that's possible.

Theoretically, we need to gate access to _interactivity and to _core behind some sort of safer accessor that won't just throw when the content process has died unexpectedly. A prototype was started in dev/migrie/oop/2/cptn-marvel, but that can definitely wait. Best to get this section of the merge conflicts done with now.

We've added a Control.ContentProcess class, which is responsible for owning a single ControlInteractivity bound to a GUID. You can use ContentProcess.RequestSwapChainHandle to get the swapchain of that content process.
- this includes the TermControlContentManagement.cpp, which handles the TermControl side of connecting to one of these.
We've had to re-wire some InteractivityAutomationPeer methods, because we couldn't have a XAML AutomationPeer in the content process without XAML. I've checked that this still works in narrator and accessibility insights.
There's another file in WindowsTerminal for ContentProcessMain, for handling all the weird lifetime semantics of the content process.

Validation Steps Performed

It works pretty cool in the scratch sln.

…ntrol doesn't render right, but I think that's because the actual HEAD of all this work is in `connection-factory`

…ere are many issues.

…but it _works_

… just going to disable this for now

This reverts commit 64533c8.

…sn't going to work

… thing isn't going to work" This reverts commit fd364db.

…ng for the actual text ranges. That's not what you want.

…the control

…roject

…he core. I think every x-proc call in TermControl logs that message, so _crap_

…nity-war

DHowett · 2022-05-18T18:46:26Z

src/cascadia/TerminalConnection/ConnectionInformation.cpp

-        // When we get to extensions (GH#4000), we may want to revisit.
-        if (LOG_IF_FAILED(RoActivateInstance(name, raw)))
+        // So to avoid those, we'll manually instantiate these
+        if (info.ClassName() == winrt::name_of<TerminalConnection::ConptyConnection>())


notes from discussion: it is pretty cheap to try to activate a class from our own DLL, so we may just want to do that

barring that, can we use the same reverse compare optimization that cppwinrt does?

DHowett · 2022-05-18T18:50:26Z

src/cascadia/TerminalControl/ContentProcess.cpp

+        // get destructed. This eliminates the need to do any sort of
+        // ref-counting weirdness. This entire process exists to host one
+        // singular ContentProcess instance. When we're destructed, it's because
+        // every other window process was done with us. We can die now, knowing


You might want to move this into a final_release block. Otherwise, COM might consider it a reportable RPC failure when the final ->Release() call results in the RPC server "crashing"

Note to self: I did this in e9375d6...089c015. Probably explodes when merged with #13882. Right now those commits are on wandavision, which is definitely after endgame (and maybe an additional black-widow PR for preemptive cleanup)

Probably want to bring fadd028 with it

DHowett · 2022-05-18T18:51:47Z

src/cascadia/TerminalControl/ContentProcess.cpp

+    }
+
+    // Method Description:
+    // - Duplicate the swap chain handle to the provided process.


Danger, Will Robinson!

Michael got system_handle published for consumption on the promise that it would stop app developers from having to roll their own handle security and ownership management!

There must be some way for us to leverage that here.

so I see you mentioned it below. I'd still like to see us dig further into making it a COM interface and doing it "right" if possible...

It does seem like this counts as system_handle(sh_composition). Trick is plumbing it all through.

We've got a WinRT object, the ContentProcess.

The window needs to ask ^ that thing ^ for a COM type, a ISwapChainProvider (whatever).

The Control dll needs to also expose COM type information?

But this sample on MSDN makes it look... really easy: A simple example of a COM component

Based on my prior experience with COM, I'm certain that it's not gonna be that easy. It doesn't seem like it's something trivial to add a COM interfact to ContentProcess, via just a c++ virtual class like in that example. Plus, that doesn't invoke the system_handle magic that's important here.

How much do we hate filing this as a code-health follow up? I don't want to burn a week investigating how to re-jigger ContentProcessMain to serve as a factory for two interfaces, the second being the ISwapChainProvider one.

bad first attempt in dev/migrie/oop/2/COM-ISwapChainProvider-attempt-1

I'd ask BriAl for help early -- I don't want to break into jail handling our own handle security unless we absolutely need to.

src/cascadia/TerminalControl/InteractivityAutomationPeer.cpp

DHowett · 2022-05-18T18:54:22Z

src/cascadia/TerminalControl/InteractivityAutomationPeer.cpp

+        // If we were being used in a process that had a CoreWindow, we could just call
+        //  DisplayInformation::GetForCurrentView().RawPixelsPerViewPixel();
+        //
+        // We won't always be though, so instead, ask our model what the DPI is.


torn state opportunity?

Yea, that's definitely a torn state opportunity. Like, especially those few frames where the Terminal doesn't seem like it reacted to the DPI change. Lemme see if we can wrap that up in a try/catch - at least the default behavior is "definitely the right thing", and it falls back to "hey what did you think it was"

src/cascadia/WindowsTerminal/ContentProcessMain.cpp

DHowett · 2022-05-18T19:08:39Z

src/cascadia/WindowsTerminal/ContentProcessMain.cpp

+
+            // Stash away that weak ref for future callers.
+            g_weak = weak;
+            return strong.as(iid, result);


wait, as returns an hresult we can use? it doesn't throw? what if it throws...

DHowett · 2022-05-18T19:09:49Z

src/cascadia/WindowsTerminal/ContentProcessMain.cpp

+        wil::unique_any<LPWSTR*, decltype(&::LocalFree), ::LocalFree> argv{ CommandLineToArgvW(commandline, &argc) };
+        if (argv)
+        {
+            for (auto& elem : wil::make_range(argv.get(), argc))
+            {
+                args.emplace_back(elem);
+            }
+        }


This is expensive to do just to throw away and then do again later, isn't it? Can we just strictly check for --content in an expected location and not do a full deep string split?

note: I might just leave this, cause I couldn't find a good way to handle wt --content "foo --signal" this-is-dumb-but-you-get-it without CommandLineToArgvW, and splitting the commandline args won't be that expensive, in the big picture.

src/cascadia/WindowsTerminal/main.cpp

…nity-war

scratch/ScratchIslandApp/SampleApp/MyPage.cpp

lhecker · 2022-05-25T15:53:05Z

src/cascadia/TerminalConnection/ConnectionInformation.cpp

+        // Sometimes, RoActivateInstance is weird and fails with errors like the
+        // following


That sounds weird? It shouldn't be doing that. 😰
Do we have to do that?

src/cascadia/TerminalControl/ContentProcess.cpp

lhecker · 2022-05-25T16:03:08Z

src/cascadia/TerminalControl/TermControlContentManagement.cpp

+        try
+        {


I don't quite see what could throw here? 🤔

lhecker · 2022-05-25T16:28:21Z

src/cascadia/TerminalControl/TermControlContentManagement.cpp

+
+            waits[0] = hContent.get();
+
+            switch (WaitForMultipleObjects(2, waits, FALSE, INFINITE))


Technically we can use RegisterWaitForSingleObject here.

This differs from WaitForSingleObject because the wait is performed by a different thread that combines several such calls for efficiency.

Something like that:

class TermControl { // Can't use wil::unique_event, because RegisterWaitForSingleObject // requires UnregisterWaitEx and not CloseHandle. HANDLE contentWaitHandle = nullptr; }; void TermControl::~TermControl() { if (const auto handle = std::exchange(self->contentWaitHandle, nullptr)) { // INVALID_HANDLE_VALUE = block until currently running callbacks are finished. UnregisterWaitEx(handle, INVALID_HANDLE_VALUE); } } void TermControl::waitCallback(_In_ PVOID lpParameter, _In_ BOOLEAN TimerOrWaitFired) try { const auto self = static_cast<TermControl*>(lpParameter); if (const auto handle = std::exchange(self->contentWaitHandle, nullptr)) { UnregisterWaitEx(handle, nullptr); self->_raiseContentDied(); } } CATCH_LOG() void TermControl::s_waitOnContentProcess() { RegisterWaitForSingleObject(&contentWaitHandle, _contentWaitInterrupt, &TermControl::waitCallback, static_cast<void*>(this), INFINITE, WT_EXECUTEONLYONCE); }

src/cascadia/WindowsTerminal/ContentProcessMain.cpp

src/cascadia/TerminalControl/TermControlContentManagement.cpp

src/cascadia/TerminalControl/TermControl.cpp

…nity-war

src/cascadia/TerminalControl/TermControl.cpp

…nity-war

zadjii-msft · 2022-08-01T21:50:07Z

I'm putting this in 1.17 but let's be real - I won't be around for most of 1.17 and 1.18. So maybe we merge this at the start of 1.17 just to clean it off the PR backlog, but it won't be used by the real Terminal so maybe it's okay?

zadjii-msft · 2022-12-12T22:31:15Z

We're gonna pursue another solution going into 2023. Some recent research showed that we might be able to do this all much, much easier.

zadjii-msft added 30 commits April 11, 2022 13:17

I think this merges the-whole-thing into this branch. The remote co…

e168413

…ntrol doesn't render right, but I think that's because the actual HEAD of all this work is in `connection-factory`

I believe this merges the buisness of connection-factory, though th…

bb49d50

…ere are many issues.

Some comments because everything is hard

7731f59

Add a signal that the content can use to tell the window it's ready

996c71a

A close button, and more logging

2f23e1f

some short-circuits for these inits, to make them more stable

c3a9445

You know, there's 0% chance that this is the right pattern for this, …

7e792b2

…but it _works_

Add a kill button for manually killing the content

5a07282

This works to kill the content and have the app live

3d15b09

a fix for a crash when closing

ea9d3cb

The sample app has a hard time loading TermControl resources so we're…

4a8f0e9

… just going to disable this for now

this was the part where I realized I dun goofed

eeb01a1

Revert "this was the part where I realized I dun goofed"

dafb627

This reverts commit 64533c8.

This too didn't work. Creating the XAML thing not on the XAML thing i…

1a78557

…sn't going to work

Revert "This too didn't work. Creating the XAML thing not on the XAML…

35ca313

… thing isn't going to work" This reverts commit fd364db.

This doesn't immediately crash, but it does crash when you start aski…

8ceb9ba

…ng for the actual text ranges. That's not what you want.

some cleanup

768d4c0

add a dialog internally to the bounds of the control, not outside of …

651efe2

…the control

Move the content process handling to a separate file in TermControl p…

738d191

…roject

move the content process main to another file as well

f818885

simplify the interface here a bit

07ba33b

BODGY, don't raise an event on destruction, that would be too...

cf70797

more cleanup

34601f7

Definitely needed those

46e299d

The sample works again

5429fca

add a velocity flag for this

b253440

derp

b312919

add localization back

857fb39

tried to make these blindli fire_and_forgets. Didn't work

9141f04

start thinking about moving the timers to being threadpooltimers in t…

76d8fa7

…he core. I think every x-proc call in TermControl logs that message, so _crap_

zadjii-msft added 2 commits April 20, 2022 06:36

Merge remote-tracking branch 'origin/main' into dev/migrie/oop/2/infi…

02fa24c

…nity-war

Merge remote-tracking branch 'origin/main' into dev/migrie/oop/2/infi…

7d4ee45

…nity-war

zadjii-msft added this to the Terminal v1.15 milestone Apr 28, 2022

Merge remote-tracking branch 'origin/main' into dev/migrie/oop/2/infi…

eac5ceb

…nity-war

DHowett reviewed May 18, 2022

View reviewed changes

zadjii-msft added 3 commits May 24, 2022 10:23

Merge remote-tracking branch 'origin/main' into dev/migrie/oop/2/infi…

223e270

…nity-war

some nits

52f7664

more nits, more a11y stuff

44ac527

lhecker reviewed May 25, 2022

View reviewed changes

carlos-zamora assigned zadjii-msft Jun 1, 2022

zadjii-msft modified the milestones: Terminal v1.15, Terminal v1.16 Jun 9, 2022

zadjii-msft added 3 commits June 21, 2022 11:44

Merge remote-tracking branch 'origin/main' into dev/migrie/oop/2/infi…

7707716

…nity-war

this was LOAD BEARING

f7d9384

this was definitely a bug

023eb75

lhecker reviewed Jun 21, 2022

View reviewed changes

src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved

that was vestigial anyways

b8cd2b2

lhecker approved these changes Jun 22, 2022

View reviewed changes

zadjii-msft added 2 commits July 6, 2022 13:14

Merge remote-tracking branch 'origin/main' into dev/migrie/oop/2/infi…

702e15c

…nity-war

Merge remote-tracking branch 'origin/main' into dev/migrie/oop/2/infi…

3434208

…nity-war

zadjii-msft modified the milestones: Terminal v1.16, Terminal v1.17 Aug 1, 2022

zadjii-msft and others added 3 commits September 1, 2022 12:33

Merge branch 'main' into dev/migrie/oop/2/infinity-war

e09b674

nots from review

852208a

Migrate spelling-0.0.21 changes from main

e52e720

zadjii-msft removed this from the Terminal v1.17 milestone Dec 12, 2022

zadjii-msft closed this Dec 12, 2022

zadjii-msft mentioned this pull request Dec 13, 2022

Refactor how connection restarting is handled #14548

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for running the terminal core as a content process #12938

Add support for running the terminal core as a content process #12938

zadjii-msft commented Apr 19, 2022 •

edited

Loading

DHowett May 18, 2022

DHowett May 18, 2022

DHowett May 18, 2022

zadjii-msft Sep 1, 2022

zadjii-msft Sep 2, 2022

DHowett May 18, 2022

DHowett May 18, 2022

zadjii-msft May 24, 2022

zadjii-msft May 24, 2022

zadjii-msft May 24, 2022

DHowett May 24, 2022

DHowett May 18, 2022

zadjii-msft May 24, 2022

DHowett May 18, 2022

DHowett May 18, 2022

zadjii-msft Sep 1, 2022

lhecker May 25, 2022

lhecker May 25, 2022

lhecker May 25, 2022 •

edited

Loading

zadjii-msft commented Aug 1, 2022

zadjii-msft commented Dec 12, 2022

		// Sometimes, RoActivateInstance is weird and fails with errors like the
		// following


		waits[0] = hContent.get();

		switch (WaitForMultipleObjects(2, waits, FALSE, INFINITE))

Add support for running the terminal core as a content process #12938

Add support for running the terminal core as a content process #12938

Conversation

zadjii-msft commented Apr 19, 2022 • edited Loading

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker May 25, 2022 • edited Loading

Choose a reason for hiding this comment

zadjii-msft commented Aug 1, 2022

zadjii-msft commented Dec 12, 2022

zadjii-msft commented Apr 19, 2022 •

edited

Loading

lhecker May 25, 2022 •

edited

Loading