Forgot to mention in post:
The app above is using mkbundle to bundle the Mono framework in the app itself, instead of depending on the user to install the framework on his mac.

Not sure it matters, will try.

Created attachment 317434 [details]
Output of the Apple EWrror Reporter when it crashes

Attached the output of the Apple Crash reporter for this issue.

As you can see, this is coming from the garbage collector in mono.
To make it less random, I've added GC.collect() statements in some parts of my code, attempting to narrow it down. The GC.collect() statements cause it to crash (only on snow leopard, this works fine on leopard).

So far, haven't been able to get this behaviour in a sample app yet.

Anyone here have any clue on how I could reproduce this?
Any other known issues? (haven't found any here yet)

I have similar problem with a fairly complex app.  It crashes randomly on GC too (I see 'thread_get_state failed' followed by a stack trace).  The crash happens only on Mac OS X 10.6 (Snow Leopard) and not on 10.5.

The app uses Monobjc to embed WebKit into Carbon view.  Could not narrow it down, though.

Created attachment 318374 [details]
Sample app and code which reproduces the problem

I have found a way to reproduce this behaviour in a smaller application.

Basically, I took one of the MonobjC samples (SimpleCocoaApp), removed all unrequired stuff, and did a forced GC.collect() when pressing a button.

I noticed that when I opened the about form, and then forced GC, it crashed.

So, in the current version, hitting the button on the app will open the default about panel, sleep 1 millisec, and then force GC.
This causes a crash immediately.
(the thread.sleep is optional, you can do the 2 actions separately yourself as well)

You'll see that the app is VERY basic, no special threading, ...
I'll try and narrow it down further (repro without MonObjc).
note that opening the about panel is not the only way to force this crash.

To build the app: 
nant build

Anyway, I hope this helps.

I can confirm that the application, submitted by Kenny, indeed systematically crashes.

Tested this with the latest Mono release on both OSX 10.6 and 10.6.1

I've been having this problem with my full application as well, and hope that with this small demo program, debugging and fixing will be a lot easier.

Thanks

If I compile with DEBUG_THREADS it looks like some foreign threads are being created and being allocated in the GC_threads-array.
However, when the garbage collector runs, this thread is already gone again and when we perform the thread_get_state() in darwin_stop_world() this returns an error.

To be sure that the filters from thread_get_state() are not incorrect I added a thread_info call right before the thread_get_state and in the GC_start_routine_head. 
The GC_start_routine_head returns a success... The call right before thread_get_state fails... 

It seems like the thread already went away... 

Was the mach_thread-properly 'retained'?

I created the following work around:

macbook-pro-van-admin-3:~ kenny$ diff -w mono-2.4.2.3/libgc/darwin_stop_world.c  orig/mono-2.4.2.3/libgc/darwin_stop_world.c 
105,119d104
< 	if(r != KERN_SUCCESS) {
< 		mach_port_urefs_t refs=0;
< 		kern_return_t r=mach_port_get_refs(mach_task_self(), p->stop_info.mach_thread, MACH_PORT_RIGHT_DEAD_NAME, &refs);
< 
< 		if (r != KERN_SUCCESS) ABORT("Could not query thread_refs");
< 		if (refs == 0) ABORT("this is not a deadname");
< 
< 		printf("We will be cleaning dead thread %p\n", p->stop_info.mach_thread);
< 		
< 		r = mach_port_deallocate(mach_task_self(), p->stop_info.mach_thread);
< 		if (r != KERN_SUCCESS) ABORT("Could not deallocate thread");
< 
< 		p -> flags |= FINISHED;  //This might be a minor memory leak
< 		continue;
< 	}


Apparently snow leopard 6 creates an additional thread which is killed/terminated in such a way that it does not execute the exit functions as required by mono. 
This patch will mark the entry in the GC_Threads table as finished so it never is referenced anymore.
There is a small memory leak as this entry is never cleaned from the GC_Threads table. In the application I use there is only one instance of a lost thread so it should be a minor issue.

Could this patch be validated on other environments?

I have done some tests with a lot of debug statements on both Leopard and Snow Leopard, to compare the behavior of the foreign thread registration/deregistration.


On Leopard (Mac OS X 10.5):
- Before the run loop start, there is 3 threads (one is the main one)
id=0xb0187000 (flags=2)
id=0xb0081000 (flags=2)
id=0xa0745720 (flags=6)
- After the run loop start, there is still 3 threads
id=0xb0187000 (flags=2)
id=0xb0081000 (flags=2)
id=0xa0745720 (flags=6)
- When clicking on the button, a foreign thread is registered with the GC
id=0xb022d000 (flags=8) <- New foreign thread
id=0xb0187000 (flags=2)
id=0xb0081000 (flags=2)
id=0xa0745720 (flags=6)
- When the GC is invoked, the thread_get_state function return KERN_SUCCESS for all the threads
- The foreign thread keeps registered with the GC, and is never deregistered.


On Snow Leopard (Mac OS X 10.6):
- Before the run loop start, there is 3 threads (one is the main one)
id=0xb0187000 (flags=2)
id=0xb0081000 (flags=2)
id=0xa0af4500 (flags=6)
- After the run loop start (shortly after), some foreign threads are registered with the GC
id=0xb0398000 (flags=8) <- New foreign thread
id=0xb0187000 (flags=2)
id=0xb0081000 (flags=2)
id=0xa0af4500 (flags=6)
...
id=0xb0316000 (flags=8) <- New foreign thread
id=0xb0398000 (flags=8)
id=0xb0187000 (flags=2)
id=0xb0081000 (flags=2)
id=0xa0af4500 (flags=6)
- After a few seconds, some foreign threads are either registered or deregistered. The whole process does not seems to follow a pattern.
id=0xb0398000 (flags=8)
id=0xb0187000 (flags=2)
id=0xb0081000 (flags=2)
id=0xa0af4500 (flags=6)
...
id=0xb0316000 (flags=8) <- New foreign thread (hey, we've seen it before !!!)
id=0xb0398000 (flags=8)
id=0xb0187000 (flags=2)
id=0xb0081000 (flags=2)
id=0xa0af4500 (flags=6)
...
id=0xb0316000 (flags=8)
id=0xb0187000 (flags=2)
id=0xb0081000 (flags=2)
id=0xa0af4500 (flags=6)
...
id=0xb0398000 (flags=8) <- New foreign thread (hey, we've seen it before !!!)
id=0xb0316000 (flags=8)
id=0xb0187000 (flags=2)
id=0xb0081000 (flags=2)
id=0xa0af4500 (flags=6)
- When clicking on the button, the thread_get_state function fails on the first foreign thread with the return value 0x10000003 (Note that KERN_NO_SPACE is defined as 0x3, so I don't really know if it a combination of return values or something else)

Some Points:
- On Snow Leopard, it seems that there is some kind of "yoyo effect" in the registration and deregistration of foreign threads (sometimes the same thread got registered/deregistered several times). I don't have a sufficient knowledge of the Mach kernel and the libgc to clearly see how and why it happens.
- The same foreign thread (same thread_id and same mach_thread_id) got registered and then deregistered, with no visible reason (no user action)
- When a foreign thread is registered in the GC, a thread_get_state is not guaranteed to be successful. And when the function fails, the abort call crash the application.
- I would suggest to adapt the previous patch to mark foreign thread as STALLED (new state) instead of FINISHED if the thread_get_state function fails. This way, a foreign thread marked as STALLED would still have a chance to be deregistered in a clean way without any leak (removal from the GC_thread table and deallocation of the mach_port).

Any thoughts ?

Created attachment 320558 [details]
Add STALE state to mark stale foreign thread

Created attachment 320559 [details]
Mark foreign thread as stale if thread_get_state fails

I have attached below two diff files that are a variant of proposed fix:
- When a thread_get_state call fails, the corresponding thread is marked stale
- When iterating, stale foreign thread are skipped.

I have tested this fix with the sample application, and it solves the crash. It needs to be tested with larger application to see if side-effects occur.

IMHO, there is still a remaining issue regarding the removal of the stale thread. I think it should be a good idea to understand why some thread are not properly removed and lead to failure of thread_get_state call.

hmmm... it gets interesting when I break on GC_thread_register_foreign just after setting the me-variable (so you can read out the me variable in gdb)

Is anyone able to explain the following stack trace?

Thread 8 (process 64873):
#0  GC_thread_register_foreign (base_addr=0xb0397a94) at pthread_support.c:1364
#1  0x000fc0b3 in mono_gc_register_thread (baseptr=0xb0397a94) at boehm-gc.c:218
#2  0x001afae1 in mono_thread_attach (domain=0x616e58) at threads.c:813
#3  0x000067a2 in mono_jit_thread_attach (domain=0x616e58) at mini.c:2128
#4  0x00779295 in ?? ()
#5  0x91d6f53c in -[NSConcreteNotification dealloc] ()
#6  0x91d6fdae in __NSFinalizeThreadData ()
#7  0x933eba1d in _pthread_tsd_cleanup ()
#8  0x933eb5ca in _pthread_exit ()
#9  0x91d7a984 in +[NSThread exit] ()
#10 0x91d7a92c in __NSThread__main__ ()
#11 0x933e2f39 in _pthread_start ()
#12 0x933e2dbe in thread_start ()


In this stacktrace I read that a separate thread is being stopped  (_pthread_exit), hoewever while cleaning up some thread specific data this thread is being picked up (again?) to be added in the GC_threads-array, which results in the crash... I think we should look at why we end up in this situation and fix this.. If this is fixed we can probably get rid of the patch in the darwin_stop_world.c

Anyone a good idea how we could test if the cleanup is started to prevent adding this thread again to the GC_threads-table? Maybe we should add a thread specific data which says that this thread is about to go dead.

The re-registration of the dying thread is linked to the way Monobjc bridges the Objective-C runtime. In Snow Leopard, the NSThreadWillExitNotification is posts AFTER all the thread's clean-up calls. So when the exit notification is deallocated, the Monobjc bridge is invoked, leading to the re-registration of the thread. And as the clean-up is already done, there is no means store/retrieve the fact that the thread is gone (according to my tests and to my light knowledge of the Mach/BSD kernel API).

One possible fix may be to mark the thread as stalled if the thread_get_state call fails, and then to clean-up all the threads flagged as stalled at the end of the GC_push_all_stacks function. This way, we ensure that dead threads are unregistered and that memory does not leak.

Adding rodrigo to the CC

Kumpera and I discussed this today and both agree that allowing re-registration of threads after the runtime has de-registered them is a hack.  It will introduce runtime issues if we allow even partial thread re-registration which has to happen for the managed callback to complete.

There are two options here:

#1 Refactor Monobjc to not use NSThreadWillExitNotification
#2 Introduce new public API into the mono embedding API to support notification of thread death/creation.

There are some issues with #2 so it should be discussed on the mailing list to figure out the right public api to add to the embedding API for people to use.

I would prefer leave the runtime as is and found a solution within the bridge, so I think I will go #1 for the moment and post my results here.

Laurent,

I am having the same problem as described in this bug report.  I have a client that is very interested in getting an existing application to run stable on OS X 10.6.  Do you have a target date for releasing a solution for this problem?  I would be happy to do some testing if you need.  Thank you.

Created attachment 330599 [details]
Avoid re-registration of foreign threads on Darwin platforms

Here are the result of my investigations:

I have first try to solve the issue by modifying the way Monobjc plugs into the Objective-C runtime. There were some success, but the crash did not go away. It seems that the workaround did not catch all the paths. I don't think we can achieve a correction this way.

So, I have investigated a bit more about the double-registration of foreign threads. It appears that it already occurs in Leopard but the crash does not show.

Here is my understanding:
- When some managed code is called from native library, Mono registers the current thread as foreign.
- In order to de-register the thread when it exits, Mono binds a destuctor function (GC_thread_deregister_foreign) to a TSD (Thread Specific Data) key.
- When the foreign thread exits (pthread_exit), all the TSD are cleared and their non-NULL destructor function are called. Mono de-register the foreign thread.
- After the TSD are cleared, the Objective-C runtime sends a notification (NSThreadWillExitNotification). Some code in Monobjc is invoked leading to a native/managed call. The thread is re-registered as foreign.
- The GC_thread_deregister_foreign is re-bound to its TSD.
- On Leopard (Mac OS X 10.5), the TSD are cleared again thus leading the re-registered thread to be re-de-registered.
- On Snow Leopard (Mac OS X 10.6), the TSD are not cleared in the same way thus leaving the re-registered thread as is. The GC has now a stale reference on a destroyed thread. As the order of the TSD destruction is unspecified, I assume that it is the cause of the crash.

The first proposed patches aim at hiding the crash when a stale thread was suspended. It was leaving the GC with some leaked bits.

The attached patch goes a step further. Instead of masking the result of the double-registration, it avoids it. The trick used is to use a TSD mark on foreign thread during the first register, so if the same thread is registered again, the GC does nothing when it detects the mark. To avoid the result of the unspecified order of the TSD destruction, we re-set the TSD mark in destructor function (permitted by http://developer.apple.com/mac/library/documentation/Darwin/Reference/ManPages/man3/pthread_key_create.3.html).

Your patch is not acceptable as it leaves the thread unregistered but been able to
access managed objects. If a GC happens, all sort of bad stuff will happen.

It looks like an OSX 10.6 bug and unless we can properly work-around it, we simply won't be able to support NSThreadWillExitNotification.

Laurent, unless we figure out a way to properly support gc re-register/deregister from TSD destructor, we can't support it.

I still believe Geoff's suggestion (comment 15) of extending the embedding API is the way to go as it's safer and portable.

Created attachment 331453 [details]
Patch for proper foreign thread lifecycle tracking

I have made some research to track down what was going during thread cleanup on Mac OS X (10.4 to 10.6). It appears that TSD are widely used by many libraries and frameworks to track the thread lifecycle. Here the analysis:

*) Mac OS X 10.4.11:
TSD are dynamically assigned to all callers of "pthread_key_create".
When the TSD are cleanup, PTHREAD_DESTRUCTOR_ITERATIONS iterations are made on all the TSD. As long as a TSD is non-null, its destructor function is called. A destructor functions can be called up to PTHREAD_DESTRUCTOR_ITERATIONS times. All the TSD cleanup occur in the same loop.
 
http://www.opensource.apple.com/source/Libc/Libc-391.5.22/pthreads/pthread_tsd.c

*) Mac OS X 10.5.8:
Apple has introduced non-portable extension to TSD management. The non-portable extensions brings the ability for libraries and framework to have fixed thread key values, requesting them through the "pthread_key_init_np" function. Two ranges exists: one for dynamic requested keys with "pthread_key_create" function, and one for static keys dedicated to libraries and frameworks.
When the TSD are cleanup, PTHREAD_DESTRUCTOR_ITERATIONS iterations are made on all the thread keys, both dynamic and static ones. As long as a TSD is non-null, its destructor function is called. A destructor functions can be called up to PTHREAD_DESTRUCTOR_ITERATIONS times. All the TSD cleanup occur in the same loop.

http://www.opensource.apple.com/source/Libc/Libc-498.1.7/pthreads/pthread_machdep.h
http://www.opensource.apple.com/source/Libc/Libc-498.1.7/pthreads/pthread_tsd.c

In Mono, the "deregister_foreign_thread" function maybe called before or after the thread exit notification poster of Cocoa. In the worst case scenario, "deregister_foreign_thread" function is called twice, but the GC is left in a consistent state.

*) Mac OS X 10.6.2:
Apple has extended the concept of static keys, as new libraries and frameworks used them (libdispatch for example). One major change come to the cleanup.
When the TSD are cleanup, first PTHREAD_DESTRUCTOR_ITERATIONS iterations are made on the dynamic keys (obtained with "pthread_key_create" function). As long as a TSD is non-null, its destructor function is called. A destructor functions can be called up to PTHREAD_DESTRUCTOR_ITERATIONS times.
Then PTHREAD_DESTRUCTOR_ITERATIONS iterations are made on the static keys (declared with "pthread_key_init_np" function). As long as a TSD is non-null, its destructor function is called. A destructor functions can be called up to PTHREAD_DESTRUCTOR_ITERATIONS times.

http://www.opensource.apple.com/source/Libc/Libc-583/pthreads/pthread_machdep.h
http://www.opensource.apple.com/source/Libc/Libc-583/pthreads/pthread_tsd.c

In Mono, "deregister_foreign_thread" function is bound to a dynmiac key whereas the thread exit notification poster of Cocoa is bound to a static key. That means that the foreign thread is deregistered in the first cleanup pass. But when the notification occurs (in the second pass), there is no way to call Mono cleanup again as the dynamic keys have been discarded before the cleanup of the static keys.

*) A workaround
A solution is to use the non-portable TSD extensions of Apple when dealing with foreign thread. This way, the "deregister_foreign_thread" function is called within the same loop used by Objective-C runtime for its notification. This implies to use non-portable function and a statically assigned thread key.
There are two important points:
- the non-portable function "pthread_key_init_np" is only available starting with Mac OS X 10.5. As Mono must run on Mac OS X 10.4, this leads to a symbol lookup to detect the support of the function.
- the static key must be chosen in a way that does not conflict with other ones. Currently, 768 slots are created for TSD. Static keys go from 0 to 255, the range 30 to 255 is reserved for non-libSystem use. Dynamic keys go from 256 to 512. Static key range from 30 to 90 is already assigned. A good pick is to use the value 255, as it is the higher limit of static key reserved for non-libSystem use. Maybe an official reservation is needed like the libdispatch project has done it.

The implementation is therefore straightforward:

-> On Mac OS X 10.4:
We use dymanic TSD as usual (because "pthread_key_init_np" is not available). During the cleanup, we wait until the PTHREAD_DESTRUCTOR_ITERATIONS calls to "deregister_foreign_thread" before removing GC_thread instance by re-setting the TSD value each time. This assure that the GC_Thread instance is there even during the notification posting.

-> On Mac OS X 10.5/10.6
We use static TSD. This only change the value of the thread_key used when calling pthread_setspecific. During the cleanup, we wait until the PTHREAD_DESTRUCTOR_ITERATIONS calls to "deregister_foreign_thread" before removing GC_thread instance by re-setting the TSD value each time. This assure that the GC_Thread instance is there even during the notification posting.

This way of dealing with the foreign threads lifecycle ensures that thread are properly tracked from their first native/managed invocation until their full clean-up.

I have decorated the specific code with GC_DARWIN_THREADS defines. I have tested the implementation on Mac OS X 10.4 to 10.6 with success. Tests on iPhone may be necessary to ensure that the code is properly bound.

Hi Laurent,

We can't realistically depend on pthread_key_init_np, as this is part of Apple's internal API.

Not only that, but your patch depends on guessing a number that Apple won't eventually use or will make it back a dynamic TSD.

What about the idea of extending the embedding API to support proper thread create/attach detach/destroy events?

I believe we should move forward with this discussion and we can even put those changes back into 2.4/2.6.

I agree with rodrigo here.  All it takes is apple to change the signature of pthread_key_init_np, or have slightly different semantics on say the iphone, and we'll get runtime crashes.  This isn't an acceptible solution going forward.

Extendeding the embedding api is something that monobjc could consume to get this information, as well as having the added benefit of not relying on internal mutable apis, and also giving this information to anyone else interested in it going forward as well.

Fair enough, I understand your points.

Well, I must admit that I(m running out of patches to fix the GC, so let's move to  Mono-Dev and discuss how to have a proper thread tracking through API.

Created attachment 334416 [details]
Extension of embedding API to allow foreign thread tracking

Here is a proposal for an embedding API to allow foreign thread tracking.

The API allows:
- To be notified when a foreign thread is registered with the Mono runtime
- To be notified when a foreign thread is de-registered from the Mono runtime
- To override default Mono behavior by telling that foreign thread are de-registered externally
- To de-register a foreign thread from the Mono runtime

There are two cases of use:
1) An application hooks the Mono runtime to be notified when foreign thread are registered/de-registered.
2) An application hooks the Mono runtime to handle the foreign thread de-registration. When a foreign thread is registered, the application is notified. The application is then responsible for the de-registration of the foreign thread.

Some notes:
- The patch has been generated against the 2.6.1 release sources.
- I have put the embedding API in the "mono/utils" folder, because it is referenced both as a public API and used in the GC.
- I have decorated the API to work only if pthread is present. We can see later if other platforms have similar needs.
- I only work on the Bohem GC. I don't have taken time to look into the SGen one.
- I have tested the embedding API on Leopard and Snow Leopard, and it works at it should for the 2 cases above.
- I will post on Mono-Dev this entry in order to get a larger feedback.

Laurent,

  There are a few issues with this patch that Rodrigo and I discussed:

#1> It shouldn't be implemented in the GC, but in the runtime.  We have no knowledge in the GC about runtime health of a thread, so it should hook into the runtime thread_start / thread_end methods probably
#2> Its pthread only, it needs to work on win32.  Probably pass a MonoThread or the thread_id from the thread as the arg
#3> Its not consistent with other embedding callbacks, we dont add / pass public structs around, add a function that takes 2 args of the callback type (see the profiler api for examples).

Thanks

Geoff and Rodriguo,

#1>
I don't understand this point. From my understanding, foreign threads are neither created nor destructed by the Mono runtime, but enrolled when they make their first managed call and destroyed when their TSD are cleanup. So, the only hooks I have found are in the libgc, but maybe I have missed something.

#2>
I don't know how Mono deals with Win32 threads, especially how it handles foreign threads. My primary goal is to make the API work with pthread and then get more information on how to integrate them with Win32.

#3>
Got it. I will look at the profiler to make a more consistent proposal.

For now, the API is only targeting the foreign thread lifecycle, not the threads own by the Mono's runtime. Maybe it could be extended to support all the threads (owned and foreign).

Thanks.

A workaround has been implemented in Monobjc (starting with 4.0.479) to avoid re-addition of foreign threads after their removal from the GC.

It seems that the fix is sufficient to consider this bug as worked around.

I will continue my research on the embedding API if I got some spare time.

I'm updating this bug to reflect the ENH