Allow WinUI to shut down/restart in a process - #11854
George Gao (gegao18) wants to merge 4 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. #Closed |
microsoft-github-policy-service
Bot
added
the
needs-triage
9b83302 to
e730c4d
Compare
George Gao (gegao18)
changed the title
e730c4d to
276d749
Compare
|
/azp run #Closed |
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. #Closed |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. #Closed |
|
|
||
| #include "XamlControlsXamlMetaDataProvider.g.cpp" | ||
|
|
||
| extern "C" void __stdcall DeinitializeMUXC(); |
There was a problem hiding this comment.
We do call it from the test suite from WindowHelper::ShutdownXaml, but we should probably leave that alone until the tests properly start up and shut down WinUI. They'd need to be Win32 apps that create and tear down a Xaml island in order to get the shutdown event that automatically calls DeinitializeMUXC.
In the meantime the WinUICppDesktopSampleAppTests verifies that these events are raised during island teardown.
| State m_state {State::Normal}; | ||
|
|
||
| inline static std::atomic<int> s_instancesInProcess{}; | ||
| inline static bool s_processShutdownInProgress{ false }; |
| static std::once_flag processShutdownSubscription; | ||
| std::call_once(processShutdownSubscription, []() | ||
| { | ||
| winrt::Microsoft::UI::Xaml::Hosting::WindowsXamlManager::WinUIProcessShutdownStarting( |
There was a problem hiding this comment.
Good call on the phone binary. I checked and it's actually using private APIs not available to MUXC. It hooks into Private::XamlRuntimeType::ResetDependencyProperties to clear out its custom DPs, and hooks into EnsureDependencyProperties to lazily create them afterwards.
Yes, we do need to tell other controls-library DLLs to subscribe to this and release custom DPs (and reinitialize when WinUI starts again in the process). The WinUI Community Toolkit currently has this problem, caught by the new WinUICsIslandsSampleApp. We can do this separately on a case-by-case basis. If a controls library isn't used by a Win32 app that wants to restart WinUI (i.e. shell), it doesn't need to update itself.
There was a problem hiding this comment.
...Part of this is by design and part of this is follow-up work. I'll resolve this thread as "Won't fix" for now.
276d749 to
d999cf3
Compare
| WindowsXamlManager_XamlShutdownStartingInProcess_Deleted0,UnknownType_UnknownEvent,340 | ||
| WindowsXamlManager_XamlShutdownCompletedInProcess_Deleted0,UnknownType_UnknownEvent,341 | ||
| WindowsXamlManager_XamlProcessShutdownStarting_Deleted0,UnknownType_UnknownEvent,342 | ||
| WindowsXamlManager_XamlProcessShutdownCompleted_Deleted0,UnknownType_UnknownEvent,343 |
There was a problem hiding this comment.
Remove these Deleted0 items (and renumber the remaining items). I think codegen will need to be rerun after that. #Resolved
| _In_ msy::IDispatcherQueueShutdownStartingEventArgs* args) override | ||
| { | ||
| auto managerStrongRef = m_manager; | ||
| auto core = WindowsXamlManager::tls_xamlCore; |
There was a problem hiding this comment.
Why going to the tls_xamlCore? That should be the same as "this". #Resolved
There was a problem hiding this comment.
Close() sets "tls_xamlCore = nullptr;", which might be the last reference to the XamlCore.
When I switch this to "this" though, everything still works. It turns out we have a leak here. WindowsXamlManager::m_xamlCore is a shared_ptr on the XamlCore, and XamlCoreNewShutdown::m_manager is a ComPtr back to the WindowsXamlManager. There's a reference cycle that keeps both alive.
I'll see if I can clear out this cycle, in which case this should continue setting a shared_ptr here to keep the XamlCore valid past the Close() call.
|
|
||
| if (lastInstanceInProcess) | ||
| { | ||
| CApplicationLock applicationLock; |
There was a problem hiding this comment.
Why release the lock only to immediately retake it if lastInstanceInProcess? #Resolved
| tls_xamlCore = nullptr; // Close() already does this, but adding here for clarity. | ||
| auto core = tls_xamlCore; | ||
| bool shouldRaiseProcessShutdownEvents = false; | ||
| IFC_RETURN(core->Close(&shouldRaiseProcessShutdownEvents)); |
There was a problem hiding this comment.
Pre-existing: This is bypassing the managerStrongRef->RaiseXamlShutdownCompletedOnThreadEvent(args); call which normally happens in OnFrameworkShutdownStarting. The added code duplication here makes me wonder if this should all be handled directly in Close(). (Moving all to Close would be easier if we can delete the old XamlCoreLegacyShutdown mode -- can we? That full cleanup should probably be separate if we agree it can and should be done.) #WontFix
There was a problem hiding this comment.
Yes let's delete the old shutdown path. I can do that in a separate PR if that's easier
There was a problem hiding this comment.
Sounds like a good candidate for separate cleanup. XamlShutdownCompletedOnThread was WASDK 1.5, so can we take the behavior change of deleting the old shutdown path now?
| static std::once_flag processShutdownSubscription; | ||
| std::call_once(processShutdownSubscription, []() | ||
| { | ||
| winrt::Microsoft::UI::Xaml::Hosting::WindowsXamlManager::WinUIProcessShutdownStarting( |
There was a problem hiding this comment.
You mentioned something about a thread being able to get and process WinUIProcessShutdownStarting/Completed before another thread has received(?) or finished(?) processing thread shutdown? Please clarify that scenario. Specifically, could it result in a background thread thinking it can still access data which this thread is about to release in this DeinitializeMUXC() call? (If so, does the data cleared in DeinitializeMUXC need locks?) #ByDesign
There was a problem hiding this comment.
Yep. Here's a potential order of things:
- Thread A shuts down its DispatcherQueueController. WinUI tears down on thread A. This raises WindowsXamlManager.XamlShutdownCompletedOnThread on thread A. Thread A switches out before its handler can run.
- Thread B shuts down its DispatcherQueueController. WinUI tears down on thread B. This raises XamlShutdownCompletedOnThread on thread B. Thread B keeps running and finishes its handler.
- Thread B was the last thread using WinUI, so after its XamlShutdownCompletedOnThread, WinUI raises WinUIProcessShutdownStarting on thread B. Note that thread A is still switched out right before its thread shutdown completed handler.
- The app clears out its custom DPs on thread B, and returns from WinUIProcessShutdownStarting.
- WinUI then raises WinUIProcessShutdownCompleted on thread B. Thread B marks a flag saying it's ok to reinitialize WinUI. Thread B switches out.
- Thread A switches back in and runs its XamlShutdownCompletedOnThread handler, but at this point the process shutdown events have already run and completed.
The conscious choice was to not wait for all thread completions before doing step 3. The reasoning was that any pending XamlShutdownCompletedOnThread handlers should be cleaning up state that does not depend on custom DPs or metadata. Since it's XamlShutdownCompletedOnThread, the core is already closed and the thread can't touch WinUI state again. It should just release whatever thread-dependent state it has and maybe ask for a deferral on the DQ shutdown.
| static event Windows.Foundation.EventHandler<Object> WinUIProcessShutdownStarting; | ||
| static event Windows.Foundation.EventHandler<Object> WinUIProcessShutdownCompleted; |
There was a problem hiding this comment.
These should be somehow called out as even newer. #Resolved
|
|
||
| When the final Xaml thread shuts down, Xaml immediately blocks new initialization and resets its process-wide metadata | ||
| and activation-factory caches. After the thread that closes the final Xaml core has raised | ||
| `XamlShutdownCompletedOnThread` and its synchronous handlers have returned, `WinUIProcessShutdownStarting` is raised. |
There was a problem hiding this comment.
Should have a callout that the WinUIProcess* events fire on the thread which does the last shutdown, and event handlers are expected to be agile and correctly handle a call on any thread. #Resolved
| After the starting handlers return, `WinUIProcessShutdownCompleted` is raised. Its sender and event arguments are null. | ||
| Apps may retry initialization after this event. Because another Xaml generation can start and shut down before a waiting thread runs, | ||
| `InitializeForCurrentThread` remains the authoritative check and can still return `ERROR_INVALID_STATE`. | ||
|
|
There was a problem hiding this comment.
I think additional caveats are required, such as to note that all control libraries and app code must handle the WinUIProcessShutdownStarting even to clear necessary state and also re-register everything, as needed, when attempting to restart. #Resolved
|
|
||
| if (shouldRaiseProcessShutdownEvents) | ||
| { | ||
| core->RaiseProcessShutdownEvents(); |
There was a problem hiding this comment.
The ProcessShutdownCompleted event is supposed to tell listeners they could now restart WinUI, right? But if that is called synchronously while in OnFrameworkShutdownStarting, they probably can't actually restart now, since the DispatcherQueue is still shutting down. What happens if they try this? Hopefully this will cause a clear failure message, and we need the docs (in the spec) to state that the restart needs to either happen on a different thread or wait until after the DQ shutdown has completed. #Resolved
There was a problem hiding this comment.
I think that falls under the category of "initializing while there's already a DispatcherQueue". The public documentation just says that "an error results", not which error:
https://learn.microsoft.com/en-us/windows/windows-app-sdk/api/winrt/microsoft.ui.dispatching.dispatcherqueuecontroller.createoncurrentthread?view=windows-app-sdk-2.0
A DispatcherQueue is created, and associated with the current thread. An error results if there's already a DispatcherQueue associated with the current thread.
I'll make a note on the API spec.
| @@ -0,0 +1,7 @@ | |||
| <Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | |||
| <PropertyGroup> | |||
| <WindowsAppSdkPackageVersion Condition="'$(WindowsAppSdkPackageVersion)' == ''">2.2.2-experimental9</WindowsAppSdkPackageVersion> | |||
There was a problem hiding this comment.
Is this needed? No other CS samples do this, and should pick up what they need from the inherited props/targets. #Resolved
There was a problem hiding this comment.
This is for Windows Community Toolkit. It has dependencies on the WASDK and the BuildTools packages that must come from shine-oss. I get sign in prompts that lead to "the admin requires this device to be managed by Microsoft" errors, even though my device is manage by Microsoft. This line picks dependency versions that are anonymously downloadable from shine-oss, which avoids the sign in prompts. I'll update this to 2.4.0 so we don't depend on an experimental package.
|
No pipelines are associated with this pull request. #Closed |
…ownStarting/Completed events, update WinUICppIslandsSampleApp
|
/azp run #Closed |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. #Closed |
|
No pipelines are associated with this pull request. #Closed |
|
/azp run #Closed |
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. #Closed |
|
No pipelines are associated with this pull request. #Closed |
2 similar comments
|
No pipelines are associated with this pull request. #Closed |
|
No pipelines are associated with this pull request. #Closed |
|
No pipelines are associated with this pull request. #Closed |
|
/azp run #Resolved |
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. #Closed |
|
No pipelines are associated with this pull request. #Closed |
|
/azp run #Resolved |
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. #Closed |
|
No pipelines are associated with this pull request. |
3 participants
Addresses #11741
Fixes
WinUI cannot be shut down and restarted in a process. This change fixes that problem, allowing for shut down and restart.
Addresses #11741
PR Type
Description
Shutting down WinUI and restarting results in a crash in
MicaBackdrop'sKindproperty. The problem here is stale data. MUXC registers customDependencyPropertyobjects (includingMicaBackdrop.Kind) with MUX, and also has references to those DP objects. When MUX shuts down, it clears out all fields in its registeredCCustomDependencyPropertyobjects, including those from MUXC. MUXC continues to hold on to its old and cleared out DP objects. When MUXC restarts, it sees that its caches are still filled, so it doesn't bother registering new custom DPs. It then tries to reuse its oldKindproperty, finds that the default value is now null, and crashes.The general problem is that MUXC holds on to data that should be thrown out once WinUI shuts down in the process. In addition to custom DPs, MUXC also has a bunch of
IXamlTypeobjects in itsc_typeEntriesarray. These objects are registered with WinUI, and MUX released its reference to them during shutdown, while MUXC held on to them.This change adds a mechanism for components to clean up after WinUI shuts down for the entire process. MUXC subscribes to this mechanism and calls
DeinitializeMuxc()to clear its caches, so that when WinUI starts up again it can create new and valid objects.There are two new static events on
WindowsXamlManager:WindowsXamlManager.WinUIProcessShutdownStartingWindowsXamlManager.XamlShutdownCompletedOnThread).XamlShutdownCompletedOnThreadevent on the thread that shut down the last runtime. WinUI makes no guarantees about this event's order withXamlShutdownCompletedOnThreadevents on other threads. This is the event that should trigger components to release any WinUI-related cached objects associated with the entire process (e.g. custom DependencyProperties, metadata objects).DeinitializeMuxc(), which releases its cached objects and allows WinUI to restart.WindowsXamlManager.WinUIProcessShutdownCompletedWinUIProcessShutdownStarting, after it returns from all event handlers.ShutdownStartingmay have multiple listeners, and we want all of them to clean up before WinUI can be initialized again.Note that this event is not a guarantee that initialization will be successful. Another thread may initialize and shut down WinUI again while the current thread is switched out, and we may be in the middle of another
WinUIProcessShutdownStarting/Completedpass.This change also updates the existing
WinUICppIslandsSampleApp:Application,DispatcherQueueController,WindowsXamlManager, andDesktopWindowXamlSourceobjects.While this change allows all instances of WinUI to shut down, it does not fully shut down WinUI. Doing that means also unloading all WinUI-related binaries, which we still cannot do yet.
Current Behavior
Shutting down all WinUI runtimes and attempting to restart one will hit a crash during startup.
New Behavior
WinUI can be restarted after shutting down all runtimes.
Customer Impact
Apps using Xaml islands can now shut down all instances of the WinUI runtime when WinUI is no longer needed, rather than having to keep one alive all the time.
Regression Potential
How Has This Been Tested?
Screenshots (if appropriate)