Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,9 @@ internal static unsafe void OnFirstChanceException(Exception* pException, Except
{
OnFirstChanceException(*pException, AppDomain.CurrentDomain);
}
catch (Exception ex)
catch
{
*pOutException = ex;
// The VM does not expect exceptions to propagate out of this callback
}
}

Expand Down
37 changes: 30 additions & 7 deletions src/libraries/System.Private.CoreLib/src/System/AppContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -130,21 +130,44 @@ internal static void OnUnhandledException(object e)
}
}

[ThreadStatic]
private static bool t_deliveringFirstChanceNotification;

private static void OnFirstChanceException(Exception e, object? sender)
{
if (FirstChanceException is EventHandler<FirstChanceExceptionEventArgs> handlers)
{
FirstChanceExceptionEventArgs args = new(e);
foreach (EventHandler<FirstChanceExceptionEventArgs> handler in Delegate.EnumerateInvocationList(handlers))
// Guard against reentrancy. Allocating the event args below or running a
// handler may itself throw (e.g. OutOfMemoryException in a low-memory
// situation). That exception would trigger another first-chance
// notification on this same thread, allocate again, throw again, and
// recurse until the stack overflows. Skip nested notifications to break
// the recursion.
if (t_deliveringFirstChanceNotification)
{
try
{
handler(sender, args);
}
catch
return;
}

t_deliveringFirstChanceNotification = true;
try
{
FirstChanceExceptionEventArgs args = new(e);

foreach (EventHandler<FirstChanceExceptionEventArgs> handler in Delegate.EnumerateInvocationList(handlers))
Comment thread
VSadov marked this conversation as resolved.
{
try
{
handler(sender, args);
}
catch
{
}
}
}
finally
{
t_deliveringFirstChanceNotification = false;
}
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can change https://github.com/dotnet/runtime/blob/main/src/coreclr/System.Private.CoreLib/src/System/AppContext.CoreCLR.cs#L58 to "// The VM does not expect exceptions to propagate out of this callback" (same as OnUnhandledException) to reduce unnecessary code that executes when something goes wrong recursively

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I assume you are suggesting a failfast at this point, not just a comment?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am suggesting comment. The VM wraps this in another try/catch that swallows all exceptions -

EX_TRY
{
UnmanagedCallersOnlyCaller deliverNotification(METHOD__APPCONTEXT__ON_FIRST_CHANCE_EXCEPTION);
deliverNotification.InvokeThrowing(&oThrowable);
}
EX_CATCH
{
}
EX_END_CATCH
- so we can skip the extra work and swallow the exceptions here.

It matches UnhandledException notification.


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,34 @@ public void FirstChanceException_Called()
}).Dispose();
}

[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
[SkipOnMono("Mono does not suppress reentrant first chance exception notifications")]
public void FirstChanceException_HandlerThrows_DoesNotRecurse()
{
// A handler that throws must not cause the runtime to recursively deliver
// first-chance notifications for the exceptions it throws, which would
// otherwise recurse until the stack overflows. The handler should be
// invoked exactly once for the original exception.
RemoteExecutor.Invoke(() => {
int count = 0;
EventHandler<FirstChanceExceptionEventArgs> handler = (sender, e) =>
{
count++;
throw new FirstChanceTestException("from handler");
};
Comment on lines +208 to +212
AppDomain.CurrentDomain.FirstChanceException += handler;
try
{
throw new FirstChanceTestException("outer");
}
catch
{
}
AppDomain.CurrentDomain.FirstChanceException -= handler;
Assert.Equal(1, count);
}).Dispose();
}

class FirstChanceTestException : Exception
{
public FirstChanceTestException(string message) : base(message)
Expand Down
Loading