GNOME Bugzilla – Bug 320153
IconView#setCursor doesn't comply with the documentation - and crashes VM
Last modified: 2007-01-05 00:59:25 UTC
The documentation of the method says that the second parameter, the renderer, can be null. The implementation of the method doesn't handle a null renderer and will throw a NullPointerException.
Created attachment 54034 [details] [review] A patch that will fix the null pointer exception. ** WARNING ** The patch file was edited by hand because I don't have an updated version of IconView.java, so I had to remove manually the diffs of a previous bug report.
Thanks for the patch. It has been applied to CVS HEAD.
I have also discovered that there is no native method gtk_icon_view_set_cursor.
The following call iconView.setCursor(path, new CellRendererText(), true) causes # # An unexpected error has been detected by HotSpot Virtual Machine: # # SIGSEGV (0xb) at pc=0xb1c64b36, pid=13839, tid=3084916416 # # Java VM: Java HotSpot(TM) Client VM (1.5.0_05-b05 mixed mode, sharing) # Problematic frame: # C [libgtk-x11-2.0.so.0+0xf7b36] # # An error report file with more information is saved as hs_err_pid13839.log # # If you would like to submit a bug report, please visit: # http://java.sun.com/webapps/bugreport/crash.jsp # A testcase will follow latter.
Created attachment 54045 [details] Testcase that will show the JVM crash This testcase can be used to trouble-shoot the problem with the call to IconView#setCursor(TreePath, CellRenderer, boolean)
This is strange. The problem only occurs when the boolean startEditing is set to true. The problem therefore doesn't seem to be in the way that specific function is wrapped (since we are simply passing the boolean as we receive it).
It seems that there is no problem with the Java bindings nor with the JNI code. The WebCVS log of /gtk+/gtk/gtkiconview.c shows that lately there was some activity with the function gtk_icon_view_set_cursor(), probably they fixed the problem. I wrote a testcase in C that mimics the previous Java test case in order to see how the gtk_icon_view_set_cursor() function behaved and I got a Segmentation Fault. ** NOTE ** I am a little bit rusted with C, I haven't done any C code since 1999! Perhaps, my sample testcase in C is wrong and it can't be used to confirm that the problem lies withing the GTK library. I will attach my C testcase can you have a look at it and confirm that the bug is within the GTK function? Maybe we should ask the GTK developers for some input.
Created attachment 54061 [details] Testcase in 'C' This is a rewrite of the test case in 'C'.
The C documentation for the function gtk_icon_view_set_cursor() says that: "Please note that editing can only happen when the widget is realized." I'm not sure but I don't think that in my previous Java testcase that the widgets where yet realized. So I updated the testcase to make sure that the widgets where realized. An attachment will follow.
Created attachment 54062 [details] The widgets are now realized. The call to set_cursor is now done when the widgets are realized.
Yes, I had noticed that note about the widget having to be realized, but changing that part had no difference in the outcome. I will have a look at the c test case soon, thanks.
The initial patch was commited to CVS HEAD some time ago. Marking it as such.
Emmanuel, Ok to close? AfC
I din't had time to work with the Java-Gnome bindings in a long time. This weekend I will try to find some time and see if the bug is fixed. The problem is that I don't have the Java-Gnome bindings installed yet, and this could take some time (compiling and resolving all dependencies ...). Perhaps, some one can run the test case to see it the bug still applies.
Ok I tried the both files that are attached (I gather the second one is the better test case) and they both cause the JVM to crash. That's with current CVS head. I confirmed that the patch was indeed committed, so something else is [still] wrong. AfC
As per comment #8, it seemed like in addition to the problem fixed by the patch, there was a gtk+ issue. Perhaps a bug should be filed with them if it's not fixed in gtk+ 2.8.15.
Ah, thanks for that. I hadn't quite understood that part of the comment stream. What's an appropriate action for us to take now with this bug? Close it? Or mark it depending on a bug against GTK when such is created? When 2.8.15 becomes available for my distro I will run the test case again and let you know... AfC
Okay, I just built gtk+ from cvs head using jhbuild and the virtual machine crashed into a fiery death.
I tried the Java test case and the C test case and both crashed. I am using gtk2 2.8.16. I will create a bug for GTK.
I created a bug for GTK http://bugzilla.gnome.org/show_bug.cgi?id=335001 and I marked this bug as depending on bug 335001
So Matthias closed it. Fine. That means we have to do some kind of state check on our side to prevent this code path from being exercised. Any suggestions as to exactly what would be an appropriate guard? AfC
I will try take a look at it this afternoon, after work. I think that there are still some issues with the C documentation of gtk_icon_view_set_cursor. Their documentation clearly states that cell can be NULL, but this is also known to create a crash. We will probably need to check that the cell render given in argument is already set and not null. I was trying to see how I could fix the test case, in order to work properly but I didn't find a way for adding/changing the cell render of an IconView. Can someone help me?
From gtk-demo's source code, they have: renderer = gtk_cell_renderer_pixbuf_new (); gtk_cell_layout_pack_start (GTK_CELL_LAYOUT (icon_view), renderer, TRUE); It seems to me like we've got to implement the CellLayout interface, it might be a straightforward cut and paste of what's being used in ComboBox right now, I admit I haven't looked too closely just yet though.
Created attachment 61641 [details] [review] Implementation of CellLayout This is the implementation suggested by Remy Suen in the comment #24. With this patch it is possible to add a CellRenderer to an IconView.
I agree implementing CellLayout is a good idea. Did this alone fix the bug for you? I just tried your patch and the test case still crashes. AfC
After updating from gtk+ CVS, there are no segfaults in the original test case. However, two assertion errors appears when trying to set the cursor on the CellRenderers from the true/false (nothing happens when using null in either case). java.lang.Exception: gtk_icon_view_set_cursor: assertion `cell == NULL || info != NULL' failed It may be worthwhile to try to check that the CellRenderer being used is actually a part of the IconView and try to throw a more meaningful exception. However, I'm not sure how that's going to work right now since I don't see a way of performing this CellRenderer check.
(In reply to comment #26) > I agree implementing CellLayout is a good idea. > > Did this alone fix the bug for you? I just tried your patch and the test case > still crashes. > > AfC > My test case is broken because I can't call iconView.setCursor until the widget is realized. I modified the test case, I am now using a button for invoking the method iconView.setCursor() and it kind of works, there are no crashes but the CellRenderer has no width (but this is another issue). Implementing the CellLayout interface is needed otherwise I couldn't add the CellRenderers.
Created attachment 61672 [details] Bug is fixed This test case shows that the bug is fixed, the first test case is broken because among other things the call to setCursor has to be done after the widget is realized.
Emmanuel, you were testing your test case in comment #29 with an updated gtk+ with Matthias's fix, yes?
Actually no, my version of GTK+ is plain vanilla 2.8.16 as provided by Ubuntu Dapper.
Right, never mind, there was something wrong with my configuration. So yes, it seems that this bug is essentially fixed just by implementing CellLayout. Of course, the segfaults will still occur if the user is using 2.8.16 or below whilst passing CellRenderers that aren't part of the IconView, but there isn't really much we can do about that since I still don't see a way of checking whether the CellRenderer is one of the IconView's. Per discussion with Ismael on IRC, we'll be updating the method to throw an IAE if the renderer is null and startEditing is true.
> Per discussion with Ismael on IRC, we'll be updating the method to throw an IAE > if the renderer is null and startEditing is true. > Great, but don't forget to update the Javadoc!
... and are you {going to, planning to, or have you already} applied the "Implementation of CellLayout" patch? AfC
(In reply to comment #33) > Great, but don't forget to update the Javadoc! > Yes, I will be updating it accordingly. (In reply to comment #34) > ... and are you {going to, planning to, or have you already} applied the > "Implementation of CellLayout" patch? I haven't had time to actually test all the methods (I've only tried packStart), but if someone else has done otherwise and feels pretty confident that it's all fine and dandy (it is a copy-paste job after all, so it's hard to say how stable the code is), that person could certainly commit it.
IAE update committed to CVS head. I didn't really add much javadoc besides the @throws and saying that the CellRenderer needs to be of the IconView's (although I'm aware that the CellLayout implementation has not been committed). I was rather hesitant in adding the whole segfaulting situation and gtk+ versions and so forth, so I didn't.
After chatting with Remy, it seems like we have the pieces to solve this; let's see if we can't get it fixed before the next release. Targetted for 2.14.2 AfC
Hey Emmanuel, have you been using the methods that's included in the CellLayout implementation that I mentioned in comment #24 that you wrote a simple copy-paste patch for in comment #25? I've done a bit of testing, but not very extensive ones. If you've tested it a bit and didn't hit on any problems, I'm thinking about committing it. I am not too familiar with the CellRenderers myself, so the test cases I used back from in March and April probably weren't the best, perhaps you've applied them in some applications you've written?
Hi Remy, the copy-paste patch worked for me, although I didn't use all methods that are available in the CellLayout interface. I will appreciate if the patch gets committed into the main trunk.
Remy, Can you commit this? AfC
(In reply to comment #40) > Remy, > Can you commit this? > AfC Unlikely as I have limited and unstable Internet connectivity where I live (I'm typing this from work right now, actually). Emmanuel seems pretty confident about the stability and I've ran some tests before, so I don't think there should be any problems. Anyone with commit rights is welcome to do it in my place. The code's obviously not in front of me, but if the patch doesn't apply cleanly, one can just co a copy-paste from another one of CellLayout's implementing classes.
Ok Remy, thanks for the update. That does make it a bit clearer. Any takers? AfC
Committed. Emmanuel, be sure to try java-gnome 2.16.0 and let us know if it works ok for you. AfC
I tried the code and it works.