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.

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

GUI host fails to give unhandled exception message box #3594

Closed
ghost opened this issue May 14, 2019 · 29 comments
Closed

GUI host fails to give unhandled exception message box #3594

ghost opened this issue May 14, 2019 · 29 comments
Assignees
Milestone

Comments

@ghost
Copy link

ghost commented May 14, 2019

Steps to reproduce

  1. clone https://github.com/rogersachan/NetRadio
  2. pull the dotnetcore3 branch
  3. cd into NetRadio
  4. dotnet restore
  5. dotnet build
  6. click resulting executable or run it from command line

Expected behavior

it should be showing an unhandled exception messagebox from the GUI host with the following:

Unhandled Exception: System.InvalidCastException: Unable to cast object of type 'System.String' to type 'System.Drawing.Bitmap'.
   at NetRadio.Properties.Resources.get_edit() in ...\NetRadio\NetRadio\Properties\Resources.Designer.cs:line 109
   at NetRadio.frmMain.InitializeComponent() in ...\NetRadio\NetRadio\frmMain.Designer.cs:line 241
   at NetRadio.frmMain..ctor() in ...\NetRadio\NetRadio\frmMain.cs:line 43
   at NetRadio.Program.Main() in ...\NetRadio\NetRadio\Program.cs:line 25

thanks to @peterhuene

Actual behavior

Nothing happens

Environment data

C:\Users\sacha>dotnet --info
.NET Core SDK (reflecting any global.json):
 Version:   3.0.100-preview5-011568
 Commit:    b487ff10aa

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.18362
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\3.0.100-preview5-011568\

Host (useful for support):
  Version: 3.0.0-preview5-27626-15
  Commit:  61f30f5a23

.NET Core SDKs installed:
  3.0.100-preview5-011568 [C:\Program Files\dotnet\sdk]

.NET Core runtimes installed:
  Microsoft.AspNetCore.App 3.0.0-preview5-19227-01 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 3.0.0-preview5-27626-15 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 3.0.0-preview5-27626-15 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

related to dotnet/msbuild#2221

@peterhuene
Copy link

Note that the unhandled exception is currently expected because MSBuild for Core doesn't support non-string resources such as bitmaps yet (support is being worked on for a future 3.0 preview).

@dagood dagood marked this as a duplicate of dotnet/msbuild#2221 May 15, 2019
@dagood dagood closed this as completed May 15, 2019
@peterhuene
Copy link

@dagood this is not a duplicate of the MSBuild issue, rather that issue caused this issue (no error reported on unhandled exception) in the host. Please reopen.

@dagood
Copy link
Member

dagood commented May 15, 2019

I see, we looked over it in triage and didn't spot the Core-Setup work. Reopening.

@dagood dagood reopened this May 15, 2019
@dagood dagood marked this as not a duplicate of dotnet/msbuild#2221 May 15, 2019
@sdmaclea
Copy link
Contributor

Changed this to 3.0 because it negatively impacts developer experience

@sdmaclea sdmaclea self-assigned this Jun 21, 2019
@sdmaclea
Copy link
Contributor

sdmaclea commented Jun 21, 2019

/cc @vitek-karas @elinor-fung

I am thinking a way to solve this could be to create an anonymous pipe and connect it stderr. Then add a thread to read from the pipe and write to Vitek's message buffer.

ASP.NET IIS is using an approach like this. See aspnet/IISIntegration#549

The anonymous pipe has the disadvantage of requiring an additional thread which I am not too excited about. Seems heavy weight for what seems like a development scenario.

Maybe only enable it in debug builds?

@sdmaclea
Copy link
Contributor

sdmaclea commented Jun 21, 2019

As @vitek-karas mentioned in #25279, the stderr can be captured by running with a stderr redirect.

MyApp.exe 2>stderr.txt

Is documenting this sufficient? This is certainly the simplest and most robust solution.

@sdmaclea
Copy link
Contributor

The third option is to attempt to catch the unhandled exception and log it when using a WInExe.

@peterhuene
Copy link

peterhuene commented Jun 21, 2019

I think the winexe host should be consistent with the .NET Framework UX of logging the unhandled exception with the Windows Event Log, since users will probably check there first to see why the application didn't "activate".

@elinor-fung
Copy link
Member

I tried running the repro in this issue and eventvwr shows the error from .NET Runtime:

Event 1026, .NET Runtime

Application: NetRadio.exe
CoreCLR Version: 4.6.27622.75
.NET Core Version: 3.0.0-preview5-27626-15
Description: The process was terminated due to an unhandled exception.
Exception Info: System.InvalidCastException: Unable to cast object of type 'System.String' to type 'System.Drawing.Bitmap'.
   at NetRadio.Properties.Resources.get_edit() in D:\NetRadio\NetRadio\Properties\Resources.Designer.cs:line 109
   at NetRadio.frmMain.InitializeComponent() in D:\NetRadio\NetRadio\frmMain.Designer.cs:line 241
   at NetRadio.frmMain..ctor() in D:\NetRadio\NetRadio\frmMain.cs:line 43
   at NetRadio.Program.Main() in D:\NetRadio\NetRadio\Program.cs:line 25

@sdmaclea
Copy link
Contributor

Event log is also working for the dotnet/coreclr#25279 repro

@sdmaclea
Copy link
Contributor

Catching the unhandled exception would disable the event log, unless we go through extra hurdles.

Given this is triggered by an unhandled exception, I do not think the pipe solution would work. Process would likely end with error in the pipe.

Error is showing in the Windows Event Viewer.
Error is also available with stderr redirect.

I am beginning to think the best solution is to just document this.

@sdmaclea
Copy link
Contributor

It also appears that when the GUI bit is set, the resultant app does not set %ERRORLEVEL%. The result is unchanged from the previous command.

@sdmaclea sdmaclea removed their assignment Jun 21, 2019
@jkotas
Copy link
Member

jkotas commented Jun 21, 2019

It would be nice to be consistent and have the same plan for all errors. We started displaying dialog boxes for host errors: https://github.com/dotnet/core-setup/pull/6325/files#diff-d879441c77c53723780bea5613c6056dL319 . If we go with the Windows Event Viewer + stderr redirect plan for the unhandled exceptions, should we switch the host to the same plan?

It also appears that when the GUI bit is set, the resultant app does not set %ERRORLEVEL%

The Windows GUI apps are always launched in the background when you run them from command prompt. You have to go out of your way to wait for them to finish.

@sdmaclea
Copy link
Contributor

We started displaying dialog boxes for host errors
should we switch the host to the same plan?

The dialog boxes are only enabled for GUI apphost.
Do you mean remove the dialog box in all cases?

I guess it is also worth noting that the dotnet host also reports the error fine on stderr. So even a gui app will report its error on stderr if run with the dotnet host. dotnet myguiapp.dll instead of myguiapp.exe

@jkotas
Copy link
Member

jkotas commented Jun 22, 2019

Do you mean remove the dialog box in all cases?

Yes. I meant that either all fatal errors from .NET Core GUI apps should be dialog boxes; or none of them should be dialog boxes. I do not think it makes sense to have the dialog boxes for some, but not others.

dotnet myguiapp.dll instead of myguiapp.exe

We do not recommend to run GUI apps this way. It may not even work in some cases when the GUI parts depend on the manifest in the .exe.

@sdmaclea
Copy link
Contributor

Looks like the errors reported in hostpolicy are not using ETW, but only the dialog.
Looks like the errors reported in coreclr are not using ETW, but not the dialog.

Seems like ETW is preferred.

We may not have time to fix this for preview7.

@jkotas
Copy link
Member

jkotas commented Jun 22, 2019

ETW is tracing that you need to configure and turn on upfront. It is meant to be used for in-depth diagnostic, by more advanced users like developers.

EventLog is "always on" and much simpler. It is meant to be used for basic diagnostic, by less advanced users like sys admins. EventLog is built on top ETW as a dedicated channel. Events written to EventLog are available via ETW as well.

More details: https://social.msdn.microsoft.com/forums/en-US/a1aa1350-41a0-4490-9ae3-9b4520aeb9d4/faq-common-questions-for-etw-and-windows-event-log

EventLog is the appropriate mechanism for fatal errors.

.NET Framework logged fatal errors to EventLog. .NET Core 1.0 did not do it, but we have received feedback about it and added it back: https://github.com/dotnet/coreclr/issues/14037, https://github.com/dotnet/coreclr/issues/16735

@vitek-karas
Copy link
Member

I tend to agree that Event Logs is probably the better solution here. Making the runtime show dialogs is something I would like to avoid adding to the runtime itself. But doing this from the host is pretty tricky (given how runtime handles unhandles exceptions). Also if logging to EventLog matches .NET Framework behavior, that adds additional appeal to it.

@sdmaclea
Copy link
Contributor

sdmaclea commented Jun 24, 2019

EventLog is "always on" and much simpler.

@jkotas From an event provider perspective it is not immediately obvious to me how it is simpler.

From reading the docs it seems:

  • TraceLogging is simpler than Manifest based tracing (https://docs.microsoft.com/en-us/windows/desktop/ETW/about-event-tracing)
    • MOF & WPP look obsolete (older than ETW)
  • TraceLogging requires Win10
    • Win 7 support ends 1/2020, it may be premature to move to the TraceLogging API for CoreCLR unless we are prepared to drop Win7 & support for 3.0.
  • Manifest based tracing requires deploying an additional etw manifest dll. Affects installers. May be too late for .NET 3.0

@sdmaclea
Copy link
Contributor

sdmaclea commented Jun 24, 2019

Found this quote:

For C/C++ code (kernel-mode or user-mode), use the Windows 10 SDK and include the 
<TraceLoggingProvider.h> header. 
Note that while this header is new to the Windows 10 SDK, you can use this header in 
projects that target Vista or later.

here https://blogs.msdn.microsoft.com/dcook/2015/09/24/tracelogging-background/

@dotMorten
Copy link

EventLog is the appropriate mechanism for fatal errors.

I completely agree with that when trouble-shooting apps outside VS. However I do not agree with that in an F5-Debugging scenario. In that case I'd expect to be told why my app didn't run.

@jkotas
Copy link
Member

jkotas commented Jun 24, 2019

However I do not agree with that in an F5-Debugging scenario. In that case I'd expect to be told why my app didn't run.

Agree. It is the case today for unhandled exceptions.

@vitek-karas
Copy link
Member

@dotMorten The debugger should be notified about unhandled exception and show the appropriate UI for that.

@jkotas
Copy link
Member

jkotas commented Jun 24, 2019

TraceLogging is simpler than Manifest based tracing

EventLog is two orders of magnitudes simpler than either of these. It is like 10 lines of code to write a message into event log. This is what it looks like in CoreCLR: https://github.com/dotnet/coreclr/blob/030a3ea9b8dbeae89c90d34441d4d9a1cf4a7de6/src/utilcode/safewrap.cpp#L251

@vitek-karas
Copy link
Member

I think the reason Steve brought up TraceLogging is that the docs say that the API which CoreCLR uses is "old" and new code should use TraceLogging - see: https://docs.microsoft.com/en-us/windows/desktop/EventLog/about-event-logging (the first Note on the page). But if CoreCLR uses the old APIs I see no reason why the host should do anything different.

@sdmaclea sdmaclea self-assigned this Jun 25, 2019
@sdmaclea
Copy link
Contributor

Consensus...

I think the winexe host should be consistent with the .NET Framework UX of logging the unhandled exception with the Windows Event Log

It would be nice to be consistent and have the same plan for all errors.

Fixed in dotnet/core-setup#6921 by removing the message box in case of host errors and using Windows Event Log to report errors.

@GSPP
Copy link

GSPP commented Jun 27, 2019

The anonymous pipe has the disadvantage of requiring an additional thread which I am not too excited about. Seems heavy weight for what seems like a development scenario.

Using async IO there would be no need for a thread.

@danmoseley
Copy link
Member

EventLog is windows only. What is the story for Unix?

@jkotas
Copy link
Member

jkotas commented Jul 16, 2019

What is the story for Unix?

stderr

https://github.com/dotnet/coreclr/issues/15466 has some more discussion on this. We follow the common practices. Other runtimes do not seem to log crashes into system log on Linux either.

@msftgits msftgits transferred this issue from dotnet/core-setup Jan 30, 2020
@msftgits msftgits added this to the 3.0 milestone Jan 30, 2020
@msftbot msftbot bot locked as resolved and limited conversation to collaborators Dec 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests