Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removing Silent Failures from Bootstrap Autoinitialization #2315

Closed
wants to merge 1 commit into from
Closed

Removing Silent Failures from Bootstrap Autoinitialization #2315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions dev/WindowsAppRuntime_BootstrapDLL/MddBootstrapAutoInitializer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,12 @@ namespace Microsoft::Windows::ApplicationModel::DynamicDependency::Bootstrap
const HRESULT hr{ ::MddBootstrapInitialize(c_majorMinorVersion, c_versionTag, c_minVersion) };
if (FAILED(hr))
{
if (IsDebuggerPresent()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use "chameleon coding" - stick with original style/conventions, like brace placement

DebugBreak();
}
Comment on lines +32 to +34
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opinion: I travel around a lot with the debugger and little is more annoying than hitting breakpoints I didn't set. I think an interested party would set a bp on MddBootstrapInitialize anyway, making this of no value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reasoning here is that many developers are not even aware of MddBootstrapInitialize living in generated code. So knowing to set a BP on that in case of failure is a catch-22, and the problem we're trying to solve here - drawing the developer's attention to that generated call. Without the DebugBreak, we could pop the MessageBox unconditionally, but that's even more annoying to debug (break on all thread, find the one with a message pump, find the call frame ...).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized this is the consumer side, not the Microsoft impl side, so agree @Scottj1s!

else {
MessageBox(NULL,L"You need to run the Windows App Runtime Installer and its MSIX components", L"MddBootstrap", 0x00000010L);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is consistent with what I suggested during initial planning with @DrusTheAxe @zaryaf. This would align bootstrap-required app failures with common .NET runtime-required app failures.

I think the message should be reworded to use as much prior art as possible, so power users+ instinctively know what to do next.

Example:
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beware without a fusion manifest, the message box will be unthemed/ugly. It may or may not get dark mode treatment angering users.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed about rewording it, we can say something like: "This application requires installing the Windows App Runtime and its MSIX components."
At least, can we add a link to the page to install it?
https://docs.microsoft.com/en-us/windows/apps/windows-app-sdk/downloads

Beware without a fusion manifest, the message box will be unthemed/ugly. It may or may not get dark mode treatment angering users.

At this point, there is neither WinUI3 nor OS XAML. So we can only delegate on Win32 dialogs, so I hope that at some points all these dialogs support dark more too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@riverar yes, that was my opinion too - model this as closely as possible on the precedent set for the CLR and CRT message boxes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's replace "you need to" with "please verify" language. We actually don't have certainty that the WAR isn't installed here, and misdiagnosing can be even more frustrating to users.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The title should be "[application_exe] - This application could not be started" versus "MddBootstrap" which doesn't point to the app and has little meaning for end users.

}
exit(hr);
}
}
Expand Down
11 changes: 11 additions & 0 deletions dev/WindowsAppRuntime_BootstrapDLL/MddBootstrapAutoInitializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ namespace Microsoft.Windows.ApplicationModel.DynamicDependency.BootstrapCS
{
class AutoInitialize
{
[global::System.Runtime.InteropServices.DllImport("user32.dll", CharSet = global::System.Runtime.InteropServices.CharSet.Unicode, SetLastError = true)]
private static extern int MessageBox(global::System.IntPtr hWnd, string lpText, string lpCaption, uint uType);

[global::System.Runtime.CompilerServices.ModuleInitializer]
internal static void AccessWindowsAppSDK()
{
Expand All @@ -17,6 +20,14 @@ internal static void AccessWindowsAppSDK()
}
catch (global::System.Exception e)
{
if (global::System.Diagnostics.Debugger.IsAttached)
{
global::System.Diagnostics.Debugger.Break();
}
else
{
MessageBox(global::System.IntPtr.Zero, "You need to run the Windows App Runtime Installer and its MSIX components", "MddBootstrap", (uint)0x00000010L);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it either or?

}
global::System.Environment.FailFast(e.Message);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could implement the C++ goo in the bootstrap DLL and then none of this C# is needed.

I'd started working on a MddBootstrapInitialize2(). Let me steal parts of this PR over into it and share

}
Expand Down