Skip to content

Remove support for using Java's WeakReference instead of JNI's weak handles #1257

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.

Sign up for GitHub

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Remove support for using Java's WeakReference instead of JNI's weak handles #1257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
49e01de
Remove support for using Java's WeakReference instead of JNI's weak h…
filipnavara Sep 23, 2024
0087a52
Remove unused weak_handle field
filipnavara Sep 23, 2024
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
1 change: 0 additions & 1 deletion src/Java.Interop/Java.Interop/JavaException.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ unsafe public class JavaException : Exception, IJavaPeerable
JniObjectReferenceType handle_type;
#pragma warning disable 0169
// Used by JavaInteropGCBridge
IntPtr weak_handle;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't remove this field yet, as it's still used by dotnet/android, even if it isn't "used":

Removing it now will result in app startup aborts until dotnet/android is updated to no longer mention weak_handle.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realize that all this code is duplicated in dotnet/android. I'll return back the field and try to see if we can remove it on dotnet/android side too. The lowest supported API level is 21 for .NET 9, so there doesn't seem to be a valid reason to keep this around.

int refs_added;
#pragma warning restore 0169
#endif // FEATURE_JNIOBJECTREFERENCE_INTPTRS
Expand Down
1 change: 0 additions & 1 deletion src/Java.Interop/Java.Interop/JavaObject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ unsafe public class JavaObject : IJavaPeerable
JniObjectReferenceType handle_type;
#pragma warning disable 0169
// Used by JavaInteropGCBridge
IntPtr weak_handle;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

int refs_added;
#pragma warning restore 0169
#endif // FEATURE_JNIOBJECTREFERENCE_INTPTRS
Expand Down
9 changes: 2 additions & 7 deletions src/Java.Runtime.Environment/Java.Interop/MonoRuntimeValueManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,6 @@

namespace Java.Interop {

enum GCBridgeUseWeakReferenceKind {
Java,
Jni,
}

class MonoRuntimeValueManager : JniRuntime.JniValueManager {

#pragma warning disable 0649
Expand Down Expand Up @@ -52,7 +47,7 @@ public override void OnSetRuntime (JniRuntime runtime)
bridge = IntPtr.Zero;
throw;
}
if (JreNativeMethods.java_interop_gc_bridge_register_hooks (bridge, GCBridgeUseWeakReferenceKind.Jni) < 0)
if (JreNativeMethods.java_interop_gc_bridge_register_hooks (bridge) < 0)
throw new NotSupportedException ("Could not register GC Bridge with Mono!");
}

Expand Down Expand Up @@ -393,7 +388,7 @@ partial class JreNativeMethods {
internal static extern int java_interop_gc_bridge_set_current_once (IntPtr bridge);

[DllImport (JavaInteropLib, CallingConvention=CallingConvention.Cdecl)]
internal static extern int java_interop_gc_bridge_register_hooks (IntPtr bridge, GCBridgeUseWeakReferenceKind weak_ref_kind);
internal static extern int java_interop_gc_bridge_register_hooks (IntPtr bridge);
Copy link
Member

@jonpryor jonpryor Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change introduces a "mismatch" between the native side and the managed side. (Native side has an additional [[maybe_unused]] int weak_ref_kind parameter.) This is nominally "fine" because it's C calling convention, but if we ever accidentally-on-purpose change the calling convention, the difference in number of parameters could cause stack corruption.

This should instead be:

internal static extern int java_interop_gc_bridge_register_hooks (IntPtr bridge, int must_be_one);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was unintentional. I initially kept the parameter as unused and then changed my mind to remove it…


[DllImport (JavaInteropLib, CallingConvention=CallingConvention.Cdecl)]
internal static extern IntPtr java_interop_gc_bridge_new (IntPtr jvm);
Expand Down
118 changes: 10 additions & 108 deletions src/java-interop/java-interop-gc-bridge-mono.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ typedef struct MonoJavaGCBridgeInfo {
MonoClassField *handle;
MonoClassField *handle_type;
MonoClassField *refs_added;
MonoClassField *weak_handle;
} MonoJavaGCBridgeInfo;

#define NUM_GC_BRIDGE_TYPES (4)
Expand All @@ -60,10 +59,6 @@ struct JavaInteropGCBridge {
jobject Runtime_instance;
jmethodID Runtime_gc;

jclass WeakReference_class;
jmethodID WeakReference_init;
jmethodID WeakReference_get;

jclass GCUserPeerable_class;
jmethodID GCUserPeerable_add;
jmethodID GCUserPeerable_clear;
Expand Down Expand Up @@ -106,12 +101,9 @@ java_interop_gc_bridge_destroy (JavaInteropGCBridge *bridge)
JNIEnv *env = ensure_jnienv (bridge);
if (env != NULL) {
env->DeleteGlobalRef (bridge->Runtime_instance);
env->DeleteGlobalRef (bridge->WeakReference_class);
env->DeleteGlobalRef (bridge->GCUserPeerable_class);

bridge->Runtime_instance = NULL;
bridge->WeakReference_class = NULL;

bridge->Runtime_instance = NULL;
bridge->GCUserPeerable_class = NULL;
}

Expand Down Expand Up @@ -277,13 +269,6 @@ java_interop_gc_bridge_new (JavaVM *jvm)
env->DeleteLocalRef (Runtime_class);
}

jclass WeakReference_class = env->FindClass ("java/lang/ref/WeakReference");
if (WeakReference_class != NULL) {
bridge.WeakReference_init = env->GetMethodID (WeakReference_class, "<init>", "(Ljava/lang/Object;)V");
bridge.WeakReference_get = env->GetMethodID (WeakReference_class, "get", "()Ljava/lang/Object;");
bridge.WeakReference_class = static_cast<jclass>(lref_to_gref (env, WeakReference_class));
}

jclass GCUserPeerable_class = env->FindClass ("net/dot/jni/GCUserPeerable");
if (GCUserPeerable_class) {
bridge.GCUserPeerable_add = env->GetMethodID (GCUserPeerable_class, "jiAddManagedReference", "(Ljava/lang/Object;)V");
Expand All @@ -295,8 +280,7 @@ java_interop_gc_bridge_new (JavaVM *jvm)
JavaInteropGCBridge *p = static_cast<JavaInteropGCBridge*>(calloc (1, sizeof (JavaInteropGCBridge)));

if (p == NULL || bridge.jvm == NULL ||
bridge.Runtime_instance == NULL || bridge.Runtime_gc == NULL ||
bridge.WeakReference_class == NULL || bridge.WeakReference_init == NULL || bridge.WeakReference_get == NULL) {
bridge.Runtime_instance == NULL || bridge.Runtime_gc == NULL) {
java_interop_gc_bridge_destroy (&bridge);
free (p);
return NULL;
Expand Down Expand Up @@ -369,10 +353,9 @@ java_interop_gc_bridge_register_bridgeable_type (
info->handle = mono_class_get_field_from_name (info->klass, const_cast<char*> ("handle"));
info->handle_type = mono_class_get_field_from_name (info->klass, const_cast<char*> ("handle_type"));
info->refs_added = mono_class_get_field_from_name (info->klass, const_cast<char*> ("refs_added"));
info->weak_handle = mono_class_get_field_from_name (info->klass, const_cast<char*> ("weak_handle"));

if (info->klass == NULL || info->handle == NULL || info->handle_type == NULL ||
info->refs_added == NULL || info->weak_handle == NULL)
info->refs_added == NULL)
return -1;
bridge->num_bridge_types++;
return 0;
Expand Down Expand Up @@ -772,72 +755,8 @@ get_gc_bridge_info_for_object (JavaInteropGCBridge *bridge, MonoObject *object)
return get_gc_bridge_info_for_class (bridge, mono_object_get_class (object));
}

typedef mono_bool (*MonodroidGCTakeRefFunc) (JavaInteropGCBridge *bridge, JNIEnv *env, MonoObject *obj, const char *thread_name, int64_t thread_id);

static MonodroidGCTakeRefFunc take_global_ref;
static MonodroidGCTakeRefFunc take_weak_global_ref;

static mono_bool
take_global_ref_java (JavaInteropGCBridge *bridge, JNIEnv *env, MonoObject *obj, const char *thread_name, int64_t thread_id)
{
MonoJavaGCBridgeInfo *bridge_info = get_gc_bridge_info_for_object (bridge, obj);
if (bridge_info == NULL)
return 0;

jobject weak;
mono_field_get_value (obj, bridge_info->weak_handle, &weak);

jobject handle = env->CallObjectMethod (weak, bridge->WeakReference_get);
log_gref (bridge, "*try_take_global_2_1 obj=%p -> wref=%p handle=%p\n", obj, weak, handle);

if (handle) {
jobject h = env->NewGlobalRef (handle);
env->DeleteLocalRef (handle);
handle = h;
java_interop_gc_bridge_gref_log_new (bridge, weak, get_object_ref_type (env, weak),
handle, get_object_ref_type (env, handle), thread_name, thread_id, "take_global_ref_java");
}
java_interop_gc_bridge_weak_gref_log_delete (bridge, weak, get_object_ref_type (env, weak), thread_name, thread_id, "take_global_ref_java");
env->DeleteGlobalRef (weak);
weak = NULL;
mono_field_set_value (obj, bridge_info->weak_handle, &weak);

mono_field_set_value (obj, bridge_info->handle, &handle);

int type = JNIGlobalRefType;
mono_field_set_value (obj, bridge_info->handle_type, &type);

return handle != NULL;
}

static mono_bool
take_weak_global_ref_java (JavaInteropGCBridge *bridge, JNIEnv *env, MonoObject *obj, const char *thread_name, int64_t thread_id)
{
MonoJavaGCBridgeInfo *bridge_info = get_gc_bridge_info_for_object (bridge, obj);
if (bridge_info == NULL)
return 0;

jobject handle;
mono_field_get_value (obj, bridge_info->handle, &handle);

jobject weaklocal = env->NewObject (bridge->WeakReference_class, bridge->WeakReference_init, handle);
jobject weakglobal = env->NewGlobalRef (weaklocal);
env->DeleteLocalRef (weaklocal);

log_gref (bridge, "*take_weak_2_1 obj=%p -> wref=%p handle=%p\n", obj, weakglobal, handle);
java_interop_gc_bridge_weak_gref_log_new (bridge, handle, get_object_ref_type (env, handle),
weakglobal, get_object_ref_type (env, weakglobal), thread_name, thread_id, "take_weak_global_ref_2_1_compat");

java_interop_gc_bridge_gref_log_delete (bridge, handle, get_object_ref_type (env, handle), thread_name, thread_id, "take_weak_global_ref_2_1_compat");
env->DeleteGlobalRef (handle);

mono_field_set_value (obj, bridge_info->weak_handle, &weakglobal);

return 1;
}

static mono_bool
take_global_ref_jni (JavaInteropGCBridge *bridge, JNIEnv *env, MonoObject *obj, const char *thread_name, int64_t thread_id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer not changing these method names & related strings, as:

  1. The same names are used in dotnet/android, and consistency is "nice"
  2. The strings are present in tools/logcat-parse unit tests (tests/logcat-parse-Tests), which implies (a) they've been around awhile, and (2) I'm not immediately sure what'll happen to logcat-parse we change them. (Hopefully nothing! But…)

take_global_ref (JavaInteropGCBridge *bridge, JNIEnv *env, MonoObject *obj, const char *thread_name, int64_t thread_id)
{
MonoJavaGCBridgeInfo *bridge_info = get_gc_bridge_info_for_object (bridge, obj);
if (bridge_info == NULL)
Expand All @@ -853,11 +772,11 @@ take_global_ref_jni (JavaInteropGCBridge *bridge, JNIEnv *env, MonoObject *obj,
java_interop_gc_bridge_gref_log_new (bridge, weak, get_object_ref_type (env, weak),
handle, get_object_ref_type (env, handle),
thread_name, thread_id,
"take_global_ref_jni");
"take_global_ref");
}

java_interop_gc_bridge_weak_gref_log_delete (bridge, weak, 'W',
thread_name, thread_id, "take_global_ref_jni");
thread_name, thread_id, "take_global_ref");
env->DeleteWeakGlobalRef (weak);

mono_field_set_value (obj, bridge_info->handle, &handle);
Expand All @@ -868,7 +787,7 @@ take_global_ref_jni (JavaInteropGCBridge *bridge, JNIEnv *env, MonoObject *obj,
}

static mono_bool
take_weak_global_ref_jni (JavaInteropGCBridge *bridge, JNIEnv *env, MonoObject *obj, const char *thread_name, int64_t thread_id)
take_weak_global_ref (JavaInteropGCBridge *bridge, JNIEnv *env, MonoObject *obj, const char *thread_name, int64_t thread_id)
{
MonoJavaGCBridgeInfo *bridge_info = get_gc_bridge_info_for_object (bridge, obj);
if (bridge_info == NULL)
Expand All @@ -882,10 +801,10 @@ take_weak_global_ref_jni (JavaInteropGCBridge *bridge, JNIEnv *env, MonoObject *
jobject weak = env->NewWeakGlobalRef (handle);
java_interop_gc_bridge_weak_gref_log_new (bridge, handle, get_object_ref_type (env, handle),
weak, get_object_ref_type (env, weak),
thread_name, thread_id, "take_weak_global_ref_jni");
thread_name, thread_id, "take_weak_global_ref");

java_interop_gc_bridge_gref_log_delete (bridge, handle, get_object_ref_type (env, handle),
thread_name, thread_id, "take_weak_global_ref_jni");
thread_name, thread_id, "take_weak_global_ref");
env->DeleteGlobalRef (handle);

mono_field_set_value (obj, bridge_info->handle, &weak);
Expand Down Expand Up @@ -1272,7 +1191,7 @@ gc_cross_references (int num_sccs, MonoGCBridgeSCC **sccs, int num_xrefs, MonoGC
}

int
java_interop_gc_bridge_register_hooks (JavaInteropGCBridge *bridge, int weak_ref_kind)
java_interop_gc_bridge_register_hooks (JavaInteropGCBridge *bridge, [[maybe_unused]] int weak_ref_kind)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tried building this, but I'm kinda shocked at the idea that this would build and/or run, considering that you updated the header file to remove the second parameter.

So of course I need to play around…

extern "C" {
    void foo();
}

void foo([[maybe_unused]] int x) {
}

int main() {
    foo(0);
}

It compiles without error:

% clang++ mu.cc -std=c++17

…but the symbols, they're mangled!

% nm a.out   
0000000100003f80 T __Z3fooi
0000000100000000 T __mh_execute_header
0000000100003f90 T _main

There is no foo symbol, but there is __Z3fooi, the C++ mangled version of foo(int).

My guess -- again, without compiling this PR -- is that src/java-interop will compile, but the resulting native library will not contain a C callable java_interop_gc_bridge_register_hooks, but instead a C++ mangled __Z37java_interop_gc_bridge_register_hooks… symbol, and things will blow up at runtime because the P/Invoke for java_interop_gc_bridge_register_hooks() won't find that symbol.

{
if (bridge == NULL)
return -1;
Expand All @@ -1284,23 +1203,6 @@ java_interop_gc_bridge_register_hooks (JavaInteropGCBridge *bridge, int weak_ref
MonoGCBridgeCallbacks bridge_cbs;
memset (&bridge_cbs, 0, sizeof (bridge_cbs));

switch (weak_ref_kind) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should assert that weak_ref_kind is one, and bail otherwise:

if (weak_ref_kind != 1) {
    return -1;
}

case JAVA_INTEROP_GC_BRIDGE_USE_WEAK_REFERENCE_KIND_JAVA:
message = "Using java.lang.ref.WeakReference for JNI Weak References.";
take_global_ref = take_global_ref_java;
take_weak_global_ref = take_weak_global_ref_java;
break;
case JAVA_INTEROP_GC_BRIDGE_USE_WEAK_REFERENCE_KIND_JNI:
message = "Using JNIEnv::NewWeakGlobalRef() for JNI Weak References.";
take_global_ref = take_global_ref_jni;
take_weak_global_ref = take_weak_global_ref_jni;
break;
default:
return -1;
}

log_gref (mono_bridge, "%s\n", message);

bridge_cbs.bridge_version = SGEN_BRIDGE_VERSION;
bridge_cbs.bridge_class_kind = gc_bridge_class_kind;
bridge_cbs.is_bridge_object = gc_is_bridge_object;
Expand Down
7 changes: 1 addition & 6 deletions src/java-interop/java-interop-gc-bridge.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,6 @@ JAVA_INTEROP_BEGIN_DECLS

typedef struct JavaInteropGCBridge JavaInteropGCBridge;

typedef enum JavaInteropGCBridgeUseWeakReferenceKind {
JAVA_INTEROP_GC_BRIDGE_USE_WEAK_REFERENCE_KIND_JAVA,
JAVA_INTEROP_GC_BRIDGE_USE_WEAK_REFERENCE_KIND_JNI,
} JavaInteropGCBridgeUseWeakReferenceKind;

struct JavaInterop_System_RuntimeTypeHandle {
void *value;
};
Expand All @@ -25,7 +20,7 @@ JAVA_INTEROP_API int java_interop_gc_bridge_set_current_o
JAVA_INTEROP_API JavaInteropGCBridge *java_interop_gc_bridge_new (JavaVM *jvm);
JAVA_INTEROP_API int java_interop_gc_bridge_free (JavaInteropGCBridge *bridge);

JAVA_INTEROP_API int java_interop_gc_bridge_register_hooks (JavaInteropGCBridge *bridge, int weak_ref_kind);
JAVA_INTEROP_API int java_interop_gc_bridge_register_hooks (JavaInteropGCBridge *bridge);
JAVA_INTEROP_API int java_interop_gc_bridge_wait_for_bridge_processing (JavaInteropGCBridge *bridge);

JAVA_INTEROP_API int java_interop_gc_bridge_add_current_app_domain (JavaInteropGCBridge *bridge);
Expand Down