MemChecker: attempt to release non-allocated block #146
Comments
|
It can't hurt, but i may need to try and use the ownership information (if available) from the GIR files, and only free the handle in the destructor if we own de handle. |
|
Thanks, I'm going to spend some time tonight trying to reproduce it and generate additional stack traces just for additional data points. The previous issue was very easy to reproduce, this one is much more difficult so maybe the previous fix covered the majority of cases but something is still missing. As well, if there is anything in my code I should check in terms of things that could be causing this feel free to let me know, I don't think I'm doing anything weird but who knows. |
|
So I was able to reproduce a few more times tonite, all stack traces (appended below) point to the RGBA destructor. I tried adding the code to null the reference but it didn't make any difference. I also tried my first fix from the original thread and it still happened. If you need to use Terminix to reproduce the problem, editing the profile and moving the transparency slider back and forth repeatably causes the crash to happen. Before your original fix it was almost immediate, now you have to move it back and forth quite a bit to get it to happen. |
|
Just to have the info here: I commented on gnunn1/tilix#101 (comment) , the stack trace I posted there also refers to the RGBA destructor. |
|
I've been thinking about this a bit and one thing I'm wondering about is when I add code to set the gdkRGBA reference to null I still get the error "attempt to release non-allocated block", I'm having a hard time understanding how this could be happening since there is a null check in the destructor before calling free. I'm not going to claim to have a deep understanding of C style memory management since I come from a Java background but I can only see a few scenarios for this: a. Something is calling the RGBA class b. Something in GTK is freeing the RGBA struct, i.e. a widget that is assuming ownership of the struct contrary to convention c. The D GC is freeing the struct memory, but since this struct is allocated using either gmallloc or rgba_copy it should be outside the scope of D's GC correct? This is what the D GC page seems to state d. The gtkdRGBA struct was never allocated to begin or it is using a D allocated struct, looking at the code I don't see a path for this as I can't find any obvious cases where the D allocated struct is used by the class, it's always copied. Only exception would be if someone called the The other thing I wanted to ask about, purely as a curiousity question, is whether it makes sense for gdkRGBA to be managed by GLib at all. You could convert the gdkRGBA to use the same semantics as ObjectG where it is a D allocated struct and use addRoot to prevent it from being GC'ed. It looks like the GTK widget methods that work with RGBA either copy the struct and manage the copy internally or fill in an existing struct. Obviously the copy method would need to change and not call the underlying rgba_copy method but instead copy the struct using the D method of doing so. Note I'm not saying that the RGBA memory management should be changed, just an interesting thought as I was looking at things. |
|
It's option B. Something is returning a RGBA pointer but isn't transferring the ownership, so it still frees the object itself. And since it doesn't know about GtkD the handle used by GtkD isn't set to null, and the GC then later on calls free om a dangling pointer. ObjectG can use addRoot because it's ref counted, and we remove the root when the ref count is one. RGBA isn't ref counted so that solution doesn't work there. I possibly need to "temporarily" remove the RGBA destructor until the ownership is sorted out. |
The next step is to also do this in the cases where ownership is transfered to us. See #146
|
Thanks Mike, there are very few spots where I'm dealing with RGBA, I'll check the GTK source code for those spots and see if something pops up. I'll test that commit you just made, I assume the downside of it is leaking gdkRGBA in cases where we get it from a widget and are supposed to free it but don't. In my case since I deal with this struct very infrequently, mostly when users edit the profile, the downside of a small leak is better then a crash IMHO. Also, if that commit becomes a longer term solution, would it make sense to tweak code like ColorChooserT.getRgba from the current gmalloc to code that would set the ownership of the RGBA? Alternatively, change the parameter from an out to a ref and force the developer to create a RGBA class and use it's gdkRGBA when calling the gtk function. |
|
Using the updated RGBA.d it appears with initial testing I no longer get the allocation issue, however as expected it leaks a bit of memory. I spent about fifteen minutes moving the transparency slider back and forth in my app which generates a lot of RGBA from ColorChooser and went from 18 MB to 30 MB in size. However, given it's unlikely users will do this I'll take the memory leak over the crash as a temporary workaround so thanks for the quick reply Mike. I'll do some more testing over the next few days to confirm the workaround, sometimes it can be stubborn to reproduce. |
|
The returned structs either directly or via out params now pass on the ownership information, this should reduce the memory leak. |
|
I've tested out this new version but I still seem to be leaking memory. Let me step back a bit and describe more what I'm seeing and the code I'm using to make sure what I'm seeing is really a leak and not normal D behavior. In my app Terminix, a terminal emulator, there is a profile editor similar to the one in Gnome Terminal where you can configure the profile. An option I have here is to control the transparency via a slider which is binded to a GSettings option. As the user waves the slider back and forth, GSettings gets updated and generates change messages which is sent to my code in the terminal view to update the colors. When I use the Gnome System Monitor and wave the slider back and forth I can see the process size growing from 14.8 MB up to 30 MB in a period of 5 to 10 minutes and it never seems to drop. My expectation is that since the D GC kicks in on new allocations, as we see with the original RGBA object, the process size would drop. This drop probably would not get you back to the original heap size but it should level off at some point. Would this be your expectation as well? Originally I was thinking that moving this slider generates a lot of RGBA because the original I code constantly created new RGBAs, however I forgot I optimized this code in the terminal after the first issue to use a fixed set of RGBA classes. The only exception is if you have an option turned on to use the system theme which does a getColor/getBackgroundColor against the style context but I don't use this in testing. The new code when it gets the slider message simply calls the parse method on each RGBA since it is reading the colors from GSettings. Looking at the source code for RGBA, parse simply re-uses the existing RGBA struct so that should not be causing the leak. Here's my code in snippets: So here is it where gets a bit fuzzy and my lack of background in C hurts. I was looking at the other methods in play here and started looking at gio.Settings.getStrv(). It's code in GtkD appears as follows: Looking at the code for Str.toStringArray it looks like it is making a copy of the array and strings which is then presumably managed by D from then on. However, I was wondering who is responsible for freeing the C array we just got from g_settings_get_strv. I asked about it on the GTK IRC and one of the people their mentioned that arrays are never allocated on the stack so the caller is responsible for freeing it which is why the Gtk documentation has it annotated as transfer: full (https://developer.gnome.org/gio/stable/GSettings.html#g-settings-get-strv). Based on that, I tried changing this code to a quick and dirty version as follows to explicitly free the memory: The code runs fine with no complaints, the memory still grows albeit it feels slower to grow but that could be wishful thinking on my part. Does this make any sense at all? |
Yes that makes sense, strings currently aren't freed. |
|
I'm just going by the GIO Settings API, but it seems like there none of the functions that return strings (gchar*) specify transfer at least in the documentation. Does that mean they are allocated on the stack and freed automatically? If you want to make another attempt, I'm happy to work with you on it at least with regards to testing and debugging anyway. |
|
In the GIO gir file the return types of g_settings_get_string and g_settings_get_strv are annotated with transfer-ownership="full". |
|
That sucks that the docs are not correct, must be tough as a C programmer having to manually check GIR files if you can't rely on the docs. I updated my test code for gio.Settings to also handle getString, seems to have slowed down the rate of memory growth quite a bit but not eliminated it completely. Like I said, I'm happy to help if you want to take a run at this however if you don't have time at this moment no worries. |
|
I've added the freeing of C strings for functions that return them. No problems sofar running the demos. |
|
I've re-built Terminix using this change and upon initial testing it looks like everything works fine. I assume the change is looking at the transfer attribute since I noticed that methods that have Transfer: None in the docs like gio.Settings.listRelocatableSchemas are not freeing the string which is awesome. Thanks for then effort on this Mike, really appreciate it. |
|
Closing this issue as I haven't had any complaints of this from users for over a week now. Any chance of getting a 3.23 release of GtkD with these changes? |
I think my application may be experiencing a similar memory issue that we thought we resolved in this thread http://forum.gtkd.org/groups/GtkD/thread/336/
I have a user that is complaining about infrequent segmentation faults in Terminix (gnunn1/tilix#101) that are difficult to reproduce. He's posted a few stack traces in that issue, however I've managed to reproduce it once myself with the
G_SLICE=debug-blocksin gdb. As shown below, the stack trace I get looks pretty similar to that previous issue, a failure in the destructor for the RGBA class.One question, looking at the code of the RGBA destructor, I see the following:
Would it help at all for this code to explicitly null the gdkRGBA pointer after the free call?
The text was updated successfully, but these errors were encountered: