Throwing an exception through a synchronized shared generic method does not release the lock on Windows X86

[davidwrighton]

Description

Run the test application below on a Windows X86 build of the runtime

Reproduction Steps

using System.Runtime.CompilerServices;

namespace TestThrowExceptionAcrossStaticMethodOnCanonGenericType
{
    internal class Program
    {
        [MethodImpl(MethodImplOptions.NoInlining)]
        static void Throw()
        {
            throw new NotSupportedException();
        }

        class Test<T>
        {
            [MethodImpl(MethodImplOptions.Synchronized | MethodImplOptions.NoInlining)]
            public static void CallThrow()
            {
                Throw();
            }
        }

        static void ThrowAndCatchEHThroughSharedGenericSyncrhonizedMethod()
        {
            try
            {
                Test<object>.CallThrow();
            }
            catch (Exception ex)
            {
                Console.WriteLine(ex.ToString());
            }
        }

        static void Main(string[] args)
        {
            Thread thread = new Thread(ThrowAndCatchEHThroughSharedGenericSyncrhonizedMethod);
            thread.Start();
            thread.Join();
            ThrowAndCatchEHThroughSharedGenericSyncrhonizedMethod();
            Console.WriteLine("Passed!");
        }
    }
}

Expected behavior

Program should complete

Actual behavior

Program deadlocks

Regression?

Not a regression. Likely broken since .NET 2.0

Known Workarounds

Instead of implementing the method as a synchronized method, use a lock on the typeof the Type object of the method's containing type.

Configuration

Windows X86

Other information

No response

[dotnet-policy-service]

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

[davidwrighton]

This is a bug in StackFrameIterator::NextRaw where the special logic to unwind and unlock in synchronized regions unlocks the class associated with the MethodDesc that represents the code. This works unless the code is shared generic, in which case it is attempting to unlock the canonical type instance, which doesn't do anything (GetManagedClassObjectIfExists returns NULL). The correct fix is to detect the shared generic case, and use the generics context argument to resolve to the exact MethodTable.

[davidwrighton]

A probably better fix would be to remove the entire concept of the unwinder cleaning up synchronized regions, and convert the JIT to just inject a try/finally.

[EgorBo]

convert the JIT to just inject a try/finally.

hm.. I thought that JIT already does that for Synchronized 🤔 cc @dotnet/jit-contrib

UPD: Ah, yes, JIT doesn't do it for win-x86 (needs funclets)

[davidwrighton]

This is done. We converted X86 to use the funclet model, and so the special logic and handling for this is now no longer compiled into the product!