From a3ddef16075b03928beb167cf6df8cdad5db8329 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Wed, 28 Jul 2021 07:59:44 -0700 Subject: [PATCH] Introduce new AddEventListener and RemoveEventListener APIs on JSObject (#55849) Introduces new AddEventListener and RemoveEventListener methods on JSObject that create a delegate wrapper with its lifetime managed by the JavaScript GC instead of managed types, to ensure that delegates don't go away while JS is still using them. Migrates BrowserWebSocket to use the new APIs. --- .../src/Interop/Browser/Interop.Runtime.cs | 5 + .../BrowserWebSockets/BrowserWebSocket.cs | 34 +--- .../InteropServices/JavaScript/JSObject.cs | 55 ++++++ .../InteropServices/JavaScript/Runtime.cs | 68 +++++++ .../JavaScript/JavaScriptTests.cs | 184 ++++++++++++++++++ src/mono/wasm/runtime/binding_support.js | 103 ++++++++++ src/mono/wasm/runtime/corebindings.c | 4 + 7 files changed, 428 insertions(+), 25 deletions(-) diff --git a/src/libraries/Common/src/Interop/Browser/Interop.Runtime.cs b/src/libraries/Common/src/Interop/Browser/Interop.Runtime.cs index 2c31b557cdcac..67509d17a362e 100644 --- a/src/libraries/Common/src/Interop/Browser/Interop.Runtime.cs +++ b/src/libraries/Common/src/Interop/Browser/Interop.Runtime.cs @@ -52,6 +52,11 @@ internal static partial class Runtime [MethodImplAttribute(MethodImplOptions.InternalCall)] internal static extern object TypedArrayCopyFrom(int jsObjHandle, int arrayPtr, int begin, int end, int bytesPerElement, out int exceptionalResult); + [MethodImplAttribute(MethodImplOptions.InternalCall)] + internal static extern string? AddEventListener(int jsObjHandle, string name, int weakDelegateHandle, int optionsObjHandle); + [MethodImplAttribute(MethodImplOptions.InternalCall)] + internal static extern string? RemoveEventListener(int jsObjHandle, string name, int weakDelegateHandle, bool capture); + // / // / Execute the provided string in the JavaScript context // / diff --git a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs index b052020b11e0f..e46a6160ae105 100644 --- a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs +++ b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs @@ -167,13 +167,13 @@ internal async Task ConnectAsyncJavaScript(Uri uri, CancellationToken cancellati _onError = errorEvt => errorEvt.Dispose(); // Attach the onError callback - _innerWebSocket.SetObjectProperty("onerror", _onError); + _innerWebSocket.AddEventListener("error", _onError); // Setup the onClose callback _onClose = (closeEvent) => OnCloseCallback(closeEvent, cancellationToken); // Attach the onClose callback - _innerWebSocket.SetObjectProperty("onclose", _onClose); + _innerWebSocket.AddEventListener("close", _onClose); // Setup the onOpen callback _onOpen = (evt) => @@ -203,13 +203,13 @@ internal async Task ConnectAsyncJavaScript(Uri uri, CancellationToken cancellati }; // Attach the onOpen callback - _innerWebSocket.SetObjectProperty("onopen", _onOpen); + _innerWebSocket.AddEventListener("open", _onOpen); // Setup the onMessage callback _onMessage = (messageEvent) => OnMessageCallback(messageEvent); // Attach the onMessage callaback - _innerWebSocket.SetObjectProperty("onmessage", _onMessage); + _innerWebSocket.AddEventListener("message", _onMessage); await _tcsConnect.Task.ConfigureAwait(continueOnCapturedContext: true); } catch (Exception wse) @@ -298,7 +298,7 @@ private void OnMessageCallback(JSObject messageEvent) } } }; - reader.Invoke("addEventListener", "loadend", loadend); + reader.AddEventListener("loadend", loadend); reader.Invoke("readAsArrayBuffer", blobData); } break; @@ -318,26 +318,10 @@ private void NativeCleanup() { // We need to clear the events on websocket as well or stray events // are possible leading to crashes. - if (_onClose != null) - { - _innerWebSocket?.SetObjectProperty("onclose", ""); - _onClose = null; - } - if (_onError != null) - { - _innerWebSocket?.SetObjectProperty("onerror", ""); - _onError = null; - } - if (_onOpen != null) - { - _innerWebSocket?.SetObjectProperty("onopen", ""); - _onOpen = null; - } - if (_onMessage != null) - { - _innerWebSocket?.SetObjectProperty("onmessage", ""); - _onMessage = null; - } + _innerWebSocket?.RemoveEventListener("close", _onClose); + _innerWebSocket?.RemoveEventListener("error", _onError); + _innerWebSocket?.RemoveEventListener("open", _onOpen); + _innerWebSocket?.RemoveEventListener("message", _onMessage); } public override void Dispose() 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 73032ff7b3a07..85eb8285f3932 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 @@ -77,6 +77,61 @@ public object Invoke(string method, params object?[] args) return res; } + public struct EventListenerOptions { + public bool Capture; + public bool Once; + public bool Passive; + public object? Signal; + } + + public int AddEventListener(string name, Delegate listener, EventListenerOptions? options = null) + { + var optionsDict = options.HasValue + ? new JSObject() + : null; + + try { + if (options?.Signal != null) + throw new NotImplementedException("EventListenerOptions.Signal"); + + var jsfunc = Runtime.GetJSOwnedObjectHandle(listener); + // int exception; + if (options.HasValue) { + // TODO: Optimize this + var _options = options.Value; + optionsDict?.SetObjectProperty("capture", _options.Capture, true, true); + optionsDict?.SetObjectProperty("once", _options.Once, true, true); + optionsDict?.SetObjectProperty("passive", _options.Passive, true, true); + } + + // TODO: Pass options explicitly instead of using the object + // TODO: Handle errors + // We can't currently do this because adding any additional parameters or a return value causes + // a signature mismatch at runtime + var ret = Interop.Runtime.AddEventListener(JSHandle, name, jsfunc, optionsDict?.JSHandle ?? 0); + if (ret != null) + throw new JSException(ret); + return jsfunc; + } finally { + optionsDict?.Dispose(); + } + } + + public void RemoveEventListener(string name, Delegate? listener, EventListenerOptions? options = null) + { + if (listener == null) + return; + var jsfunc = Runtime.GetJSOwnedObjectHandle(listener); + RemoveEventListener(name, jsfunc, options); + } + + public void RemoveEventListener(string name, int listenerHandle, EventListenerOptions? options = null) + { + var ret = Interop.Runtime.RemoveEventListener(JSHandle, name, listenerHandle, options?.Capture ?? false); + if (ret != null) + throw new JSException(ret); + } + /// /// Returns the named property from the object, or throws a JSException on error. /// 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 34467dbdb9a2e..d30808f44d17d 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 @@ -218,6 +218,74 @@ public static int BindExistingObject(object rawObj, int jsId) return jsObject.Int32Handle; } + private static int NextJSOwnedObjectID = 1; + private static object JSOwnedObjectLock = new object(); + private static Dictionary IDFromJSOwnedObject = new Dictionary(); + private static Dictionary JSOwnedObjectFromID = new Dictionary(); + + // A JSOwnedObject is a managed object with its lifetime controlled by javascript. + // The managed side maintains a strong reference to the object, while the JS side + // maintains a weak reference and notifies the managed side if the JS wrapper object + // has been reclaimed by the JS GC. At that point, the managed side will release its + // strong references, allowing the managed object to be collected. + // This ensures that things like delegates and promises will never 'go away' while JS + // is expecting to be able to invoke or await them. + public static int GetJSOwnedObjectHandle (object o) { + if (o == null) + return 0; + + int result; + lock (JSOwnedObjectLock) { + if (IDFromJSOwnedObject.TryGetValue(o, out result)) + return result; + + result = NextJSOwnedObjectID++; + IDFromJSOwnedObject[o] = result; + JSOwnedObjectFromID[result] = o; + return result; + } + } + + // The JS layer invokes this method when the JS wrapper for a JS owned object + // has been collected by the JS garbage collector + public static void ReleaseJSOwnedObjectByHandle (int id) { + lock (JSOwnedObjectLock) { + if (!JSOwnedObjectFromID.TryGetValue(id, out object? o)) + throw new Exception($"JS-owned object with id {id} was already released"); + IDFromJSOwnedObject.Remove(o); + JSOwnedObjectFromID.Remove(id); + } + } + + // The JS layer invokes this API when the JS wrapper for a delegate is invoked. + // In multiple places this function intentionally returns false instead of throwing + // in an unexpected condition. This is done because unexpected conditions of this + // type are usually caused by a JS object (i.e. a WebSocket) receiving an event + // after its managed owner has been disposed - throwing in that case is unwanted. + public static bool TryInvokeJSOwnedDelegateByHandle (int id, JSObject? arg1) { + Delegate? del; + lock (JSOwnedObjectLock) { + if (!JSOwnedObjectFromID.TryGetValue(id, out object? o)) + return false; + del = (Delegate)o; + } + + if (del == null) + return false; + +// error CS0117: 'Array' does not contain a definition for 'Empty' [/home/kate/Projects/dotnet-runtime-wasm/src/libraries/System.Private.Runtime.InteropServices.JavaScript/src/System.Private.Runtime.InteropServices.JavaScript.csproj] +#pragma warning disable CA1825 + + if (arg1 != null) + del.DynamicInvoke(new object[] { arg1 }); + else + del.DynamicInvoke(new object[0]); + +#pragma warning restore CA1825 + + return true; + } + public static int GetJSObjectId(object rawObj) { JSObject? jsObject; 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 f06939870b594..54a8037a1c46a 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 @@ -223,5 +223,189 @@ public static IEnumerable ToEnumerable(this JSObject iterrator) nextResult?.Dispose(); } } + + private static JSObject SetupListenerTest (string prefix) { + Runtime.InvokeJS($"globalThis.{prefix} = {{" + @" + listeners: [], + addEventListener: function (name, listener, options) { + if (name === 'throwError') + throw new Error('throwError throwing'); + var capture = !options ? false : !!options.capture; + for (var i = 0; i < this.listeners.length; i++) { + var item = this.listeners[i]; + if (item[0] !== name) + continue; + var itemCapture = !item[2] ? false : !!item[2].capture; + if (itemCapture !== capture) + continue; + if (item[1] === listener) + return; + } + this.listeners.push([name, listener, options || null]); + }, + removeEventListener: function (name, listener, capture) { + for (var i = 0; i < this.listeners.length; i++) { + var item = this.listeners[i]; + if (item[0] !== name) + continue; + if (item[1] !== listener) + continue; + var itemCapture = !item[2] ? false : !!item[2].capture; + if (itemCapture !== !!capture) + continue; + this.listeners.splice(i, 1); + return; + } + }, + fireEvent: function (name, evt) { + this._fireEventImpl(name, true, evt); + this._fireEventImpl(name, false, evt); + }, + _fireEventImpl: function (name, capture, evt) { + for (var i = 0; i < this.listeners.length; i++) { + var item = this.listeners[i]; + if (item[0] !== name) + continue; + var itemCapture = !item[2] ? false : (item[2].capture || false); + if (itemCapture !== capture) + continue; + item[1].call(this, evt); + } + }, +}; +"); + return (JSObject)Runtime.GetGlobalObject(prefix); + } + + [Fact] + public static void AddEventListenerWorks () { + var temp = new bool[1]; + var obj = SetupListenerTest("addEventListenerWorks"); + obj.AddEventListener("test", () => { + temp[0] = true; + }); + obj.Invoke("fireEvent", "test"); + Assert.True(temp[0]); + } + + [Fact] + public static void AddEventListenerPassesOptions () { + var log = new List(); + var obj = SetupListenerTest("addEventListenerPassesOptions"); + obj.AddEventListener("test", () => { + log.Add("Capture"); + }, new JSObject.EventListenerOptions { Capture = true }); + obj.AddEventListener("test", () => { + log.Add("Non-capture"); + }, new JSObject.EventListenerOptions { Capture = false }); + obj.Invoke("fireEvent", "test"); + Assert.Equal("Capture", log[0]); + Assert.Equal("Non-capture", log[1]); + } + + [Fact] + public static void AddEventListenerForwardsExceptions () { + var obj = SetupListenerTest("addEventListenerForwardsExceptions"); + obj.AddEventListener("test", () => { + throw new Exception("Test exception"); + }); + var exc = Assert.Throws(() => { + obj.Invoke("fireEvent", "test"); + }); + Assert.Contains("Test exception", exc.Message); + + exc = Assert.Throws(() => { + obj.AddEventListener("throwError", () => { + throw new Exception("Should not be called"); + }); + }); + Assert.Contains("throwError throwing", exc.Message); + obj.Invoke("fireEvent", "throwError"); + } + + [Fact] + public static void RemovedEventListenerIsNotCalled () { + var obj = SetupListenerTest("removedEventListenerIsNotCalled"); + Action del = () => { + throw new Exception("Should not be called"); + }; + obj.AddEventListener("test", del); + Assert.Throws(() => { + obj.Invoke("fireEvent", "test"); + }); + + obj.RemoveEventListener("test", del); + obj.Invoke("fireEvent", "test"); + } + + [Fact] + public static void RegisterSameEventListener () { + var counter = new int[1]; + var obj = SetupListenerTest("registerSameDelegateTwice"); + Action del = () => { + counter[0]++; + }; + + obj.AddEventListener("test1", del); + obj.AddEventListener("test2", del); + + obj.Invoke("fireEvent", "test1"); + Assert.Equal(1, counter[0]); + obj.Invoke("fireEvent", "test2"); + Assert.Equal(2, counter[0]); + + obj.RemoveEventListener("test1", del); + obj.Invoke("fireEvent", "test1"); + obj.Invoke("fireEvent", "test2"); + Assert.Equal(3, counter[0]); + + obj.RemoveEventListener("test2", del); + obj.Invoke("fireEvent", "test1"); + obj.Invoke("fireEvent", "test2"); + Assert.Equal(3, counter[0]); + } + + [Fact] + public static void UseAddEventListenerResultToRemove () { + var obj = SetupListenerTest("useAddEventListenerResultToRemove"); + Action del = () => { + throw new Exception("Should not be called"); + }; + var handle = obj.AddEventListener("test", del); + Assert.Throws(() => { + obj.Invoke("fireEvent", "test"); + }); + + obj.RemoveEventListener("test", handle); + obj.Invoke("fireEvent", "test"); + } + + [Fact] + public static void RegisterSameEventListenerToMultipleSources () { + var counter = new int[1]; + var a = SetupListenerTest("registerSameEventListenerToMultipleSourcesA"); + var b = SetupListenerTest("registerSameEventListenerToMultipleSourcesB"); + Action del = () => { + counter[0]++; + }; + + a.AddEventListener("test", del); + b.AddEventListener("test", del); + + a.Invoke("fireEvent", "test"); + Assert.Equal(1, counter[0]); + b.Invoke("fireEvent", "test"); + Assert.Equal(2, counter[0]); + + a.RemoveEventListener("test", del); + a.Invoke("fireEvent", "test"); + b.Invoke("fireEvent", "test"); + Assert.Equal(3, counter[0]); + + b.RemoveEventListener("test", del); + a.Invoke("fireEvent", "test"); + b.Invoke("fireEvent", "test"); + Assert.Equal(3, counter[0]); + } } } diff --git a/src/mono/wasm/runtime/binding_support.js b/src/mono/wasm/runtime/binding_support.js index d96eeed4ff6d8..d9d02da6db89f 100644 --- a/src/mono/wasm/runtime/binding_support.js +++ b/src/mono/wasm/runtime/binding_support.js @@ -146,6 +146,8 @@ var BindingSupportLib = { this.safehandle_release = get_method ("SafeHandleRelease"); this.safehandle_get_handle = get_method ("SafeHandleGetHandle"); this.safehandle_release_by_handle = get_method ("SafeHandleReleaseByHandle"); + this.release_js_owned_object_by_handle = bind_runtime_method ("ReleaseJSOwnedObjectByHandle", "i"); + this.try_invoke_js_owned_delegate_by_handle = bind_runtime_method ("TryInvokeJSOwnedDelegateByHandle", "io"); this._are_promises_supported = ((typeof Promise === "object") || (typeof Promise === "function")) && (typeof Promise.resolve === "function"); @@ -155,6 +157,54 @@ var BindingSupportLib = { this._interned_string_current_root_buffer = null; this._interned_string_current_root_buffer_count = 0; this._interned_js_string_table = new Map (); + + this._js_owned_object_table = new Map (); + this._js_owned_object_registry = new FinalizationRegistry(this._js_owned_object_finalized.bind(this)); + }, + + _get_weak_delegate_from_handle: function (id) { + var result = null; + + // Look up this handle in the weak delegate table, and if we find a matching weakref, + // deref it to try and get a JS function. This function may have been collected. + if (this._js_owned_object_table.has(id)) { + var wr = this._js_owned_object_table.get(id); + result = wr.deref(); + // Note that we could check for a null result (i.e. function was GC'd) here and + // opt to abort since resurrecting a given ID probably shouldn't happen. + // However, this design makes resurrecting an ID harmless, so there's not any + // value in doing that (and we would then need to differentiate 'new' vs 'get') + // Tracking whether an ID is being resurrected also would require us to keep track + // of every ID that has ever been used, which will harm performance long-term. + } + + // If the function for this handle was already collected (or was never created), + // we create a new function that will invoke the corresponding C# delegate when + // called, and store it into our weak mapping (so we can look it up again by id) + // and register it with the finalization registry so that the C# side can release + // the associated object references + if (!result) { + result = (arg1) => { + if (!this.try_invoke_js_owned_delegate_by_handle(id, arg1)) + // Because lifetime is managed by JavaScript, it *is* an error for this + // invocation to ever fail. If we have a JS wrapper for an ID, there + // should be no way for the managed delegate to have disappeared. + throw new Error(`JS-owned delegate invocation failed for id ${id}`); + }; + + this._js_owned_object_table.set(id, new WeakRef(result)); + this._js_owned_object_registry.register(result, id); + } + return result; + }, + + _js_owned_object_finalized: function (id) { + // The JS function associated with this ID has been collected by the JS GC. + // As such, it's not possible for this ID to be invoked by JS anymore, so + // we can release the tracking weakref (it's null now, by definition), + // and tell the C# side to stop holding a reference to the managed delegate. + this._js_owned_object_table.delete(id); + this.release_js_owned_object_by_handle(id); }, // Ensures the string is already interned on both the managed and JavaScript sides, @@ -2043,6 +2093,59 @@ var BindingSupportLib = { return BINDING.js_to_mono_obj (res) }, + mono_wasm_add_event_listener: function (objHandle, name, listenerId, optionsHandle) { + var nameRoot = MONO.mono_wasm_new_root (name); + try { + BINDING.bindings_lazy_init (); + var obj = BINDING.mono_wasm_require_handle(objHandle); + if (!obj) + throw new Error("Invalid JS object handle"); + var listener = BINDING._get_weak_delegate_from_handle(listenerId); + if (!listener) + throw new Error("Invalid listener ID"); + var sName = BINDING.conv_string(nameRoot.value); + + var options = optionsHandle + ? BINDING.mono_wasm_require_handle(optionsHandle) + : null; + + if (options) + obj.addEventListener(sName, listener, options); + else + obj.addEventListener(sName, listener); + return 0; + } catch (exc) { + return BINDING.js_string_to_mono_string(exc.message); + } finally { + nameRoot.release(); + } + }, + + mono_wasm_remove_event_listener: function (objHandle, name, listenerId, capture) { + var nameRoot = MONO.mono_wasm_new_root (name); + try { + BINDING.bindings_lazy_init (); + var obj = BINDING.mono_wasm_require_handle(objHandle); + if (!obj) + throw new Error("Invalid JS object handle"); + var listener = BINDING._get_weak_delegate_from_handle(listenerId); + // Removing a nonexistent listener should not be treated as an error + if (!listener) + return; + var sName = BINDING.conv_string(nameRoot.value); + + obj.removeEventListener(sName, listener, !!capture); + // We do not manually remove the listener from the delegate registry here, + // because that same delegate may have been used as an event listener for + // other events or event targets. The GC will automatically clean it up + // and trigger the FinalizationRegistry handler if it's unused + return 0; + } catch (exc) { + return BINDING.js_string_to_mono_string(exc.message); + } finally { + nameRoot.release(); + } + }, }; diff --git a/src/mono/wasm/runtime/corebindings.c b/src/mono/wasm/runtime/corebindings.c index a0263c2a8ba9d..9ff03d9a88ec6 100644 --- a/src/mono/wasm/runtime/corebindings.c +++ b/src/mono/wasm/runtime/corebindings.c @@ -25,6 +25,8 @@ extern MonoObject* mono_wasm_typed_array_to_array (int js_handle, int *is_except extern MonoObject* mono_wasm_typed_array_copy_to (int js_handle, int ptr, int begin, int end, int bytes_per_element, int *is_exception); extern MonoObject* mono_wasm_typed_array_from (int ptr, int begin, int end, int bytes_per_element, int type, int *is_exception); extern MonoObject* mono_wasm_typed_array_copy_from (int js_handle, int ptr, int begin, int end, int bytes_per_element, int *is_exception); +extern MonoString* mono_wasm_add_event_listener (int jsObjHandle, MonoString *name, int weakDelegateHandle, int optionsObjHandle); +extern MonoString* mono_wasm_remove_event_listener (int jsObjHandle, MonoString *name, int weakDelegateHandle, int capture); // Compiles a JavaScript function from the function data passed. // Note: code snippet is not a function definition. Instead it must create and return a function instance. @@ -85,6 +87,8 @@ void core_initialize_internals () mono_add_internal_call ("Interop/Runtime::TypedArrayFrom", mono_wasm_typed_array_from); mono_add_internal_call ("Interop/Runtime::TypedArrayCopyFrom", mono_wasm_typed_array_copy_from); mono_add_internal_call ("Interop/Runtime::CompileFunction", mono_wasm_compile_function); + mono_add_internal_call ("Interop/Runtime::AddEventListener", mono_wasm_add_event_listener); + mono_add_internal_call ("Interop/Runtime::RemoveEventListener", mono_wasm_remove_event_listener); }