Fixes for a few crashes, change DO timeout to 60 seconds default, upd…

…ate version that I forgot (microsoft#973) Fixes a few crashes that were happening, increase the default DO timeout for no progress to 60 seconds, and bump the minor version for the previous command releases.
DHenthorn · May 20, 2021 · 2e3fc15 · 2e3fc15
1 parent fc20f32
commit 2e3fc15
Show file tree

Hide file tree

Showing 7 changed files with 41 additions and 16 deletions.
diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp
@@ -461,10 +461,13 @@ namespace AppInstaller::CLI::Workflow
             for (const auto& entry : arpSource->Search({}).Matches)
             {
                 auto installed = entry.Package->GetInstalledVersion();
-                entries.emplace_back(std::make_tuple(
-                    entry.Package->GetProperty(PackageProperty::Id),
-                    installed->GetProperty(PackageVersionProperty::Version),
-                    installed->GetProperty(PackageVersionProperty::Channel)));
+                if (installed)
+                {
+                    entries.emplace_back(std::make_tuple(
+                        entry.Package->GetProperty(PackageProperty::Id),
+                        installed->GetProperty(PackageVersionProperty::Version),
+                        installed->GetProperty(PackageVersionProperty::Channel)));
+                }
             }
 
             std::sort(entries.begin(), entries.end());
@@ -492,15 +495,19 @@ namespace AppInstaller::CLI::Workflow
             for (auto& entry : arpSource->Search({}).Matches)
             {
                 auto installed = entry.Package->GetInstalledVersion();
-                auto entryKey = std::make_tuple(
-                    entry.Package->GetProperty(PackageProperty::Id),
-                    installed->GetProperty(PackageVersionProperty::Version),
-                    installed->GetProperty(PackageVersionProperty::Channel));
 
-                auto itr = std::lower_bound(entries.begin(), entries.end(), entryKey);
-                if (itr == entries.end() || *itr != entryKey)
+                if (installed)
                 {
-                    changes.emplace_back(std::move(entry));
+                    auto entryKey = std::make_tuple(
+                        entry.Package->GetProperty(PackageProperty::Id),
+                        installed->GetProperty(PackageVersionProperty::Version),
+                        installed->GetProperty(PackageVersionProperty::Channel));
+
+                    auto itr = std::lower_bound(entries.begin(), entries.end(), entryKey);
+                    if (itr == entries.end() || *itr != entryKey)
+                    {
+                        changes.emplace_back(std::move(entry));
+                    }
                 }
             }
 

diff --git a/src/AppInstallerCLITests/main.cpp b/src/AppInstallerCLITests/main.cpp
@@ -9,6 +9,7 @@
 
 #include <Public/AppInstallerLogging.h>
 #include <Public/AppInstallerTelemetry.h>
+#include <Telemetry/TraceLogging.h>
 
 #include "TestCommon.h"
 #include "TestHooks.h"
@@ -122,6 +123,9 @@ int main(int argc, char** argv)
     Logging::Log().SetLevel(Logging::Level::Verbose);
     Logging::EnableWilFailureTelemetry();
 
+    // Forcibly enable event writing to catch bugs in that code
+    g_IsTelemetryProviderEnabled = true;
+
     // Force all tests to run against settings inside this container.
     // This prevents test runs from trashing the users actual settings.
     Runtime::TestHook_SetPathOverride(Runtime::PathName::LocalState, Runtime::GetPathTo(Runtime::PathName::LocalState) / "Tests");

diff --git a/src/AppInstallerCommonCore/AppInstallerTelemetry.cpp b/src/AppInstallerCommonCore/AppInstallerTelemetry.cpp
@@ -95,13 +95,15 @@ namespace AppInstaller::Logging
     {
         if (IsTelemetryEnabled())
         {
+            auto anonMessage = AnonymizeString(failure.pszMessage);
+
             TraceLoggingWriteActivity(g_hTelemetryProvider,
                 "FailureInfo",
                 GetActivityId(),
                 nullptr,
                 TraceLoggingUInt32(s_subExecutionId, "SubExecutionId"),
                 TraceLoggingHResult(failure.hr, "HResult"),
-                AICLI_TraceLoggingWStringView(AnonymizeString(failure.pszMessage), "Message"),
+                AICLI_TraceLoggingWStringView(anonMessage, "Message"),
                 TraceLoggingString(failure.pszModule, "Module"),
                 TraceLoggingUInt32(failure.threadId, "ThreadId"),
                 TraceLoggingUInt32(static_cast<uint32_t>(failure.type), "Type"),
@@ -206,14 +208,16 @@ namespace AppInstaller::Logging
     {
         if (IsTelemetryEnabled())
         {
+            auto anonMessage = AnonymizeString(Utility::ConvertToUTF16(message));
+
             TraceLoggingWriteActivity(g_hTelemetryProvider,
                 "Exception",
                 GetActivityId(),
                 nullptr,
                 TraceLoggingUInt32(s_subExecutionId, "SubExecutionId"),
                 AICLI_TraceLoggingStringView(commandName, "Command"),
                 AICLI_TraceLoggingStringView(type, "Type"),
-                AICLI_TraceLoggingWStringView(AnonymizeString(Utility::ConvertToUTF16(message)), "Message"),
+                AICLI_TraceLoggingWStringView(anonMessage, "Message"),
                 TraceLoggingUInt32(s_executionStage, "ExecutionStage"),
                 TelemetryPrivacyDataTag(PDT_ProductAndServicePerformance),
                 TraceLoggingKeyword(MICROSOFT_KEYWORD_CRITICAL_DATA));
@@ -515,6 +519,11 @@ namespace AppInstaller::Logging
         return g_IsTelemetryProviderEnabled && m_isSettingEnabled && m_isRuntimeEnabled;
     }
 
+    std::wstring TelemetryTraceLogger::AnonymizeString(const wchar_t* input) const noexcept
+    {
+        return input ? AnonymizeString(std::wstring_view{ input }) : std::wstring{};
+    }
+
     std::wstring TelemetryTraceLogger::AnonymizeString(std::wstring_view input) const noexcept try
     {
         return Utility::ReplaceWhileCopying(input, m_userProfile, s_UserProfileReplacement);

diff --git a/src/AppInstallerCommonCore/Public/AppInstallerTelemetry.h b/src/AppInstallerCommonCore/Public/AppInstallerTelemetry.h
@@ -121,6 +121,7 @@ namespace AppInstaller::Logging
 
         // Used to anonymize a string to the best of our ability.
         // Should primarily be used on failure messages or paths if needed.
+        std::wstring AnonymizeString(const wchar_t* input) const noexcept;
         std::wstring AnonymizeString(std::wstring_view input) const noexcept;
 
         bool m_isSettingEnabled = true;

diff --git a/src/AppInstallerCommonCore/Public/winget/UserSettings.h b/src/AppInstallerCommonCore/Public/winget/UserSettings.h
@@ -117,7 +117,7 @@ namespace AppInstaller::Settings
         SETTINGMAPPING_SPECIALIZATION(Setting::InstallScopePreference, std::string, ScopePreference, ScopePreference::User, ".installBehavior.preferences.scope"sv);
         SETTINGMAPPING_SPECIALIZATION(Setting::InstallScopeRequirement, std::string, ScopePreference, ScopePreference::None, ".installBehavior.requirements.scope"sv);
         SETTINGMAPPING_SPECIALIZATION(Setting::NetworkDownloader, std::string, InstallerDownloader, InstallerDownloader::Default, ".network.downloader"sv);
-        SETTINGMAPPING_SPECIALIZATION(Setting::NetworkDOProgressTimeoutInSeconds, uint32_t, std::chrono::seconds, 20s, ".network.doProgressTimeoutInSeconds"sv);
+        SETTINGMAPPING_SPECIALIZATION(Setting::NetworkDOProgressTimeoutInSeconds, uint32_t, std::chrono::seconds, 60s, ".network.doProgressTimeoutInSeconds"sv);
         SETTINGMAPPING_SPECIALIZATION(Setting::InstallLocalePreference, std::vector<std::string>, std::vector<std::string>, {}, ".installBehavior.preferences.locale"sv);
         SETTINGMAPPING_SPECIALIZATION(Setting::InstallLocaleRequirement, std::vector<std::string>, std::vector<std::string>, {}, ".installBehavior.requirements.locale"sv);
 

diff --git a/src/AppInstallerRepositoryCore/CompositeSource.cpp b/src/AppInstallerRepositoryCore/CompositeSource.cpp
@@ -39,7 +39,11 @@ namespace AppInstaller::Repository
                 // Grab the installed version's channel to allow for filtering in calls to get available info.
                 if (m_installedPackage)
                 {
-                    m_installedChannel = m_installedPackage->GetInstalledVersion()->GetProperty(PackageVersionProperty::Channel);
+                    auto installedVersion = m_installedPackage->GetInstalledVersion();
+                    if (installedVersion)
+                    {
+                        m_installedChannel = installedVersion->GetProperty(PackageVersionProperty::Channel);
+                    }
                 }
             }
 

diff --git a/src/binver/binver/version.h b/src/binver/binver/version.h
@@ -2,7 +2,7 @@
 #define STRINGIZE(s) STRINGIZE2(s)
 
 #define VERSION_MAJOR               0
-#define VERSION_MINOR               3
+#define VERSION_MINOR               4
 #define VERSION_BUILD               0
 #define VERSION_REVISION            0