Skip to content

Attach unattached threads before querying for the current domain #6223

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

Merged
merged 1 commit into from
Aug 26, 2021
Merged

Attach unattached threads before querying for the current domain #6223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions src/monodroid/jni/embedded-assemblies.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,7 @@ EmbeddedAssemblies::typemap_java_to_managed (const char *java_type_name)
return nullptr;
}

MonoReflectionType *ret = mono_type_get_object (mono_domain_get (), type);
MonoReflectionType *ret = mono_type_get_object (utils.get_current_domain (), type);
if (XA_UNLIKELY (ret == nullptr)) {
log_warn (LOG_ASSEMBLY, "typemap: unable to instantiate managed type '%s'", managed_type_name);
return nullptr;
Expand Down Expand Up @@ -552,7 +552,7 @@ EmbeddedAssemblies::typemap_java_to_managed (const char *java_type_name)
// calls `mono_get_root_domain`. Thus, we can save on a one function call here by passing `nullptr`
constexpr MonoDomain *domain = nullptr;
#else
MonoDomain *domain = mono_domain_get ();
MonoDomain *domain = utils.get_current_domain ();
#endif
MonoReflectionType *ret = mono_type_get_object (domain, mono_class_get_type (klass));
if (ret == nullptr) {
Expand Down
10 changes: 6 additions & 4 deletions src/monodroid/jni/monodroid-glue.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1741,14 +1741,16 @@ MonodroidRuntime::load_assembly (MonoDomain *domain, jstring_wrapper &assembly)
log_debug (LOG_ASSEMBLY, "Dynamically opened assembly %s", mono_assembly_name_get_name (aname));
} else
#endif
if (domain != mono_domain_get ()) {
MonoDomain *current = mono_domain_get ();
{
MonoDomain *current = utils.get_current_domain ();
if (domain != current) {
mono_domain_set (domain, FALSE);
mono_assembly_load_full (aname, NULL, NULL, 0);
mono_domain_set (current, FALSE);
} else {
mono_assembly_load_full (aname, NULL, NULL, 0);
}
}

mono_assembly_name_free (aname);

Expand Down Expand Up @@ -2327,7 +2329,7 @@ MonodroidRuntime::Java_mono_android_Runtime_register (JNIEnv *env, jstring manag

MonoMethod *register_jni_natives = registerType;
#if !defined (NET6)
MonoDomain *domain = mono_domain_get ();
MonoDomain *domain = utils.get_current_domain (/* attach_thread_if_needed */ false);
Copy link
Contributor

Choose a reason for hiding this comment

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

How does mono_thread_attach() differ from mono_jit_thread_attach()? Why use utils.get_current_domain (/* attach_thread_if_needed */ false) instead of using utils.get_current_domain () and removing the mono_jit_thread_attach() invocation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mono_jit_thread_attach will call mono_get_root_domain() when the domain passed to it is NULL, basically what our utils.get_current_domain() does and it also switches to the new domain if it's different than the current one (something mono_thread_attach doesn't do). The reason I left it around and added the call to utils.get_current_domain is because I usually try not to make as few changes to existing code as possible, using utils.get_current_domain is consistent throughout the code and there's the subtle side effect of mono_jit_thread_attach switching to the passed domain.

mono_jit_thread_attach (domain);
// Refresh current domain as it might have been modified by the above call
domain = mono_domain_get ();
Expand Down Expand Up @@ -2405,7 +2407,7 @@ JNICALL Java_mono_android_Runtime_propagateUncaughtException (JNIEnv *env, [[may
#if defined (NET6)
monodroidRuntime.propagate_uncaught_exception (env, javaThread, javaException);
#else // def NET6
MonoDomain *domain = mono_domain_get ();
MonoDomain *domain = utils.get_current_domain ();
monodroidRuntime.propagate_uncaught_exception (domain, env, javaThread, javaException);
#endif // ndef NET6
}
2 changes: 1 addition & 1 deletion src/monodroid/jni/osbridge.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1041,7 +1041,7 @@ OSBridge::ensure_jnienv (void)
JNIEnv *env;
jvm->GetEnv ((void**)&env, JNI_VERSION_1_6);
if (env == nullptr) {
mono_thread_attach (mono_domain_get ());
mono_thread_attach (utils.get_current_domain (/* attach_thread_if_needed */ false));
Copy link
Contributor

Choose a reason for hiding this comment

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

If utils.get_current_domain() calls mono_thread_attach() if the domain isn't set -- which presumably it won't be if this thread hasn't been seen before (?) -- then do we need this separate mono_thread_attach() call?

Why not just call utils.get_current_domain() and let the side-effect attach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't do that. If mono_domain_get does not return NULL then we wouldn't call mono_thread_attach at all. And it's most likely that mono_domain_get will not return NULL.

jvm->GetEnv ((void**)&env, JNI_VERSION_1_6);
}
return env;
Expand Down
2 changes: 1 addition & 1 deletion src/monodroid/jni/timezones.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ init ()
if (AndroidEnvironment_NotifyTimeZoneChanged)
return;

Mono_Android_dll = utils.monodroid_load_assembly (mono_domain_get (), SharedConstants::MONO_ANDROID_ASSEMBLY_NAME);
Mono_Android_dll = utils.monodroid_load_assembly (utils.get_current_domain (), SharedConstants::MONO_ANDROID_ASSEMBLY_NAME);
Mono_Android_image = mono_assembly_get_image (Mono_Android_dll);
AndroidEnvironment = mono_class_from_name (Mono_Android_image, SharedConstants::ANDROID_RUNTIME_NS_NAME, SharedConstants::ANDROID_ENVIRONMENT_CLASS_NAME);
AndroidEnvironment_NotifyTimeZoneChanged = mono_class_get_method_from_name (AndroidEnvironment, "NotifyTimeZoneChanged", 0);
Expand Down
10 changes: 5 additions & 5 deletions src/monodroid/jni/util.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ Util::monodroid_load_assembly (MonoDomain *domain, const char *basename)
MonoImageOpenStatus status;

aname = mono_assembly_name_new (basename);
MonoDomain *current = mono_domain_get ();
MonoDomain *current = get_current_domain ();

if (domain != current) {
mono_domain_set (domain, FALSE);
Expand All @@ -233,7 +233,7 @@ Util::monodroid_load_assembly (MonoDomain *domain, const char *basename)
MonoObject *
Util::monodroid_runtime_invoke (MonoDomain *domain, MonoMethod *method, void *obj, void **params, MonoObject **exc)
{
MonoDomain *current = mono_domain_get ();
MonoDomain *current = get_current_domain ();
if (domain == current) {
return mono_runtime_invoke (method, obj, params, exc);
}
Expand All @@ -247,7 +247,7 @@ Util::monodroid_runtime_invoke (MonoDomain *domain, MonoMethod *method, void *ob
void
Util::monodroid_property_set (MonoDomain *domain, MonoProperty *property, void *obj, void **params, MonoObject **exc)
{
MonoDomain *current = mono_domain_get ();
MonoDomain *current = get_current_domain ();
if (domain == current) {
mono_property_set_value (property, obj, params, exc);
return;
Expand Down Expand Up @@ -289,7 +289,7 @@ MonoClass*
Util::monodroid_get_class_from_name ([[maybe_unused]] MonoDomain *domain, const char* assembly, const char *_namespace, const char *type)
{
#if !defined (NET6)
MonoDomain *current = mono_domain_get ();
MonoDomain *current = get_current_domain ();

if (domain != current)
mono_domain_set (domain, FALSE);
Expand Down Expand Up @@ -318,7 +318,7 @@ Util::monodroid_get_class_from_name ([[maybe_unused]] MonoDomain *domain, const
MonoClass*
Util::monodroid_get_class_from_image (MonoDomain *domain, MonoImage *image, const char *_namespace, const char *type)
{
MonoDomain *current = mono_domain_get ();
MonoDomain *current = get_current_domain ();

if (domain != current)
mono_domain_set (domain, FALSE);
Expand Down
19 changes: 19 additions & 0 deletions src/monodroid/jni/util.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ constexpr int FALSE = 0;
#include <jni.h>
#include <mono/metadata/assembly.h>
#include <mono/metadata/appdomain.h>
#include <mono/metadata/threads.h>

#include "monodroid.h"
#include "jni-wrappers.hh"
Expand Down Expand Up @@ -110,6 +111,24 @@ namespace xamarin::android
return (log_categories & category) != 0;
}

MonoDomain *get_current_domain (bool attach_thread_if_needed = true) const noexcept
{
MonoDomain *ret = mono_domain_get ();
if (ret != nullptr) {
return ret;
}

// It's likely that we got a nullptr because the current thread isn't attached (see
// https://github.com/xamarin/xamarin-android/issues/6211), so we need to attach the thread to the root
// domain
ret = mono_get_root_domain ();
if (attach_thread_if_needed) {
mono_thread_attach (ret);
}

return ret;
}

private:
//char *monodroid_strdup_printf (const char *format, va_list vargs);
#if !defined (NET6)
Expand Down