Bug 41705 - MonoTests.System.Threading.MonitorTest.Enter_Null crashes test runtime with assertion
Summary: MonoTests.System.Threading.MonitorTest.Enter_Null crashes test runtime with a...
Alias: None
Product: Runtime
Classification: Mono
Component: JIT ()
Version: 4.5.X
Hardware: PC Mac OS
: --- normal
Target Milestone: ---
Assignee: Aleksey Kliger
Depends on:
Reported: 2016-06-10 16:04 UTC by Alexander Kyte
Modified: 2016-06-13 21:05 UTC (History)
4 users (show)

Tags: bitcode_mobile_static
Is this bug a regression?: ---
Last known good build:

Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.

Please create a new report on GitHub or Developer Community with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:

Description Alexander Kyte 2016-06-10 16:04:33 UTC
The test ends up here:

    frame #10: 0x000000010036d8e8 mono-sgen`mono_monitor_try_enter_internal(obj=0x0000000000000000, ms=4294967295, allow_interruption=0) + 88 at monitor.c:955
    frame #11: 0x000000010036d87c mono-sgen`mono_monitor_enter(obj=0x0000000000000000) + 28 at monitor.c:1004
    frame #12: 0x0000000105308f88 mscorlib.dll.dylib`wrapper_managed_to_native_object___icall_wrapper_mono_monitor_enter_object + 56
    frame #13: 0x0000000108666a4e mobile_static_corlib_test.dll.dylib`MonoTests_System_Threading_MonitorTest_Enter_Null + 62

which is a problem because 

frame #10: 0x000000010036d8e8 mono-sgen`mono_monitor_try_enter_internal(obj=0x0000000000000000, ms=4294967295, allow_interruption=0) + 88 at monitor.c:955
   952 		LOCK_DEBUG (g_message("%s: (%d) Trying to lock object %p (%d ms)", __func__, id, obj, ms));
   954 		if (G_UNLIKELY (!obj)) {
-> 955 			mono_set_pending_exception (mono_get_exception_argument_null ("obj"));
   956 			return FALSE;
   957 		}


frame #7: 0x0000000100286e80 mono-sgen`mono_exception_from_name_domain(domain=0x0000000101405620, image=0x0000000103018e00, name_space="System", name="ArgumentNullException") + 192 at exception.c:78
   75  		if (domain != caller_domain)
   76  			mono_domain_set_internal (domain);
   77  		mono_runtime_object_init_checked (o, &error);
-> 78  		mono_error_assert_ok (&error);

which bubbles up.

The problem is that when we try to create this exception, we instead stash it in the MonoError here:

(lldb) up
frame #2: 0x000000010001d7fb mono-sgen`mono_llvmonly_runtime_invoke(method=0x000000010316c518, info=0x00000001022e5120, obj=0x0000000103bbf890, params=0x0000000000000000, exc=0x00007fff5fbfd270, error=0x00007fff5fbfd718) + 1259 at mini-runtime.c:2352
   2350		runtime_invoke (NULL, args, exc, info->compiled_method);
   2351		if (exc && *exc)
-> 2352			mono_error_set_exception_instance (error, (MonoException*) *exc);
   2354		if (sig->ret->type != MONO_TYPE_VOID && info->ret_box_class)
   2355			return mono_value_box_checked (domain, info->ret_box_class, retval, error);

(lldb) p exc->object.vtable->klass->name
(const char *) $18 = 0x00000001025bcbdc "ArgumentNullException"
Comment 1 Aleksey Kliger 2016-06-10 20:23:14 UTC
So here's what I think is going on.  Monitor.Enter is a special method in that when the JIT sees:
    Monitor.Enter (x);
it turns it into
    ok = mono_monitor_enter_fast (x); /* fastpath */
    if (!ok)
        ok = mono_monitor_enter (x);  /* slow path */

So in our case x == NULL.  So mono_monitor_enter_fast sets the pending exception to an ArugmentNullException instance and returns 0.

After that, we go into the slow path, WITHOUT checking for a pending interruption.

There we again see that x is NULL, and go to construct an ArgumentNullException.
That invokes the constructor for the exception, and at that point we end up checking for a pending interupt, and note the pending exception.
At that point we return from managed and it appears to the invoke code that the constructor for ArgumentNullException failed by throwing an ArgumentNullException.

So that gets packed into a MonoError (because we're in coop), and shipped  out to mono_exception_from_name_domain which asserts that there's couldn't have been an error while calling the constructor of an exception.  And that assert fails.

What needs to be done:
 1. The fast path shouldn't set the pending exception (just let the slow path take care of it) or we should check for a pending interrupt before going into the slow path.
 2. mono_exception_from_name_domain shouldn't assume that it can't fail.
Comment 2 Zoltan Varga 2016-06-12 21:45:40 UTC
Should be fixed by 81a6cf5ece5a0f75f8db725f60953299d1aefef4.