diff --git a/src/AppInstallerCLICore/COMContext.cpp b/src/AppInstallerCLICore/COMContext.cpp index bd7bea1dbd..6d10de46a2 100644 --- a/src/AppInstallerCLICore/COMContext.cpp +++ b/src/AppInstallerCLICore/COMContext.cpp @@ -86,21 +86,23 @@ namespace AppInstaller::CLI::Execution Logging::Log().SetEnabledChannels(channel.has_value() ? channel.value() : Settings::User().Get()); Logging::Log().SetLevel(level.has_value() ? level.value() : Settings::User().Get()); - // TODO: Log to file for COM API calls only when debugging in visual studio - Logging::FileLogger::Add(s_comLogFileNamePrefix); - - Logging::OutputDebugStringLogger::Remove(); + Logging::TraceLogger::Add(); + Logging::EnableWilFailureTelemetry(); + // File logging must not prevent COM functionality. + try + { + Logging::FileLogger::Add(s_comLogFileNamePrefix); #ifndef AICLI_DISABLE_TEST_HOOKS - if (!Settings::User().Get()) + if (!Settings::User().Get()) #endif - { - // Initiate the background cleanup of the log file location. - Logging::FileLogger::BeginCleanup(); + { + // Initiate the background cleanup of the log file location. + Logging::FileLogger::BeginCleanup(); + } } + CATCH_LOG(); - Logging::TraceLogger::Add(); - - Logging::EnableWilFailureTelemetry(); + Logging::OutputDebugStringLogger::Remove(); } } diff --git a/src/AppInstallerCLIE2ETests/InprocTestbedTests.cs b/src/AppInstallerCLIE2ETests/InprocTestbedTests.cs index 0fdb87b6b6..092990c032 100644 --- a/src/AppInstallerCLIE2ETests/InprocTestbedTests.cs +++ b/src/AppInstallerCLIE2ETests/InprocTestbedTests.cs @@ -6,6 +6,7 @@ namespace AppInstallerCLIE2ETests { + using System.Collections.Generic; using System.IO; using System.Reflection; using AppInstallerCLIE2ETests.Helpers; @@ -93,6 +94,30 @@ public void DefaultTest() this.RunInprocTestbed(new TestbedParameters() { WorkTestSleepInterval = 1000 }); } + /// + /// Verifies that an unusable file logging path does not prevent activation or settings calls. + /// + [Test] + public void FileLoggingFailureDoesNotPreventActivation() + { + // An existing file cannot serve as the parent directory for the unpackaged log path. + var environmentVariables = new Dictionary() + { + ["TMP"] = Path.GetFullPath(this.InprocTestbedPath), + ["TEMP"] = Path.GetFullPath(this.InprocTestbedPath), + }; + + var result = TestCommon.RunProcess( + this.InprocTestbedPath, + string.Empty, + "-itr 2", + null, + 60000, + true, + environmentVariables); + Assert.That(result.ExitCode, Is.Zero, result.StdOut + result.StdErr); + } + /// /// Tests using the CLSID with CoCreateInstance. /// diff --git a/src/AppInstallerCLITests/FileLogger.cpp b/src/AppInstallerCLITests/FileLogger.cpp index a5e79dc74f..9050490a86 100644 --- a/src/AppInstallerCLITests/FileLogger.cpp +++ b/src/AppInstallerCLITests/FileLogger.cpp @@ -2,8 +2,11 @@ // Licensed under the MIT License. #include "pch.h" #include "TestCommon.h" +#include "TestHooks.h" #include #include +#include +#include #include #include @@ -201,6 +204,41 @@ namespace } } +TEST_CASE("COMFileLogging_InitializationFailureIsNonFatal", "[logging]") +{ + TestUserSettings settings; + AppInstaller::ThreadLocalStorage::WingetThreadGlobals threadGlobals; + auto previousThreadGlobals = threadGlobals.SetForCurrentThread(); + + TempFile blocker{ "FileLogger_BlockedParent", ".tmp" }; + { + std::ofstream file{ blocker.GetPath() }; + file << "Not a directory"; + REQUIRE(file.good()); + } + + auto logPath = blocker.GetPath() / "test.log"; + REQUIRE_THROWS(FileLogger{ logPath }); + REQUIRE_THROWS(FileLogger::Add(logPath)); + REQUIRE_FALSE(Log().ContainsLogger(FileLogger::GetNameForPath(logPath))); + + using namespace AppInstaller::Runtime; + auto restorePaths = wil::scope_exit([]() + { + TestHook_ClearPathOverrides(); + SetTestPathOverrides(); + }); + AppInstaller::Filesystem::PathDetails blockedPath; + blockedPath.Path = blocker.GetPath() / "logs"; + blockedPath.Create = GENERATE(false, true); + TestHook_SetPathOverride(PathName::DefaultLogLocation, blockedPath); + + REQUIRE_NOTHROW(AppInstaller::CLI::Execution::COMContext::SetLoggers(AppInstaller::Logging::Channel::All, Level::Verbose)); + REQUIRE_FALSE(Log().ContainsLogger("file")); + REQUIRE(Log().ContainsLogger("Trace")); + REQUIRE_FALSE(Log().ContainsLogger("OutputDebugStringLogger")); +} + TEST_CASE("FileLogger_MaximumSize", "[logging]") { auto tagState = GENERATE(TagState_Unset, TagState_SetAtStart, TagState_SetAfterLogging); diff --git a/src/Microsoft.Management.Deployment.InProc/dllmain.cpp b/src/Microsoft.Management.Deployment.InProc/dllmain.cpp index 24e228f6ae..3217ae9039 100644 --- a/src/Microsoft.Management.Deployment.InProc/dllmain.cpp +++ b/src/Microsoft.Management.Deployment.InProc/dllmain.cpp @@ -3,23 +3,28 @@ #include "pch.h" #include #include +#include + +namespace +{ + HRESULT EnsureInitialized() try + { + static std::once_flag initializeOnceFlag; + std::call_once(initializeOnceFlag, + []() + { + THROW_IF_FAILED(WindowsPackageManagerInProcModuleInitialize()); + }); + return S_OK; + } + CATCH_RETURN(); +} EXTERN_C BOOL WINAPI DllMain( HMODULE /* hModule */, - DWORD reason, + DWORD /* reason */, LPVOID /* lpReserved */) { - switch (reason) - { - case DLL_PROCESS_ATTACH: - { - if (FAILED(WindowsPackageManagerInProcModuleInitialize())) - { - return FALSE; - } - } - break; - } return TRUE; } @@ -28,6 +33,9 @@ STDAPI DllGetClassObject( REFIID riid, LPVOID* ppv) { + RETURN_HR_IF_NULL(E_POINTER, ppv); + *ppv = nullptr; + RETURN_IF_FAILED(EnsureInitialized()); RETURN_HR(WindowsPackageManagerInProcModuleGetClassObject(rclsid, riid, ppv)); } @@ -38,5 +46,8 @@ STDAPI DllCanUnloadNow() STDAPI DllGetActivationFactory(HSTRING classId, void** factory) { + RETURN_HR_IF_NULL(E_POINTER, factory); + *factory = nullptr; + RETURN_IF_FAILED(EnsureInitialized()); return WindowsPackageManagerInProcModuleGetActivationFactory(classId, factory); } diff --git a/src/WindowsPackageManager/ConfigurationStaticFunctions.cpp b/src/WindowsPackageManager/ConfigurationStaticFunctions.cpp index e904c39101..35821a8654 100644 --- a/src/WindowsPackageManager/ConfigurationStaticFunctions.cpp +++ b/src/WindowsPackageManager/ConfigurationStaticFunctions.cpp @@ -90,7 +90,11 @@ namespace ConfigurationShim auto& diagnosticsLogger = m_threadGlobals.GetDiagnosticLogger(); diagnosticsLogger.SetEnabledChannels(AppInstaller::Logging::Channel::All); diagnosticsLogger.SetLevel(AppInstaller::Logging::Level::Verbose); - diagnosticsLogger.AddLogger(std::make_unique("WinGetCFG"sv)); + try + { + diagnosticsLogger.AddLogger(std::make_unique("WinGetCFG"sv)); + } + CATCH_LOG(); if (IsConfigurationAvailable()) { diff --git a/src/WindowsPackageManager/WindowsPackageManager.h b/src/WindowsPackageManager/WindowsPackageManager.h index 7832160137..1d3226614f 100644 --- a/src/WindowsPackageManager/WindowsPackageManager.h +++ b/src/WindowsPackageManager/WindowsPackageManager.h @@ -34,7 +34,7 @@ extern "C" // Creates an out-of-proc instance for manual activation scenarios. WINDOWS_PACKAGE_MANAGER_API WindowsPackageManagerServerCreateInstance(REFCLSID rclsid, REFIID riid, void** out); - // Creates module for in-proc COM invocation. + // Initializes the module and CLI infrastructure for in-proc COM invocation. Must be called outside DllMain. WINDOWS_PACKAGE_MANAGER_API WindowsPackageManagerInProcModuleInitialize(); // Try to terminate the module for in-proc COM. Returns false if there's still active objects.