Bug 237 - Gtk# generator creates delegates that create new IDisposable objects without disposing them
Summary: Gtk# generator creates delegates that create new IDisposable objects without ...
Status: RESOLVED FIXED
Alias: None
Product: Gtk#
Classification: Mono
Component: gtk-sharp ()
Version: unspecified
Hardware: All All
: --- normal
Target Milestone: ---
Assignee: Bugzilla
URL:
Depends on:
Blocks:
 
Reported: 2011-08-09 05:53 UTC by Andres G. Aragoneses
Modified: 2013-11-24 11:05 UTC (History)
5 users (show)

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


Attachments
the patch to "fix" the generator (1.12 KB, patch)
2013-10-22 11:25 UTC, Andres G. Aragoneses
Details
Proposed patches (5.35 KB, text/plain)
2013-10-23 04:58 UTC, Andres G. Aragoneses
Details
Proposed patch to handle a "dispose" markup on method parameters (7.57 KB, patch)
2013-11-04 16:51 UTC, Bertrand Lorentz
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 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:
Status:
RESOLVED FIXED

Description Andres G. Aragoneses 2011-08-09 05:53:10 UTC
When running banshee with gtk-sharp master, I found several traces like this:

Cairo.Context: called from finalization thread, programmer is missing a call to Dispose

So I debugged them and found out that the creator of those Cairo.Context objects is not banshee or hyena, but Gtk# itself. The culprit, line 590 of gtk/generated/CellRenderer.cs:

static void Render_cb (IntPtr inst, IntPtr cr, IntPtr widget, IntPtr background_area, IntPtr cell_area, int flags)
{
  try {
    CellRenderer __obj = GLib.Object.GetObject (inst, false) as CellRenderer;
    __obj.OnRender (new Cairo.Context (cr, false), GLib.Object.GetObject(widget) as Gtk.Widget, (Gdk.Rectangle) Marshal.PtrToSt$
  } catch (Exception e) {
    GLib.ExceptionManager.RaiseUnhandledException (e, false);
  }
}

I guess that this could be fixed if we change the generator to create this delegate instead:

static void Render_cb (...)
{
  try {
    var ctxt = new Cairo.Context (cr, false);
    CellRenderer __obj = GLib.Object.GetObject (inst, false) as CellRenderer;
    __obj.OnRender (ctxt, GLib.Object.GetObject(widget) as Gtk.Widget, (Gdk.Rectangle) Marshal.PtrToSt$
  } catch (Exception e) {
    GLib.ExceptionManager.RaiseUnhandledException (e, false);
  } finally {
    ctxt.Dispose();
  }
}

Right?

I've been looking at how to do this and I guess we would need:
- In the Setup() method of ManagedCallString.cs, make it so this kind of parameters (with a CSType that is IDisposable) are inside the "special" arraylist too, so:
- The call to the constructor is generated in this method (Setup) before the call to ToString.
- The ToString() method should generate the var name used in the Setup() method.
- We would need a new Finally() method that generates the call to Dispose() of all the vars created this way in the Setup method.
- In the GenerateCallback() method of VirtualMethod class, we would generate the "finally" block that calls to the new Finally() method of ManagedCallString mentioned above.

How does that sound?
Right now I'm kind of stuck trying to understand how could we detect that a CSType is IDisposable (because CSType is a string property of the Parameter class, thus we don't have that kind of info at generation time...).
Comment 1 olivier dufour 2011-08-14 04:07:44 UTC
Hmmm, I do not think this is the right fix.
Render_cb is call with native cairo context as param.
So it must be free in C part.
The issue is that we wrap it in managed object and if we dispose it we will free the native object too whereas it must be free in C part.
So best is to let the gc dispose it as now but remove the warning in cairo dispose(bool disposing) when disposing is set to false:

if (!disposing){
				Console.Error.WriteLine ("Cairo.Context: called from finalization thread, programmer is missing a call to Dispose");
Comment 2 Mike Kestner 2011-10-12 18:14:17 UTC
I'm not sure I follow what Olivier is saying in comment #1.  If the cairo_t received in a render signal must be freed by the user, then that's a different issue, ie owned=true.

By doing new Cairo.Context (handle, false) we are taking out a ref, and the Dispose call will release the ref, so it ends up balanced, and the initial passed in ref is still owned by the native caller.

I'm not entirely comfortable with disposing things that have been passed to user code, since presumably, they could keep a ref.  In this case, that would be fairly unusual, I guess.  But trying to implement some sort of general IDisposable marshaling with automatic post-Dispose mechanism sounds a bit grandiose and possibly perilous.

I think I'd rather see this be a markup thing that is explicitly performed for an API call to ensure it's not done by accident.  Maybe a dispose="true" attr with the generator magic to do a finally call.  It probably needs to be null-guarded in the finally, too.

If you want some more clarification on the idea, let me know.
Comment 3 Andres G. Aragoneses 2012-05-03 19:51:03 UTC
(In reply to comment #2)
> If you want some more clarification on the idea, let me know.

I would love a clarification, yes.

But before that, Mike, do you think we can postpone a bugfix for this, or do you think this is a blocker for the 3.0 release? IMO it shouldn't be a blocker because the context is disposed anyway (in the finalizer instead, and it prints an ugly message, but at least there is no leak, right?).

I ask this because I think it would be great to have a gtk-sharp 3.0 release soon, what do you think? sorry for getting a bit off-topic here! hehe.

Thanks
Comment 4 Andres G. Aragoneses 2013-09-24 09:32:14 UTC
After lot of thinking about this problem again, I think the solution is more simple than we thought: the Dispose call needs to be in the OnRender() method, not in the Render_cb() one. This way, if a derived class overrides the virtual method, then that method will be in charge of disposing the parameter, but if it's not overriden, the parent method will do it.

I'll try to come up with a patch in the following days.
Comment 5 Andres G. Aragoneses 2013-10-22 11:22:46 UTC
Hey Bertrand!

CCing because I wanted to let you know that I worked on this today (thanks to the inspiration I got when I wrote comment#4) and produced an interesting patch for the generator (which I'm going to attach after I add this comment).

Interestingly, however, even if the patch seemed to generate proper .Dispose() calls on this Cairo.Context objects in the places where I wanted, caused the inverse issue of what I was trying to address: it generated an unmanaged crash (possibly because of a double free).

So Dispose() was being called twice; or once from the managed land, and then the unmanaged land freeing it via a _destroy() call).

Then I tracked down again when was the output "Cairo.Context: called from finalization thread, programmer is missing a call to Dispose" happening, and I found that it was indeed a red herring! Because it was a finalizer call to an object that had already been disposed. The fix, then, is much simpler than we thought: 

https://github.com/mono/gtk-sharp/pull/84
Comment 6 Andres G. Aragoneses 2013-10-22 11:25:06 UTC
Created attachment 5207 [details]
the patch to "fix" the generator

For the record, this was the patch I did to modify the generator to call Dispose() on this objects.
Comment 7 Bertrand Lorentz 2013-10-22 17:16:13 UTC
As I commented on the pull request (let's keep the discussion here), I don't think that's the correct fix:
If disposing is false, it means we're coming from the finalizer. This means the object was not explicitly disposed before being GC'd. This is not a permanent leak, we were just holding onto the managed resource longer than necessary.

With your change, if disposing is false, the method will return before notifying of this problem.

Keep in mind that the Context constructor can be called with owner=false, and in this case we will not increment the reference count on the native handle.
If we decrease the ref count with the Dispose calls you added, the native Cairo.Context might be destroyed.
But then, GTK+ might still try to use it after we return from the Draw or Render signal.
This might explain the crashes you mention.

This memory management stuff hurts my brain... :(
Comment 8 Andres G. Aragoneses 2013-10-22 17:39:02 UTC
(In reply to comment#7):
> Keep in mind that the Context constructor can be called with owner=false, and
> in this case we will not increment the reference count on the native handle.

You're describing it the other way around as it currently is. From the code:

public Context (IntPtr handle, bool owner)
{
	this.handle = handle;
	if (!owner)
		NativeMethods.cairo_reference (handle);

Above, if owner==false, it is incremented!

So, is this constructor wrong, to begin with?
Comment 9 Andres G. Aragoneses 2013-10-23 04:58:59 UTC
Created attachment 5214 [details]
Proposed patches

(In reply to comment#7):
>This might explain the crashes you mention.

Nope! I tracked down the crash, I think it is a bug in CairoContext dispose pattern. After I could fix this crash, I fixed other things, and I applied a tweaked version of the patch I posted in comment#6. After this, I was still getting "Cairo.Context: called from finalization thread, programmer is missing a call to Dispose" logs when running banshee, but this time they were fixable! (I could create a banshee patch to call all the Dispose() methods required.) And after I fixed those, I got no more logs about leaks! \o/

So, I think this time we're much more closer to the solution, and I'm posting my patches here. Let me know if I should open a new PR with them.
Comment 10 Andres G. Aragoneses 2013-11-02 18:27:35 UTC
Damn, I don't think anymore the patches above are correct... Because I guess that when calling cairo_create() and not calling cairo_reference() ever on that context created, cairo_destroy() still needs to be called (only *once*).

And then, the crashes I was seeing, are because gtk-sharp generator is assuming that some CairoContexts, when passed from the native side, are owned, while in reality they are not, and then destroy() is called two times, once from the managed side, and once from the unmanaged one (an example of this is the OnDrawn method, how does the generator deduce that the cairo passed in is owned? why?, from my investigation, it is not, as for example the function gtk_widget_send_expose [https://git.gnome.org/browse/gtk+/tree/gtk/gtkwidget.c#n6978] creates a cairo_context, then calls _gtk_widget_draw(), which emits the DRAW signal, and then destroys the context. However the receiver of the DRAW signal in the managed side is, AFAIU, the OnDrawn method, which then wraps the Cairo.Context as owner=true, incorrectly).

I'll be online tomorrow for the most part if you want to discuss.
Comment 11 Andres G. Aragoneses 2013-11-03 10:48:05 UTC
> how does the generator deduce that the cairo passed in is
> owned? why?

Bertrand, I guess you have already received the PR where I fixed what I wonder above. (Reverting a commit you did, otherwise there would be double-frees after fixing the leak in Dispose() when being called from the finalizer.)

I didn't reference this bug in the PR because it's kind of unrelated. Although I will wait for that PR to be reviewed/merged before I propose new patch for this.
Comment 12 Bertrand Lorentz 2013-11-03 11:23:47 UTC
I've merged the commits from the PR at https://github.com/mono/gtk-sharp/pull/91

From my understanding, what still needs to be done is something like what's done in patch n°4 that you attached here: dispose the parameter after the virtual method has been called.

But I think there are 2 things wrong with that patch:
1/ Requiring anyone overriding the method to dispose the parameter feels strange, and there is no clear indication that this is required. Furthermore, if the overriding method happen to call the base method, for example base.OnDrawn(cr), then the Context will be already be disposed when that method returns.

2/ The condition "parm.Generatable is OwnableGen && !parm.Owned" works right now, but I think it's more of a coincidence, because Cairo.Context is the only thing right now that has type=ownable.
So i think Mike's suggestion in the last part of comment #2 is probably the best way to go: add a specific "dispose" attributethat would mean:
 a) For the generator, generate the code in the callback (Drawn_cb) to call Dispose in a finally block
 b) For users overriding the method, that the parameter will be disposed when the method return, so they should not keep a reference to it.

Of course, b) will have to be reflected in the API documentation.

I'll try to implement this, unless you were already working on this.
Comment 13 Andres G. Aragoneses 2013-11-03 11:51:37 UTC
> Requiring anyone overriding the method to dispose the parameter feels strange,

I can see that! I've been thinking about this a lot, and been researching (most places recommend to not anyone in charge of calling .Dispose() except the creator of the object), and I found a possible idea to fix this "strangeness": we would pass a delegate that creates the disposable object instead of passing the disposable object (inspiration from http://stackoverflow.com/questions/7289910/passing-idisposable-as-a-parameter#answer-7289964 although in our case we don't need the ugly cast)

That is, instead of:

		protected virtual bool OnDrawn (Cairo.Context cr)

We would have:

		protected virtual bool OnDrawn (Func<Cairo.Context> new_cr)


And then the receiver of the delegate, if she decides to get the element, then he would be the creator, and therefore it should be his responsibility to call Dispose().

Do you like this idea?

If not, I have even another one.

When you think about these methods, like OnDrawn, we're mainly talking about signals. So then the objects that are being passed are mainly supposed to be short-lived. So I think it's not a big problem if, in this generated code:

static bool Drawn_cb (IntPtr inst, IntPtr cr)
{
	try {
		Widget __obj = GLib.Object.GetObject (inst, false) as Widget;
		bool __result = __obj.OnDrawn (new Cairo.Context (cr, false));
		return __result;
	} catch (Exception e) {
		GLib.ExceptionManager.RaiseUnhandledException (e, true);
		// NOTREACHED: above call does not return.
		throw e;
	}
}

We would turn it to this:

static bool Drawn_cb (IntPtr inst, IntPtr cr)
{
	var cr = new Cairo.Context (cr, false);
	try {
		Widget __obj = GLib.Object.GetObject (inst, false) as Widget;
		bool __result = __obj.OnDrawn (cr);
		return __result;
	} catch (Exception e) {
		GLib.ExceptionManager.RaiseUnhandledException (e, true);
		// NOTREACHED: above call does not return.
		throw e;
	} finally {
		if (cr != null)
			cr.Dispose ();
	}
}

For this to be even more straight-forward to understand, just in case the developers ends up storing the object somehow, we should add this to Cairo.Context (and other Disposable objects affected by this):

[PATCH] cairo: check if Cairo.Context was disposed before using it

Potentially a Cairo.Context could be used after being disposed, which
would result in native crashes. This way we avoid this and would throw
an exception in managed land instead.
---
 cairo/Context.cs | 157 +++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 140 insertions(+), 17 deletions(-)

diff --git a/cairo/Context.cs b/cairo/Context.cs
index b420f77..4887703 100644
--- a/cairo/Context.cs
+++ b/cairo/Context.cs
@@ -112,6 +112,8 @@ namespace Cairo {
 
 		protected virtual void Dispose (bool disposing)
 		{
+			disposed = true;
+
 			if (!disposing || CairoDebug.Enabled)
 				CairoDebug.OnDisposed<Context> (handle, disposing);
 
@@ -122,23 +124,39 @@ namespace Cairo {
 			handle = IntPtr.Zero;
 		}
 
+		bool disposed;
+		void CheckDisposed ()
+		{
+			if (disposed)
+				throw new ObjectDisposedException ("Object has already been disposed");
+		}
+
 		public void Save ()
 		{
+			CheckDisposed ();
 			NativeMethods.cairo_save (handle);
 		}
 
 		public void Restore ()
 		{
+			CheckDisposed ();
 			NativeMethods.cairo_restore (handle);
 		}

...and so on.

Do you think any of those 2 approaches is good? (I don't like manual approaches like "metadata" fixups :-( )
Comment 14 Bertrand Lorentz 2013-11-03 13:13:02 UTC
Approach 1 is interesting, but I think it would bring us a bit too far away from the C API. Anyone who, like me, almost only looks at the C docs for GTK+, would probably have a hard time figuring out what to do.
The SO question was about a situation where the caller/creator *can't* dispose the object, but in our situation we can, we just need to do it right. ;)

Approach 2 is similar to my suggestion, but I would only have the Dispose call(s) if the method is marked with the "dispose" attribute.

When would you generate *_cb methods with the dispose call(s) ?
And would it dispose each parameter that is an IDisposable ?
I'm wondering if that wouldn't cause problem in some cases. Maybe for some other signals you need to hold a reference to one of the parameters ?

We could indeed add CheckDisposed () in Cairo.Context if the error when using a disposed Context is not clear enough.

I'll attach a patch for my suggestion when it's done. The modifications to the generator are a bit tricky...
Comment 15 Bertrand Lorentz 2013-11-04 16:51:03 UTC
Created attachment 5333 [details]
Proposed patch to handle a "dispose" markup on method parameters

This patch implements the change I proposed, adding the "dispose" attribute to a few methods, in particular GtkWidget.Draw.

With this patch, it is now generated like this:

	static bool Drawn_cb (IntPtr inst, IntPtr cr)
	{
		try {
			Widget __obj = GLib.Object.GetObject (inst, false) as Widget;
			bool __result;
			using (var mycr = new Cairo.Context (cr, false))
			{
				__result = __obj.OnDrawn (mycr);
			}
			return __result;
		} catch (Exception e) {
			GLib.ExceptionManager.RaiseUnhandledException (e, true);
			// NOTREACHED: above call does not return.
			throw e;
		}
	}
Comment 16 Andres G. Aragoneses 2013-11-04 17:05:20 UTC
> Approach 1 is interesting, but I think it would bring us a bit too far away
> from the C API. Anyone who, like me, almost only looks at the C docs for GTK+,
> would probably have a hard time figuring out what to do.

Good point.

> The SO question was about a situation where the caller/creator *can't* dispose
> the object, but in our situation we can, we just need to do it right. ;)

Yes, but doing it right "manually" (in a case-by-case basis via metadata fixups) is what in my opinion is not "right".

> Approach 2 is similar to my suggestion, but I would only have the Dispose
> call(s) if the method is marked with the "dispose" attribute.
> 
> When would you generate *_cb methods with the dispose call(s) ?

When parameters are OwnableGen.

> And would it dispose each parameter that is an IDisposable ?

Yes, if they are OwnableGen.

> I'm wondering if that wouldn't cause problem in some cases. Maybe for some
> other signals you need to hold a reference to one of the parameters ?

Maybe, but if we don't do it for all, and then do a release so people start using this, we will never know.

> 
> We could indeed add CheckDisposed () in Cairo.Context if the error when using a
> disposed Context is not clear enough.

Ok, I'm working on a patch to fix all cases where a CheckDisposed() is needed (it would be useful even if we don't submit a bugfix for this bug).


> This patch implements the change I proposed, adding the "dispose" attribute to
> a few methods, in particular GtkWidget.Draw.

Nice one, but I still think that I don't like to do this in case-by-case basis. We still don't know if doing it every time would cause problems, so why add a feature when we don't know yet if we need the feature.

Also, if anything, I would do it inverse: make the dispose attribute be true by default, and disable it via fixups if needed.

> 
> With this patch, it is now generated like this:
> 
>     static bool Drawn_cb (IntPtr inst, IntPtr cr)
>     {
>         try {
>             Widget __obj = GLib.Object.GetObject (inst, false) as Widget;
>             bool __result;
>             using (var mycr = new Cairo.Context (cr, false))
>             {
>                 __result = __obj.OnDrawn (mycr);
>             }

Why "my". I would prefer some other prefix like "disposable_".

Other than those concerns, thanks for helping on this!
Comment 17 Bertrand Lorentz 2013-11-09 10:38:19 UTC
After giving it some more thought, I implemented the approach suggested by Andrés.
It does generated Dispose call for Cairo.Context in a few more places, so I went ahead and committed that change:
https://github.com/mono/gtk-sharp/commit/e48ac63d54

With this, the callback is generated like this:

	static bool Drawn_cb (IntPtr inst, IntPtr cr)
	{
		Cairo.Context mycr = null;

		try {
			Widget __obj = GLib.Object.GetObject (inst, false) as Widget;
			bool __result;
			mycr = new Cairo.Context (cr, false);
			__result = __obj.OnDrawn (mycr);
			return __result;
		} catch (Exception e) {
			GLib.ExceptionManager.RaiseUnhandledException (e, true);
			// NOTREACHED: above call does not return.
			throw e;
		} finally {
			var disposable_cr = mycr as IDisposable;
			if (disposable_cr != null)
				disposable_cr.Dispose ();
		}
	}

The ownable parameter has a "my" prefix to be consistent with the other special parameters in ManagedCallString, which reduces a bit the impacts on that class.

So I think this bug can be closed (can't do it myself).
Comment 18 Andres G. Aragoneses 2013-11-10 03:01:16 UTC
Cool thanks!

>So I think this bug can be closed (can't do it myself).

I would be more comfortable closing this bug when this PR is committed too (https://github.com/mono/gtk-sharp/pull/93) because this way any consumer that would be storing the Cairo object and using it later would get this exception (instead of a native crash) and therefore it will be hinted much easily that it needs to create a new object with that handle (to increase the refcount).
Comment 19 Bertrand Lorentz 2013-11-17 13:41:34 UTC
For the record, a similar leak was happening for signal handlers, specifically Widget.Drawn.
I've just fixed it with that commit:
https://github.com/mono/gtk-sharp/commit/51f102bc34de

Before the fix, the generated code was:

static bool DrawnSignalCallback (IntPtr inst, IntPtr arg0, IntPtr gch)
{
    Gtk.DrawnArgs args = new Gtk.DrawnArgs ();
    try {
        ...
        args.Args = new object[1];
        args.Args[0] = new Cairo.Context (arg0, false);
        Gtk.DrawnHandler handler = (Gtk.DrawnHandler) sig.Handler;
        handler (GLib.Object.GetObject (inst), args);
	} catch (Exception e) {
		GLib.ExceptionManager.RaiseUnhandledException (e, false);
	}
    ...
}

After the fix, the generated code is now:

static bool DrawnSignalCallback (IntPtr inst, IntPtr arg0, IntPtr gch)
{
    Gtk.DrawnArgs args = new Gtk.DrawnArgs ();
    Cairo.Context cr = null;
    try {
        ...
        args.Args = new object[1];
        cr = new Cairo.Context (arg0, false);
        args.Args[0] = cr;
        Gtk.DrawnHandler handler = (Gtk.DrawnHandler) sig.Handler;
        handler (GLib.Object.GetObject (inst), args);
        } catch (Exception e) {
            GLib.ExceptionManager.RaiseUnhandledException (e, false);
        } finally {
            var disposable_cr = cr as IDisposable;
            if (disposable_cr != null)
                disposable_cr.Dispose ();
	}
        ...
}