Bug 2508 - NSObject.SetNativeField doesn't retain/release
Summary: NSObject.SetNativeField doesn't retain/release
Alias: None
Product: iOS
Classification: Xamarin
Component: Tools ()
Version: 5.0
Hardware: PC Mac OS
: --- normal
Target Milestone: Untriaged
Assignee: Bugzilla
Depends on:
Reported: 2011-12-14 19:00 UTC by Chris Toshok
Modified: 2016-02-05 15:57 UTC (History)
3 users (show)

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 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:

Description Chris Toshok 2011-12-14 19:00:53 UTC
Outlets created in interface builder seem to be marked as (retain) either implicitly by xcode or by the headers monodevelop generates when generating the project.

Either through this implicit attribute or by design, the nib loader -retain's the value assigned to an outlet when the nib is loaded.

If you then set the property in C# using the autogenerated setter, it fails to:

1) release the current value if non-nil
2) retain the new value if non-nil

This means that if you load a nib and set one of the outlet properties to a different value, you leak the old value forever.  It'll have a RC of 1, but nothing has a reference to it.

I wrote my own version of the method and it fixes the problem:

public static void SetNativeField_ (NSObject obj, string name, NSObject v)
	IntPtr ip = GetObjCIvar_ (obj, name);
	if (ip != IntPtr.Zero) {
		// release the previous value
		Messaging.void_objc_msgSend (ip, new Selector ("release").Handle);
	if (v != null && v.Handle != IntPtr.Zero)
		Messaging.void_objc_msgSend (v.Handle, new Selector ("retain").Handle);
	obj.SetNativeField (name, v);

This bug definitely applies to at least the old-style generated [Connect] properties, the ones which used SetNativeField/GetNativeField.  The situation *should* be the same with the new [Outlet] properties, though, since presumably the nib loader will be -retaining them too.
Comment 2 Mikayla Hutchinson [MSFT] 2011-12-14 23:56:15 UTC
This might not affect [Outlet], at least on iOS.

According to http://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/LoadingResources/CocoaNibs/CocoaNibs.html#//apple_ref/doc/uid/10000051i-CH4-SW18, for iOS, properties should be of the form
@property (nonatomic, retain) IBOutlet UserInterfaceElementClass *anOutlet;

This is pretty much equivalent to what we do implicitly for [Outlet] - we keep a reference to a managed wrapper which retains the underlying native object, and releases it when the class is collected/destroyed.

The doc also says:
"Objects in the nib file are created with a retain count of 1 and then autoreleased. As it rebuilds the object hierarchy, UIKit reestablishes connections between the objects using setValue:forKey:, which uses the available setter method or retains the object by default if no setter method is available"

Since the [Outlet]s have setter methods, they shouldn't be retained again by the nib loader. I would assume ivars are different because they do not have setters, so cannot retain the object themselves.

This does raise the question why the old ivar-based outlets aren't leaking anyway, since presumably they should be manually [release]'d when deallocing the class that holds the ivars?

I did some basic testing for iOS and the objects did appear to be dealloced but I may not have been reproducing the problem, could you confirm?

MacOS is another matter, it has different retain semantics for deserialization, and the docs aren't as clear on how it works.
Comment 3 Mikayla Hutchinson [MSFT] 2011-12-15 00:34:04 UTC
I think I verified that my sample reproduces the issue.

Using the master-view template, in the detail view's ViewDidLoad  I assign a new UILabel to the outlet after calling base.I also call GC.Collect immediately before PushViewController in the root controller.

Showing and closing the detail view repeatedly is fine. The GC.Collect and the mainloop's NSAutoReleasePool should be ensuring that everything's cleaned up. I confirmed this by storing label's handle and using
Console.WriteLine (Messaging.int_objc_msgSend (oldLabelPtr, Selector.GetHandle ("retainCount")));
and it would frequently show errors indicating that the objects had been dealloc'ed.

When I change the outlet to an old-style [Connect] outlet, I was able to reliably get a double-free. I believe this means that the new UILabel object I had assigned to the outlet was over-released, indicating that something had retained it and was expecting to be able to release it again. However, I'm not actually sure offhand what's doing the releasing.

Could you verify that switching to [Outlet] fixes the problem for you?
Comment 4 Mikayla Hutchinson [MSFT] 2011-12-15 00:42:23 UTC
For MonoMac/AppKit, the older memory management docs indicate you don't need to [release] outlets on MacOS: http://www.cocoabuilder.com/archive/cocoa/308357-nonatomic-vs-atomic-assign-retain.html#308359

I don't know whether that means AppKit retains and releases the outlets - if it does, that means we'd have this problem on MonoMac for both [Outlet] and [Connect]. If it just assumes they're kept alive by the UI object graph, we should be okay.
Comment 5 Chris Toshok 2011-12-15 12:27:53 UTC
> This does raise the question why the old ivar-based outlets aren't leaking
> anyway, since presumably they should be manually [release]'d when deallocing
> the class that holds the ivars?

That's indeed what happens.  The monotouch code should be releasing all ivars when it deallocs the object.
Comment 6 Mikayla Hutchinson [MSFT] 2011-12-15 14:43:05 UTC
I guess [Outlet] will leak on MonoMac then. We could fix that by making the obj-c setter for the [Outlet] properties autorelease the incoming value, since it will be retained by the managed wrapper.
Comment 7 Chris Toshok 2011-12-15 23:01:11 UTC
they're already going to be released when the object is dealloc'ed.  better to just retain the incoming value.
Comment 8 Mikayla Hutchinson [MSFT] 2011-12-15 23:20:56 UTC
No they won't, [Outlet] is backed by a managed field not an ivar. If AppKit retains the object before assigning to the property and expects us to release it, it will leak, since nothing will perform the corresponding release.

{Outlet]s in UIKit don't have this problem. UIKit autoreleases the object before assigning it to the property setter. The setter is expected to have retain semantics, i.e. retain the object itself, and release when a new value is assigned or the containing object is dealloc'ed. We basically get that behaviour via the GC.

If we autorelease objects on obj-c setter wrappers for MonoMac then our [Outlet] properties will behave exactly the same as with UIKit.
Comment 9 Chris Toshok 2011-12-15 23:29:30 UTC
sorry, misread that [Outlet] as [Connect] in comment 6.
Comment 10 Mikayla Hutchinson [MSFT] 2011-12-16 00:04:10 UTC
Yeah, [Connect] in MonoMac should behave the exact same way as MonoTouch, i.e. it will behave correctly in normal usage but in your use case it will cause a leak and a double-free.
Comment 11 Rolf Bjarne Kvinge [MSFT] 2016-02-05 15:57:18 UTC
I can't find any current uses of GetNativeObject/SetNativeObject, so I've deprecated these functions (and removed them from watchOS/tvOS).

If we need them, they should be re-implemented with proper semantics.

maccore/master: https://github.com/xamarin/maccore/commit/0ad5ca566308085ae56ca21b000c9d6c57479a5d