Bug 1170

Summary: Removal Of GTK_WIDGET_WINDOW In SWT 4.5 Causes A JOGL Runtime Error
Product: [JogAmp] Jogl Reporter: Rob Hatcherson <rob.hatcherson>
Component: swtAssignee: Petros Koutsolampros <pjgl>
Severity: critical CC: gouessej, rob.hatcherson, xerxes
Priority: ---    
Version: 2.3.2   
Hardware: pc_x86_64   
OS: linux   
Type: --- SCM Refs:
Workaround: ---

Description Rob Hatcherson 2015-07-02 21:35:27 CEST
I ran into a runtime error attempting to run with JOGL 2.2.4 in an Eclipse/RCP app built against the recent Eclipse Mars release along with its associated SWT 4.5 upgrade:

>>>> ExternalContributionFactory.create: failed, throwable = java.lang.ExceptionInInitializerError
	at com.jogamp.newt.swt.NewtCanvasSWT.<init>(NewtCanvasSWT.java:127)
	at com.jogamp.newt.swt.NewtCanvasSWT$1.run(NewtCanvasSWT.java:106)
Caused by: javax.media.nativewindow.NativeWindowException: java.lang.NoSuchMethodException: org.eclipse.swt.internal.gtk.OS.GTK_WIDGET_WINDOW(long)
	at com.jogamp.nativewindow.swt.SWTAccessor.<clinit>(SWTAccessor.java:203)
	... 87 more


In SWT 4.5 the GTK_WIDGET_WINDOW method has been removed from OS.java, and I think this has exposed a bug in SWTAccessor.java.

The SWTAccessor class in JOGL includes code to handle using either the GTK_WIDGET_WINDOW method or the gtk_widget_get_window method, preferring one or the other based on the GTK version:

                if (_gtk_version.compareTo(GTK_VERSION_2_14_0) >= 0) {
                    m4 = c.getDeclaredMethod(str_gtk_widget_get_window, handleType);
                } else {
                    m3 = c.getDeclaredMethod(str_GTK_WIDGET_WINDOW, handleType);

On my particular system the GTK version is (I think) 2.20.1.  Or at least this is what's reported by:

pkg-config --modversion gtk+-2.0

It looks like the GTK version check in SWTAccessor.java should do the right thing in this case and prefer gtk_widget_get_window, but it doesn't and instead a runtime error is raised citing the absence of method GTK_WIDGET_WINDOW.

I *think* this probably is because of three too-short bitmasks in the GTK_VERSION method (?).  This method currently looks like this both in 2.2.4 and 2.3.1 (and I assume everywhere in between, but I haven't looked):

    private static VersionNumber GTK_VERSION(final int version) {
        // return (major << 16) + (minor << 8) + micro;
        final int micro = ( version       ) & 0x0f;
        final int minor = ( version >>  8 ) & 0x0f;
        final int major = ( version >> 16 ) & 0x0f;
        return new VersionNumber(major, minor, micro);

If I expand the masks to 8 bits intead of 4, then everything appears to work:

    private static VersionNumber GTK_VERSION(final int version) {
        // return (major << 16) + (minor << 8) + micro;
        final int micro = ( version       ) & 0xff;
        final int minor = ( version >>  8 ) & 0xff;
        final int major = ( version >> 16 ) & 0xff;
        return new VersionNumber(major, minor, micro);

This probably has been working up to this point because the SWT guys had left GTK_WIDGET_WINDOW in there for a while alongside the other method.  I'm not sure when it was removed.  I can say that OS.java is grossly different in 4.5 compared to the version of SWT we had been using previously.

Sven, you probably are inclined to bust me for not submitting this through a git pull request.  I am embarrassed to admit that my clone on github is - through my own git incompetence followed by a period of neglect - currently in an extremely messed up state that I haven't a clue how to recover from (yet).  So... for the time being I've resorted to an old-fashioned textual description.  Hopefully the explanation is clear enough and I'm not too far off on the diagnosis.
Comment 1 Julien Gouesse 2015-07-03 10:40:47 CEST
Does your modification still work with previous versions of SWT?
Comment 2 Rob Hatcherson 2015-07-03 19:49:32 CEST
Great question Julien :-).  I'm out of the office until next Monday, but will try it then.
Comment 3 Julien Gouesse 2015-07-04 00:36:57 CEST
(In reply to comment #2)
> Great question Julien :-).  I'm out of the office until next Monday, but
> will try it then.

Thank you. I can help you to fix your GIT problems, I had messed up several times with my repository and I wasn't a GIT expert ;) The time shift doesn't help but you can bump me on IRC, here or by email if you want. I would be glad if you could make your own pull request. In the worst case, I'll make it for you (+ full credits to you of course).
Comment 4 Rob Hatcherson 2015-07-07 18:04:06 CEST
The suggested change worked OK with the previous version of SWT that we were using here.  The version file inside the older SWT plugin jar file that was distributed with Eclipse said "4.333".  The newer one says "4.527".

SWTAccessor is also dependent on the GTK version on the system, but I doubt any of us are surrounded by sufficient machines to allow testing of dependencies on the explicitly mentioned GTK versions (2.14.0, 2.24.0, and 3.0.0).

The code as it existed before would only exhibit a problem when any component in the version of GTK reported by the system exceeded 15.  In my case the version of GTK I have on CentOS 6.5 is 2.20.1, so that "20" in there exposes the bug.  The bitmask flaw in the GTK_VERSION method drops the high-order four bits so it thought my version number was 2.4.1 instead of 2.20.1.  This subsequently drove a test down the wrong side of an "if".

In summary: if the intention of the "int" form of the version argument to the private static GTK_VERSION method is that each version number element is a full byte - and I think this is the intent - then we need to change those bitmasks from 0x0f to 0xff.

Now... I can't say whether or not this will expose any other downstream problem that wasn't getting hit because of this bug, but IMO we should make this change anyway let the variety of machines in the user community uncover anything else waiting to be found.

What do you think?

Regarding git... sigh.  Thanks for the offer to help.  I'll shoot you an email to take the conversation offline from Bugzilla.

Comment 5 Julien Gouesse 2015-07-17 10:09:19 CEST
Rob, I'm still waiting for your pull request, is there anything wrong?
Comment 6 Rob Hatcherson 2015-07-17 15:56:49 CEST
Hmmmmm... I issued a pull request last Friday 07/10.  github shows that it's still open.

Looks like I've deleted the email you sent me, so maybe I messed up an instruction.

The pull request was opened against sgothel/jogl.  If that isn't the right place let me know and I'll try again.
Comment 7 Julien Gouesse 2015-07-17 18:53:31 CEST
Ok: https://github.com/sgothel/jogl/pull/88

Bump Sven on IRC then.
Comment 8 Rob Hatcherson 2015-07-17 22:31:45 CEST
Sven has merged the fix.
Comment 9 Xerxes RĂ„nby 2015-07-17 22:44:51 CEST
commit 179222835fae0cc93b20aef2f877f47c9626f15a

Expand bitmasks in SWTAccessor GTK_VERSION method

SWTAccessor's GTK_VERSION method accepts a single int argument.  The
argument is interpreted as a bit-packed version number with the apparent
intent that the three least significant bytes of the int version number
are the major, minor, and micro version number components.

The code that extracts these three components from the int argument was
using four-bit mask 0x0f instead of eight-bit mask 0xff, and therefore
was discarding the four most significant bits of each component.  This
caused any component greater than 15 to lose information.  For example,
a component whose value should have been 20 would end up as 4.

The version number is used in comparisons in a static initializer to
determine how to retrieve references to Method objects via reflection.
One such comparison decides whether to retrieve a reference to method
GTK_WIDGET_WINDOW or method gtk_widget_get_window.

The problem initially presented itself after an attempt to use JOGL
with SWT 4.527 and GTK 2.20.1 because this version of SWT removed the
GTK_WIDGET_WINDOW method.  Due to the bug SWTAccessor believed the GTK
version was 2.4.1 instead of 2.20.1, so the code attempted to find
GTK_WIDGET_WINDOW instead of gtk_widget_get_window.  Because this
method was no longer there a runtime exception was raised.