From 2fe44b98900b2b2a52f699b5d3ed1980387c1bb3 Mon Sep 17 00:00:00 2001 From: Rolf Bjarne Kvinge Date: Tue, 13 Mar 2018 12:30:32 +0100 Subject: [PATCH] [ObjCRuntime] Don't double-retain blocks. (#3717) * [ObjCRuntime] Don't double-retain blocks. First there was darkness; no blocks were retained. Then came the light; and all blocks were retained [1] Forever. But all that once is, must one day not be, and thus the light gave way to darkness, and blocks were only retained as long as need be [2]. But before there was a balance, there was a crossroad. In some places the light shone forever, and all blocks were retained. In other places there was a balance, and the light shone only as long as needed. A desire to unify arose. Alas, it could not be. It was a bright and sunny day When a merge failed [3]. And all blocks were retained. Twice. Once [here][4] and once [there][5]. For many years we could not see. Until a dark and rainy night, when an awareness arose. And the desire to unify the balance could finally be fulfilled. [1]: https://github.com/xamarin/maccore/commit/6efca92acb5d0f2b1b495a40ac2e89586200d206 [2]: https://github.com/xamarin/maccore/commit/a22f877539058b889555ae4decae0f2e9aca029c [3]: https://github.com/xamarin/maccore/commit/befa0477cf9f3c9602d3fda6b9dee65e5bba506f [4]: https://github.com/xamarin/xamarin-macios/blob/5158a3c00138ae9bc45979487ca6a0fbc3fccc4d/src/ObjCRuntime/Runtime.cs#L858 [5]: https://github.com/xamarin/xamarin-macios/blob/5158a3c00138ae9bc45979487ca6a0fbc3fccc4d/runtime/runtime.m#L2091 * [tests] Fix test builds. * [monotouch-test] RegistrarTest.BlockCollection: allocate more and wait longer for the GC. Allocate more objects and wait longer for the GC to run. Hopefully fixes this problem: [FAIL] RegistrarTest.BlockCollection : freed blocks Expected: greater than 0 But was: 0 The blocks are freed if we just wait long enough... The problem is that we don't want to wait very long (makes the tests slow to run), so try to speed things up by allocating more. --- src/ObjCRuntime/Runtime.cs | 1 - tests/bindings-test/ApiDefinition.cs | 7 +++++ tests/bindings-test/RegistrarBindingTest.cs | 3 ++- tests/bindings-test/bindings-test-mac.csproj | 1 + .../ObjCRuntime/RegistrarTest.cs | 24 +++++++++++++++++ tests/test-libraries/libtest.h | 9 +++++++ tests/test-libraries/libtest.m | 26 +++++++++++++++++++ tests/test-libraries/rename.h | 2 ++ 8 files changed, 71 insertions(+), 2 deletions(-) diff --git a/src/ObjCRuntime/Runtime.cs b/src/ObjCRuntime/Runtime.cs index f58d2b04b9..3606e27f88 100644 --- a/src/ObjCRuntime/Runtime.cs +++ b/src/ObjCRuntime/Runtime.cs @@ -855,7 +855,6 @@ namespace ObjCRuntime { #endif static Delegate CreateBlockProxy (MethodInfo method, IntPtr block) { - NSObject.DangerousRetain (block); return (Delegate) method.Invoke (null, new object [] { block } ); } diff --git a/tests/bindings-test/ApiDefinition.cs b/tests/bindings-test/ApiDefinition.cs index e92457191c..4c99638094 100644 --- a/tests/bindings-test/ApiDefinition.cs +++ b/tests/bindings-test/ApiDefinition.cs @@ -284,6 +284,13 @@ namespace Bindings.Test { [Export ("callOptionalCallback")] void CallOptionalCallback (); + + [Export ("testFreedBlocks")] + void TestFreedBlocks (); + + [Static] + [Export ("freedBlockCount")] + int FreedBlockCount { get; } } } diff --git a/tests/bindings-test/RegistrarBindingTest.cs b/tests/bindings-test/RegistrarBindingTest.cs index 495de89ac3..dc2441da23 100644 --- a/tests/bindings-test/RegistrarBindingTest.cs +++ b/tests/bindings-test/RegistrarBindingTest.cs @@ -1,4 +1,5 @@ using System; +using System.Threading; #if __UNIFIED__ using Foundation; @@ -63,7 +64,7 @@ namespace Xamarin.BindingTests } } - class BlockCallbackTester : ObjCBlockTester + public class BlockCallbackTester : ObjCBlockTester { public override void ClassCallback (Action completionHandler) { diff --git a/tests/bindings-test/bindings-test-mac.csproj b/tests/bindings-test/bindings-test-mac.csproj index f950c3b6ef..4e89ea09d8 100644 --- a/tests/bindings-test/bindings-test-mac.csproj +++ b/tests/bindings-test/bindings-test-mac.csproj @@ -62,6 +62,7 @@ Registrar.cs + diff --git a/tests/monotouch-test/ObjCRuntime/RegistrarTest.cs b/tests/monotouch-test/ObjCRuntime/RegistrarTest.cs index 75c1e1f378..a8cb32f9ae 100644 --- a/tests/monotouch-test/ObjCRuntime/RegistrarTest.cs +++ b/tests/monotouch-test/ObjCRuntime/RegistrarTest.cs @@ -2026,6 +2026,30 @@ namespace MonoTouchFixtures.ObjCRuntime { } } + [Test] + public void BlockCollection () + { + Exception ex = null; + int initialFreedCount = ObjCBlockTester.FreedBlockCount; + var thread = new Thread (() => { + try { + using (var obj = new Xamarin.BindingTests.RegistrarBindingTest.BlockCallbackTester ()) { + for (int i = 0; i < 10000; i++) + obj.TestFreedBlocks (); + } + } catch (Exception e) { + ex = e; + } + }); + thread.Start (); + thread.Join (); + GC.Collect (); + GC.WaitForPendingFinalizers (); + TestRuntime.RunAsync (DateTime.Now.AddSeconds (30), () => { }, () => ObjCBlockTester.FreedBlockCount > initialFreedCount); + Assert.IsNull (ex, "No exceptions"); + Assert.That (ObjCBlockTester.FreedBlockCount, Is.GreaterThan (initialFreedCount), "freed blocks"); + } + [Test] public void TestCtors () { diff --git a/tests/test-libraries/libtest.h b/tests/test-libraries/libtest.h index a9998bd2ba..9d86bb10aa 100644 --- a/tests/test-libraries/libtest.h +++ b/tests/test-libraries/libtest.h @@ -1,5 +1,6 @@ #import #include +#include #include "rename.h" @@ -149,6 +150,14 @@ typedef unsigned int (^RegistrarTestBlock) (unsigned int magic); -(void) callClassCallback; -(void) callRequiredCallback; -(void) callOptionalCallback; + +-(void) testFreedBlocks; ++(int) freedBlockCount; +@end + +@interface FreedNotifier : NSObject { +} +-(void) dealloc; @end #ifdef __cplusplus diff --git a/tests/test-libraries/libtest.m b/tests/test-libraries/libtest.m index ca6a7da5e0..5a8a4375a1 100644 --- a/tests/test-libraries/libtest.m +++ b/tests/test-libraries/libtest.m @@ -510,6 +510,8 @@ static UltimateMachine *shared; } @end +static volatile int freed_blocks = 0; + @implementation ObjCBlockTester -(void) classCallback: (void (^)(int32_t magic_number))completionHandler { @@ -549,6 +551,30 @@ static UltimateMachine *shared; assert (called); } +-(void) testFreedBlocks +{ + FreedNotifier* obj = [[FreedNotifier alloc] init]; + __block bool success = false; + [self classCallback: ^(int magic_number) + { + assert (magic_number == 42); + success = obj != NULL; // this captures the 'obj', and it's only freed when the block is freed. + }]; + assert (success); + [obj release]; +} ++(int) freedBlockCount +{ + return freed_blocks; +} +@end + +@implementation FreedNotifier +-(void) dealloc +{ + OSAtomicIncrement32 (&freed_blocks); + [super dealloc]; +} @end #include "libtest.decompile.m" diff --git a/tests/test-libraries/rename.h b/tests/test-libraries/rename.h index d11719d189..47f6f40490 100644 --- a/tests/test-libraries/rename.h +++ b/tests/test-libraries/rename.h @@ -4,6 +4,7 @@ #define useZLib object_useZLib #define ObjCRegistrarTest object_ObjCRegistrarTest #define ObjCBlockTester object_ObjCBlockTester + #define FreedNotifier object_FreedNotifier #define FakeType2 object_FakeType2 #define UltimateMachine object_UltimateMachine #define FrameworkTest object_FrameworkTest @@ -67,6 +68,7 @@ #define useZLib ar_useZLib #define ObjCRegistrarTest ar_ObjCRegistrarTest #define ObjCBlockTester ar_ObjCBlockTester + #define FreedNotifier ar_FreedNotifier #define FakeType2 ar_FakeType2 #define UltimateMachine ar_UltimateMachine #define FrameworkTest ar_FrameworkTest