Use [DebuggerDisableUserUnhandledExceptions]

[halter73]

This PR takes advantage of [DebuggerDisableUserUnhandledExceptions] which was recently added to the runtime. dotnet/runtime#103105

In .NET 9, Visual Studio is adding support for catching async user-unhandled exceptions which will be enabled by default. This feature has existed for a long time for synchronous methods, but not for async methods. There are several exceptions in ASP.NET Core that propagate through user code but are expected to be handled by framework code. One example is the NavigationException used to force redirects when calling NavigationManager.NavigateTo() during static Blazor rendering.

This covers two cases:

• NavigationExceptions that are properly handled by Blazor and turned into a redirect.
• Exceptions handled by a custom filter in DeveloperExceptionPageMiddleware like DatabaseDeveloperPageExceptionFilter. I think this one is a little more questionable because there could be a custom filter registered that "handles" exceptions that still should be considered unhandled, but I think it's okay to slightly over suppress to begin with considering that the debugger would never break user-unhandled exceptions before .NET 9.

Fixes #57085

@martincostello FYI

[halter73]

@gregg-miskelly Is this necessary given that the method rethrows?

[gregg-miskelly]

That is a good question. The VS Debugger's current implementation is that we will wind up eventually breaking as long as some other part of non-user code catches that exception. I think that behavior sounds right to me, but I could be convinced in either direction. Do you have an opinion?

CC @asundheim

[asundheim]

It's probably for the best if we break as close to the original user-code throw as possible, in case the HttpContext is altered and the user wants to inspect it in as close to a state as it was when they threw? (in which case, maybe this should be the first line of the catch?)

To the question, it looks like we actually toggle the notifications for the exception off once we get CatchHandlerFound in a non-user frame, regardless if the frame has the Disable attribute, maybe that should be changed on our end? My original thought was that if one frame says we shouldn't break, then we shouldn't make every single frame above it add the attribute if it rethrew through a couple of frames before landing where it needed to be caught, but it looks like that's not the case?

If we don't change it, then this is necessary, otherwise, it becomes this method's callers problem, but we'll not get a chance to break until the caller catches it (implicitly or otherwise)

[halter73]

I'll move this up to the start of the catch (Exception ex) block since it sounds like it doesn't hurt regardless of how we decide the debugger should handle rethrows. I like that this would avoid mutating the HttpContext before the debugger breaks as you point out.

This would only be problematic if the debugger stopped both for the Debugger.BreakForUserUnhandledException(ex) and later after the rethrow, but it sounds like that would never happen.

My original thought was that if one frame says we shouldn't break, then we shouldn't make every single frame above it add the attribute if it rethrew through a couple of frames before landing where it needed to be caught, but it looks like that's not the case?

My intuition was that [DebuggerDisableUserUnhandledExceptions] would only affect exceptions that were caught and swallowed by the attributed method. I think the method either not catching an exception or rethrowing it is a pretty clear indication that the exception should still be considered "unhandled".

If there's an outer method that is catching and handling these rethrown exceptions, I would expect to be required to use [DebuggerDisableUserUnhandledExceptions] on that method too. Or more likely, I would only attribute the outermost method call that does the catching.

[halter73]

@gregg-miskelly I'm guessing that calling BreakForUserUnhandledException multiple times in the same invocation is fine, but I want to double check this makes sense.

[gregg-miskelly]

Doing so would wind up breaking more than once, so I think you don't want to do that

[asundheim]

We will definitely break here and show the provided exception, but the debugging experience will be a bit weird since:

1. I don't think there will be any user-code frames on the exception stack (or the real stack), when I tried it out, you just get a bunch of non-user code frames in the debugger. We also don't have a link between ex2 and the async user frame that threw, so we won't show any [Exception] frames that will give them a chance to inspect the program state or show the source frames where they initially threw when we stop here.
2. Is the exception caught here related to the user-code exception, besides the fact that we're in the exception page middleware? I'm not sure how likely it is that a developer would care about this particular exception, seeing as they didn't throw it.

[halter73]

Doing so would wind up breaking more than once, so I think you don't want to do that

Yes, but my assumption is that the debugger would only break if the exception propagated through user code. In this case, that would mean the exception was thrown from a user-defined exception filter or a filter that called user-defined code. I think the weirdest thing about the logic in the current PR is that the debugger would break out of order with causality seemingly reversed.

1. The original user-unhandled exception gets processed by the user-defined exception filter which then throws a secondary exception. The debugger hasn't stopped for the original user-unhandled exception yet because we were still waiting to see if any exception filters handle the exception.
2. We then call Debugger.BreakForUserUnhandledException(ex2) to break for the secondary exception thrown by the user-defined exception filter.
3. Then right after that, we call Debugger.BreakForUserUnhandledException(ex) with the original exception the triggered the exception filter.

I think this is pretty bad, and I should not have even opened the PR with this behavior. I think any of the following are better options.

• Option A: Never call BreakForUserUnhandledException for exceptions thrown from an exception filter since user-defined exception filters are pretty uncommon to begin with and handling this scenario is complicated.
• Option B: Don't use [DebuggerDisableUserUnhandledExceptions] in the developer exception page middleware at all and live with the debugger breaking before the DatabaseDeveloperPageExceptionFilter gets to run and offer the option to run the database migration by default.
• Option C: Once we see an exception filter has thrown an uncaught exception, call BreakForUserUnhandledException for the original exception first then the secondary exception from the exception filter second immediately afterwards.

Do either of you have a preference for which option we should take? Or know of a better option?

I don't think there will be any user-code frames on the exception stack (or the real stack), when I tried it out, you just get a bunch of non-user code frames in the debugger. We also don't have a link between ex2 and the async user frame that threw

Maybe this is the disconnect. My impression is that Debugger.BreakForUserUnhandledException will only do anything if both of the following conditions are met:

1. You have "Just my code" enabled and you have not unchecked "Break when this exception type is user-unhandled"
   
   or configured the "Additional Actions" in the exception settings to "Continue when unhandled in user code"
   https://learn.microsoft.com/en-us/visualstudio/debugger/managing-exceptions-with-the-debugger?view=vs-2022#BKMK_UserUnhandled

   P.S. the discoverability of disabling this for all exception types just disabling "Just my code" should probably be improved. There's no way I would have considered adding the "Additional Actions" column to exception settings without reading the docs. Can we just have it on by default? There aren't many columns to begin with. Also, it would be nice if I could enable breaking on user-unhandled exceptions even if "Just my code" is disabled.

2. The exception actually propagates through user code. If there are no user-code frames on the exception stack, I would expect Debugger.BreakForUserUnhandledException to never do anything regardless of how VS was configured because the user never had the opportunity to handle the exception.

Is the exception caught here related to the user-code exception, besides the fact that we're in the exception page middleware?

We don't know if the secondary exception relates to the original exception. It certainly could be related if something about the original exception triggered different behavior in a user-defined filter.

I'm not sure how likely it is that a developer would care about this particular exception, seeing as they didn't throw it.

The developer exception page middleware has no good way of knowing if either exception was thrown from user code. I thought it would be up to the debugger to figure this out. Even the original exceptions from inner middleware that the developer exception page middleware handles may not come from or propagate through user code. The same goes for any secondary exceptions thrown by exception filters.

[gregg-miskelly]

Do either of you have a preference for which option we should take? Or know of a better option?

I don't have a strong opinion. In option 'A', are you still waiting to find out if there is an exception filter that handles the scenario? If so, that sounds reasonable to me. I certainly don't think we need to go out of the way to support breaking when there is a user-defined exception filter. I definitely agree with you that I think breaking in the "wrong" order is more confusing than only breaking for the triggering exception.

My impression is that Debugger.BreakForUserUnhandledException will only do anything if both of the following conditions are met...

The behavior that you are expecting matches what I think the behavior should be as well.

[halter73]

In option 'A', are you still waiting to find out if there is an exception filter that handles the scenario?

We know that an exception filter must not have gracefully handled the scenario since the filter pipeline threw an exception. We know for sure that there are two unhandled exceptions at that point.

I went with option 'A' even though option 'C' is arguably more correct, because option 'C' would be about 5 more lines and I really doubt many people are writing custom IDeveloperPageExceptionFilter implementations, but I left a comment saying option 'C' might be sensible in case it comes back up.

I could be convinced to do option 'C' right now though if you think it's the best course of action.

[javiercn]

@halter73 looks good to me from the Blazor perspective.

Should we also look at the ErrorBoundary component? That catches unhandled exceptions and displays an element on the UI.

[halter73]

Should we also look at the ErrorBoundary component? That catches unhandled exceptions and displays an element on the UI.

I think that a debugger configured to stop on user unhandled exceptions should still stop for exceptions handled by a Blazor ErrorBounrdary. I'm also not making changes to the ExceptionHandlerMiddleware which would be the most obvious analogue. I think most customers would want to the debugger to stop for exceptions handled by the ExceptionHandlerMiddleware, because the handlers usually only do stuff like log the error and select an error page rather than properly handle an expected exception. I see displaying default or even custom ErrorContent similarly.

I did make changes to the DeveloperExceptionPageMiddleware pretty much exclusively to prevent the debugger from stopping when there's a pending database migration that is fully handled by clicking a link produced by the DatabaseDeveloperPageExceptionFilter. I figured that if a filter prevented the DeveloperExceptionPageMiddleware from showing the default page, the exception is something expected in development. But this is the part of this change I'm most unsure about. I still think it's the right thing to do, but it's less clear cut than preventing the debugger stopping when Blazor throws a NavigationException.