From 04e6dcc6ae1425f90dc9a2a34629e7f53a3275dc Mon Sep 17 00:00:00 2001 From: Pavel Savara Date: Thu, 15 Jul 2021 19:23:27 +0200 Subject: [PATCH] inFlight ref-counting of strong GCHandle for JSObject, while in flight over the managed/JS boundary (#54453) --- .../src/Interop/Browser/Interop.Runtime.cs | 8 ++ .../HttpClientHandlerTest.RemoteServer.cs | 1 - .../System/Net/Http/HttpClientHandlerTest.cs | 1 - .../tests/SendReceiveTest.cs | 2 - .../InteropServices/JavaScript/AnyRef.cs | 34 ++++++ .../InteropServices/JavaScript/Array.cs | 1 + .../InteropServices/JavaScript/JSObject.cs | 2 + .../InteropServices/JavaScript/Runtime.cs | 17 ++- .../JavaScript/JavaScriptTests.cs | 108 ++++++++++++++++++ src/mono/wasm/runtime/binding_support.js | 28 +++-- 10 files changed, 185 insertions(+), 17 deletions(-) diff --git a/src/libraries/Common/src/Interop/Browser/Interop.Runtime.cs b/src/libraries/Common/src/Interop/Browser/Interop.Runtime.cs index e325e0489fab5..2c31b557cdcac 100644 --- a/src/libraries/Common/src/Interop/Browser/Interop.Runtime.cs +++ b/src/libraries/Common/src/Interop/Browser/Interop.Runtime.cs @@ -70,6 +70,7 @@ public static string InvokeJS(string str) object res = CompileFunction(snippet, out int exception); if (exception != 0) throw new JSException((string)res); + ReleaseInFlight(res); return res as System.Runtime.InteropServices.JavaScript.Function; } @@ -97,6 +98,7 @@ public static object GetGlobalObject(string? str = null) if (exception != 0) throw new JSException($"Error obtaining a handle to global {str}"); + ReleaseInFlight(globalHandle); return globalHandle; } @@ -121,5 +123,11 @@ public static unsafe void DumpAotProfileData(ref byte buf, int len, string extra module.SetObjectProperty("aot_profile_data", Uint8Array.From(span)); } } + + public static void ReleaseInFlight(object? obj) + { + JSObject? jsObj = obj as JSObject; + jsObj?.ReleaseInFlight(); + } } } diff --git a/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.RemoteServer.cs b/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.RemoteServer.cs index 316a58cb33f3f..9b99ef1d13ff6 100644 --- a/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.RemoteServer.cs +++ b/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.RemoteServer.cs @@ -178,7 +178,6 @@ public async Task GetAsync_ServerNeedsAuthAndNoCredential_StatusCodeUnauthorized [OuterLoop("Uses external servers", typeof(PlatformDetection), nameof(PlatformDetection.LocalEchoServerIsNotAvailable))] [Theory] [MemberData(nameof(RemoteServersAndHeaderEchoUrisMemberData))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/53872", TestPlatforms.Browser)] public async Task GetAsync_RequestHeadersAddCustomHeaders_HeaderAndEmptyValueSent(Configuration.Http.RemoteServer remoteServer, Uri uri) { if (IsWinHttpHandler && !PlatformDetection.IsWindows10Version1709OrGreater) diff --git a/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.cs b/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.cs index 54497e28618f7..27a8fc9bed247 100644 --- a/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.cs +++ b/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.cs @@ -577,7 +577,6 @@ public async Task PostAsync_ManyDifferentRequestHeaders_SentCorrectly() [Theory] [MemberData(nameof(GetAsync_ManyDifferentResponseHeaders_ParsedCorrectly_MemberData))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/54655", TestPlatforms.Browser)] public async Task GetAsync_ManyDifferentResponseHeaders_ParsedCorrectly(string newline, string fold, bool dribble) { if (LoopbackServerFactory.Version >= HttpVersion20.Value) diff --git a/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs b/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs index bc70d2a4fdf55..6266f01e8ede5 100644 --- a/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs +++ b/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs @@ -217,7 +217,6 @@ public async Task SendAsync_MultipleOutstandingSendOperations_Throws(Uri server) [OuterLoop("Uses external servers", typeof(PlatformDetection), nameof(PlatformDetection.LocalEchoServerIsNotAvailable))] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/53957", TestPlatforms.Browser)] public async Task ReceiveAsync_MultipleOutstandingReceiveOperations_Throws(Uri server) { using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output)) @@ -321,7 +320,6 @@ public async Task SendAsync_SendZeroLengthPayloadAsEndOfMessage_Success(Uri serv [OuterLoop("Uses external servers", typeof(PlatformDetection), nameof(PlatformDetection.LocalEchoServerIsNotAvailable))] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/53957", TestPlatforms.Browser)] public async Task SendReceive_VaryingLengthBuffers_Success(Uri server) { using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output)) diff --git a/src/libraries/System.Private.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/AnyRef.cs b/src/libraries/System.Private.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/AnyRef.cs index 5cfb8aa1e1e4a..58587eb15a479 100644 --- a/src/libraries/System.Private.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/AnyRef.cs +++ b/src/libraries/System.Private.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/AnyRef.cs @@ -11,6 +11,8 @@ namespace System.Runtime.InteropServices.JavaScript { public abstract class AnyRef : SafeHandleMinusOneIsInvalid { + private GCHandle? InFlight; + private int InFlightCounter; private GCHandle AnyRefHandle; public int JSHandle => (int)handle; @@ -21,9 +23,41 @@ internal AnyRef(IntPtr jsHandle, bool ownsHandle) : base(ownsHandle) { SetHandle(jsHandle); AnyRefHandle = GCHandle.Alloc(this, ownsHandle ? GCHandleType.Weak : GCHandleType.Normal); + InFlight = null; + InFlightCounter = 0; } internal int Int32Handle => (int)(IntPtr)AnyRefHandle; + internal void AddInFlight() + { + lock (this) + { + InFlightCounter++; + if (InFlightCounter == 1) + { + Debug.Assert(InFlight == null); + InFlight = GCHandle.Alloc(this, GCHandleType.Normal); + } + } + } + + internal void ReleaseInFlight() + { + lock (this) + { + Debug.Assert(InFlightCounter != 0); + + InFlightCounter--; + if (InFlightCounter == 0) + { + Debug.Assert(InFlight.HasValue); + InFlight.Value.Free(); + InFlight = null; + } + } + } + + protected void FreeGCHandle() { AnyRefHandle.Free(); diff --git a/src/libraries/System.Private.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/Array.cs b/src/libraries/System.Private.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/Array.cs index 3ca4f4ac45f5d..591c125dbb490 100644 --- a/src/libraries/System.Private.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/Array.cs +++ b/src/libraries/System.Private.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/Array.cs @@ -86,6 +86,7 @@ internal Array(IntPtr jsHandle, bool ownsHandle) : base(jsHandle, ownsHandle) if (exception != 0) throw new JSException((string)indexValue); + Interop.Runtime.ReleaseInFlight(indexValue); return indexValue; } set diff --git a/src/libraries/System.Private.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSObject.cs b/src/libraries/System.Private.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSObject.cs index af4b00060d9c4..73032ff7b3a07 100644 --- a/src/libraries/System.Private.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSObject.cs +++ b/src/libraries/System.Private.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSObject.cs @@ -73,6 +73,7 @@ public object Invoke(string method, params object?[] args) object res = Interop.Runtime.InvokeJSWithArgs(JSHandle, method, args, out int exception); if (exception != 0) throw new JSException((string)res); + Interop.Runtime.ReleaseInFlight(res); return res; } @@ -103,6 +104,7 @@ public object GetObjectProperty(string name) object propertyValue = Interop.Runtime.GetObjectProperty(JSHandle, name, out int exception); if (exception != 0) throw new JSException((string)propertyValue); + Interop.Runtime.ReleaseInFlight(propertyValue); return propertyValue; } diff --git a/src/libraries/System.Private.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/Runtime.cs b/src/libraries/System.Private.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/Runtime.cs index 09e30d4bb4622..34467dbdb9a2e 100644 --- a/src/libraries/System.Private.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/Runtime.cs +++ b/src/libraries/System.Private.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/Runtime.cs @@ -91,6 +91,8 @@ public static int BindJSObject(int jsId, bool ownsHandle, int mappedType) } } + target.AddInFlight(); + return target.Int32Handle; } @@ -236,12 +238,21 @@ public static int GetJSObjectId(object rawObj) return jsObject?.JSHandle ?? -1; } - public static object? GetDotNetObject(int gcHandle) + /// + /// when true, we would create Normal GCHandle to the JSObject, so that it would not get collected before passing it back to managed code + public static object? GetDotNetObject(int gcHandle, int shouldAddInflight) { GCHandle h = (GCHandle)(IntPtr)gcHandle; - return h.Target is JSObject js ? - js.GetWrappedObject() ?? h.Target : h.Target; + if (h.Target is JSObject jso) + { + if (shouldAddInflight != 0) + { + jso.AddInFlight(); + } + return jso.GetWrappedObject() ?? jso; + } + return h.Target; } public static bool IsSimpleArray(object a) diff --git a/src/libraries/System.Private.Runtime.InteropServices.JavaScript/tests/System/Runtime/InteropServices/JavaScript/JavaScriptTests.cs b/src/libraries/System.Private.Runtime.InteropServices.JavaScript/tests/System/Runtime/InteropServices/JavaScript/JavaScriptTests.cs index 23d57c25b5539..f06939870b594 100644 --- a/src/libraries/System.Private.Runtime.InteropServices.JavaScript/tests/System/Runtime/InteropServices/JavaScript/JavaScriptTests.cs +++ b/src/libraries/System.Private.Runtime.InteropServices.JavaScript/tests/System/Runtime/InteropServices/JavaScript/JavaScriptTests.cs @@ -1,7 +1,10 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Collections.Generic; +using System.Linq; using System.Runtime.InteropServices.JavaScript; +using System.Threading.Tasks; using Xunit; namespace System.Runtime.InteropServices.JavaScript.Tests @@ -115,5 +118,110 @@ public static void FunctionMath() minValue = (int)mathMin.Call(null, 5, 6, 2, 3, 7); Assert.Equal(2, minValue); } + + [Fact] + public static async Task BagIterator() + { + await Task.Delay(1); + + var bagFn = new Function(@" + var same = { + x:1 + }; + return Object.entries({ + a:1, + b:'two', + c:{fold:{}}, + d:same, + e:same, + f:same + }); + "); + + for (int attempt = 0; attempt < 100_000; attempt++) + { + try + { + using var bag = (JSObject)bagFn.Call(null); + using var entriesIterator = (JSObject)bag.Invoke("entries"); + + var cnt = entriesIterator.ToEnumerable().Count(); + Assert.Equal(6, cnt); + + // fill GC helps to repro + var x = new byte[100 + attempt / 100]; + if (attempt % 1000 == 0) + { + GC.Collect(); + } + } + catch (Exception ex) + { + throw new Exception(ex.Message + " At attempt=" + attempt, ex); + } + } + } + + [Fact] + public static async Task Iterator() + { + await Task.Delay(1); + + var makeRangeIterator = new Function("start", "end", "step", @" + let nextIndex = start; + let iterationCount = 0; + + const rangeIterator = { + next: function() { + let result; + if (nextIndex < end) { + result = { value: {}, done: false } + nextIndex += step; + iterationCount++; + return result; + } + return { value: {}, done: true } + } + }; + return rangeIterator; + "); + + for (int attempt = 0; attempt < 100_000; attempt++) + { + try + { + using (var entriesIterator = (JSObject)makeRangeIterator.Call(null, 0, 500)) + { + var cnt = entriesIterator.ToEnumerable().Count(); + } + } + catch (Exception ex) + { + throw new Exception(ex.Message + " At attempt=" + attempt, ex); + } + } + } + + public static IEnumerable ToEnumerable(this JSObject iterrator) + { + JSObject nextResult = null; + try + { + nextResult = (JSObject)iterrator.Invoke("next"); + var done = (bool)nextResult.GetObjectProperty("done"); + while (!done) + { + object value = nextResult.GetObjectProperty("value"); + nextResult.Dispose(); + yield return value; + nextResult = (JSObject)iterrator.Invoke("next"); + done = (bool)nextResult.GetObjectProperty("done"); + } + } + finally + { + nextResult?.Dispose(); + } + } } } diff --git a/src/mono/wasm/runtime/binding_support.js b/src/mono/wasm/runtime/binding_support.js index 916032d0db0a0..d96eeed4ff6d8 100644 --- a/src/mono/wasm/runtime/binding_support.js +++ b/src/mono/wasm/runtime/binding_support.js @@ -126,7 +126,7 @@ var BindingSupportLib = { this._bind_existing_obj = bind_runtime_method ("BindExistingObject", "mi"); this._unbind_raw_obj_and_free = bind_runtime_method ("UnBindRawJSObjectAndFree", "ii"); this._get_js_id = bind_runtime_method ("GetJSObjectId", "m"); - this._get_raw_mono_obj = bind_runtime_method ("GetDotNetObject", "i!"); + this._get_raw_mono_obj = bind_runtime_method ("GetDotNetObject", "ii!"); this._is_simple_array = bind_runtime_method ("IsSimpleArray", "m"); this.setup_js_cont = get_method ("SetupJSContinuation"); @@ -779,15 +779,21 @@ var BindingSupportLib = { return this._get_js_id (mono_obj); }, - wasm_get_raw_obj: function (gchandle) + // when should_add_in_flight === true, the JSObject would be temporarily hold by Normal GCHandle, so that it would not get collected during transition to the managed stack. + // its InFlight handle would be freed when the instance arrives to managed side via Interop.Runtime.ReleaseInFlight + wasm_get_raw_obj: function (gchandle, should_add_in_flight) { - return this._get_raw_mono_obj (gchandle); + if(!gchandle){ + return 0; + } + + return this._get_raw_mono_obj (gchandle, should_add_in_flight ? 1 : 0); }, try_extract_mono_obj:function (js_obj) { if (js_obj === null || typeof js_obj === "undefined" || typeof js_obj.__mono_gchandle__ === "undefined") return 0; - return this.wasm_get_raw_obj (js_obj.__mono_gchandle__); + return this.wasm_get_raw_obj (js_obj.__mono_gchandle__, true); }, mono_method_get_call_signature: function(method, mono_obj) { @@ -810,7 +816,7 @@ var BindingSupportLib = { tcs.is_mono_tcs_task_bound = true; js_obj.__mono_bound_tcs__ = tcs.__mono_gchandle__; tcs.__mono_bound_task__ = js_obj.__mono_gchandle__; - return this.wasm_get_raw_obj (js_obj.__mono_gchandle__); + return this.wasm_get_raw_obj (js_obj.__mono_gchandle__, true); }, free_task_completion_source: function (tcs) { @@ -831,7 +837,7 @@ var BindingSupportLib = { var result = null; var gc_handle = js_obj.__mono_gchandle__; if (gc_handle) { - result = this.wasm_get_raw_obj (gc_handle); + result = this.wasm_get_raw_obj (gc_handle, true); // It's possible the managed object corresponding to this JS object was collected, // in which case we need to make a new one. @@ -842,8 +848,8 @@ var BindingSupportLib = { } if (!result) { - gc_handle = this.mono_wasm_register_obj(js_obj); - result = this.wasm_get_raw_obj (gc_handle); + var { gc_handle: new_gc_handle, should_add_in_flight } = this.mono_wasm_register_obj(js_obj); + result = this.wasm_get_raw_obj (new_gc_handle, should_add_in_flight); } return result; @@ -1613,10 +1619,12 @@ var BindingSupportLib = { js_obj.__owns_handle__ = true; gc_handle = js_obj.__mono_gchandle__ = this.wasm_binding_obj_new(handle + 1, js_obj.__owns_handle__, typeof wasm_type === "undefined" ? -1 : wasm_type); this.mono_wasm_object_registry[handle] = js_obj; - + // as this instance was just created, it was already created with Inflight strong GCHandle, so we do not have to do it again + return { gc_handle, should_add_in_flight: false }; } } - return gc_handle; + // this is pre-existing instance, we need to add Inflight strong GCHandle before passing it to managed + return { gc_handle, should_add_in_flight: true }; }, mono_wasm_require_handle: function(handle) { if (handle > 0)