Interop: a JavaScript function converts to the delegate type the host asked for

Jint.Tests.PublicInterface/HostDelegateConversionTests.cs

@@ -0,0 +1,95 @@
+#nullable enable
+
+namespace Jint.Tests.PublicInterface;
+
+/// <summary>
+/// Pins that the CLR delegate a JavaScript function converts to is the one built for the target type the
+/// host asked for, in every engine that asks (sebastienros/jint#3434).
+/// </summary>
+/// <remarks>
+/// <para>
+/// The conversion is memoized so <c>host.on(f)</c> and <c>host.off(f)</c> hand the host the same
+/// <see cref="Delegate"/> instance, which is the identity <c>-=</c> needs. The memo used to be keyed on the
+/// function alone, and one level down on that function's AST node — neither of which carries the delegate
+/// type the compiled binder was built for. The AST node is process-wide state and
+/// <see cref="Engine.PrepareScript"/> is documented as shareable across engines, so whichever engine
+/// evaluated a shared preparation first decided the target type for every engine after it, and the
+/// reflection invoke that followed rejected the delegate it was handed with
+/// <c>ArgumentException: Object of type 'Notify' cannot be converted to type 'Transform'</c>.
+/// </para>
+/// <para>
+/// This is the embedder-visible half of it: two engines, different host types, one cached
+/// <c>Prepared&lt;Script&gt;</c> — the shape the README recommends for production.
+/// </para>
+/// </remarks>
+public class HostDelegateConversionTests
+{
+    public delegate void Notify(int value);
+
+    public delegate string Transform(string value);
+
+    public sealed class NotifyHost
+    {
+        public string Call(Notify f)
+        {
+            f(1);
+            return "notify";
+        }
+    }
+
+    public sealed class TransformHost
+    {
+        public string Call(Transform f) => "transform:" + f("x");
+    }
+
+    private const string Source = "host.call(function (x) { return String(x); });";
+
+    [Test]
+    public void TwoEnginesSharingOnePreparedScript()
+    {
+        var prepared = Engine.PrepareScript(Source);
+
+        var first = new Engine();
+        first.SetValue("host", new NotifyHost());
+        first.Evaluate(prepared).AsString().Should().Be("notify");
+
+        var second = new Engine();
+        second.SetValue("host", new TransformHost());
+        second.Evaluate(prepared).AsString().Should().Be("transform:x");
+    }
+
+    [Test]
+    public void TwoEnginesSharingOnePreparedScriptInTheOtherOrder()
+    {
+        var prepared = Engine.PrepareScript(Source);
+
+        var first = new Engine();
+        first.SetValue("host", new TransformHost());
+        first.Evaluate(prepared).AsString().Should().Be("transform:x");
+
+        var second = new Engine();
+        second.SetValue("host", new NotifyHost());
+        second.Evaluate(prepared).AsString().Should().Be("notify");
+    }
+
+    /// <summary>
+    /// The same preparation alternating between the two engines: each engine keeps answering for its own
+    /// host type however many times the other one has run in between.
+    /// </summary>
+    [Test]
+    public void AlternatingEnginesOnOnePreparedScript()
+    {
+        var prepared = Engine.PrepareScript(Source);
+
+        var notify = new Engine();
+        notify.SetValue("host", new NotifyHost());
+        var transform = new Engine();
+        transform.SetValue("host", new TransformHost());
+
+        for (var i = 0; i < 5; i++)
+        {
+            notify.Evaluate(prepared).AsString().Should().Be("notify");
+            transform.Evaluate(prepared).AsString().Should().Be("transform:x");
+        }
+    }
+}

Jint.Tests/Runtime/DelegateConversionTargetTypeTests.cs

@@ -0,0 +1,119 @@
+#nullable enable
+
+namespace Jint.Tests.Runtime;
+
+/// <summary>
+/// A JavaScript function converted to a CLR delegate is cached so that <c>host.on(f)</c> and
+/// <c>host.off(f)</c> hand the host the same <see cref="Delegate"/> instance. These cover the other half of
+/// that contract: the cached delegate is the one built for the target type being asked for, not for whichever
+/// target type asked first (https://github.com/sebastienros/jint/issues/3434).
+/// </summary>
+public class DelegateConversionTargetTypeTests
+{
+    public delegate void Notify(int value);
+
+    public delegate string Transform(string value);
+
+    public sealed class NotifyHost
+    {
+        public string Call(Notify f)
+        {
+            f(1);
+            return "notify";
+        }
+    }
+
+    public sealed class TransformHost
+    {
+        public string Call(Transform f) => "transform:" + f("x");
+    }
+
+    public sealed class BothHost
+    {
+        public Notify? LastNotify { get; private set; }
+
+        public Transform? LastTransform { get; private set; }
+
+        public string A(Notify f)
+        {
+            LastNotify = f;
+            f(1);
+            return "notify";
+        }
+
+        public string B(Transform f)
+        {
+            LastTransform = f;
+            return "transform:" + f("x");
+        }
+    }
+
+    /// <summary>Control: each host alone works, and always did.</summary>
+    [Test]
+    public void ControlEachHostAlone()
+    {
+        var one = new Engine();
+        one.SetValue("host", new NotifyHost());
+        one.Evaluate("host.call(function (x) { return String(x); })").AsString().Should().Be("notify");
+
+        var two = new Engine();
+        two.SetValue("host", new TransformHost());
+        two.Evaluate("host.call(function (x) { return String(x); })").AsString().Should().Be("transform:x");
+    }
+
+    /// <summary>The bound-delegate cache: one function instance, two delegate types, one engine.</summary>
+    [Test]
+    public void OneFunctionInstanceTwoDelegateTypes()
+    {
+        var engine = new Engine();
+        engine.SetValue("host", new BothHost());
+
+        var result = engine.Evaluate("""
+            var f = function (x) { return String(x); };
+            host.a(f) + '|' + host.b(f);
+            """).AsString();
+
+        result.Should().Be("notify|transform:x");
+    }
+
+    /// <summary>The binder cache: two instances of one AST node, two delegate types, one engine.</summary>
+    [Test]
+    public void TwoInstancesOfOneAstNodeTwoDelegateTypes()
+    {
+        var engine = new Engine();
+        engine.SetValue("host", new BothHost());
+
+        var result = engine.Evaluate("""
+            function make() { return function (x) { return String(x); }; }
+            host.a(make()) + '|' + host.b(make());
+            """).AsString();
+
+        result.Should().Be("notify|transform:x");
+    }
+
+    /// <summary>
+    /// The reason the cache exists: one function instance converted twice to the same delegate type is the
+    /// same <see cref="Delegate"/> instance, which is the identity <c>-=</c> needs. Per target type now,
+    /// rather than for the first target type only.
+    /// </summary>
+    [Test]
+    public void SameFunctionAndSameTargetTypeIsTheSameDelegateInstance()
+    {
+        var host = new BothHost();
+        var engine = new Engine();
+        engine.SetValue("host", host);
+
+        engine.Execute("var f = function (x) { return String(x); };");
+
+        engine.Execute("host.a(f);");
+        var firstNotify = host.LastNotify;
+        engine.Execute("host.b(f);");
+        var firstTransform = host.LastTransform;
+
+        engine.Execute("host.a(f);");
+        engine.Execute("host.b(f);");
+
+        host.LastNotify.Should().BeSameAs(firstNotify);
+        host.LastTransform.Should().BeSameAs(firstTransform);
+    }
+}

Jint.Tests/Runtime/GarbageCollectionTests.cs

@@ -228,6 +228,47 @@ static WeakReference ImportOnceAndForget(Prepared<Module> prepared)
         }
     }
 
+    [Test]
+    public void SharedPreparedScriptConvertingFunctionsToDelegatesDoesNotRetainEngines()
+    {
+        // The delegate a JavaScript function converts to is memoized in two process-wide tables, the lower of
+        // which is keyed on the function's AST node — shared, and outliving every engine that runs a prepared
+        // script. Both are keyed by the target delegate type as well since #3434, so the AST node now holds one
+        // compiled binder per delegate type rather than one in total; this is the pin that those binders stayed
+        // engine-neutral, taking their target as a parameter rather than closing over the engine that built them.
+
+        var prepared = Engine.PrepareScript("""
+            host.a(function (x) { { let y = x; return String(y); } });
+            host.b(function (x) { { let y = x; return String(y); } });
+            """);
+
+        const int count = 20;
+        var references = new List<WeakReference>(count);
+        for (var i = 0; i < count; i++)
+        {
+            references.Add(ConvertOnceAndForget(prepared));
+        }
+
+        GC.Collect(2, GCCollectionMode.Forced, blocking: true);
+        GC.WaitForPendingFinalizers();
+        GC.Collect(2, GCCollectionMode.Forced, blocking: true);
+
+        var aliveCount = references.Count(static r => r.IsAlive);
+        prepared.Program.ShouldCarryPublishedInterpreterState();
+
+        aliveCount.Should().Be(0, $"{aliveCount} of {count} engines were not collected — the shared delegate binder cache still pins engines.");
+
+        // NoInlining so the engine reference cannot be stack-rooted in this frame across the GC.Collect calls.
+        [System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.NoInlining)]
+        static WeakReference ConvertOnceAndForget(Prepared<Script> prepared)
+        {
+            var engine = new Engine();
+            engine.SetValue("host", new DelegateConversionTargetTypeTests.BothHost());
+            engine.Execute(prepared);
+            return new WeakReference(engine);
+        }
+    }
+
     private static void AssertNoEngineRetained(Prepared<Script> prepared)
     {
         const int count = 20;