Merge pull request #2034 from alexrp/ctx-cleanup

[runtime] Actually clean up context-static data segments.

diff --git a/mcs/class/corlib/System.Runtime.Remoting.Contexts/Context.cs b/mcs/class/corlib/System.Runtime.Remoting.Contexts/Context.cs
index d94abd59f38704967dabba3f322a2023726e38c9..1820cbe7fef5598aeb1d071a95dfcc65599cd283 100644 (file)
--- a/mcs/class/corlib/System.Runtime.Remoting.Contexts/Context.cs
+++ b/mcs/class/corlib/System.Runtime.Remoting.Contexts/Context.cs
@@ -55,6 +55,7 @@ namespace System.Runtime.Remoting.Contexts {
                int domain_id;
                int context_id;
                UIntPtr static_data; /* GC-tracked */
+               UIntPtr data;
                #endregion
 #pragma warning restore 169, 414
 

diff --git a/mcs/class/corlib/System/Environment.cs b/mcs/class/corlib/System/Environment.cs
index 2553a400be58081e176f67775efca560b44ee7fb..d463fb1285854b57d6354ca64464e57a5a92d0c4 100644 (file)
--- a/mcs/class/corlib/System/Environment.cs
+++ b/mcs/class/corlib/System/Environment.cs
@@ -57,7 +57,7 @@ namespace System {
                 * of icalls, do not require an increment.
                 */
 #pragma warning disable 169
-               private const int mono_corlib_version = 138;
+               private const int mono_corlib_version = 139;
 #pragma warning restore 169
 
                [ComVisible (true)]

diff --git a/mono/metadata/appdomain.c b/mono/metadata/appdomain.c
index 89c163c83ee5e7f52538738000a3882499852f28..ca424f3268ad4ef93ad76fd1c381747866d19586 100644 (file)
--- a/mono/metadata/appdomain.c
+++ b/mono/metadata/appdomain.c
@@ -79,7 +79,7 @@
  * Changes which are already detected at runtime, like the addition
  * of icalls, do not require an increment.
  */
-#define MONO_CORLIB_VERSION 138
+#define MONO_CORLIB_VERSION 139
 
 typedef struct
 {

diff --git a/mono/metadata/domain-internals.h b/mono/metadata/domain-internals.h
index 2159f50f0e6dfcd04852820981aef3e040099614..4cb4af6c02d1aca82dfca8f81fd08e5c0a867688 100644 (file)
--- a/mono/metadata/domain-internals.h
+++ b/mono/metadata/domain-internals.h
@@ -238,11 +238,17 @@ struct _MonoJitInfo {
 
 #define MONO_SIZEOF_JIT_INFO (offsetof (struct _MonoJitInfo, clauses))
 
+typedef struct {
+       gpointer *static_data; /* Used to free the static data without going through the MonoAppContext object itself. */
+       uint32_t gc_handle;
+} ContextStaticData;
+
 struct _MonoAppContext {
        MonoObject obj;
        gint32 domain_id;
        gint32 context_id;
        gpointer *static_data;
+       ContextStaticData *data;
 };
 
 /* Lock-free allocator */

diff --git a/mono/metadata/threads.c b/mono/metadata/threads.c
index 75bdcb3d0e5dae3e3e04b65baa9f08964d7a1b6e..112c8779454ed5ef48ce13245dd4e04ada9483d5 100644 (file)
--- a/mono/metadata/threads.c
+++ b/mono/metadata/threads.c
@@ -149,6 +149,9 @@ static MonoGHashTable *threads=NULL;
  */
 static GHashTable *contexts = NULL;
 
+/* Cleanup queue for contexts. */
+static MonoReferenceQueue *context_queue;
+
 /*
  * Threads which are starting up and they are not in the 'threads' hash yet.
  * When handle_store is called for a thread, it will be removed from this hash table.
@@ -2576,6 +2579,31 @@ ves_icall_System_Threading_Volatile_Write_T (void *ptr, MonoObject *value)
        mono_gc_wbarrier_generic_store_atomic (ptr, value);
 }
 
+static void
+free_context (void *user_data)
+{
+       ContextStaticData *data = user_data;
+
+       mono_threads_lock ();
+
+       /*
+        * There is no guarantee that, by the point this reference queue callback
+        * has been invoked, the GC handle associated with the object will fail to
+        * resolve as one might expect. So if we don't free and remove the GC
+        * handle here, free_context_static_data_helper () could end up resolving
+        * a GC handle to an actually-dead context which would contain a pointer
+        * to an already-freed static data segment, resulting in a crash when
+        * accessing it.
+        */
+       g_hash_table_remove (contexts, GUINT_TO_POINTER (data->gc_handle));
+
+       mono_threads_unlock ();
+
+       mono_gchandle_free (data->gc_handle);
+       mono_free_static_data (data->static_data);
+       g_free (data);
+}
+
 void
 ves_icall_System_Runtime_Remoting_Contexts_Context_RegisterContext (MonoAppContext *ctx)
 {
@@ -2586,10 +2614,24 @@ ves_icall_System_Runtime_Remoting_Contexts_Context_RegisterContext (MonoAppConte
        if (!contexts)
                contexts = g_hash_table_new (NULL, NULL);
 
-       context_adjust_static_data (ctx);
+       if (!context_queue)
+               context_queue = mono_gc_reference_queue_new (free_context);
+
        gpointer gch = GUINT_TO_POINTER (mono_gchandle_new_weakref (&ctx->obj, FALSE));
        g_hash_table_insert (contexts, gch, gch);
 
+       /*
+        * We use this intermediate structure to contain a duplicate pointer to
+        * the static data because we can't rely on being able to resolve the GC
+        * handle in the reference queue callback.
+        */
+       ContextStaticData *data = g_new0 (ContextStaticData, 1);
+       data->gc_handle = GPOINTER_TO_UINT (gch);
+       ctx->data = data;
+
+       context_adjust_static_data (ctx);
+       mono_gc_reference_queue_add (context_queue, &ctx->obj, data);
+
        mono_threads_unlock ();
 
        mono_profiler_context_loaded (ctx);
@@ -2601,9 +2643,7 @@ ves_icall_System_Runtime_Remoting_Contexts_Context_ReleaseContext (MonoAppContex
        /*
         * NOTE: Since finalizers are unreliable for the purposes of ensuring
         * cleanup in exceptional circumstances, we don't actually do any
-        * cleanup work here. We instead do this when we iterate the `contexts`
-        * hash table. The only purpose of this finalizer, at the moment, is to
-        * notify the profiler.
+        * cleanup work here. We instead do this via a reference queue.
         */
 
        //g_print ("Releasing context %d in domain %d\n", ctx->context_id, ctx->domain_id);
@@ -3869,6 +3909,7 @@ context_adjust_static_data (MonoAppContext *ctx)
        if (context_static_info.offset || context_static_info.idx > 0) {
                guint32 offset = MAKE_SPECIAL_STATIC_OFFSET (context_static_info.idx, context_static_info.offset, 0);
                mono_alloc_static_data (&ctx->static_data, offset, FALSE);
+               ctx->data->static_data = ctx->static_data;
        }
 }
 
@@ -3887,21 +3928,17 @@ alloc_thread_static_data_helper (gpointer key, gpointer value, gpointer user)
 /*
  * LOCKING: requires that threads_mutex is held
  */
-static gboolean
+static void
 alloc_context_static_data_helper (gpointer key, gpointer value, gpointer user)
 {
-       uint32_t gch = GPOINTER_TO_INT (key);
-       MonoAppContext *ctx = (MonoAppContext *) mono_gchandle_get_target (gch);
+       MonoAppContext *ctx = (MonoAppContext *) mono_gchandle_get_target (GPOINTER_TO_INT (key));
 
-       if (!ctx) {
-               mono_gchandle_free (gch);
-               return TRUE; // Remove this key/value pair
-       }
+       if (!ctx)
+               return;
 
        guint32 offset = GPOINTER_TO_UINT (user);
        mono_alloc_static_data (&ctx->static_data, offset, FALSE);
-
-       return FALSE; // Don't remove it
+       ctx->data->static_data = ctx->static_data;
 }
 
 static StaticDataFreeList*
@@ -3993,7 +4030,7 @@ mono_alloc_special_static_data (guint32 static_type, guint32 size, guint32 align
                        mono_g_hash_table_foreach (threads, alloc_thread_static_data_helper, GUINT_TO_POINTER (offset));
        } else {
                if (contexts != NULL)
-                       g_hash_table_foreach_remove (contexts, alloc_context_static_data_helper, GUINT_TO_POINTER (offset));
+                       g_hash_table_foreach (contexts, alloc_context_static_data_helper, GUINT_TO_POINTER (offset));
 
                ACCESS_SPECIAL_STATIC_OFFSET (offset, type) = SPECIAL_STATIC_OFFSET_TYPE_CONTEXT;
        }
@@ -4047,16 +4084,13 @@ free_thread_static_data_helper (gpointer key, gpointer value, gpointer user)
 /*
  * LOCKING: requires that threads_mutex is held
  */
-static gboolean
+static void
 free_context_static_data_helper (gpointer key, gpointer value, gpointer user)
 {
-       uint32_t gch = GPOINTER_TO_INT (key);
-       MonoAppContext *ctx = (MonoAppContext *) mono_gchandle_get_target (gch);
+       MonoAppContext *ctx = (MonoAppContext *) mono_gchandle_get_target (GPOINTER_TO_INT (key));
 
-       if (!ctx) {
-               mono_gchandle_free (gch);
-               return TRUE; // Remove this key/value pair
-       }
+       if (!ctx)
+               return;
 
        OffsetSize *data = (OffsetSize *)user;
        int idx = ACCESS_SPECIAL_STATIC_OFFSET (data->offset, index);
@@ -4064,12 +4098,10 @@ free_context_static_data_helper (gpointer key, gpointer value, gpointer user)
        char *ptr;
 
        if (!ctx->static_data || !ctx->static_data [idx])
-               return FALSE; // Don't remove this key/value pair
+               return;
 
        ptr = ((char*) ctx->static_data [idx]) + off;
        mono_gc_bzero_atomic (ptr, data->size);
-
-       return FALSE; // Don't remove this key/value pair
 }
 
 static void
@@ -4098,7 +4130,7 @@ do_free_special_slot (guint32 offset, guint32 size)
                        mono_g_hash_table_foreach (threads, free_thread_static_data_helper, &data);
        } else {
                if (contexts != NULL)
-                       g_hash_table_foreach_remove (contexts, free_context_static_data_helper, &data);
+                       g_hash_table_foreach (contexts, free_context_static_data_helper, &data);
        }
 
        if (!mono_runtime_is_shutting_down ()) {