Skip to content

Commit dabd473

Browse files
JohnMcPMSJohnMcPMS
authored
Improve COM static store usage (microsoft#5680)
## Issue The use of the COM static store was causing crashes on long lived processes because the implementation of `DllCanUnloadNow` did not count those objects, nor did it clean them up. This led to our module being unloaded, then when it was reloaded, the recreation of the static store object would invoke the deletion of the old one, which was pointing to the old, unloaded module location. ## Design Considerations WindowsPackageManager.dll serves many purposes, making the lifetime of statics complex. - It serves as "in-proc" for the CLI - It is the core implementation for the in-proc COM - It is the core implementation for the OOP COM In order to support in-proc COM, we must put static lifetime COM objects in the static store. But in order to support unloading, we must also clean them up. Additionally, we don't want to claim to be in use if the only active objects are our statics (which are typically just event handlers and their owners). We already use the WRL object count to track OOP COM server lifetime, and similarly we use it to implement `DllCanUnloadNow`. This is the count externally owned objects; those that the client has requested directly or indirectly. ## Change The major change is to remove all of our static store objects when WRL says we have no more externally owned. This is achieved by tracking the names of the objects that we insert and attempting to remove them when appropriate. The original change to use the static store was templatized and reused to hold the termination signal handler. ## Validation The new test uses the CLI to validate that the implementation for `DllCanUnloadNow` (`WindowsPackageManagerInProcModuleTerminate`) detects the unload state and properly destroys the relevant objects. This is done by checking that there are internal objects allocated before the call, but none after.
1 parent a992bfd commit dabd473

File tree

12 files changed

+270
-60
lines changed

12 files changed

+270
-60
lines changed

‎src/AppInstallerCLICore/Commands/TestCommand.cpp‎

Lines changed: 63 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
#include "Public/ShutdownMonitoring.h"
1212
#include "Workflows/ConfigurationFlow.h"
1313
#include "Workflows/MSStoreInstallerHandler.h"
14+
#include <winget/RepositorySource.h>
1415
#include <winrt/Microsoft.Management.Configuration.h>
1516

1617
using namespace AppInstaller::CLI::Workflow;
@@ -29,7 +30,7 @@ namespace AppInstaller::CLI
2930
HRESULT WaitForShutdown(Execution::Context& context)
3031
{
3132
LogAndReport(context, "Waiting for app shutdown event");
32-
if (!ShutdownMonitoring::TerminationSignalHandler::Instance().WaitForAppShutdownEvent())
33+
if (!ShutdownMonitoring::TerminationSignalHandler::Instance()->WaitForAppShutdownEvent())
3334
{
3435
LogAndReport(context, "Failed getting app shutdown event");
3536
return APPINSTALLER_CLI_ERROR_INTERNAL_ERROR;
@@ -41,7 +42,7 @@ namespace AppInstaller::CLI
4142

4243
HRESULT AppShutdownWindowMessage(Execution::Context& context)
4344
{
44-
auto windowHandle = ShutdownMonitoring::TerminationSignalHandler::Instance().GetWindowHandle();
45+
auto windowHandle = ShutdownMonitoring::TerminationSignalHandler::Instance()->GetWindowHandle();
4546

4647
if (windowHandle == NULL)
4748
{
@@ -217,6 +218,65 @@ namespace AppInstaller::CLI
217218
InvokeFindUnitProcessors;
218219
}
219220
};
221+
222+
struct TestCanUnloadNowCommand final : public Command
223+
{
224+
TestCanUnloadNowCommand(std::string_view parent) : Command("can-unload-now", {}, parent, Visibility::Hidden) {}
225+
226+
Resource::LocString ShortDescription() const override
227+
{
228+
return "Test DllCanUnloadNow"_lis;
229+
}
230+
231+
Resource::LocString LongDescription() const override
232+
{
233+
return "Verifies that the function that implements the inproc DllCanUnloadNow properly blocks unload due to static storage object."_lis;
234+
}
235+
236+
protected:
237+
void ExecuteInternal(Execution::Context& context) const override
238+
{
239+
Repository::Source source{ Repository::PredefinedSource::Installed };
240+
241+
ProgressCallback progress;
242+
source.Open(progress);
243+
244+
HMODULE self = GetModuleHandle(L"WindowsPackageManager.dll");
245+
if (!self)
246+
{
247+
LogAndReport(context, "Couldn't get WindowsPackageManager module");
248+
return;
249+
}
250+
251+
auto WindowsPackageManagerInProcModuleTerminate = reinterpret_cast<bool (__stdcall *)()>(GetProcAddress(self, "WindowsPackageManagerInProcModuleTerminate"));
252+
253+
// Report the object counts, attempt to terminate, report the object counts again
254+
ReportObjectCounts(context);
255+
LogAndReport(context, WindowsPackageManagerInProcModuleTerminate() ? "DllCanUnloadNow" : "DllCannotUnloadNow");
256+
ReportObjectCounts(context);
257+
}
258+
259+
private:
260+
void ReportObjectCounts(Execution::Context& context) const
261+
{
262+
std::ostringstream stream;
263+
stream << "Internal objects: " << GetInternalObjectCount() << '\n';
264+
stream << "External objects: " << GetExternalObjectCount();
265+
266+
LogAndReport(context, stream.str());
267+
}
268+
269+
uint32_t GetInternalObjectCount() const
270+
{
271+
return winrt::get_module_lock().operator unsigned int();
272+
}
273+
274+
unsigned long GetExternalObjectCount() const
275+
{
276+
auto module = Microsoft::WRL::GetModuleBase();
277+
return module ? module->GetObjectCount() : 0;
278+
}
279+
};
220280
}
221281

222282
std::vector<std::unique_ptr<Command>> TestCommand::GetCommands() const
@@ -226,6 +286,7 @@ namespace AppInstaller::CLI
226286
std::make_unique<TestAppShutdownCommand>(FullName()),
227287
std::make_unique<TestConfigurationExportCommand>(FullName()),
228288
std::make_unique<TestConfigurationFindUnitProcessorsCommand>(FullName()),
289+
std::make_unique<TestCanUnloadNowCommand>(FullName()),
229290
});
230291
}
231292

‎src/AppInstallerCLICore/Public/ShutdownMonitoring.h‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,15 +5,20 @@
55
#include <AppInstallerProgress.h>
66
#include <winrt/Windows.ApplicationModel.h>
77
#include <wil/resource.h>
8+
#include <memory>
89
#include <mutex>
910

1011
namespace AppInstaller::ShutdownMonitoring
1112
{
1213
// Type to contain the CTRL signal and window messages handler.
1314
struct TerminationSignalHandler
1415
{
16+
TerminationSignalHandler();
17+
18+
~TerminationSignalHandler();
19+
1520
// Gets the singleton handler.
16-
static TerminationSignalHandler& Instance();
21+
static std::shared_ptr<TerminationSignalHandler> Instance();
1722

1823
// Add a termination listener.
1924
void AddListener(ICancellable* cancellable);
@@ -33,10 +38,6 @@ namespace AppInstaller::ShutdownMonitoring
3338
#endif
3439

3540
private:
36-
TerminationSignalHandler();
37-
38-
~TerminationSignalHandler();
39-
4041
void StartAppShutdown();
4142

4243
static BOOL WINAPI StaticCtrlHandlerFunction(DWORD ctrlType);

‎src/AppInstallerCLICore/ShutdownMonitoring.cpp‎

Lines changed: 23 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,19 @@
55
#include <AppInstallerErrors.h>
66
#include <AppInstallerLogging.h>
77
#include <AppInstallerRuntime.h>
8+
#include <winget/COMStaticStorage.h>
89

910
namespace AppInstaller::ShutdownMonitoring
1011
{
11-
TerminationSignalHandler& TerminationSignalHandler::Instance()
12+
std::shared_ptr<TerminationSignalHandler> TerminationSignalHandler::Instance()
1213
{
13-
static TerminationSignalHandler s_instance;
14-
return s_instance;
14+
struct Singleton : public WinRT::COMStaticStorageBase<TerminationSignalHandler>
15+
{
16+
Singleton() : COMStaticStorageBase(L"WindowsPackageManager.TerminationSignalHandler") {}
17+
};
18+
19+
static Singleton s_instance;
20+
return s_instance.Get();
1521
}
1622

1723
void TerminationSignalHandler::AddListener(ICancellable* cancellable)
@@ -28,19 +34,25 @@ namespace AppInstaller::ShutdownMonitoring
2834
std::lock_guard<std::mutex> lock{ m_listenersLock };
2935

3036
auto itr = std::find(m_listeners.begin(), m_listeners.end(), cancellable);
31-
THROW_HR_IF(E_NOT_VALID_STATE, itr == m_listeners.end());
32-
m_listeners.erase(itr);
37+
if (itr == m_listeners.end())
38+
{
39+
AICLI_LOG(CLI, Warning, << "TerminationSignalHandler::RemoveListener did not find requested object");
40+
}
41+
else
42+
{
43+
m_listeners.erase(itr);
44+
}
3345
}
3446

3547
void TerminationSignalHandler::EnableListener(bool enabled, ICancellable* cancellable)
3648
{
3749
if (enabled)
3850
{
39-
Instance().AddListener(cancellable);
51+
Instance()->AddListener(cancellable);
4052
}
4153
else
4254
{
43-
Instance().RemoveListener(cancellable);
55+
Instance()->RemoveListener(cancellable);
4456
}
4557
}
4658

@@ -109,7 +121,7 @@ namespace AppInstaller::ShutdownMonitoring
109121

110122
BOOL WINAPI TerminationSignalHandler::StaticCtrlHandlerFunction(DWORD ctrlType)
111123
{
112-
return Instance().CtrlHandlerFunction(ctrlType);
124+
return Instance()->CtrlHandlerFunction(ctrlType);
113125
}
114126

115127
LRESULT WINAPI TerminationSignalHandler::WindowMessageProcedure(HWND hWnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
@@ -118,7 +130,7 @@ namespace AppInstaller::ShutdownMonitoring
118130
{
119131
case WM_QUERYENDSESSION:
120132
AICLI_LOG(CLI, Verbose, << "Received WM_QUERYENDSESSION");
121-
Instance().StartAppShutdown();
133+
Instance()->StartAppShutdown();
122134
return TRUE;
123135
case WM_ENDSESSION:
124136
case WM_CLOSE:
@@ -294,12 +306,12 @@ namespace AppInstaller::ShutdownMonitoring
294306

295307
ServerShutdownSynchronization::ServerShutdownSynchronization()
296308
{
297-
TerminationSignalHandler::Instance().AddListener(this);
309+
TerminationSignalHandler::Instance()->AddListener(this);
298310
}
299311

300312
ServerShutdownSynchronization::~ServerShutdownSynchronization()
301313
{
302-
TerminationSignalHandler::Instance().RemoveListener(this);
314+
TerminationSignalHandler::Instance()->RemoveListener(this);
303315
if (m_shutdownThread.joinable())
304316
{
305317
m_shutdownThread.detach();

‎src/AppInstallerCLICore/pch.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,4 +57,5 @@
5757
#pragma warning( pop )
5858

5959
#include <wrl/client.h>
60+
#include <wrl/implements.h>
6061
#include <AppxPackaging.h>

‎src/AppInstallerCLIE2ETests/AppShutdownTests.cs‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
namespace AppInstallerCLIE2ETests
88
{
99
using System;
10+
using System.Diagnostics;
1011
using System.IO;
1112
using System.Threading;
1213
using System.Threading.Tasks;
@@ -35,6 +36,12 @@ public void RegisterApplicationTest()
3536
Assert.Ignore("This test won't work on Window Server as non-admin");
3637
}
3738

39+
if (!Environment.Is64BitProcess)
40+
{
41+
// My guess is that HAM terminates us faster after the CTRL-C on x86...
42+
Assert.Ignore("This test is flaky when run as x86.");
43+
}
44+
3845
if (string.IsNullOrEmpty(TestSetup.Parameters.AICLIPackagePath))
3946
{
4047
throw new NullReferenceException("AICLIPackagePath");
@@ -95,5 +102,24 @@ public void RegisterApplicationTest()
95102
// Look for the output.
96103
Assert.True(testCmdTask.Result.StdOut.Contains("Succeeded waiting for app shutdown event"));
97104
}
105+
106+
/// <summary>
107+
/// Runs winget test can-unload-now expecting that it cannot be unloaded.
108+
/// </summary>
109+
[Test]
110+
public void CanUnloadNowTest()
111+
{
112+
var result = TestCommon.RunAICLICommand("test", "can-unload-now --verbose");
113+
114+
var lines = result.StdOut.Split('\n', StringSplitOptions.TrimEntries | StringSplitOptions.RemoveEmptyEntries);
115+
116+
Assert.AreEqual(5, lines.Length);
117+
Assert.True(lines[0].Contains("Internal objects:"));
118+
Assert.False(lines[0].Contains("Internal objects: 0"));
119+
Assert.True(lines[1].Contains("External objects: 0"));
120+
Assert.True(lines[2].Contains("DllCanUnloadNow"));
121+
Assert.True(lines[3].Contains("Internal objects: 0"));
122+
Assert.True(lines[4].Contains("External objects: 0"));
123+
}
98124
}
99125
}

‎src/AppInstallerRepositoryCore/Microsoft/PredefinedInstalledSourceFactory.cpp‎

Lines changed: 3 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
#include "Microsoft/SQLiteIndex.h"
77
#include "Microsoft/SQLiteIndexSource.h"
88
#include <winget/ManifestInstaller.h>
9-
9+
#include <winget/COMStaticStorage.h>
1010
#include <winget/Registry.h>
1111
#include <AppInstallerArchitecture.h>
1212
#include <winget/ExperimentalFeature.h>
@@ -270,41 +270,9 @@ namespace AppInstaller::Repository::Microsoft
270270

271271
struct CachedInstalledIndex
272272
{
273-
// https://devblogs.microsoft.com/oldnewthing/20210215-00/?p=104865
274-
struct Singleton
273+
struct Singleton : public WinRT::COMStaticStorageBase<CachedInstalledIndex>
275274
{
276-
struct Holder : public winrt::implements<Holder, winrt::Windows::Foundation::IInspectable>
277-
{
278-
static constexpr std::wstring_view Guid{ L"{48c47064-4fff-4eca-812c-dbb4f33a8fcb}" };
279-
std::shared_ptr<CachedInstalledIndex> m_shared{ std::make_shared<CachedInstalledIndex>() };
280-
};
281-
282-
std::weak_ptr<CachedInstalledIndex> m_weak;
283-
winrt::slim_mutex m_lock;
284-
285-
std::shared_ptr<CachedInstalledIndex> Get()
286-
{
287-
{
288-
const std::shared_lock lock{ m_lock };
289-
if (auto cachedIndex = m_weak.lock())
290-
{
291-
return cachedIndex;
292-
}
293-
}
294-
295-
auto value = winrt::make_self<Holder>();
296-
297-
const std::shared_lock lock{ m_lock };
298-
if (auto cachedIndex = m_weak.lock())
299-
{
300-
return cachedIndex;
301-
}
302-
303-
winrt::Windows::ApplicationModel::Core::CoreApplication::Properties().Insert(Holder::Guid, value.as<winrt::Windows::Foundation::IInspectable>());
304-
305-
m_weak = value->m_shared;
306-
return value->m_shared;
307-
}
275+
Singleton() : COMStaticStorageBase(L"WindowsPackageManager.CachedInstalledIndex") {}
308276
};
309277

310278
CachedInstalledIndex()

‎src/AppInstallerSharedLib/AppInstallerSharedLib.vcxproj‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -349,6 +349,7 @@
349349
<ClInclude Include="Public\winget\AsyncTokens.h" />
350350
<ClInclude Include="Public\winget\Certificates.h" />
351351
<ClInclude Include="Public\winget\Compression.h" />
352+
<ClInclude Include="Public\winget\COMStaticStorage.h" />
352353
<ClInclude Include="Public\winget\ConfigurationSetProcessorHandlers.h" />
353354
<ClInclude Include="Public\winget\Filesystem.h" />
354355
<ClInclude Include="Public\winget\GroupPolicy.h" />
@@ -379,6 +380,7 @@
379380
<ClCompile Include="AppInstallerStrings.cpp" />
380381
<ClCompile Include="Certificates.cpp" />
381382
<ClCompile Include="Compression.cpp" />
383+
<ClCompile Include="COMStaticStorage.cpp" />
382384
<ClCompile Include="DateTime.cpp" />
383385
<ClCompile Include="Errors.cpp" />
384386
<ClCompile Include="Filesystem.cpp" />
@@ -436,4 +438,4 @@
436438
<Error Condition="!Exists('$(SolutionDir)\packages\Microsoft.Windows.CppWinRT.2.0.230706.1\build\native\Microsoft.Windows.CppWinRT.props')" Text="$([System.String]::Format('$(ErrorText)', '$(SolutionDir)\packages\Microsoft.Windows.CppWinRT.2.0.230706.1\build\native\Microsoft.Windows.CppWinRT.props'))" />
437439
<Error Condition="!Exists('$(SolutionDir)\packages\Microsoft.Windows.CppWinRT.2.0.230706.1\build\native\Microsoft.Windows.CppWinRT.targets')" Text="$([System.String]::Format('$(ErrorText)', '$(SolutionDir)\packages\Microsoft.Windows.CppWinRT.2.0.230706.1\build\native\Microsoft.Windows.CppWinRT.targets'))" />
438440
</Target>
439-
</Project>
441+
</Project>

‎src/AppInstallerSharedLib/AppInstallerSharedLib.vcxproj.filters‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,9 @@
140140
<ClInclude Include="Public\winget\ModuleCountBase.h">
141141
<Filter>Public\winget</Filter>
142142
</ClInclude>
143+
<ClInclude Include="Public\winget\COMStaticStorage.h">
144+
<Filter>Public\winget</Filter>
145+
</ClInclude>
143146
</ItemGroup>
144147
<ItemGroup>
145148
<ClCompile Include="pch.cpp">
@@ -229,6 +232,9 @@
229232
<ClCompile Include="SQLiteDynamicStorage.cpp">
230233
<Filter>SQLite</Filter>
231234
</ClCompile>
235+
<ClCompile Include="COMStaticStorage.cpp">
236+
<Filter>Source Files</Filter>
237+
</ClCompile>
232238
</ItemGroup>
233239
<ItemGroup>
234240
<None Include="PropertySheet.props" />

0 commit comments

Comments
 (0)