Bugzilla – Full Text Bug Listing
| Summary: | Memory leak in Table when changing cell background colors | ||
|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Michael Ihde <mike.ihde> |
| Component: | SWT | Assignee: | Arun Thondapu <arunkumar.thondapu> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | arunkumar.thondapu, eclipse.felipe, gheorghe, remy.suen, Silenio_Quarti |
| Version: | 3.8 | ||
| Target Milestone: | 4.2 M4 | ||
| Hardware: | PC | ||
| OS: | Linux-GTK | ||
| Whiteboard: | |||
| Attachments: | |||
Created attachment 207416 [details]
Snippet of code that *does not* leak memory
Arun, please investigate this case. This is important. Thank you. (In reply to comment #2) > Arun, please investigate this case. This is important. Thank you. I have verified the behavior of continually increasing memory usage with the LeakyTable example as described above. Will try to debug and find out the root cause for the same. For what it's worth, I can create a leak if I modify RawGtkTable.java to call int /*long*/ ptr = new int /*long*/ [1]; OS.gtk_tree_model_get(model, iter, (c*2)+1, ptr, -1); This simulates similar code in the SWT baseline TableItem._getBackground(int) function. It appears that I can stop the memory leak by doing the following: 1. In TableItem.setBackground(int, Color) comment out the line that does the _getBackground() check. 2. In Table.cellDataProc(...) comment out the section that starts with "if (customDraw)" because it includes three calls to gtk_tree_model_get(...); After doing this, the cells will no longer properly color themselves but the memory does not leak (or if it does it's so slow to even notice). To get the cells to color themselves 3. In Table.createRenderers(...) change the calls OS.gtk_tree_view_column_add_attribute(columnHandle, textRenderer, OS.cell_background_gdk, BACKGROUND_COLUMN) and OS.gtk_tree_view_column_add_attribute(columnHandle, pixbufRenderer, OS.cell_background_gdk, BACKGROUND_COLUMN) to use "modelIndex + CELL_BACKGROUND" instead of "BACKGROUND_COLUMN" There are probably other side-effects to these changes so they aren't a patch per-se, but this information might help nail down the root cause. I don't know what specifically is wrong with gtk_tree_model_get(...). This call is used in TableItem.setText() and does not appear to leak. I tried calling OS.g_free(...) on the returned color ptr and, as expected, this caused the program to crash. Arun, we need to check all the references to gtk_tree_model_get() and determine wheter we need to g_free() or g_object_unref() the returned value based on the type of the property store in that column in the model. The doc says: "Returned values with type G_TYPE_OBJECT have to be unreferenced, values with type G_TYPE_STRING or G_TYPE_BOXED have to be freed. Other values are passed by value. " I have always assumed that a GdkColor was passed by value, so the returned value did not have to be freed. I quickly looked the code in I believe we are leaking fonts and pixbufs as well. Created attachment 207699 [details]
Patch to fix memory leak in setBackground
This patch appears to fix the memory leak relating to setBackground(). There are other places (i.e. setForeground()) where this would need to be applied.
I've attached a patch that seems to stop the memory leak in my tests. Basically after getting a color it frees it using gdk_color_free(ptr). I'm not an expert at GTK/GLIB internals...but it looks like the reason you need to free the color is that it is registered as a "boxed" type which has a copy function of gdk_color_copy. Through some degree of indirection (GValue, G_VALUE_LCOPY, ???) this might be getting called. The supplied patch only fixes the issue for the test case in LeakyTable.java. There are other places that will need similar treatment (i.e. setForeground). Thanks Michael for the patch and for tracking this down. I believe the patch is correct. Arun, let's get the complete patch done for M4 (which next week). (In reply to comment #8) > Arun, let's get the complete patch done for M4 (which next week). I'm extending the patch provided by Michael to handle all the references of gtk_tree_model_get() that need to be addressed. Will upload the latest patch ASAP (need to complete few more changes and do the testing). Created attachment 207917 [details]
Extended patch to fix all memory leaks caused by usage of gtk_tree_model_get()
I'm attaching the complete patch to handle all memory leaks caused via references to gtk_tree_model_get(). I have tested most of the changes but I'm still in the process of doing more exhaustive testing.
Silenio, please review and let me know if I've missed something.
Sorry for the delay in submitting the patch, I had to revisit the changes a couple of times to understand a few things and to add null checks before invoking gdk_color_free() and other methods since they do not handle passing null references as opposed to g_free() which does.
Bogdan, thanks for committing the patch, the call to OS.gdk_color_free() seems to be missing in the method _getBackground() in TreeItem.java. Fixed in master > 20111205 |
Created attachment 207415 [details] Snippet of code that leaks memory The SWT Table class leaks memory if the cell colors are changed (i.e. TableItem.setBackground). The leak is not in the Java Heap space, which remains stable. Using "ps u", "top", or "pmap" you can see the total memory usage grow continually...eventually it will cause some form of crash, such as a gslice Reproducible: Always. Steps to Reproduce: 1. Run the attached LeakyTable example for an extended period and watch the memory usage. Expected Results: 1. The memory usage of the application remains stable (once the initial start-up transients have finished). PyGTK examples and the attached RawGtkTable example do not leak, so it is unlikely that the leak is due to a bug in GTK itself. VSZ RSS TIME LeakyTable 1498004 327420 33:59 RawGtkTable 1213512 43700 25:57 Other Platforms: The leak appears on SWT 3.7 and 3.8 using GTK 2.20.1 (Ubuntu 10.04) and 2.10.4 (RHEL5)