Bug 6338 - Exception thrown trying to reconstruct managed object that should not be necessary
Summary: Exception thrown trying to reconstruct managed object that should not be nece...
Status: RESOLVED FIXED
Alias: None
Product: iOS
Classification: Xamarin
Component: XI runtime ()
Version: 5.2
Hardware: PC Mac OS
: --- normal
Target Milestone: Untriaged
Assignee: Rodrigo Kumpera
URL:
Depends on:
Blocks:
 
Reported: 2012-07-31 11:17 UTC by Adam Kemp
Modified: 2013-08-26 13:22 UTC (History)
7 users (show)

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


Attachments
Example code (9.72 KB, application/zip)
2012-07-31 11:17 UTC, Adam Kemp
Details
Second example (7.51 KB, application/zip)
2012-09-11 18:52 UTC, Adam Kemp
Details


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 Developer Community or GitHub 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:
Status:
RESOLVED FIXED

Description Adam Kemp 2012-07-31 11:17:31 UTC
Created attachment 2281 [details]
Example code

I am hitting an exception trying to reconstruct a managed object for a view controller which should no longer be needed. This happens during a memory warning situation in which I am calling Dispose() on this view controller. This view controller happens to be the root view controller of a UINavigationController which is also no longer needed (not on screen), but has not yet been disposed. It appears that the UINavigationController is holding this view controller in memory (not surprising) and also trying to call some kind of method on it. I have no idea what that method is, but I know that there is no managed implementation of it because the only override I have implemented in this view controller is ViewDidLoad. Since I can't think of any reason that the runtime should need to construct the managed object in this situation I think it's a bug. If it turns out there is a legitimate reason then I would like to know what that reason is so that I can understand what is going on and how to avoid this in the future.

In the meantime my workaround is to call Dispose on the navigation controller as well. This appears to prevent the exception.

Attached is an example project. You can reproduce the exception through the following steps:

1. Run the app in the simulator.
2. Press the button that says "Popover".
3. Dismiss the popover by tapping anywhere outside it.
4. Simulate a memory warning.
Comment 2 Adam Kemp 2012-07-31 18:56:32 UTC
I was testing in the simulator using 5.2.12.
Comment 3 René Ruppert 2012-08-02 15:06:06 UTC
 I added all kinds of overrides to CrashingViewController() and also added the IntPtr constructor.
Then I put breakpoints in the ctor and all overrides.

I can see the following happening:

* After the object has been disposed...
* CrashingViewController.ClassHandle gets called
* the IntPtr ctor gets called at some point.
* Then the breakpoint in CrashingViewConrtroller.DidReceiveMemoryWarning() is hit (coming from UIApplication.Main)
* Next CrashingViewController.Dispose(false) is triggered (Stacktrace shows it is coming from NSObject.Finalize)

In rare cases I'm getting this output:

at (wrapper managed-to-native) MonoTouch.ObjCRuntime.Messaging.IntPtr_objc_msgSendSuper (intptr,intptr) <IL 0x00026, 0xffffffff>
  at MonoTouch.Foundation.NSObject.get_Description () [0x00021] in /Developer/MonoTouch/Source/monotouch/src/Foundation/NSObject.g.cs:538
  at MonoTouch.Foundation.NSObject.ToString () [0x00000] in /Developer/MonoTouch/Source/monotouch/src/shared/Foundation/NSObject2.cs:126
  at (wrapper runtime-invoke) <Module>.runtime_invoke_object__this__ (object,intptr,intptr,intptr) <IL 0x00050, 0xffffffff>
  at MemoryWarningTest.CrashingViewController.get_ClassHandle () [0x00000] in /Users/rene/Downloads/MemoryWarningTest/MemoryWarningTest/CrashingViewController.cs:59
Comment 4 Adam Kemp 2012-08-02 16:35:22 UTC
If you add overrides of methods that the native code would call during a memory warning then it is expected that it would try to reconstruct the object in order to call those overrides. What is not expected is that it would try to reconstruct the object even in the absence of those overrides.
Comment 5 René Ruppert 2012-08-02 17:04:46 UTC
En contraire. The native world IS trying to call a method on the disposed object. Adding the overrides will show you WHAT it is calling. If that wasn't clear: with the overrides I get exactly the crash you describe! Only in rare cases I get the exception I added. So the question for me is: why is something calling DidReceveiveMemoryWarning() on a disposed object, because that is what is getting called after the object has been resurrected via the IntPtr ctor.
Comment 6 Adam Kemp 2012-08-02 17:15:43 UTC
If UIViewController's native code calls a method which is not overridden in managed code then it can do so without resurrecting the managed object. It will just call native code.

I know why DidReceiveMemoryWarning is being called: as long as the view controller exists in memory (the native object) it has a subscription (using NSNotificationCenter) to receive memory warning notifications. It unsubscribes from those notifications when it is deallocated. The code that is called in response to that notification calls DidReceiveMemoryWarning.

Again, if you don't override that method in your managed object then it should be able to call the native version of that method in the base class, and it should not have any reason to resurrect the object.

I can prove this by making the same kind of example as above, but don't put the view controller that is disposed in a UINavigationController. My earlier experiment did the following:

1. Construct a UIViewController. Don't show it on screen or add it to any navigation controller.
2. In DidReceiveMemoryWarning of the top-level view controller do the following:
2.a. Call retain and autorelease on the view controller created in step 1. This requires using objc_msgsend.
2.b. Dispose the view controller's managed object and null it out.
3. Run and trigger a memory warning. You won't crash.

If you repeat those steps but add a DidReceiveMemoryWarning override in the disposed view controller then you will crash. You can add the IntPtr constructor to fix that crash and verify that that method is called. Remove both the IntPtr constructor and the DidReceiveMemoryWarning override and the crash goes away.

We shouldn't need a managed object to call a method that is only implemented in native code. The bug here is that for some reason the runtime is constructing a managed object even when we don't have an override in managed code to call.
Comment 7 René Ruppert 2012-08-03 01:43:29 UTC
Hm, my understanding of the cooperation of native objects and MT has been so far: if the managed version no longer exists, the unmanaged one shouldn't exist either any longer. No matter if you do overrides or not. But following your argumentation, this is wrong. So  let's see what the specialists at Xamarin say and find. I'm very curious if there is a bug (I hope so), because it would explain quite some odd crashes.
Comment 8 Rolf Bjarne Kvinge [MSFT] 2012-08-07 09:42:52 UTC
The current behavior is like this:

The unmanaged peer will stay alive as long as the managed object is alive, unless the managed object is disposed. When Dispose is called on a managed object, we break the link between the native peer and the managed object, and we stop keeping track of the native peer (so if there are other native references to it, it isn't freed).

Then a problem arises if an overridden selector is called. We try to find the managed object corresponding to the native peer, we don't find any, and try to create a new instance of the managed object (and if we fail to create a new instance, you'll get the 'object has been gc'ed' exception).

Note that we override retain and release, so it is very likely that you'll end up with the exception (or a re-created managed object if the IntPtr ctor exists).

I believe the fix is to continue keeping track of the native peers after a managed object has been Disposed, but this is quite a major change (memory usage will likely go up, managed objects will stay alive longer, etc), so it will need some testing. It will likely not be in 5.4, but may get into the next release after that.
Comment 9 René Ruppert 2012-08-07 09:56:36 UTC
How hard would it be to add as an enhancement for error tracking the (managed) stak trace? Usually when I'm confronted with the issues above I want to know what the heck it was trying to call. Was it GetCell()? Dispose()? DidReceiveMemoryWarning()? It would take the mystery out of those crashes and maybe help to understand WHY things go wrong.
Comment 10 Adam Kemp 2012-08-07 14:10:56 UTC
What does the managed implementation of retain/release actually do? It seems like a bad idea to have managed code that has to run for those methods because that basically makes it impossible to have the native object outlive the managed object (releasing the native object forces the managed object back into existence). I think there are too many cases where this might happen (such as passing your object into a method which calls retain and then autorelease). 

Could you push a flag into the native object to mark it as disposed and have retain/release not go back into managed code in that case? Or just not do anything in retain/release in managed code at all?

I'm not optimistic about keeping all of the managed objects alive for longer. I explicitly want it to leave memory, and in this case it will leave memory but just after a bit of a delay. There is nothing in my code which requires the managed object after I have disposed it. I don't want the runtime keeping an object in memory which I no longer have any use for.
Comment 11 Sebastien Pouliot 2012-08-07 15:54:01 UTC
I think there's a bit of misunderstanding (it's a complex subject). 

> What does the managed implementation of retain/release actually do?

There's no such thing - at least not "managed". The "override" term was used as a too general way (not the .NET way). Objects will answer to the retain/release selectors (so in that sense they are overridden) but it's not coming from managed code (it's implemented in objective-c in monotouch runtime) and won't trigger managed code.

Here's two examples where retain/release are used after the managed object was disposed. In neither cases the runtime will try to call the (non-existing) .ctor(IntPtr)

		class MyUIView : MonoTouch.UIKit.UIView {
		}

		static IntPtr Retain = MonoTouch.ObjCRuntime.Selector.GetHandle ("retain");
		static IntPtr Release = MonoTouch.ObjCRuntime.Selector.GetHandle ("release");

		[Test]
		public void ReleaseAfterDispose ()
		{
			var v = new MyUIView ();
			var h = v.Handle;
			Assert.That (v.RetainCount, Is.EqualTo (1), "a");
			var c = new MonoTouch.UIKit.UIViewController ();
			c.Add (v);
			int rc = v.RetainCount;
			Assert.That (rc, Is.GreaterThanOrEqualTo (2), "b");
			// we call retain again so we do not crash the test with an extra release later
			MonoTouch.ObjCRuntime.Messaging.void_objc_msgSend (h, Retain);
			Assert.That (v.RetainCount, Is.EqualTo (rc + 1), "c");
			v.Dispose ();
			// at this stage the managed object is invalid - don't even ask for retainCount
			v = null;
			GC.Collect (GC.MaxGeneration);
			GC.WaitForPendingFinalizers ();
			MonoTouch.ObjCRuntime.Messaging.void_objc_msgSend (h, Release);
			// note: we're not calling back to managed code, e.g. the .ctor(IntPtr)
		}

		[Test]
		public void RetainAfterDispose ()
		{
			var v = new MyUIView ();
			var h = v.Handle;
			Assert.That (v.RetainCount, Is.EqualTo (1), "a");
			var c = new MonoTouch.UIKit.UIViewController ();
			c.Add (v);
			Assert.That (v.RetainCount, Is.GreaterThanOrEqualTo (2), "b");
			v.Dispose ();
			// at this stage the managed object is invalid - don't even ask for retainCount
			v = null;
			GC.Collect (GC.MaxGeneration);
			GC.WaitForPendingFinalizers ();
			MonoTouch.ObjCRuntime.Messaging.void_objc_msgSend (h, Retain);
			// native object will be retained forever
		}
Comment 12 Adam Kemp 2012-08-07 18:05:37 UTC
I was confused by your comment above:

"Then a problem arises if an overridden selector is called. We try to find the
managed object corresponding to the native peer, we don't find any, and try to
create a new instance of the managed object (and if we fail to create a new
instance, you'll get the 'object has been gc'ed' exception).

"Note that we override retain and release, so it is very likely that you'll end
up with the exception (or a re-created managed object if the IntPtr ctor
exists)."

If retain and release are not overridden in managed code then why is it likely that we would end up with the exception? The question still remains: which function is being called in my example which causes the monotouch runtime to think that it needs the managed object? If it doesn't need to call any managed functions then why is it reconstructing the object? It should be possible for the native object to exist and even have methods called on it without the managed object existing so long as no managed functions need to be called. 

I don't want the managed object to live longer. I want it to be collected and stop existing, and I don't want a method call that needs only native code to force it back into memory. Especially when the native object is going to leave memory as well very shortly after this method call.
Comment 13 Rolf Bjarne Kvinge [MSFT] 2012-08-08 09:42:31 UTC
Sorry, my comment wasn't quite accurate.

For release we do not need the managed object so this won't happen if only release is called, and for retain we only need the managed object in a few circumstances (when retainCount goes from 1 to 2), not for all retain calls.

In fact, that scenario is exactly what's happening in the provided sample. UINavigationController likes to keep caches of things, so it keeps the instance of CrashingViewController around. When the memory warning is triggered, it decides to purge the cache, and in the process retain is called on the CrashingViewController. Since the UINavigationController had the only reference to the CrashingViewController, retainCount went from 1 to 2, and we ended up trying to re-create the managed peer.

It *may* be possible to remove the need for creating a managed object in retain, but it doesn't really solve the problem: native code may still call overridden methods after Dispose (and you have no control of what native code does and which objects it caches / keeps in memory - it may even change between iOS versions). I will look into this too to see if it is indeed possible to make retain work better.

We are working on providing better diagnostics for these types of failures, which will hopefully make it easier to diagnose for you.
Comment 14 Adam Kemp 2012-08-08 17:47:17 UTC
Thanks for the clarification. I don't need a solution to the general problem. If the native object lives long enough that someone calls a method on it which is implemented in our code then we should obviously not call Dispose on it, and we should keep a reference to that managed object around. I am only worried about the cases like the attached example where we really don't need the managed object anymore, the native object is on its way out (but has perhaps been delayed), and I know for sure that none of my managed code for that object is being called. I want that case to be safe.

If you could make retain not try to recreate the object then I think that would fix this issue, and that would be good enough for me. Retain is called in a lot of situations where it is just temporarily retained and autoreleased just to ensure that it can be used further up the stack. In fact, I think the getter methods for retained properties use retain and autorelease before returning the value. It's just a common scenario to retain something temporarily even if it is about to be deallocated.
Comment 15 Sebastien Pouliot 2012-08-14 13:04:07 UTC
As stated on the mailing-list:

Rolf added more details (e.g. the selector name) when this exception
occurs. This will be available in our next release (5.3.6, already in
progress) and should help you (and us) better identify why those cases
occurs (leading the way to better fixes).
Comment 16 Adam Kemp 2012-08-23 17:49:11 UTC
I am still hitting this exception in some cases where I have disposed both the UINavigationController and its first contained view controller. We hit it today in a test build using 5.3.5.

It is very important to us that we get a fix for this issue. It seems like the way that the runtime behaves right now it would never be safe to ever call Dispose() on an NSObject because we can't guarantee that nothing in the unmanaged world has a reference to it. Even in cases where we know that our managed code will never be called again, the way the runtime works right now it may try to resurrect the managed object anyway and crash as a result.

Memory management is becoming a serious issue for our application. We cannot rely on the finalizer to eventually get around to disposing of references to objects. We have to proactively dispose things that we don't need any more, including view controllers with potentially large view hierarchies. We need those calls to Dispose() to be safe.

Just to be clear, the extra debugging information is useful, but that does not solve the original issue for this bug report. The issue is that the managed object should not be reconstructed at all in this case, and extra debugging information won't help us. We need the runtime to not ever try to recreate the managed object when it's not needed.
Comment 17 Adam Kemp 2012-09-11 18:52:16 UTC
Created attachment 2505 [details]
Second example
Comment 18 Adam Kemp 2012-09-11 19:03:04 UTC
I just attached another example that shows why I think this problem is a big deal. In the new attachment just build and run, press the button in the center, then press the new button in the center after it slides up. You will hit a crash after pressing the second button.

All I'm doing in this example is presenting a navigation controller as a modal view controller, and then when the second button is pressed I dismiss that view controller and dispose both it and its contained view controller. Then I am programmatically triggering a memory warning just after disposing those objects. This simulates what could happen any time you have the native object side of a view controller in memory still after the managed object is disposed or garbage collected and then you hit a memory warning.

This cannot be fixed in our code unless we never call Dispose on any view controllers AND never allow them to be garbage collected. That is because it is impossible for us to know when it would be safe for the managed object to go away. The refcounted nature of the native objects means that anything could keep those things in memory for some undefined period of time, and there is no notification mechanism we could use to know when nothing holds a reference to it.

We need for it to be safe for view controllers to be garbage collected and/or disposed. The current situation leads to random crashes that we have no way of fixing.
Comment 19 Miguel de Icaza [MSFT] 2012-10-02 17:07:51 UTC
Hello Adam,

The example that you uploaded on Comment #17 is disposing an object from within itself, which means that effectively the object is dead while you are still running code inside that handler.

The object state is effectively invalid after you call Dispose, but you are running inside a handler that is hosted by the very object that you just destroyed.   This is not a supported scenario.
Comment 20 Adam Kemp 2012-10-02 18:36:56 UTC
Miguel, that may be true, but that is not what is causing the crash. If you change the code which triggers a memory warning to delay the actual memory warning using a timer then it still crashes. I.e., change the memory warning part to this:

        private void HandleButtonPressed2(object sender, EventArgs args)
        {
            window.RootViewController.DismissModalViewControllerAnimated(true);
            
            NSTimer.CreateScheduledTimer(0, () =>
            {
                navController.Dispose();

                var disposable = sender as IDisposable;
                if (disposable != null)
                {
                    disposable.Dispose();
                }
                var sel = new MonoTouch.ObjCRuntime.Selector("_performMemoryWarning");
                MonoTouch.ObjCRuntime.Messaging.void_objc_msgSend(UIApplication.SharedApplication.Handle, sel.Handle);
            });
        }

That will cause the memory warning to be triggered only after returning to the event loop. At that point it will be outside the handler for the button. The problem is not that we are disposing an object that is still being used. The problem is that even if we are truly done with that object and call Dispose() on it properly the MonoTouch runtime could still resurrect it in certain circumstances.

Specifically, what is happening is that even after we call Dispose() the native object is still alive because something still has a (temporary) reference to it. Because that native object is still alive, when a memory warning occurs that native object is handling the memory warning (as all view controllers do), and this ends up triggering a call to "retain". That call provokes the MonoTouch runtime into trying to recreate the managed object, and thus we crash. It should not be trying to create that managed object in this scenario. If it just didn't do that then the example would not crash.

I created these pathological examples to demonstrate that the problem is possible. These are not the exact situations that our app runs into. The problem is that the implementation of the runtime creates a race condition any time you call Dispose() on a view controller: if the system receives a memory warning between the call to Dispose and the actual native object leaving memory then the app will crash. We have seen this occur in our app. It is very difficult to reproduce on demand, but it happens randomly every once in a while when the app is using enough memory to trigger a warning.
Comment 21 Miguel de Icaza [MSFT] 2012-10-02 20:59:47 UTC
Your new sample is just as broken.

The timer fires as soon as the mainloop executes again, which happens before the active controller that you are using has been dismissed (it takes about half a second for this).

If you change the timer to be some time like 3 seconds, you will see that the crash is gone.   And the reason is simple: you are destroying an object while it is still in use.

If you want to do this correctly, you can rewrite the code like this:


window.RootViewController.DismissViewController (true, delegate {
    // Invoked when the animation has completed, but the VC is still in the hierarchy
    // Trigger a call on the next main loop iteration:
    NSTimer.CreateScheduledTimer (0, delegate {
       // Your Dispose goes here.
    }
}
Comment 22 Adam Kemp 2012-10-03 12:04:47 UTC
I think we're nitpicking the examples, and I don't think that is valuable. It sounds like you are questioning whether there is a bug at all, so let me again make the case for why this current implement is unacceptable.

The ObjectiveC APIs that Apple provides are ref-counted. Objects in their API do not have clear owners. They exist as long as anything still refers to them, and they leave memory when nothing still refers to them. The way their APIs work is that if you hand an object to another object it may increment the refcount, possibly extending the lifetime of the object you handed it. The API documentation often doesn't even cover when the refcount will or will not be modified. That's usually not important. What is important is that the objects stay alive long enough to be used safely.

MonoTouch has wrapped that API in .Net using the IDisposable pattern. That pattern assumes that there is clear ownership: each object has a single owner, and that owner controls the lifetime of that object by calling Dispose() when the object is no longer needed. In theory, calling Dispose() means that object can leave memory because you are asserting (as the owner of the object) that no one else should need that object any more.

Unfortunately, when using the CocoaTouch APIs we cannot possibly guarantee that no one else still refers to the object on the native side. It is not possible. There are hidden relationships that are not documented, and normally have no reason to be documented. UINavigationController, for instance, does not document exactly when it calls retain and release on the view controllers you give it.

When you combine this with autorelease and the behavior of ObjectiveC properties the problem only gets worse. An ObjectiveC property getter with "strong" (in ARC) or "retain" (pre-ARC) semantics looks something like this:

-(Foo*)getFoo
{
   return [[_foo retain] autorelease];
}

That means you could call a method in preparation for disposing it, and that method you call could inadvertently extend the lifetime of that object. Then when you call Dispose() the refcount is not 1 (i.e., the object is not disposed) because something still refers to it. What happens if something else in its dealloc method also causes a retain/autorelease on that object? That would cause a crash if you had already called Dispose() on the managed object. It shouldn't, though. This kind of behavior is perfectly safe in ObjectiveC.

If you try to write the examples I have provided in ObjectiveC you would not need a completion block. The view controller API already guarantees that the view controller will survive until the animation is finished. The old APIs didn't even have completion blocks. You just told it to disiss and then immediately called release because YOUR code was done with it. It would leave memory automatically when the animation was complete.

The current implementation of the MonoTouch runtime forces us to reverse engineer all of the undocumented retain relationships between the native objects (which are totally opaque to us from the managed side). It makes writing code that would be perfectly safe in ObjectiveC fatally dangerous in MonoTouch, and figuring out how to fix it requires again determining which other object has a refcount on this one so that we can force it to release it.

We should not have to do this. If the MonoTouch runtime simply did not resurrect an object it didn't actually need to use then there would be no crash. We would not have to figure out where every automatic retain happens or worry about the order in which we call Dispose(). That is not how the CocoaTouch API was meant to be used.
Comment 24 Adam Kemp 2012-11-15 11:58:59 UTC
Miguel, I'm sorry for the lack of response to your request. Somehow I was removed from the CC list and so I never got an email notification. I saw the announcement on the forum which led me to recheck this bug report. I will try to get someone on our team to try out this build.

Thanks.
Comment 25 Adam Kemp 2012-11-15 17:19:29 UTC
A teammate did some initial testing with the beta build, and so far everything looks fine. It will probably be a while before we can give a more definitive answer than that, but so far so good.
Comment 26 Rodrigo Kumpera 2013-08-26 13:22:43 UTC
Since no update was given in more than 6 months. Assuming it's fixed.