Skip to content

[runtime] Fix race condition in xamarin_release_managed_ref for user types. Fixes #25171 #25330

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

Draft
rolfbjarne wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

[runtime] Fix race condition in xamarin_release_managed_ref for user types. Fixes #25171 #25330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5fe56b8
[tests] Add debug logging to LayerTest to diagnose random GC crash (#…
rolfbjarne May 5, 2026
68906c2
Retrigger CI (run 2)
rolfbjarne May 5, 2026
38efdd7
Retrigger CI (run 3)
rolfbjarne May 6, 2026
c53e459
[runtime] Fix race condition in xamarin_release_managed_ref for user …
rolfbjarne May 6, 2026
f94a3bb
Retrigger CI (run 5)
rolfbjarne May 6, 2026
2615017
Retrigger CI (run 6)
rolfbjarne May 7, 2026
1c255e4
Retrigger CI (run 7)
rolfbjarne May 8, 2026
97ae7fb
Merge remote-tracking branch 'origin/main' into dev/rolf/issue-25171-…
rolfbjarne May 13, 2026
a9f8acb
Merge branch 'main' into dev/rolf/issue-25171-layertest
rolfbjarne May 14, 2026
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
5 changes: 5 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,3 +68,8 @@ For legacy Xamarin.iOS and Xamarin.Mac downloads (discontinued), see [Downloads]

Copyright (c) .NET Foundation Contributors. All rights reserved.
Licensed under the [MIT](https://github.com/dotnet/macios/blob/main/LICENSE) License.





138 changes: 69 additions & 69 deletions runtime/runtime.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1752,77 +1752,77 @@ -(struct NSObjectData*) xamarinGetNSObjectData;
if (user_type) {
/* clear MANAGED_REF_BIT */
set_gchandle_flags_safe (self, (enum XamarinGCHandleFlags) (get_flags_safe (self) & ~XamarinGCHandleFlags_HasManagedRef));
} else {
//
// This waypoint (lock+unlock) is needed so that we can safely call retainCount in the
// toggleref callback.
//
// The race is between the following actions (given a managed object Z):
//
// a1) Thread A nulls out the handle for Z
// a2) Thread A calls release on Z's original handle
// b1) Thread B fetches the handle for Z
// b2) Thread B calls retainCount on Z's handle
//
// Possible execution orders:
//
// 1) a1-*: all such orders are safe, because b1 will read NULL and
// b2 won't do anything
// 2) b1-b2-a1-a2: retainCount before release is safe.
// 3) b1-a1-b2-a2: retainCount before release is safe.
// 4) b1-a1-a2-b2: unsafe; this tries to call retainCount after
// release.
//
// Order 4 would look like this:
//
// * Thread B runs a GC, and starts calling toggleref callbacks.
// * Thread B fetches the handle (H) for object Z in a toggleref
// callback.
// * Thread A calls xamarin_release_managed_ref for object Z, and
// calls release on H, deallocating it.
// * Thread B tries to call retainCount on H, which is now a
// deallocated object.
//
// Solution: lock/unlock the framework peer lock here. This looks
// weird (since nothing happens inside the lock), but it works:
//
// * Thread B runs a GC, locks the framework peer lock, and starts
// calling toggleref callbacks.
// * Thread B fetches the handle (H) for object Z in a toggleref
// callback.
// * Thread A calls xamarin_release_managed_ref for object Z (and
// also nulls out the handle for Z)
// * Thread A tries to lock the framework peer lock, and blocks
// (before calling release on H)
// * Thread B successfully calls retainCount on H
// * Thread B finishes processing all toggleref callbacks, completes
// the GC, and unlocks the framework peer lock.
// * Thread A wakes up, and calls release on H.
//
// An alternative phrasing would be to say that the lock prevents both
// a1 and a2 from happening between b1 and b2 from above, thus making
// order 4 impossible.
//
// Q) Why not just unlock after calling release, to avoid the strange-
// looking empty lock?
// A) Because calling release on an object might end up calling
// managed code (the native object can override dealloc and do all
// sorts of strange things, any of which may end up invoking
// managed code), and we can deadlock:
// 1) Thread T calls release on a native object.
// 2) Thread T executes managed code, which blocks on something
// that's supposed to happen on another thread U.
// 3) Thread U causes a garbage collection.
// 4) Thread U tries to lock the framework peer lock before running
// the GC, and deadlocks because thread T already has the
// framework peer lock.
//
// This is https://github.com/dotnet/macios/issues/3943
//
// See also comment in xamarin_marshal_return_value_impl
xamarin_framework_peer_waypoint_safe ();
}

//
// This waypoint (lock+unlock) is needed so that we can safely call retainCount in the
// toggleref callback.
//
// The race is between the following actions (given a managed object Z):
//
// a1) Thread A nulls out the handle for Z
// a2) Thread A calls release on Z's original handle
// b1) Thread B fetches the handle for Z
// b2) Thread B calls retainCount on Z's handle
//
// Possible execution orders:
//
// 1) a1-*: all such orders are safe, because b1 will read NULL and
// b2 won't do anything
// 2) b1-b2-a1-a2: retainCount before release is safe.
// 3) b1-a1-b2-a2: retainCount before release is safe.
// 4) b1-a1-a2-b2: unsafe; this tries to call retainCount after
// release.
//
// Order 4 would look like this:
//
// * Thread B runs a GC, and starts calling toggleref callbacks.
// * Thread B fetches the handle (H) for object Z in a toggleref
// callback.
// * Thread A calls xamarin_release_managed_ref for object Z, and
// calls release on H, deallocating it.
// * Thread B tries to call retainCount on H, which is now a
// deallocated object.
//
// Solution: lock/unlock the framework peer lock here. This looks
// weird (since nothing happens inside the lock), but it works:
//
// * Thread B runs a GC, locks the framework peer lock, and starts
// calling toggleref callbacks.
// * Thread B fetches the handle (H) for object Z in a toggleref
// callback.
// * Thread A calls xamarin_release_managed_ref for object Z (and
// also nulls out the handle for Z)
// * Thread A tries to lock the framework peer lock, and blocks
// (before calling release on H)
// * Thread B successfully calls retainCount on H
// * Thread B finishes processing all toggleref callbacks, completes
// the GC, and unlocks the framework peer lock.
// * Thread A wakes up, and calls release on H.
//
// An alternative phrasing would be to say that the lock prevents both
// a1 and a2 from happening between b1 and b2 from above, thus making
// order 4 impossible.
//
// Q) Why not just unlock after calling release, to avoid the strange-
// looking empty lock?
// A) Because calling release on an object might end up calling
// managed code (the native object can override dealloc and do all
// sorts of strange things, any of which may end up invoking
// managed code), and we can deadlock:
// 1) Thread T calls release on a native object.
// 2) Thread T executes managed code, which blocks on something
// that's supposed to happen on another thread U.
// 3) Thread U causes a garbage collection.
// 4) Thread U tries to lock the framework peer lock before running
// the GC, and deadlocks because thread T already has the
// framework peer lock.
//
// This is https://github.com/dotnet/macios/issues/3943
//
// See also comment in xamarin_marshal_return_value_impl
xamarin_framework_peer_waypoint_safe ();

xamarin_handle_to_be_released = self;

objc_release (self);
Expand Down
40 changes: 40 additions & 0 deletions tests/monotouch-test/CoreAnimation/LayerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

using System.Diagnostics;
using System.Linq;
using System.Runtime.CompilerServices;
using System.Threading;
using CoreGraphics;
using CoreAnimation;
Expand All @@ -19,21 +20,29 @@ namespace MonoTouchFixtures.CoreAnimation {
[Preserve (AllMembers = true)]
public class LayerTest {

static void Log (string message, [CallerMemberName] string member = "")
{
Console.WriteLine ($"[LayerTest.{member}] [Thread {Thread.CurrentThread.ManagedThreadId}] {message}");
}

[Test]
public void Mask ()
{
Log ("start");
using (CALayer layer = new CALayer ()) {
Assert.Null (layer.Mask, "Mask/default");
layer.Mask = new CALayer ();
Assert.NotNull (layer.Mask, "Mask/assigned");
layer.Mask = null;
Assert.Null (layer.Mask, "Mask/nullable");
}
Log ("done");
}

[Test]
public void CAActionTest ()
{
Log ("start");
// bug 2441
CAActionTestClass obj = new CAActionTestClass ();
Assert.IsNull (obj.ActionForKey ("animation"), "a");
Expand All @@ -53,6 +62,7 @@ public void CAActionTest ()
Assert.That (obj.ActionForKey ("basicAnimation") == dict [basicAnimationKey], "f");
Assert.IsNull (CAActionTestClass.DefaultActionForKey ("animation"), "g");
Assert.IsNull (CALayer.DefaultActionForKey ("animation"), "h");
Log ("done");
}

class CAActionTestClass : CALayer {
Expand All @@ -62,39 +72,47 @@ class CAActionTestClass : CALayer {
[Test]
public void ConvertPoint ()
{
Log ("start");
using (CALayer layer = new CALayer ()) {
Assert.True (layer.ConvertPointFromLayer (CGPoint.Empty, null).IsEmpty, "From/Empty/null");
Assert.True (layer.ConvertPointToLayer (CGPoint.Empty, null).IsEmpty, "To/Empty/null");
}
Log ("done");
}

[Test]
public void ConvertRect ()
{
Log ("start");
using (CALayer layer = new CALayer ()) {
Assert.True (layer.ConvertRectFromLayer (CGRect.Empty, null).IsEmpty, "From/Empty/null");
Assert.True (layer.ConvertRectToLayer (CGRect.Empty, null).IsEmpty, "To/Empty/null");
}
Log ("done");
}

[Test]
public void ConvertTime ()
{
Log ("start");
using (CALayer layer = new CALayer ()) {
Assert.That (layer.ConvertTimeFromLayer (0.0d, null), Is.EqualTo (0.0d), "From/0.0d/null");
Assert.That (layer.ConvertTimeToLayer (0.0d, null), Is.EqualTo (0.0d), "To/0.0d/null");
}
Log ("done");
}

[Test]
public void AddAnimation ()
{
Log ("start");
using (var layer = new CALayer ()) {
var animation = new CABasicAnimation ();
Assert.IsNull (layer.AnimationForKey ("key"), "#key A");
layer.AddAnimation (animation, "key");
Assert.IsNotNull (layer.AnimationForKey ("key"), "#key B");
}
Log ("done");
}


Expand All @@ -103,13 +121,15 @@ public void AddAnimation ()
[Test]
public void TestBug26532 ()
{
Log ("start");
TextLayersDisposed = 0;
Generation++;

const int layerCount = 50;
Exception ex = null;
var thread = new Thread (() => {
try {
Log ("background thread started");
var frame = new CGRect (0, 0, 200, 200);
using (var layer = new CALayer ()) {
for (int i = 0; i < layerCount; i++) {
Expand All @@ -119,31 +139,43 @@ public void TestBug26532 ()
layer.AddSublayer (textLayer);
}

Log ("calling GC.Collect on background thread");
GC.Collect ();
Log ("GC.Collect on background thread completed");

foreach (var slayer in layer.Sublayers.OfType<TextCALayer> ()) {
Assert.AreEqual ("42", slayer.Secret);
}

Log ("removing sublayers");
foreach (var slayer in layer.Sublayers.OfType<TextCALayer> ())
slayer.RemoveFromSuperLayer ();
Log ("sublayers removed");
}
Log ("background thread done");
} catch (Exception e) {
Log ($"background thread exception: {e}");
ex = e;
}
});
thread.Start ();
thread.Join ();

Log ("background thread joined, starting GC loop");
var watch = new Stopwatch ();
watch.Start ();
int gcCount = 0;
while (watch.ElapsedMilliseconds < 2000 && TextLayersDisposed < layerCount / 2) {
gcCount++;
Log ($"GC.Collect iteration {gcCount}, TextLayersDisposed={TextLayersDisposed}");
GC.Collect ();
NSRunLoop.Main.RunUntil (NSDate.Now.AddSeconds (0.05));
}
Log ($"GC loop done after {gcCount} iterations, TextLayersDisposed={TextLayersDisposed}");

Assert.IsNull (ex, "Exceptions");
Assert.That (TextLayersDisposed, Is.AtLeast (layerCount / 2), "disposed text layers");
Log ("done");
}

public class TextCALayer : CALayer {
Expand Down Expand Up @@ -172,22 +204,30 @@ class LayerDelegate : CALayerDelegate { }
[Test]
public void TestCALayerDelegateDispose ()
{
Log ("start");
var del = new LayerDelegate ();
var t = new Thread (() => {
Log ("background thread: creating Layer, setting delegate, disposing");
var l = new Layer ();
l.Delegate = del;
l.Dispose ();
Log ("background thread: done");
}) {
IsBackground = true,
};
t.Start ();
t.Join ();
Log ("background thread joined, calling GC.Collect #1");
GC.Collect ();
Log ("GC.Collect #1 done, running runloop");

NSRunLoop.Main.RunUntil (NSDate.Now.AddSeconds (0.1));

Log ("runloop done, calling GC.Collect #2");
GC.Collect ();
Log ("GC.Collect #2 done, disposing delegate");
del.Dispose ();
Log ("done");
}
}
}
Loading