Skip to content

Commit

Permalink
[native, Mono.Android] Remove WeakReference support
Browse files Browse the repository at this point in the history
Context: dotnet/java-interop#1255
Context: xamarin/monodroid@a5c5290
Context: https://issuetracker.google.com/issues/36986478
Context: https://github.com/dotnet/android/blob/b56d68fe2a2c2dffa47eb7e41ae08a00fda86367/Documentation/workflow/SystemProperties.md#debugmonowref

Android did not support [JNI Weak Global References][0] until API-8
(Android 2.2).  To allow Mono for Android applications to run on
API-7 and earlier devices, while using JNI Weak Global References
when possible (fewer GC allocations, better performance), the GC
bridge supported two modes of operation:

  * `debug.mono.wref=jni`: Use JNI Weak Global References.
  * `debug.mono.wref=java`: Use [`java.lang.ref.WeakReference`][1].

With the exception of a ["minor" (lol) issue][2] in KitKat v4.4.2
(API-19) -- in which `JNIEnv::NewGlobalRef()` around a collected
JNI Weak Global Reference would return a non-`NULL` value, which
would result in app crashes; xamarin/monodroid@a5c52905 added
a workaround to use the `WeakReference` backend on Android 4.4.2 --
the `debug.mono.wref=java` backend has not been used since *2014*.

Apps *could* use [`@(AndroidEnvironment)`][3] to set
`debug.mono.wref=java`, but there is no reason to do so (worse
performance!), [nor has anyone done so on GitHub.com][4]
(no matches for `debug.mono.wref` within files that would be used
with `@(AndroidEnvironment)`).

Cleanup src/native and src/Mono.Android, and *remove* support for the
`debug.mono.wref=java` backend.

[0]: https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#weak_global_references
[1]: https://developer.android.com/reference/java/lang/ref/WeakReference
[2]: https://issuetracker.google.com/issues/36986478
[3]: https://learn.microsoft.com/dotnet/android/building-apps/build-items#androidenvironment
[4]: https://github.com/search?type=code&q=debug.mono.wref
  • Loading branch information
jonpryor committed Oct 23, 2024
1 parent b56d68f commit 55632c3
Show file tree
Hide file tree
Showing 7 changed files with 5 additions and 128 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,11 @@
<field name="handle" />
<field name="handle_type" />
<field name="refs_added" />
<field name="weak_handle" />
</type>
<type fullname="Java.Interop.JavaException">
<field name="handle" />
<field name="handle_type" />
<field name="refs_added" />
<field name="weak_handle" />
</type>
</assembly>
</linker>
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,12 @@
<field name="handle" />
<field name="handle_type" />
<field name="refs_added" />
<field name="weak_handle" />
<method name="SetHandleOnDeserialized" />
</type>
<type fullname="Java.Lang.Throwable">
<field name="handle" />
<field name="handle_type" />
<field name="refs_added" />
<field name="weak_handle" />
</type>
</assembly>
</linker>
4 changes: 0 additions & 4 deletions src/Mono.Android/Java.Lang/Object.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ public partial class Object : IDisposable, IJavaObject, IJavaObjectEx
{
[NonSerialized] IntPtr key_handle;
#pragma warning disable CS0649, CS0169, CS0414 // Suppress fields are never used warnings, these fields are used directly by monodroid-glue.cc
[NonSerialized] IntPtr weak_handle;
[NonSerialized] int refs_added;
#pragma warning restore CS0649, CS0169, CS0414
[NonSerialized] JObjectRefType handle_type;
Expand Down Expand Up @@ -117,9 +116,6 @@ public virtual JniPeerMembers JniPeerMembers {
[EditorBrowsable (EditorBrowsableState.Never)]
public IntPtr Handle {
get {
if (weak_handle != IntPtr.Zero)
Logger.Log (LogLevel.Warn, "Mono.Android.dll", "Accessing object which is out for collection via original handle");

return handle;
}
}
Expand Down
1 change: 0 additions & 1 deletion src/Mono.Android/Java.Lang/Throwable.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ public partial class Throwable : global::System.Exception, IJavaObject, IDisposa
IntPtr key_handle;
JObjectRefType handle_type;
#pragma warning disable CS0649, CS0169, CS0414 // Suppress fields are never used warnings, these fields are used directly by monodroid-glue.cc
IntPtr weak_handle;
int refs_added;
#pragma warning restore CS0649, CS0169, CS0414

Expand Down
8 changes: 3 additions & 5 deletions src/native/monodroid/monodroid-glue.cc
Original file line number Diff line number Diff line change
Expand Up @@ -781,17 +781,15 @@ MonodroidRuntime::lookup_bridge_info (MonoClass *klass, const OSBridge::MonoJava
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 == nullptr || info->handle == nullptr || info->handle_type == nullptr || info->refs_added == nullptr || info->weak_handle == nullptr) {
if (info->klass == nullptr || info->handle == nullptr || info->handle_type == nullptr || info->refs_added == nullptr) {
Helpers::abort_application (
Util::monodroid_strdup_printf (
"The type `%s.%s` is missing required instance fields! handle=%p handle_type=%p refs_added=%p weak_handle=%p",
"The type `%s.%s` is missing required instance fields! handle=%p handle_type=%p refs_added=%p",
type->_namespace,
type->_typename,
info->handle,
info->handle_type,
info->refs_added,
info->weak_handle
info->refs_added
)
);
}
Expand Down
112 changes: 2 additions & 110 deletions src/native/monodroid/osbridge.cc
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ OSBridge::MonoJavaGCBridgeInfo OSBridge::empty_bridge_info = {
nullptr,
nullptr,
nullptr,
nullptr,
nullptr
};

Expand Down Expand Up @@ -81,7 +80,6 @@ OSBridge::clear_mono_java_gc_bridge_info ()
info->handle = nullptr;
info->handle_type = nullptr;
info->refs_added = nullptr;
info->weak_handle = nullptr;
}
}

Expand Down Expand Up @@ -443,67 +441,6 @@ OSBridge::monodroid_disable_gc_hooks ()
gc_disabled = 1;
}

mono_bool
OSBridge::take_global_ref_2_1_compat (JNIEnv *env, MonoObject *obj)
{
jobject handle, weak;
int type = JNIGlobalRefType;

MonoJavaGCBridgeInfo *bridge_info = get_gc_bridge_info_for_object (obj);
if (bridge_info == nullptr)
return 0;

mono_field_get_value (obj, bridge_info->weak_handle, &weak);
handle = env->CallObjectMethod (weak, weakrefGet);
if (gref_log) {
fprintf (gref_log, "*try_take_global_2_1 obj=%p -> wref=%p handle=%p\n", obj, weak, handle);
fflush (gref_log);
}
if (handle) {
void* h = env->NewGlobalRef (handle);
env->DeleteLocalRef (handle);
handle = reinterpret_cast <jobject> (h);
_monodroid_gref_log_new (weak, get_object_ref_type (env, weak),
handle, get_object_ref_type (env, handle), "finalizer", gettid (), __PRETTY_FUNCTION__, 0);
}
_monodroid_weak_gref_delete (weak, get_object_ref_type (env, weak), "finalizer", gettid(), __PRETTY_FUNCTION__, 0);
env->DeleteGlobalRef (weak);
weak = nullptr;
mono_field_set_value (obj, bridge_info->weak_handle, &weak);

mono_field_set_value (obj, bridge_info->handle, &handle);
mono_field_set_value (obj, bridge_info->handle_type, &type);
return handle != nullptr;
}

mono_bool
OSBridge::take_weak_global_ref_2_1_compat (JNIEnv *env, MonoObject *obj)
{
jobject weaklocal;
jobject handle, weakglobal;

MonoJavaGCBridgeInfo *bridge_info = get_gc_bridge_info_for_object (obj);
if (bridge_info == nullptr)
return 0;

mono_field_get_value (obj, bridge_info->handle, &handle);
weaklocal = env->NewObject (weakrefClass, weakrefCtor, handle);
weakglobal = env->NewGlobalRef (weaklocal);
env->DeleteLocalRef (weaklocal);
if (gref_log) {
fprintf (gref_log, "*take_weak_2_1 obj=%p -> wref=%p handle=%p\n", obj, weakglobal, handle);
fflush (gref_log);
}
_monodroid_weak_gref_new (handle, get_object_ref_type (env, handle),
weakglobal, get_object_ref_type (env, weakglobal), "finalizer", gettid (), __PRETTY_FUNCTION__, 0);

_monodroid_gref_log_delete (handle, get_object_ref_type (env, handle), "finalizer", gettid (), __PRETTY_FUNCTION__, 0);

env->DeleteGlobalRef (handle);
mono_field_set_value (obj, bridge_info->weak_handle, &weakglobal);
return 1;
}

mono_bool
OSBridge::take_global_ref_jni (JNIEnv *env, MonoObject *obj)
{
Expand Down Expand Up @@ -1001,58 +938,13 @@ OSBridge::gc_cross_references (int num_sccs, MonoGCBridgeSCC **sccs, int num_xre
set_bridge_processing_field (domains_list, 0);
}

int
OSBridge::platform_supports_weak_refs (void)
{
char *value;
int api_level = 0;

if (AndroidSystem::monodroid_get_system_property ("ro.build.version.sdk", &value) > 0) {
api_level = atoi (value);
free (value);
}

if (AndroidSystem::monodroid_get_system_property (SharedConstants::DEBUG_MONO_WREF_PROPERTY, &value) > 0) {
int use_weak_refs = 0;
if (!strcmp ("jni", value))
use_weak_refs = 1;
else if (!strcmp ("java", value))
use_weak_refs = 0;
else {
use_weak_refs = -1;
log_warn (LOG_GC, "Unsupported debug.mono.wref value '%s'; "
"supported values are 'jni' and 'java'. Ignoring...",
value);
}
free (value);

if (use_weak_refs && api_level < 8)
log_warn (LOG_GC, "Using JNI weak references instead of "
"java.lang.WeakReference on API-%i. Are you sure you want to do this? "
"The GC may be compromised.",
api_level);

if (use_weak_refs >= 0)
return use_weak_refs;
}

return 1;
}

void
OSBridge::register_gc_hooks (void)
{
MonoGCBridgeCallbacks bridge_cbs;

if (platform_supports_weak_refs ()) {
take_global_ref = &OSBridge::take_global_ref_jni;
take_weak_global_ref = &OSBridge::take_weak_global_ref_jni;
log_info (LOG_GC, "environment supports jni NewWeakGlobalRef");
} else {
take_global_ref = &OSBridge::take_global_ref_2_1_compat;
take_weak_global_ref = &OSBridge::take_weak_global_ref_2_1_compat;
log_info (LOG_GC, "environment does not support jni NewWeakGlobalRef");
}
take_global_ref = &OSBridge::take_global_ref_jni;
take_weak_global_ref = &OSBridge::take_weak_global_ref_jni;

bridge_cbs.bridge_version = SGEN_BRIDGE_VERSION;
bridge_cbs.bridge_class_kind = gc_bridge_class_kind_cb;
Expand Down
4 changes: 0 additions & 4 deletions src/native/monodroid/osbridge.hh
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ namespace xamarin::android::internal
MonoClassField *handle;
MonoClassField *handle_type;
MonoClassField *refs_added;
MonoClassField *weak_handle;
};

// add_reference can work with objects which are either MonoObjects with java peers, or raw jobjects
Expand Down Expand Up @@ -134,8 +133,6 @@ namespace xamarin::android::internal
int _monodroid_gref_dec ();
char* _get_stack_trace_line_end (char *m);
void _write_stack_trace (FILE *to, char *from, LogCategories = LOG_NONE);
mono_bool take_global_ref_2_1_compat (JNIEnv *env, MonoObject *obj);
mono_bool take_weak_global_ref_2_1_compat (JNIEnv *env, MonoObject *obj);
mono_bool take_global_ref_jni (JNIEnv *env, MonoObject *obj);
mono_bool take_weak_global_ref_jni (JNIEnv *env, MonoObject *obj);
mono_bool add_reference_jobject (JNIEnv *env, jobject handle, jobject reffed_handle);
Expand All @@ -152,7 +149,6 @@ namespace xamarin::android::internal
void gc_cleanup_after_java_collection (JNIEnv *env, int num_sccs, MonoGCBridgeSCC **sccs);
void java_gc (JNIEnv *env);
void set_bridge_processing_field (MonodroidBridgeProcessingInfo *list, mono_bool value);
int platform_supports_weak_refs ();

#if DEBUG
char* describe_target (AddReferenceTarget target);
Expand Down

0 comments on commit 55632c3

Please sign in to comment.