Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wasm] Fix NRE caused by WeakReference collected on JS interop boundary. #54453

Merged
merged 1 commit into from
Jul 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/libraries/Common/src/Interop/Browser/Interop.Runtime.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand All @@ -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();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -577,7 +577,6 @@ public static IEnumerable<object[]> GetAsync_ManyDifferentResponseHeaders_Parsed

[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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -321,7 +320,6 @@ await SendAsync(

[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))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ public object this[int i]

if (exception != 0)
throw new JSException((string)indexValue);
Interop.Runtime.ReleaseInFlight(indexValue);
return indexValue;
}
set
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ public static int BindJSObject(int jsId, bool ownsHandle, int mappedType)
}
}

target.AddInFlight();

return target.Int32Handle;
}

Expand Down Expand Up @@ -236,12 +238,21 @@ public static int GetJSObjectId(object rawObj)
return jsObject?.JSHandle ?? -1;
}

public static object? GetDotNetObject(int gcHandle)
/// <param name="gcHandle"></param>
/// <param name="shouldAddInflight">when true, we would create Normal GCHandle to the JSObject, so that it would not get collected before passing it back to managed code</param>
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)
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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];
pavelsavara marked this conversation as resolved.
Show resolved Hide resolved
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<object> ToEnumerable(this JSObject iterrator)
{
JSObject nextResult = null;
try
{
nextResult = (JSObject)iterrator.Invoke("next");
var done = (bool)nextResult.GetObjectProperty("done");
pavelsavara marked this conversation as resolved.
Show resolved Hide resolved
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();
}
}
}
}
28 changes: 18 additions & 10 deletions src/mono/wasm/runtime/binding_support.js
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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.
Expand All @@ -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;
Expand Down Expand Up @@ -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)
Expand Down