WASM C APIs need a rooting audit

[kg]

We pass lots of bare managed pointers to the C APIs from JavaScript, and we don't always root those pointers. Up until this point we've been operating on the assumption that the pointer is properly rooted once it enters the runtime, but I realized that the GC can't actually reach function locals/parameters, so that may not actually be true.

I expect handle-based functions are fine since they do explicit lifetime management, but almost all the stuff we call from JS doesn't use handles. We manually root things appropriately in some cases, but I think we're probably doing the wrong things in others (for example, call_method takes a raw managed pointer for 'this'.)

[ghost]

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

We pass lots of bare managed pointers to the C APIs from JavaScript, and we don't always root those pointers. Up until this point we've been operating on the assumption that the pointer is properly rooted once it enters the runtime, but I realized that the GC can't actually reach function locals/parameters, so that may not actually be true.

I expect handle-based functions are fine since they do explicit lifetime management, but almost all the stuff we call from JS doesn't use handles. We manually root things appropriately in some cases, but I think we're probably doing the wrong things in others (for example, call_method takes a raw managed pointer for 'this'.)

Reproduction Steps

N/A

Expected behavior

N/A

Actual behavior

N/A

Regression?

N/A

Known Workarounds

N/A

Configuration

N/A

Other information

N/A

Author:	kg
Assignees:	-
Labels:	arch-wasm
Milestone:	-

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[kg]

@vargaz @lambdageek (tag someone else if you think they're a better person to ask) do you know if there's a straightforward way for us to ensure all the managed pointers that hit C are reachable by the GC? Worst case, we can root them all in JS first.

[lambdageek]

I thought that in the single-threaded runtime we only kick off a GC when there's nothing on the C stack (ie we explicitly run the GC when we return to the JS event loop).

In the multi-threaded runtime I don't think we'd be able to make the same guarantee. So there we would need to ensure that all managed pointers are visible. Do we control all the JS-to-C entrypoints?

[lewing]

I thought that in the single-threaded runtime we only kick off a GC when there's nothing on the C stack (ie we explicitly run the GC when we return to the JS event loop).

This hasn't been true for a while

runtime/src/mono/wasm/runtime/driver.c

Line 1171 in 76e68db

mono_wasm_enable_on_demand_gc (int enable)

(we gc when we need space now).

cc @vargaz

[kg]

@lewing pointed out that AOT was recently changed to try and address this problem there efficiently, so maybe a similar approach in all the right spots in C would do this? I'm not certain that's enough though.

#59352

[lambdageek]

@lewing pointed out that AOT was recently changed to try and address this problem there efficiently, so maybe a similar approach in all the right spots in C would do this? I'm not certain that's enough though.

That's more or less what coop handles do. So my advice would be to use handles.

See, for example

runtime/src/mono/mono/metadata/object.c

Lines 170 to 175 in 7144c2f

	mono_runtime_object_init_checked (MonoObject *this_obj_raw, MonoError *error)
	{
	HANDLE_FUNCTION_ENTER ();
	MONO_HANDLE_DCL (MonoObject, this_obj);
	gboolean const result = mono_runtime_object_init_handle (this_obj, error);
	HANDLE_FUNCTION_RETURN_VAL (result);

Essentially you'd do:

int
some_wasm_function (MonoObject *x_raw, MonoObject *y_raw)
{
  int result;
  HANDLE_FUCTION_ENTER();
  MONO_HANDLE_DCL  (MonoObject, x);
  MONO_HANDLE_DCL (MonoObject, y);
  result = some_wasm_function_impl (x_raw, y_raw);
  HANDLE_FUNCTION_RETURN_VAL (result);
}

(note that nowadays you don't really need to pass the MonoObjectHandle x and y values down to the impl. As long as the handles are created, the callees can use raw pointers)

[kg]

OK, so if we make sure every function exposed to JS uses handles internally, we can safely pass object pointers in. That sounds reasonable.

[kg]

@lambdageek so after messing around with this, one obstacle seems to be that handles aren't exposed to wasm code (driver.c), much like some other internals. Is there a straightforward way for me to address this - just add handle.h to a list somewhere? It seems like handle.h touches other stuff that is also internal and I don't have a good understanding of our internal/external header situation when it comes to handles, I would have expected them to be available.

[ilonatommy]

@lambdageek, @lewing, does any of you have a clue how to help with exposing handlers?

[lambdageek]

@ilonatommy We're now using a slightly different plan - with @kg.

There are three things that we're doing for the mono native-to-JS APIs:

1. We're using MONO_ENTER_GC_UNSAFE in the wasm C APIs (by including gc-common.h)
2. When the C code calls out to JS, we should wrap those calls in MONO_ENTER_GC_SAFE
3. To pass pointers to mono objects between JS and C, we're using the PPVOLATILE(T) macro: for example PPVOLATILE(MonoObject) or just MonoObject * volatile * for arguments. On the JS side, we use WasmRoot<T> for the arguments. To do return values, we pass in an "out" argument that the C side fills in.

The goal of this issue is to look at all the functions in exports.ts (ie: all the functions that cross the JS to C boundary - both JS calls from C and also C calls from JS) and to verify that all of them either follow the above pattern or we have a note for why they're the exception.

[pavelsavara]

The goal of this issue is to look at all the functions in exports.ts (ie: all the functions that cross the JS to C boundary - both JS calls from C and also C calls from JS) and to verify that all of them either follow the above pattern or we have a note for why they're the exception.

We have some stop-gap functions with _ref or _root suffix.
After we merge the new interop, I hope we could get rid of most of API in exports.ts which deals with pointers to managed objects. The new interop returns proxies (with GCHandle inside) instead of fragile pointers.

We need to discuss the remaining scenarios, where we still need to expose things like MonoArray*, MonoString* or MonoObject* to JS API. What are they ?

[kg]

I believe we have comprehensively audited and revised things on our end at this point, the only remaining task is to ensure that downstream users like Blazor, Uno, etc have migrated.