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

Exception filters have different behavior when invoked via reflection and the filter throws an exception #410

Closed
tannergooding opened this Issue Mar 7, 2015 · 6 comments

Comments

Projects
None yet
4 participants
@tannergooding
Member

tannergooding commented Mar 7, 2015

This is ported over from Roslyn Issue #1114.

When invoking a method with an exception filter where the conditional expression for the filter throws an exception, the thrown exception should be swallowed and the filter treated as if the conditional expression returned false.

However, when the method is invoked via reflection, the exception thrown by the conditional expression for the filter is not swallowed and is instead wrapped by a TargetInvocationException and rethrown.

The expected behavior should be for the exception to still be swallowed and the original exception wrapped by a TargetInvocationException and rethrown.

Console Application:

Imports System

Public Class Program

    Public Shared Sub Main()
        Console.WriteLine("TestExceptionFilter() Direct Invocation")
        Invoke(Sub() TestExceptionFilter())
        Console.WriteLine()

        Console.WriteLine("TestExceptionFilter() Invocation via Reflection")
        Invoke(Sub() GetType(Program).GetMethod("TestExceptionFilter").Invoke(Nothing, New Object() {}))
        Console.WriteLine()

#If DEBUG Then
        Console.WriteLine("Press any key to exit...")
        Console.ReadKey()
#End If

    End Sub

    Public Shared Sub Invoke(ByRef action As Action)
        Try
            action()
        Catch ex As Exception
            Console.WriteLine(ex)
        End Try
    End Sub

    Public Shared Sub TestExceptionFilter()
        Try
            Throw New Exception("Original Exception")
        Catch ex As Exception When MyCondition()
            Console.WriteLine(ex)
        End Try
    End Sub

    Public Shared Function MyCondition() As Boolean
        Throw New Exception("Condition Exception")
    End Function

End Class

Expected Output:
Direct Invocation catches the original exception.
Invocation via Reflection catches a TargetInvocationException that wraps the original exception.

TestExceptionFilter() Direct Invocation
System.Exception: Original Exception
at ConsoleApplication1.Program.TestExceptionFilter() in Program.vb:line 31
at ConsoleApplication1.Program._Lambda$__1() in Program.vb:line 7
at ConsoleApplication1.Program.Invoke(Action& action) in Program.vb:line 23

TestExceptionFilter() Invocation via Reflection
System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.Exception: Original Exception
at ConsoleApplication1.Program.TestExceptionFilter() in Program.vb:line 31
--- End of inner exception stack trace ---
at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor)
at System.Reflection.RuntimeMethodInfo.UnsafeInvokeInternal(Object obj, Object[] parameters, Object[] arguments)
at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters)
at ConsoleApplication1.Program._Lambda$__2() in Program.vb:line 11
at ConsoleApplication1.Program.Invoke(Action& action) in Program.vb:line 23

Press any key to exit...

Actual Output:
Direct Invocation catches the original exception.
Invocation via Reflection catches a TargetInvocationException that wrapping the condition exception.

TestExceptionFilter() Direct Invocation
System.Exception: Original Exception
at ConsoleApplication1.Program.TestExceptionFilter() in Program.vb:line 31
at ConsoleApplication1.Program._Lambda$__1() in Program.vb:line 7
at ConsoleApplication1.Program.Invoke(Action& action) in Program.vb:line 23

TestExceptionFilter() Invocation via Reflection
System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.Exception: Condition Exception
at ConsoleApplication1.Program.MyCondition() in Program.vb:line 38
at ConsoleApplication1.Program.TestExceptionFilter() in Program.vb:line 32
--- End of inner exception stack trace ---
at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor)
at System.Reflection.RuntimeMethodInfo.UnsafeInvokeInternal(Object obj, Object[] parameters, Object[] arguments)
at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters)
at ConsoleApplication1.Program._Lambda$__2() in Program.vb:line 11
at ConsoleApplication1.Program.Invoke(Action& action) in Program.vb:line 23

Press any key to exit...

@tannergooding

This comment has been minimized.

Show comment
Hide comment
@tannergooding

tannergooding Mar 12, 2015

Member

ping
Issue has not been looked at yet.

Member

tannergooding commented Mar 12, 2015

ping
Issue has not been looked at yet.

@tannergooding

This comment has been minimized.

Show comment
Hide comment
@tannergooding

tannergooding Mar 17, 2015

Member

@jkotas Could you see if someone can route/assign this as appropriate?

Member

tannergooding commented Mar 17, 2015

@jkotas Could you see if someone can route/assign this as appropriate?

@jkotas jkotas added the up-for-grabs label Mar 17, 2015

@jkotas

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas Mar 17, 2015

Member

@gkhanna79 Could you please take a look when you get a chance?

Member

jkotas commented Mar 17, 2015

@gkhanna79 Could you please take a look when you get a chance?

@sjwoodard

This comment has been minimized.

Show comment
Hide comment
@sjwoodard

sjwoodard May 1, 2015

Any update on this?

sjwoodard commented May 1, 2015

Any update on this?

@gkhanna79

This comment has been minimized.

Show comment
Hide comment
@gkhanna79

gkhanna79 Sep 10, 2015

Member

Sorry, I am just getting to this - I will take care of it.

Member

gkhanna79 commented Sep 10, 2015

Sorry, I am just getting to this - I will take care of it.

@gkhanna79

This comment has been minimized.

Show comment
Hide comment
@gkhanna79

gkhanna79 Sep 11, 2015

Member

This issue has been present in the VM for pretty much as long as I can remember. The problem happens when we have a nested exception (E2) that is caught within the VM (using EX_CATCH), we do not reset the managed thread state tracking the last thrown exception object to be in sync with the outer exception (E1) tracked by the topmost exception tracker (corresponding to the top-most active tracker). Thus, managed thread still says active exception is E2 even though it should be E1.

As a result, when the dispatch for E1 continues and is caught within the VM again (which is what happens for Reflection based invocation and in the failing repro for this bug), VM uses GET_THROWABLE macro to access the thrown object. Since that is pulled from managed thread object, we get E2 instead of E1.

While this is broken even when exception is caught in managed code, and not the VM, it happens to work since the exception object passed to the catch funclet is pulled from the active exception tracker and not the managed thread.

I have a fix for this and validated it addresses the repro in this bug. I will test it more and hope to check it in next week.

Member

gkhanna79 commented Sep 11, 2015

This issue has been present in the VM for pretty much as long as I can remember. The problem happens when we have a nested exception (E2) that is caught within the VM (using EX_CATCH), we do not reset the managed thread state tracking the last thrown exception object to be in sync with the outer exception (E1) tracked by the topmost exception tracker (corresponding to the top-most active tracker). Thus, managed thread still says active exception is E2 even though it should be E1.

As a result, when the dispatch for E1 continues and is caught within the VM again (which is what happens for Reflection based invocation and in the failing repro for this bug), VM uses GET_THROWABLE macro to access the thrown object. Since that is pulled from managed thread object, we get E2 instead of E1.

While this is broken even when exception is caught in managed code, and not the VM, it happens to work since the exception object passed to the catch funclet is pulled from the active exception tracker and not the managed thread.

I have a fix for this and validated it addresses the repro in this bug. I will test it more and hope to check it in next week.

dotnet-bot pushed a commit to dotnet-bot/coreclr that referenced this issue Sep 16, 2015

Fix for GH issue 410 - dotnet#410
Whenever a managed exception is thrown, the details about the thrown exception are also saved off the managed thread object (as LastThrownObject). The VM also has an exception tracker that tracks its dispatch across the managed frames and incase of nested exceptions, the trackers are collapsed correctly, when the nested exception is handled, and last thrown object is updated correctly. The VM works on the premise that the LastThrownObject is updated correctly.

Incase of this bug, a method (M1)is invoked via Reflection and has an exception E1. During exception dispatch for E1, an IL filter is invoked that, in turn, has an exception (E2) that remains unhandled. While this is swallowed by the VM (as expected), the LastThrownObject is not updated to reflect the active exception to be E1. Thus, when the dispatch for original exception E1 completes and no managed handler is found, the exception is caught by Reflection subsystem that extracts the thrown exception using the GET_THROWABLE macro that uses the LastThrownObject to determine the thrown exception. Since the LTO was not updated, it still reflects E2.

The fix is to update the managed exception state, if the filter has an unhandled exception, similar to how we do when a managed catch block successfully handles the exception. I have refactored the code to make the semantic cleaner.

[tfs-changeset: 1525835]

@gkhanna79 gkhanna79 closed this Sep 17, 2015

@gkhanna79 gkhanna79 removed their assignment Feb 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment