From bce75525206e31ef9bdeef827965b9de9a09b7f3 Mon Sep 17 00:00:00 2001 From: Ruben Guerrero Samaniego Date: Wed, 8 Sep 2021 12:31:49 -0700 Subject: [PATCH 1/5] Add try catch and logging for failed to load resources file --- src/AppInstallerCLICore/Core.cpp | 11 ++++++++++- src/AppInstallerCLICore/Resources.cpp | 12 ++++++++++-- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/AppInstallerCLICore/Core.cpp b/src/AppInstallerCLICore/Core.cpp index d9a3b3af8d..2d92ece086 100644 --- a/src/AppInstallerCLICore/Core.cpp +++ b/src/AppInstallerCLICore/Core.cpp @@ -125,7 +125,16 @@ namespace AppInstaller::CLI return APPINSTALLER_CLI_ERROR_BLOCKED_BY_POLICY; } - return Execute(context, command); + try + { + return Execute(context, command); + } + catch (hresult_error const& e) + { + AICLI_LOG(CLI, Error, << "Error " << e.to_abi() << " with message '" << e.message().c_str() << + "' encountered while executing command"); + return e.to_abi(); + } } // End of the line exceptions that are not ever expected. // Telemetry cannot be reliable beyond this point, so don't let these happen. diff --git a/src/AppInstallerCLICore/Resources.cpp b/src/AppInstallerCLICore/Resources.cpp index 983004834d..04548e974c 100644 --- a/src/AppInstallerCLICore/Resources.cpp +++ b/src/AppInstallerCLICore/Resources.cpp @@ -4,7 +4,7 @@ #include "Resources.h" using namespace AppInstaller::Utility::literals; - +using namespace winrt; namespace AppInstaller::CLI::Resource { @@ -18,7 +18,15 @@ namespace AppInstaller::CLI::Resource Loader::Loader() { - m_wingetLoader = winrt::Windows::ApplicationModel::Resources::ResourceLoader::GetForViewIndependentUse(L"winget"); + try + { + m_wingetLoader = winrt::Windows::ApplicationModel::Resources::ResourceLoader::GetForViewIndependentUse(L"winget"); + } + catch (hresult_error const& e) + { + AICLI_LOG(CLI, Error, << "Failure loading resource file with error: " << e.to_abi()); + throw; + } } std::string Loader::ResolveString( From 7c3b3ce3a0b5a96a828b208c6fe50314abe86bee Mon Sep 17 00:00:00 2001 From: Ruben Guerrero Samaniego Date: Wed, 8 Sep 2021 12:56:51 -0700 Subject: [PATCH 2/5] Handle exception better --- src/AppInstallerCLICore/Command.cpp | 18 +++++++++++++++--- src/AppInstallerCLICore/Core.cpp | 11 +---------- src/AppInstallerCLICore/Resources.cpp | 6 +++--- 3 files changed, 19 insertions(+), 16 deletions(-) diff --git a/src/AppInstallerCLICore/Command.cpp b/src/AppInstallerCLICore/Command.cpp index f6dd421b57..01951a9830 100644 --- a/src/AppInstallerCLICore/Command.cpp +++ b/src/AppInstallerCLICore/Command.cpp @@ -868,9 +868,21 @@ namespace AppInstaller::CLI { std::string message = GetUserPresentableMessage(hre); Logging::Telemetry().LogException(command->FullName(), "winrt::hresult_error", message); - context.Reporter.Error() << - Resource::String::UnexpectedErrorExecutingCommand << ' ' << std::endl << - message << std::endl; + try + { + context.Reporter.Error() << + Resource::String::UnexpectedErrorExecutingCommand << ' ' << std::endl << + message << std::endl; + } + // If resource.pri is not present trying to log using Resource will throw the exception + // again. Default to English message. + catch (const winrt::hresult_error&) + { + context.Reporter.Error() << + "An unexpected error occurred while executing the command:" << ' ' << std::endl << + message << std::endl; + } + return hre.code(); } catch (const Settings::GroupPolicyException& e) diff --git a/src/AppInstallerCLICore/Core.cpp b/src/AppInstallerCLICore/Core.cpp index 2d92ece086..d9a3b3af8d 100644 --- a/src/AppInstallerCLICore/Core.cpp +++ b/src/AppInstallerCLICore/Core.cpp @@ -125,16 +125,7 @@ namespace AppInstaller::CLI return APPINSTALLER_CLI_ERROR_BLOCKED_BY_POLICY; } - try - { - return Execute(context, command); - } - catch (hresult_error const& e) - { - AICLI_LOG(CLI, Error, << "Error " << e.to_abi() << " with message '" << e.message().c_str() << - "' encountered while executing command"); - return e.to_abi(); - } + return Execute(context, command); } // End of the line exceptions that are not ever expected. // Telemetry cannot be reliable beyond this point, so don't let these happen. diff --git a/src/AppInstallerCLICore/Resources.cpp b/src/AppInstallerCLICore/Resources.cpp index 04548e974c..3984ec8f7d 100644 --- a/src/AppInstallerCLICore/Resources.cpp +++ b/src/AppInstallerCLICore/Resources.cpp @@ -4,7 +4,6 @@ #include "Resources.h" using namespace AppInstaller::Utility::literals; -using namespace winrt; namespace AppInstaller::CLI::Resource { @@ -22,9 +21,10 @@ namespace AppInstaller::CLI::Resource { m_wingetLoader = winrt::Windows::ApplicationModel::Resources::ResourceLoader::GetForViewIndependentUse(L"winget"); } - catch (hresult_error const& e) + catch (const winrt::hresult_error& hre) { - AICLI_LOG(CLI, Error, << "Failure loading resource file with error: " << e.to_abi()); + // This message cannot be localized. + AICLI_LOG(CLI, Error, << "Failure loading resource file with error: " << hre.code()); throw; } } From 4af09b3c17a4a68040dbb92f02300cfe8361b8d2 Mon Sep 17 00:00:00 2001 From: Ruben Guerrero Samaniego Date: Wed, 8 Sep 2021 14:14:01 -0700 Subject: [PATCH 3/5] Add pri to expected words --- .github/actions/spelling/expect.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/actions/spelling/expect.txt b/.github/actions/spelling/expect.txt index 2b3d7b00ce..b01c02cfd2 100644 --- a/.github/actions/spelling/expect.txt +++ b/.github/actions/spelling/expect.txt @@ -246,6 +246,7 @@ pkindex PMS positionals powertoys +pri productcode pseudocode pvk From d7d4d0bb980015ad72737b8cd1433b378db04cba Mon Sep 17 00:00:00 2001 From: Ruben Guerrero Samaniego Date: Thu, 9 Sep 2021 16:03:24 -0700 Subject: [PATCH 4/5] Add better message --- src/AppInstallerCLICore/Command.cpp | 24 +++++++------------ src/AppInstallerCLICore/Resources.cpp | 11 +++++++-- src/AppInstallerCLICore/Resources.h | 7 ++++++ .../Public/AppInstallerErrors.h | 1 + 4 files changed, 26 insertions(+), 17 deletions(-) diff --git a/src/AppInstallerCLICore/Command.cpp b/src/AppInstallerCLICore/Command.cpp index 01951a9830..38a53a73f5 100644 --- a/src/AppInstallerCLICore/Command.cpp +++ b/src/AppInstallerCLICore/Command.cpp @@ -868,21 +868,9 @@ namespace AppInstaller::CLI { std::string message = GetUserPresentableMessage(hre); Logging::Telemetry().LogException(command->FullName(), "winrt::hresult_error", message); - try - { - context.Reporter.Error() << - Resource::String::UnexpectedErrorExecutingCommand << ' ' << std::endl << - message << std::endl; - } - // If resource.pri is not present trying to log using Resource will throw the exception - // again. Default to English message. - catch (const winrt::hresult_error&) - { - context.Reporter.Error() << - "An unexpected error occurred while executing the command:" << ' ' << std::endl << - message << std::endl; - } - + context.Reporter.Error() << + Resource::String::UnexpectedErrorExecutingCommand << ' ' << std::endl << + message << std::endl; return hre.code(); } catch (const Settings::GroupPolicyException& e) @@ -891,6 +879,12 @@ namespace AppInstaller::CLI context.Reporter.Error() << Resource::String::DisabledByGroupPolicy << ": "_liv << policy.PolicyName() << std::endl; return APPINSTALLER_CLI_ERROR_BLOCKED_BY_POLICY; } + catch (const Resource::MissingResourceFileException& e) + { + Logging::Telemetry().LogException(command->FullName(), "MissingResourceFileException", e.what()); + context.Reporter.Error() << GetUserPresentableMessage(e) << std::endl; + return APPINSTALLER_CLI_ERROR_MISSING_RESOURCE_FILE; + } catch (const std::exception& e) { Logging::Telemetry().LogException(command->FullName(), "std::exception", e.what()); diff --git a/src/AppInstallerCLICore/Resources.cpp b/src/AppInstallerCLICore/Resources.cpp index 3984ec8f7d..ebeaa80585 100644 --- a/src/AppInstallerCLICore/Resources.cpp +++ b/src/AppInstallerCLICore/Resources.cpp @@ -15,17 +15,24 @@ namespace AppInstaller::CLI::Resource return instance; } - Loader::Loader() + Loader::Loader() : m_wingetLoader(nullptr) { try { + // The default constructor of ResourceLoader throws a winrt::hresult_error exception + // when resource.pri is not found. ResourceLoader::GetForViewIndependentUse also throws + // a winrt::hresult_error but for reasons unknown it only gets catch when running on the + // debugger. Running without a debugger will result in a crash that not even adding a + // catch all fix. To provide a good error message we call the default constructor + // before calling GetForViewIndependentUse. + m_wingetLoader = winrt::Windows::ApplicationModel::Resources::ResourceLoader(); m_wingetLoader = winrt::Windows::ApplicationModel::Resources::ResourceLoader::GetForViewIndependentUse(L"winget"); } catch (const winrt::hresult_error& hre) { // This message cannot be localized. AICLI_LOG(CLI, Error, << "Failure loading resource file with error: " << hre.code()); - throw; + throw MissingResourceFileException(); } } diff --git a/src/AppInstallerCLICore/Resources.h b/src/AppInstallerCLICore/Resources.h index 3e783734de..e045657392 100644 --- a/src/AppInstallerCLICore/Resources.h +++ b/src/AppInstallerCLICore/Resources.h @@ -344,6 +344,13 @@ namespace AppInstaller::CLI::Resource }; Utility::LocIndView GetFixedString(FixedString fs); + + struct MissingResourceFileException : std::exception + { + MissingResourceFileException() {} + + const char* what() const noexcept override { return "The system cannot find resource.pri file."; } + }; } inline std::ostream& operator<<(std::ostream& out, AppInstaller::CLI::Resource::StringId si) diff --git a/src/AppInstallerCommonCore/Public/AppInstallerErrors.h b/src/AppInstallerCommonCore/Public/AppInstallerErrors.h index 4a8f5e7257..12978a5305 100644 --- a/src/AppInstallerCommonCore/Public/AppInstallerErrors.h +++ b/src/AppInstallerCommonCore/Public/AppInstallerErrors.h @@ -85,6 +85,7 @@ #define APPINSTALLER_CLI_ERROR_SOURCE_OPEN_FAILED ((HRESULT)0x8a150045) #define APPINSTALLER_CLI_ERROR_SOURCE_AGREEMENTS_NOT_ACCEPTED ((HRESULT)0x8a150046) #define APPINSTALLER_CLI_ERROR_CUSTOMHEADER_EXCEEDS_MAXLENGTH ((HRESULT)0x8a150047) +#define APPINSTALLER_CLI_ERROR_MISSING_RESOURCE_FILE ((HRESULT)0x8a150048) namespace AppInstaller From 5c7e4eed762cbc1f0537b24d13ae2b2a3bb30bf8 Mon Sep 17 00:00:00 2001 From: Ruben Guerrero Samaniego Date: Fri, 10 Sep 2021 17:00:11 -0700 Subject: [PATCH 5/5] even better message --- src/AppInstallerCLICore/Command.cpp | 4 ++-- src/AppInstallerCLICore/Resources.cpp | 7 ++++++- src/AppInstallerCLICore/Resources.h | 9 ++++++--- src/AppInstallerCommonCore/Errors.cpp | 4 +++- 4 files changed, 17 insertions(+), 7 deletions(-) diff --git a/src/AppInstallerCLICore/Command.cpp b/src/AppInstallerCLICore/Command.cpp index 38a53a73f5..fd7bc5b608 100644 --- a/src/AppInstallerCLICore/Command.cpp +++ b/src/AppInstallerCLICore/Command.cpp @@ -879,9 +879,9 @@ namespace AppInstaller::CLI context.Reporter.Error() << Resource::String::DisabledByGroupPolicy << ": "_liv << policy.PolicyName() << std::endl; return APPINSTALLER_CLI_ERROR_BLOCKED_BY_POLICY; } - catch (const Resource::MissingResourceFileException& e) + catch (const Resource::ResourceOpenException& e) { - Logging::Telemetry().LogException(command->FullName(), "MissingResourceFileException", e.what()); + Logging::Telemetry().LogException(command->FullName(), "ResourceOpenException", e.what()); context.Reporter.Error() << GetUserPresentableMessage(e) << std::endl; return APPINSTALLER_CLI_ERROR_MISSING_RESOURCE_FILE; } diff --git a/src/AppInstallerCLICore/Resources.cpp b/src/AppInstallerCLICore/Resources.cpp index ebeaa80585..1105dedbf9 100644 --- a/src/AppInstallerCLICore/Resources.cpp +++ b/src/AppInstallerCLICore/Resources.cpp @@ -32,7 +32,7 @@ namespace AppInstaller::CLI::Resource { // This message cannot be localized. AICLI_LOG(CLI, Error, << "Failure loading resource file with error: " << hre.code()); - throw MissingResourceFileException(); + throw ResourceOpenException(hre); } } @@ -51,4 +51,9 @@ namespace AppInstaller::CLI::Resource THROW_HR(E_UNEXPECTED); } + + ResourceOpenException::ResourceOpenException(const winrt::hresult_error& hre) + { + m_message = "Could not open the resource file: " + GetUserPresentableMessage(hre); + } } diff --git a/src/AppInstallerCLICore/Resources.h b/src/AppInstallerCLICore/Resources.h index e045657392..f49effaecf 100644 --- a/src/AppInstallerCLICore/Resources.h +++ b/src/AppInstallerCLICore/Resources.h @@ -345,11 +345,14 @@ namespace AppInstaller::CLI::Resource Utility::LocIndView GetFixedString(FixedString fs); - struct MissingResourceFileException : std::exception + struct ResourceOpenException : std::exception { - MissingResourceFileException() {} + ResourceOpenException(const winrt::hresult_error& hre); - const char* what() const noexcept override { return "The system cannot find resource.pri file."; } + const char* what() const noexcept override { return m_message.c_str(); } + + private: + std::string m_message; }; } diff --git a/src/AppInstallerCommonCore/Errors.cpp b/src/AppInstallerCommonCore/Errors.cpp index a05c1be5ee..978ac29b9b 100644 --- a/src/AppInstallerCommonCore/Errors.cpp +++ b/src/AppInstallerCommonCore/Errors.cpp @@ -201,7 +201,9 @@ namespace AppInstaller #ifndef WINGET_DISABLE_FOR_FUZZING std::string GetUserPresentableMessage(const winrt::hresult_error& hre) { - return Utility::ConvertToUTF8(hre.message()); + std::ostringstream strstr; + GetUserPresentableMessageForHR(strstr, hre.code()); + return strstr.str(); } #endif }