EnC: Mono throws when member accessibility is changed

[tmat]

Description

Mono throws when member accessibility is changed during debugging

Reproduction Steps

Use Roslyn build from PR: dotnet/roslyn#75191 for repro.

1. Create Blazor WASM app
2. Replace @code block in Counter.razor with:

@code {
    private int currentCount = 0;

    public void IncrementCount()
    {
        currentCount++;
        var c = new C();
        //c.x = 1;
    }

    class C
    {
        //public
        int x;
    }
}

3. Place breakpoint to IncrementCount and F5.
4. Click "Increment" button and uncomment commented code after hitting the breakpoint.
5. Step

Expected behavior

Assignment to x is successfully executed.

Actual behavior

One of the following:
1)
The Blazor app crashes and disconnects:

19:29 41.06 BlazorApp26 (Web assembly): [Error] Applying updates to the application failed. The request sent to the browser failed: Web socket connection to the browser has been closed:  The request sent to the browser failed: Web socket connection to the browser has been closed: 
19:29 41.06 An unexpected error has occurred, any pending updates have been discarded.

2. VS crashes,
    
3. VS debugger shows exception in JavaScript:

Error: [MONO] * Assertion at /__w/1/s/src/mono/mono/component/hot_reload.c:2236, condition `<disabled>' not met
    at ht (https://localhost:7139/_framework/dotnet.runtime.js:3:12765)
    at Ll (https://localhost:7139/_framework/dotnet.runtime.js:3:176248)
    at https://localhost:7139/_framework/dotnet.native.wasm:wasm-function[119]:0xa308
    at https://localhost:7139/_framework/dotnet.native.wasm:wasm-function[634]:0x3f446
    at https://localhost:7139/_framework/dotnet.native.wasm:wasm-function[550]:0x3bd96
    at https://localhost:7139/_framework/dotnet.native.wasm:wasm-function[554]:0x3beb7
    at https://localhost:7139/_framework/dotnet.native.wasm:wasm-function[556]:0x3befa
    at https://localhost:7139/_framework/dotnet.native.wasm:wasm-function[555]:0x3becd
    at https://localhost:7139/_framework/dotnet.native.wasm:wasm-function[1552]:0x83413
    at https://localhost:7139/_framework/dotnet.native.wasm:wasm-function[2424]:0xbae9e

Regression?

No

Known Workarounds

No response

Configuration

No response

Other information

No response

[tmat]

@lambdageek ptal. Would be nice to get fixed for .NET 9, if not too difficult.

[lambdageek]

@tmat what's allowed? changing toward more visibility? or is public->private also legal? mono pretty intentionally assumes that there are no changes to any MONO_TABLE_FIELD entries (only additions to a class that is being defined in the current update are allowed):

runtime/src/mono/mono/component/hot_reload.c

Lines 2235 to 2237 in b5f5349

	case MONO_TABLE_FIELD: {
	g_assert (is_addition);
	g_assert (add_member_typedef);

[lambdageek]

I can't really tell without trying it, but this seems fairly straightforward (although I can't tell how risky it might be for .NET 9):
replace the assertion by an if; allow the table update if it's a modification; then trigger mono_field_resolve_type for the modified field (assuming the parent class has already been inited) and allow it to overwrite the field's MonoType with an updated one with updated attributes.

@tmat what happens if an SRE method was JITed with a public version of the field but now it's become private. What is supposed to tell the JIT to make a new version with an updated accessibility check?

[lambdageek]

attn @tommcdon

[tmat]

changing toward more visibility? or is public->private also legal?

Any visibility changes would be allowed.

[tmat]

@tmat what happens if an SRE method was JITed with a public version of the field but now it's become private. What is supposed to tell the JIT to make a new version with an updated accessibility check?

Not sure I understand the scenario. What does SRE mean? System.Reflection.Emit?
There shouldn't be a need for any extra accessibility checks or notifications. The JITed code should be able to access the member until the code is recompiled (the referring method is updated) regardless of whether or not the accessibility of the referred member changes after the JIT checks the visibility.

[lambdageek]

@tmat what happens if an SRE method was JITed with a public version of the field but now it's become private. What is supposed to tell the JIT to make a new version with an updated accessibility check?

Not sure I understand the scenario. What does SRE mean? System.Reflection.Emit? There shouldn't be a need for any extra accessibility checks or notifications. The JITed code should be able to access the member until the code is recompiled (the referring method is updated) regardless of whether or not the accessibility of the referred member changes after the JIT checks the visibility.

I'm thinking of a scenario like this:

using System.Reflection;
using System.Reflection.Emit;

public class C {
    /*private*/ public int X; /// EnC: change this
    public void Increment()
    {
        X++;
    }
}
public class Program
{
    public static void Main()
    {
        var c = new C();
        var f1 = GenerateGetter();
        while (true)
        {
            c.Increment();
            Foo(f1, c);   // Q: this should always succeed?
            var f2 = GenerateGetter();
            Foo(f2, c);   // Q: after edit, this should throw FieldAccessException?
            System.Threading.Thread.Sleep(1000);
        }
    }

    public static void Foo(Func<C,int> func, C c)
    {
        int i = func(c);
        Console.WriteLine ($"i = {i}");
    }

    public static Func<C,int> GenerateGetter()
    {
        var fi = typeof(C).GetField("X", BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.Public);

        var an = new AssemblyName("x1");
        var ab = AssemblyBuilder.DefineDynamicAssembly(an, AssemblyBuilderAccess.RunAndCollect);
        var modb = ab.DefineDynamicModule(an.FullName);
        var tb = modb.DefineType("GetterHolder", TypeAttributes.Public);
        var methodName = "GetX";
        var mb = tb.DefineMethod(methodName, MethodAttributes.Public | MethodAttributes.Static, typeof(int), [ typeof(C) ]);
        var ilg = mb.GetILGenerator();
        ilg.Emit(OpCodes.Ldarg_0);
        ilg.Emit(OpCodes.Ldfld, fi);
        ilg.Emit(OpCodes.Ret);
        
        var ti = tb.CreateType();
        var mi = ti.GetMethod (methodName, BindingFlags.Public | BindingFlags.Static);
        
        return (Func<C,int>)Delegate.CreateDelegate(typeof(Func<C,int>), mi);
    }
}

Suppose you run this app and then change C.X from public to private. Is the expected behavior that f1 will still be able to access X? but f2 will now begin throwing FieldAccessException ?

[tmat]

I think so, as long as f1 is jitted before the change is applied. The CLR behaves that way.

[lambdageek]

Thanks. In that case, I think the change I summarized in #108097 (comment) should be sufficient

[lewing]

Just so I'm clear, are any of the 3 things that might happen what is supposed to happen when EnC can't continue?

[tmat]

If Roslyn sends invalid update to the runtime the behavior is undefined. It's Roslyn's responsibility to block rude edits. That said, as much as possible a better failure than crashing VS or debugger would be certainly desirable.

Ideally, the runtime would send message to the debugger that EnC/Hot Reload failed and an error code.

[tmat]

However, I can see that it might be hard to do so if an issue is found in the middle of updating various data structures (i.e. rolling back partial updates).

[lambdageek]

@tmat Do we also need to support changing method/property/event/nested class accessibility? or is this just about fields?

[tmat]

@lambdageek All members and types.