Maintain ObjectWrapper identity (#1227)

Jint.Tests/Runtime/InteropTests.Json.cs

@@ -2,7 +2,6 @@ using System;
 using System.Collections.Generic;
 using System.Dynamic;
 using System.Linq;
-using Jint.Native;
 using Jint.Runtime.Interop;
 using Xunit;
 

Jint.Tests/Runtime/InteropTests.cs

@@ -4,6 +4,7 @@ using System.Collections.Generic;
 using System.Globalization;
 using System.Linq;
 using System.Reflection;
+using System.Runtime.CompilerServices;
 using Jint.Native;
 using Jint.Native.Object;
 using Jint.Native.Symbol;
@@ -3038,5 +3039,36 @@ namespace Jint.Tests.Runtime
                 callback.Invoke();
             }
         }
+
+        [Fact]
+        public void ObjectWrapperIdentityIsMaintained()
+        {
+            // run in separate method so stack won't keep reference
+            var reference = RunWeakReferenceTest();
+
+            GC.Collect();
+
+            // make sure no dangling reference is left
+            Assert.False(reference.IsAlive);
+        }
+
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        private static WeakReference RunWeakReferenceTest()
+        {
+            var o = new object();
+
+            var engine = new Engine()
+                .SetValue("o", o);
+
+            var wrapper1 = (ObjectWrapper) engine.GetValue("o");
+            var reference = new WeakReference(wrapper1);
+
+            Assert.Same(wrapper1, engine.GetValue("o"));
+            Assert.Same(o, wrapper1.Target);
+
+            // reset
+            engine.Realm.GlobalObject.RemoveOwnProperty("o");
+            return reference;
+        }
     }
 }

Jint.Tests/Runtime/WeakSetMapTests.cs

@@ -6,61 +6,137 @@ using Jint.Native.WeakSet;
 using Jint.Runtime;
 using Xunit;
 
-namespace Jint.Tests.Runtime
+namespace Jint.Tests.Runtime;
+
+public class WeakSetMapTests
 {
-    public class WeakSetMapTests
+    [Fact]
+    public void WeakMapShouldThrowWhenCalledWithoutNew()
+    {
+        var engine = new Engine();
+        var e = Assert.Throws<JavaScriptException>(() => engine.Execute("{ const m = new WeakMap(); WeakMap.call(m,[]); }"));
+        Assert.Equal("Constructor WeakMap requires 'new'", e.Message);
+    }
+
+    [Fact]
+    public void WeakSetShouldThrowWhenCalledWithoutNew()
+    {
+        var engine = new Engine();
+        var e = Assert.Throws<JavaScriptException>(() => engine.Execute("{ const s = new WeakSet(); WeakSet.call(s,[]); }"));
+        Assert.Equal("Constructor WeakSet requires 'new'", e.Message);
+    }
+
+    public static IEnumerable<object[]> PrimitiveKeys = new TheoryData<JsValue>
+    {
+        JsValue.Null,
+        JsValue.Undefined,
+        0,
+        100.04,
+        double.NaN,
+        "hello",
+        true,
+        new JsSymbol("hello")
+    };
+
+    [Theory]
+    [MemberData(nameof(PrimitiveKeys))]
+    public void WeakSetAddShouldThrowForPrimitiveKey(JsValue key)
+    {
+        var engine = new Engine();
+        var weakSet = new WeakSetInstance(engine);
+
+        var e = Assert.Throws<JavaScriptException>(() => weakSet.WeakSetAdd(key));
+        Assert.StartsWith("WeakSet value must be an object, got ", e.Message);
+
+        Assert.False(weakSet.WeakSetHas(key));
+    }
+
+    [Theory]
+    [MemberData(nameof(PrimitiveKeys))]
+    public void WeakMapSetShouldThrowForPrimitiveKey(JsValue key)
+    {
+        var engine = new Engine();
+        var weakMap = new WeakMapInstance(engine);
+
+        var e = Assert.Throws<JavaScriptException>(() => weakMap.WeakMapSet(key, new ObjectInstance(engine)));
+        Assert.StartsWith("WeakMap key must be an object, got ", e.Message);
+
+        Assert.False(weakMap.WeakMapHas(key));
+    }
+
+    [Fact]
+    public void WeakSetWithInteropObject()
     {
+        var engine = new Engine();
+
+        engine.SetValue("context", new { Item = new Item { Value = "Test" } });
+
+        Assert.Equal(true, engine.Evaluate(@"
+		    var set1 = new WeakSet();
+		    set1.add(context.Item);
+		    return set1.has(context.Item);
+	    "));
+
+        Assert.Equal(true, engine.Evaluate(@"
+		    var item = context.Item;
+		    var set2 = new WeakSet();
+		    set2.add(item);
+		    return set2.has(item);
+    	"));
+    }
+
+    [Fact]
+    public void StringifyWithoutCircularReferences()
+    {
+        var parent = new Parent { Value = "Parent" };
+        var child = new Child { Value = "Child" };
+        parent.Child = child;
+        child.Parent = parent;
+
+        var engine = new Engine();
+
+        engine.SetValue("context", new { Parent = parent });
 
-        private static readonly Engine _engine = new Engine();
-
-        [Fact]
-        public void WeakMapShouldThrowWhenCalledWithoutNew()
-        {
-            var e = Assert.Throws<JavaScriptException>(() => _engine.Execute("{ const m = new WeakMap(); WeakMap.call(m,[]); }"));
-            Assert.Equal("Constructor WeakMap requires 'new'", e.Message);
-        }
-
-        [Fact]
-        public void WeakSetShouldThrowWhenCalledWithoutNew()
-        {
-            var e = Assert.Throws<JavaScriptException>(() => _engine.Execute("{ const s = new WeakSet(); WeakSet.call(s,[]); }"));
-            Assert.Equal("Constructor WeakSet requires 'new'", e.Message);
-        }
-
-        public static IEnumerable<object[]> PrimitiveKeys = new TheoryData<JsValue>
-        {
-            JsValue.Null,
-            JsValue.Undefined,
-            0,
-            100.04,
-            double.NaN,
-            "hello",
-            true,
-            new JsSymbol("hello")
-        };
-
-        [Theory]
-        [MemberData(nameof(PrimitiveKeys))]
-        public void WeakSetAddShouldThrowForPrimitiveKey(JsValue key) {
-            var weakSet = new WeakSetInstance(_engine);
-
-            var e = Assert.Throws<JavaScriptException>(() => weakSet.WeakSetAdd(key));
-            Assert.StartsWith("WeakSet value must be an object, got ", e.Message);
-
-            Assert.False(weakSet.WeakSetHas(key));
-        }
-
-        [Theory]
-        [MemberData(nameof(PrimitiveKeys))]
-        public void WeakMapSetShouldThrowForPrimitiveKey(JsValue key) {
-            var weakMap = new WeakMapInstance(_engine);
-
-            var e = Assert.Throws<JavaScriptException>(() => weakMap.WeakMapSet(key, new ObjectInstance(_engine)));
-            Assert.StartsWith("WeakMap key must be an object, got ", e.Message);
-
-            Assert.False(weakMap.WeakMapHas(key));
-        }
+        engine.Execute(@"
+		    function stringifyWithoutCircularReferences(obj, setConstructor) {
+		      var getCircularReplacer = () => {
+			    var seen = new setConstructor();
+			    return (key, value) => {
+			      if (typeof value === ""object"" && value !== null) {
+				    if (seen.has(value)) {
+				      return undefined;
+				    }
+				    seen.add(value);
+			      }
+			      return value;
+			    };
+		      };
+		      return JSON.stringify(obj, getCircularReplacer());
+		    }
+	    ");
 
+        // If I use Set, everything works as expected
+        const string Expected = "{\"Value\":\"Parent\",\"Child\":{\"Value\":\"Child\"}}";
+        Assert.Equal(Expected, engine.Evaluate("stringifyWithoutCircularReferences(context.Parent, Set)"));
+
+        // With WeakSet I get an error
+        Assert.Equal(Expected, engine.Evaluate("stringifyWithoutCircularReferences(context.Parent, WeakSet)"));
     }
 
-}
+    private class Item
+    {
+        public string Value { get; set; }
+    }
+
+    private class Parent
+    {
+        public string Value { get; set; }
+        public Child Child { get; set; }
+    }
+
+    private class Child
+    {
+        public string Value { get; set; }
+        public Parent Parent { get; set; }
+    }
+}

Jint/Engine.cs

@@ -56,6 +56,9 @@ namespace Jint
         // cache of types used when resolving CLR type names
         internal readonly Dictionary<string, Type> TypeCache = new();
 
+        // cache for already wrapped CLR objects to keep object identity
+        internal readonly ConditionalWeakTable<object, ObjectWrapper> _objectWrapperCache = new();
+
         internal readonly JintCallStack CallStack;
 
         // needed in initial engine setup, for example CLR function construction
@@ -266,7 +269,7 @@ namespace Jint
 
             return this;
         }
-        
+
         /// <summary>
         /// https://tc39.es/ecma262/#sec-runtime-semantics-scriptevaluation
         /// </summary>

Jint/Native/WeakMap/WeakMapInstance.cs

@@ -3,51 +3,50 @@
 using Jint.Native.Object;
 using Jint.Runtime;
 
-namespace Jint.Native.WeakMap
+namespace Jint.Native.WeakMap;
+
+public sealed class WeakMapInstance : ObjectInstance
 {
-    public class WeakMapInstance : ObjectInstance
+    private readonly ConditionalWeakTable<JsValue, JsValue> _table;
+
+    public WeakMapInstance(Engine engine) : base(engine)
     {
-        private readonly ConditionalWeakTable<JsValue, JsValue> _table;
+        _table = new ConditionalWeakTable<JsValue, JsValue>();
+    }
 
-        public WeakMapInstance(Engine engine) : base(engine)
-        {
-            _table = new ConditionalWeakTable<JsValue, JsValue>();
-        }
+    internal bool WeakMapHas(JsValue key)
+    {
+        return _table.TryGetValue(key, out _);
+    }
 
-        internal bool WeakMapHas(JsValue key)
-        {
-            return _table.TryGetValue(key, out _);
-        }
+    internal bool WeakMapDelete(JsValue key)
+    {
+        return _table.Remove(key);
+    }
 
-        internal bool WeakMapDelete(JsValue key)
+    internal void WeakMapSet(JsValue key, JsValue value)
+    {
+        if (key.IsPrimitive())
         {
-            return _table.Remove(key);
+            ExceptionHelper.ThrowTypeError(_engine.Realm, "WeakMap key must be an object, got " + key);
         }
 
-        internal void WeakMapSet(JsValue key, JsValue value)
-        {
-            if (key.IsPrimitive())
-            {
-                ExceptionHelper.ThrowTypeError(_engine.Realm, "WeakMap key must be an object, got " + key);
-            }
-
 #if NETSTANDARD2_1
-            _table.AddOrUpdate(key, value);
+         _table.AddOrUpdate(key, value);
 #else
-            _table.Remove(key);
-            _table.Add(key, value);
+        _table.Remove(key);
+        _table.Add(key, value);
 #endif
-        }
+    }
 
-        internal JsValue WeakMapGet(JsValue key)
+    internal JsValue WeakMapGet(JsValue key)
+    {
+        if (!_table.TryGetValue(key, out var value))
         {
-            if (!_table.TryGetValue(key, out var value))
-            {
-                return Undefined;
-            }
-
-            return value;
+            return Undefined;
         }
 
+        return value;
     }
+
 }

Jint/Native/WeakSet/WeakSetInstance.cs

@@ -3,43 +3,40 @@
 using Jint.Native.Object;
 using Jint.Runtime;
 
-namespace Jint.Native.WeakSet
+namespace Jint.Native.WeakSet;
+
+public sealed class WeakSetInstance : ObjectInstance
 {
-    public class WeakSetInstance : ObjectInstance
-    {
-        private static readonly object _tableValue = new object();
+    private readonly ConditionalWeakTable<JsValue, JsValue> _table;
 
-        private readonly ConditionalWeakTable<JsValue, object> _table;
+    public WeakSetInstance(Engine engine) : base(engine)
+    {
+        _table = new ConditionalWeakTable<JsValue, JsValue>();
+    }
 
-        public WeakSetInstance(Engine engine) : base(engine)
-        {
-            _table = new ConditionalWeakTable<JsValue, object>();
-        }
+    internal bool WeakSetHas(JsValue value)
+    {
+        return _table.TryGetValue(value, out _);
+    }
 
-        internal bool WeakSetHas(JsValue value)
-        {
-            return _table.TryGetValue(value, out _);
-        }
+    internal bool WeakSetDelete(JsValue value)
+    {
+        return _table.Remove(value);
+    }
 
-        internal bool WeakSetDelete(JsValue value)
+    internal void WeakSetAdd(JsValue value)
+    {
+        if (value.IsPrimitive())
         {
-            return _table.Remove(value);
+            ExceptionHelper.ThrowTypeError(_engine.Realm, "WeakSet value must be an object, got " + value);
         }
 
-        internal void WeakSetAdd(JsValue value)
-        {
-            if (value.IsPrimitive())
-            {
-                ExceptionHelper.ThrowTypeError(_engine.Realm, "WeakSet value must be an object, got " + value.ToString());
-            }
-
 #if NETSTANDARD2_1
-            _table.AddOrUpdate(value, _tableValue);
+        _table.AddOrUpdate(value, Undefined);
 #else
-            _table.Remove(value);
-            _table.Add(value, _tableValue);
+        _table.Remove(value);
+        _table.Add(value, Undefined);
 #endif
-        }
-
     }
+
 }

Jint/Options.cs

@@ -246,17 +246,37 @@ namespace Jint
         /// </summary>
         public List<IObjectConverter> ObjectConverters { get; } = new();
 
+        /// <summary>
+        /// Whether identity map is persisted for object wrappers in order to maintain object identity.
+        /// Defaults to true.
+        /// </summary>
+        public bool TrackObjectWrapperIdentity { get; set; } = true;
+
         /// <summary>
         /// If no known type could be guessed, objects are by default wrapped as an
         /// ObjectInstance using class ObjectWrapper. This function can be used to
         /// change the behavior.
         /// </summary>
-        public WrapObjectDelegate WrapObjectHandler { get; set; } = (engine, target) => new ObjectWrapper(engine, target);
+        public WrapObjectDelegate WrapObjectHandler { get; set; } = static (engine, target) =>
+        {
+            // check global cache, have we already wrapped the value?
+            if (engine._objectWrapperCache.TryGetValue(target, out var wrapped))
+            {
+                return wrapped;
+            }
+
+            wrapped = new ObjectWrapper(engine, target);
+            if (engine.Options.Interop.TrackObjectWrapperIdentity)
+            {
+                engine._objectWrapperCache.Add(target, wrapped);
+            }
+            return wrapped;
+        };
 
         /// <summary>
         ///
         /// </summary>
-        public MemberAccessorDelegate MemberAccessor { get; set; } = (engine, target, member) => null;
+        public MemberAccessorDelegate MemberAccessor { get; set; } = static (engine, target, member) => null;
 
         /// <summary>
         /// Exceptions that thrown from CLR code are converted to JavaScript errors and
@@ -264,7 +284,7 @@ namespace Jint
         /// to the CLR host and interrupt the script execution. If handler returns true these exceptions are converted
         /// to JS errors that can be caught by the script.
         /// </summary>
-        public ExceptionHandlerDelegate ExceptionHandler { get; set; } = exception => false;
+        public ExceptionHandlerDelegate ExceptionHandler { get; set; } = static exception => false;
 
         /// <summary>
         /// Assemblies to allow scripts to call CLR types directly like <example>System.IO.File</example>.