Special case Disposing DotNetObjectReferences (#2176)

* Special case Disposing DotNetObjectReferences This removes a public JSInvokable method required for disposing DotNetObjectReferences
dotnet · Aug 14, 2019 · d6bfc28 · d6bfc28
1 parent 3a5b3a8
commit d6bfc28
Show file tree

Hide file tree

Showing 13 changed files with 109 additions and 62 deletions.
diff --git a/src/JSInterop/Microsoft.JSInterop.JS/src/src/Microsoft.JSInterop.ts b/src/JSInterop/Microsoft.JSInterop.JS/src/src/Microsoft.JSInterop.ts
@@ -66,7 +66,11 @@ module DotNet {
     }
   }
 
-  function invokePossibleInstanceMethodAsync<T>(assemblyName: string | null, methodIdentifier: string, dotNetObjectId: number | null, args: any[]): Promise<T> {
+  function invokePossibleInstanceMethodAsync<T>(assemblyName: string | null, methodIdentifier: string, dotNetObjectId: number | null, ...args: any[]): Promise<T> {
+    if (assemblyName && dotNetObjectId) {
+      throw new Error(`For instance method calls, assemblyName should be null. Received '${assemblyName}'.`) ;
+    }
+
     const asyncCallId = nextAsyncCallId++;
     const resultPromise = new Promise<T>((resolve, reject) => {
       pendingAsyncCalls[asyncCallId] = { resolve, reject };
@@ -269,10 +273,7 @@ module DotNet {
     }
 
     public dispose() {
-      const promise = invokeMethodAsync<any>(
-        'Microsoft.JSInterop',
-        'DotNetDispatcher.ReleaseDotNetObject',
-        this._id);
+      const promise = invokePossibleInstanceMethodAsync<any>(null, '__Dispose', this._id);
       promise.catch(error => console.error(error));
     }
 

diff --git a/src/JSInterop/Microsoft.JSInterop/ref/Microsoft.JSInterop.netcoreapp3.0.cs b/src/JSInterop/Microsoft.JSInterop/ref/Microsoft.JSInterop.netcoreapp3.0.cs
@@ -8,8 +8,6 @@ public static partial class DotNetDispatcher
         public static void BeginInvoke(string callId, string assemblyName, string methodIdentifier, long dotNetObjectId, string argsJson) { }
         public static void EndInvoke(string arguments) { }
         public static string Invoke(string assemblyName, string methodIdentifier, long dotNetObjectId, string argsJson) { throw null; }
-        [Microsoft.JSInterop.JSInvokableAttribute("DotNetDispatcher.ReleaseDotNetObject")]
-        public static void ReleaseDotNetObject(long dotNetObjectId) { }
     }
     public static partial class DotNetObjectRef
     {
@@ -18,7 +16,7 @@ public static partial class DotNetObjectRef
     public sealed partial class DotNetObjectRef<TValue> : System.IDisposable where TValue : class
     {
         internal DotNetObjectRef() { }
-        public TValue Value { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
+        public TValue Value { get { throw null; } }
         public void Dispose() { }
     }
     public partial interface IJSInProcessRuntime : Microsoft.JSInterop.IJSRuntime

diff --git a/src/JSInterop/Microsoft.JSInterop/ref/Microsoft.JSInterop.netstandard2.0.cs b/src/JSInterop/Microsoft.JSInterop/ref/Microsoft.JSInterop.netstandard2.0.cs
@@ -8,8 +8,6 @@ public static partial class DotNetDispatcher
         public static void BeginInvoke(string callId, string assemblyName, string methodIdentifier, long dotNetObjectId, string argsJson) { }
         public static void EndInvoke(string arguments) { }
         public static string Invoke(string assemblyName, string methodIdentifier, long dotNetObjectId, string argsJson) { throw null; }
-        [Microsoft.JSInterop.JSInvokableAttribute("DotNetDispatcher.ReleaseDotNetObject")]
-        public static void ReleaseDotNetObject(long dotNetObjectId) { }
     }
     public static partial class DotNetObjectRef
     {
@@ -18,7 +16,7 @@ public static partial class DotNetObjectRef
     public sealed partial class DotNetObjectRef<TValue> : System.IDisposable where TValue : class
     {
         internal DotNetObjectRef() { }
-        public TValue Value { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
+        public TValue Value { get { throw null; } }
         public void Dispose() { }
     }
     public partial interface IJSInProcessRuntime : Microsoft.JSInterop.IJSRuntime

diff --git a/src/JSInterop/Microsoft.JSInterop/src/DotNetDispatcher.cs b/src/JSInterop/Microsoft.JSInterop/src/DotNetDispatcher.cs
@@ -18,6 +18,7 @@ namespace Microsoft.JSInterop
     /// </summary>
     public static class DotNetDispatcher
     {
+        private const string DisposeDotNetObjectReferenceMethodName = "__Dispose";
         internal static readonly JsonEncodedText DotNetObjectRefKey = JsonEncodedText.Encode("__dotNetObject");
 
         private static readonly ConcurrentDictionary<AssemblyKey, IReadOnlyDictionary<string, (MethodInfo, Type[])>> _cachedMethodsByAssembly
@@ -38,7 +39,7 @@ public static string Invoke(string assemblyName, string methodIdentifier, long d
             // the targeted method has [JSInvokable]. It is not itself subject to that restriction,
             // because there would be nobody to police that. This method *is* the police.
 
-            var targetInstance = (object)null;
+            IDotNetObjectRef targetInstance = default;
             if (dotNetObjectId != default)
             {
                 targetInstance = DotNetObjectRefManager.Current.FindDotNetObject(dotNetObjectId);
@@ -78,7 +79,7 @@ public static void BeginInvoke(string callId, string assemblyName, string method
             // original stack traces.
             object syncResult = null;
             ExceptionDispatchInfo syncException = null;
-            object targetInstance = null;
+            IDotNetObjectRef targetInstance = null;
 
             try
             {
@@ -127,21 +128,28 @@ public static void BeginInvoke(string callId, string assemblyName, string method
             }
         }
 
-        private static object InvokeSynchronously(string assemblyName, string methodIdentifier, object targetInstance, string argsJson)
+        private static object InvokeSynchronously(string assemblyName, string methodIdentifier, IDotNetObjectRef objectReference, string argsJson)
         {
             AssemblyKey assemblyKey;
-            if (targetInstance != null)
+            if (objectReference is null)
+            {
+                assemblyKey = new AssemblyKey(assemblyName);
+            }
+            else
             {
                 if (assemblyName != null)
                 {
                     throw new ArgumentException($"For instance method calls, '{nameof(assemblyName)}' should be null. Value received: '{assemblyName}'.");
                 }
 
-                assemblyKey = new AssemblyKey(targetInstance.GetType().Assembly);
-            }
-            else
-            {
-                assemblyKey = new AssemblyKey(assemblyName);
+                if (string.Equals(DisposeDotNetObjectReferenceMethodName, methodIdentifier, StringComparison.Ordinal))
+                {
+                    // The client executed dotNetObjectReference.dispose(). Dispose the reference and exit.
+                    objectReference.Dispose();
+                    return default;
+                }
+
+                assemblyKey = new AssemblyKey(objectReference.Value.GetType().Assembly);
             }
 
             var (methodInfo, parameterTypes) = GetCachedMethodInfo(assemblyKey, methodIdentifier);
@@ -150,7 +158,8 @@ private static object InvokeSynchronously(string assemblyName, string methodIden
 
             try
             {
-                return methodInfo.Invoke(targetInstance, suppliedArgs);
+                // objectReference will be null if this call invokes a static JSInvokable method.
+                return methodInfo.Invoke(objectReference?.Value, suppliedArgs);
             }
             catch (TargetInvocationException tie) // Avoid using exception filters for AOT runtime support
             {
@@ -280,22 +289,6 @@ internal static void ParseEndInvokeArguments(JSRuntimeBase jsRuntimeBase, string
             }
         }
 
-        /// <summary>
-        /// Releases the reference to the specified .NET object. This allows the .NET runtime
-        /// to garbage collect that object if there are no other references to it.
-        ///
-        /// To avoid leaking memory, the JavaScript side code must call this for every .NET
-        /// object it obtains a reference to. The exception is if that object is used for
-        /// the entire lifetime of a given user's session, in which case it is released
-        /// automatically when the JavaScript runtime is disposed.
-        /// </summary>
-        /// <param name="dotNetObjectId">The identifier previously passed to JavaScript code.</param>
-        [JSInvokable(nameof(DotNetDispatcher) + "." + nameof(ReleaseDotNetObject))]
-        public static void ReleaseDotNetObject(long dotNetObjectId)
-        {
-            DotNetObjectRefManager.Current.ReleaseDotNetObject(dotNetObjectId);
-        }
-
         private static (MethodInfo, Type[]) GetCachedMethodInfo(AssemblyKey assemblyKey, string methodIdentifier)
         {
             if (string.IsNullOrWhiteSpace(assemblyKey.AssemblyName))

diff --git a/src/JSInterop/Microsoft.JSInterop/src/DotNetObjectRef.cs b/src/JSInterop/Microsoft.JSInterop/src/DotNetObjectRef.cs
@@ -15,8 +15,7 @@ public static class DotNetObjectRef
         /// <returns>An instance of <see cref="DotNetObjectRef{TValue}" />.</returns>
         public static DotNetObjectRef<TValue> Create<TValue>(TValue value) where TValue : class
         {
-            var objectId = DotNetObjectRefManager.Current.TrackObject(value);
-            return new DotNetObjectRef<TValue>(objectId, value);
+            return new DotNetObjectRef<TValue>(DotNetObjectRefManager.Current, value);
         }
     }
 }
diff --git a/src/JSInterop/Microsoft.JSInterop/src/DotNetObjectRefManager.cs b/src/JSInterop/Microsoft.JSInterop/src/DotNetObjectRefManager.cs
@@ -10,7 +10,7 @@ namespace Microsoft.JSInterop
     internal class DotNetObjectRefManager
     {
         private long _nextId = 0; // 0 signals no object, but we increment prior to assignment. The first tracked object should have id 1
-        private readonly ConcurrentDictionary<long, object> _trackedRefsById = new ConcurrentDictionary<long, object>();
+        private readonly ConcurrentDictionary<long, IDotNetObjectRef> _trackedRefsById = new ConcurrentDictionary<long, IDotNetObjectRef>();
 
         public static DotNetObjectRefManager Current
         {
@@ -25,15 +25,15 @@ public static DotNetObjectRefManager Current
             }
         }
 
-        public long TrackObject(object dotNetObjectRef)
+        public long TrackObject(IDotNetObjectRef dotNetObjectRef)
         {
             var dotNetObjectId = Interlocked.Increment(ref _nextId);
             _trackedRefsById[dotNetObjectId] = dotNetObjectRef;
 
             return dotNetObjectId;
         }
 
-        public object FindDotNetObject(long dotNetObjectId)
+        public IDotNetObjectRef FindDotNetObject(long dotNetObjectId)
         {
             return _trackedRefsById.TryGetValue(dotNetObjectId, out var dotNetObjectRef)
                 ? dotNetObjectRef

diff --git a/src/JSInterop/Microsoft.JSInterop/src/DotNetObjectRefOfT.cs b/src/JSInterop/Microsoft.JSInterop/src/DotNetObjectRefOfT.cs
@@ -16,23 +16,53 @@ namespace Microsoft.JSInterop
     [JsonConverter(typeof(DotNetObjectReferenceJsonConverterFactory))]
     public sealed class DotNetObjectRef<TValue> : IDotNetObjectRef, IDisposable where TValue : class
     {
+        private readonly DotNetObjectRefManager _referenceManager;
+        private readonly TValue _value;
+        private readonly long _objectId;
+
         /// <summary>
         /// Initializes a new instance of <see cref="DotNetObjectRef{TValue}" />.
         /// </summary>
-        /// <param name="objectId">The object Id.</param>
+        /// <param name="referenceManager"></param>
         /// <param name="value">The value to pass by reference.</param>
-        internal DotNetObjectRef(long objectId, TValue value)
+        internal DotNetObjectRef(DotNetObjectRefManager referenceManager, TValue value)
+        {
+            _referenceManager = referenceManager;
+            _objectId = _referenceManager.TrackObject(this);
+            _value = value;
+        }
+
+        internal DotNetObjectRef(DotNetObjectRefManager referenceManager, long objectId, TValue value)
         {
-            ObjectId = objectId;
-            Value = value;
+            _referenceManager = referenceManager;
+            _objectId = objectId;
+            _value = value;
         }
 
         /// <summary>
         /// Gets the object instance represented by this wrapper.
         /// </summary>
-        public TValue Value { get; }
+        public TValue Value
+        {
+            get
+            {
+                ThrowIfDisposed();
+                return _value;
+            }
+        }
 
-        internal long ObjectId { get; }
+        internal long ObjectId
+        {
+            get
+            {
+                ThrowIfDisposed();
+                return _objectId;
+            }
+        }
+
+        object IDotNetObjectRef.Value => Value;
+
+        internal bool Disposed { get; private set; }
 
         /// <summary>
         /// Stops tracking this object reference, allowing it to be garbage collected
@@ -41,7 +71,19 @@ internal DotNetObjectRef(long objectId, TValue value)
         /// </summary>
         public void Dispose()
         {
-            DotNetObjectRefManager.Current.ReleaseDotNetObject(ObjectId);
+            if (!Disposed)
+            {
+                Disposed = true;
+                _referenceManager.ReleaseDotNetObject(_objectId);
+            }
+        }
+
+        private void ThrowIfDisposed()
+        {
+            if (Disposed)
+            {
+                throw new ObjectDisposedException(GetType().Name);
+            }
         }
     }
 }
diff --git a/src/JSInterop/Microsoft.JSInterop/src/DotNetObjectReferenceJsonConverter.cs b/src/JSInterop/Microsoft.JSInterop/src/DotNetObjectReferenceJsonConverter.cs
@@ -40,8 +40,8 @@ public override DotNetObjectRef<TValue> Read(ref Utf8JsonReader reader, Type typ
                 throw new JsonException($"Required property {DotNetObjectRefKey} not found.");
             }
 
-            var value = (TValue)DotNetObjectRefManager.Current.FindDotNetObject(dotNetObjectId);
-            return new DotNetObjectRef<TValue>(dotNetObjectId, value);
+            var referenceManager = DotNetObjectRefManager.Current;
+            return (DotNetObjectRef<TValue>)referenceManager.FindDotNetObject(dotNetObjectId);
         }
 
         public override void Write(Utf8JsonWriter writer, DotNetObjectRef<TValue> value, JsonSerializerOptions options)

diff --git a/src/JSInterop/Microsoft.JSInterop/src/IDotNetObjectRef.cs b/src/JSInterop/Microsoft.JSInterop/src/IDotNetObjectRef.cs
@@ -7,5 +7,6 @@ namespace Microsoft.JSInterop
 {
     internal interface IDotNetObjectRef : IDisposable
     {
+        object Value { get; }
     }
 }
diff --git a/src/JSInterop/Microsoft.JSInterop/test/DotNetDispatcherTest.cs b/src/JSInterop/Microsoft.JSInterop/test/DotNetDispatcherTest.cs
@@ -142,7 +142,7 @@ public Task CanInvokeStaticWithParams() => WithJSRuntime(jsRuntime =>
             Assert.False(resultDto2Ref.TryGetProperty(nameof(TestDTO.IntVal), out _));
 
             Assert.True(resultDto2Ref.TryGetProperty(DotNetDispatcher.DotNetObjectRefKey.EncodedUtf8Bytes, out var property));
-            var resultDto2 = Assert.IsType<TestDTO>(DotNetObjectRefManager.Current.FindDotNetObject(property.GetInt64()));
+            var resultDto2 = Assert.IsType<DotNetObjectRef<TestDTO>>(DotNetObjectRefManager.Current.FindDotNetObject(property.GetInt64())).Value;
             Assert.Equal("MY STRING", resultDto2.StringVal);
             Assert.Equal(1299, resultDto2.IntVal);
         });
@@ -202,6 +202,20 @@ public Task CanInvokeBaseInstanceVoidMethod() => WithJSRuntime(jsRuntime =>
             Assert.True(targetInstance.DidInvokeMyBaseClassInvocableInstanceVoid);
         });
 
+        [Fact]
+        public Task DotNetObjectReferencesCanBeDisposed() => WithJSRuntime(jsRuntime =>
+        {
+            // Arrange
+            var targetInstance = new SomePublicType();
+            var objectRef = DotNetObjectRef.Create(targetInstance);
+
+            // Act
+            DotNetDispatcher.BeginInvoke(null, null, "__Dispose", objectRef.ObjectId, null);
+
+            // Assert
+            Assert.True(objectRef.Disposed);
+        });
+
         [Fact]
         public Task CannotUseDotNetObjectRefAfterDisposal() => WithJSRuntime(jsRuntime =>
         {
@@ -230,7 +244,7 @@ public Task CannotUseDotNetObjectRefAfterReleaseDotNetObject() => WithJSRuntime(
             var targetInstance = new SomePublicType();
             var objectRef = DotNetObjectRef.Create(targetInstance);
             jsRuntime.Invoke<object>("unimportant", objectRef);
-            DotNetDispatcher.ReleaseDotNetObject(1);
+            objectRef.Dispose();
 
             // Act/Assert
             var ex = Assert.Throws<ArgumentException>(
@@ -320,7 +334,7 @@ public Task CanInvokeInstanceMethodWithParams() => WithJSRuntime(jsRuntime =>
 
             // Assert
             Assert.Equal("[\"You passed myvalue\",{\"__dotNetObject\":3}]", resultJson);
-            var resultDto = (TestDTO)jsRuntime.ObjectRefManager.FindDotNetObject(3);
+            var resultDto = ((DotNetObjectRef<TestDTO>)jsRuntime.ObjectRefManager.FindDotNetObject(3)).Value;
             Assert.Equal(1235, resultDto.IntVal);
             Assert.Equal("MY STRING", resultDto.StringVal);
         });

diff --git a/src/JSInterop/Microsoft.JSInterop/test/DotNetObjectRefTest.cs b/src/JSInterop/Microsoft.JSInterop/test/DotNetObjectRefTest.cs
@@ -24,10 +24,11 @@ public Task NotifiesAssociatedJsRuntimeOfDisposal() => WithJSRuntime(jsRuntime =
             var objRef = DotNetObjectRef.Create(new object());
 
             // Act
+            Assert.Equal(1, objRef.ObjectId);
             objRef.Dispose();
 
             // Assert
-            var ex = Assert.Throws<ArgumentException>(() => jsRuntime.ObjectRefManager.FindDotNetObject(objRef.ObjectId));
+            var ex = Assert.Throws<ArgumentException>(() => jsRuntime.ObjectRefManager.FindDotNetObject(1));
             Assert.StartsWith("There is no tracked object with id '1'.", ex.Message);
         });
     }

diff --git a/src/JSInterop/Microsoft.JSInterop/test/JSInProcessRuntimeBaseTest.cs b/src/JSInterop/Microsoft.JSInterop/test/JSInProcessRuntimeBaseTest.cs
@@ -60,9 +60,9 @@ public void SerializesDotNetObjectWrappersInKnownFormat()
             Assert.Equal("[{\"__dotNetObject\":1},{\"obj2\":{\"__dotNetObject\":2},\"obj3\":{\"__dotNetObject\":3}}]", call.ArgsJson);
 
             // Assert: Objects were tracked
-            Assert.Same(obj1, runtime.ObjectRefManager.FindDotNetObject(1));
-            Assert.Same(obj2, runtime.ObjectRefManager.FindDotNetObject(2));
-            Assert.Same(obj3, runtime.ObjectRefManager.FindDotNetObject(3));
+            Assert.Same(obj1, runtime.ObjectRefManager.FindDotNetObject(1).Value);
+            Assert.Same(obj2, runtime.ObjectRefManager.FindDotNetObject(2).Value);
+            Assert.Same(obj3, runtime.ObjectRefManager.FindDotNetObject(3).Value);
         }
 
         [Fact]

diff --git a/src/JSInterop/Microsoft.JSInterop/test/JSRuntimeBaseTest.cs b/src/JSInterop/Microsoft.JSInterop/test/JSRuntimeBaseTest.cs
@@ -282,10 +282,10 @@ public void SerializesDotNetObjectWrappersInKnownFormat()
             Assert.Equal("[{\"__dotNetObject\":1},{\"obj2\":{\"__dotNetObject\":3},\"obj3\":{\"__dotNetObject\":4},\"obj1SameRef\":{\"__dotNetObject\":1},\"obj1DifferentRef\":{\"__dotNetObject\":2}}]", call.ArgsJson);
 
             // Assert: Objects were tracked
-            Assert.Same(obj1, runtime.ObjectRefManager.FindDotNetObject(1));
-            Assert.Same(obj1, runtime.ObjectRefManager.FindDotNetObject(2));
-            Assert.Same(obj2, runtime.ObjectRefManager.FindDotNetObject(3));
-            Assert.Same(obj3, runtime.ObjectRefManager.FindDotNetObject(4));
+            Assert.Same(obj1, runtime.ObjectRefManager.FindDotNetObject(1).Value);
+            Assert.Same(obj1, runtime.ObjectRefManager.FindDotNetObject(2).Value);
+            Assert.Same(obj2, runtime.ObjectRefManager.FindDotNetObject(3).Value);
+            Assert.Same(obj3, runtime.ObjectRefManager.FindDotNetObject(4).Value);
         }
 
         [Fact]