Bug 565

Summary: XWindow memory leak for GLCanvas (includes analysis)
Product: [JogAmp] Jogl Reporter: Daniel Balog <danielbalog86>
Component: x11Assignee: Sven Gothel <sgothel>
Status: RESOLVED INVALID    
Severity: normal CC: danielbalog86, gouessej
Priority: ---    
Version: 2   
Hardware: pc_all   
OS: linux   
Type: --- SCM Refs:
fcd0aa56a368adefbb516fcd710bec38560a2054
Workaround: ---
Attachments: This is a potential fix for the GLCanvas Linux memory leak. Needs to be reviewed.
Test that triggers the XWindow memory leak
Test cases to trigger the memory leak
Modified potential fix

Description Daniel Balog 2012-03-14 12:25:17 CET
Created attachment 337 [details]
This is a potential fix for the GLCanvas Linux memory leak. Needs to be reviewed.

*Setup:

JOGL2.0 RC5, Ubuntu 11.10 (64-bit), GeForce 285GTX

*Problem:

XLib11 requires that each XWindow opened with XOpenWindow also be closed with XCloseWindow (See X11Util). This is not the case for GLCanvas, which seems to call XOpenWindow when it is attached to a frame, but it doesn't properly close the XWindow when you remove it from a frame.

In a 3D editing application, where windows are opened and closed a lot, this could be problematic. Especially since Ubuntu seems to have a limit of maximum 180 XWindows open before it refuses to open a new one.

*Steps to reproduce:

1) Create a GLCanvas
2) Add GLCanvas to frame
3) Remove GLCanvas from frame
4) Dispose the GLCanvas

(If you repeat this 200 times, you will get an exception telling you you can't open a new XOpenWindow)

See my attachment for a test case to reproduce. You can call X11Util.dumpOpenDisplayConnections() to see how many
XWindows are actually open. This number will grow over time, and it shouldn't.

*Expected result:

XWindow is closed

*Actual result: 

XWindow remains open -> This indicates a memory leak.

*Analysis:

The problem is that when you remove the GLCanvas from the frame, the removeNotify is called, which disposes the GLCanvas. This in itself is fine, but it doesn't dispose the native surface, it only disposes the awt surface.

If you look at GLCanvas.DisposeAbstractGraphicsDeviceAction, you will see the following line:

(GLCanvas:884) boolean closed = awtConfig.getScreen().getDevice().close();

This line calls awtConfig.getScreen(), which refers to an AWTGraphicsScreen that is created at construction time. Calling close on the device of this screen will do nothing, because all implementations of AbstractGraphicsDevice close methods do nothing, apart from the X11GraphicsDevice, which closes the XWindow.

So the problem is either that the wrong screen is initialized at construction

*Possible Fix (1):

Calling 

awtConfig.getNativeGraphicsConfiguration().getScreen().getDevice().close();

right after line 884 in DisposeAbstractGraphicsDeviceAction fixes the issue, but I'm not sure if this is correct. This will call the native X11 screen to close the device when destroyed, which plugs the memory leak.

*Possible Fix (2):

Initialize the screen in the GLCanvas.awtConfig field to be the X11GraphicsScreen instead of AWTGraphicScreen. This is done in the getGraphicsConfiguration() method of GLCanvas.

Again, I'm not sure what the impact of this fix would be.

*Workaround:

The workaround I'm using currently in my application is to override the destroy of my own GLCanvas, and call close the native device myself before the canvas is destroyed. The problem is that after the GLCanvas is destroyed, the reference to getNativeGraphicsConfiguration() will be null, so I have to do it before this happens.
Comment 1 Daniel Balog 2012-03-14 12:30:05 CET
Created attachment 338 [details]
Test that triggers the XWindow memory leak

This test, when run in linux, will fail after about the 180th GLCanvas is created. There is a limit of a maximum of 180 XWindows per application in linux. the thing is that all GLCanvasses are disposed of, but incorrectly. The call for XCloseWindow is never performed.
Comment 2 Daniel Balog 2012-03-14 12:30:59 CET
Comment on attachment 337 [details]
This is a potential fix for the GLCanvas Linux memory leak. Needs to be reviewed.

Apologies for the import changes in the patch. Only the last few lines are relevant.
Comment 3 Daniel Balog 2012-03-15 15:51:20 CET
Comment on attachment 337 [details]
This is a potential fix for the GLCanvas Linux memory leak. Needs to be reviewed.

I just noticed this does not fix the issue for PBuffers... (which also seem to create an XWindow)
Comment 4 Daniel Balog 2012-03-15 16:41:53 CET
Created attachment 339 [details]
Test cases to trigger the memory leak

I added a new (easier) test case for pbuffers that fails on linux machines, due to the XWindow not being closed properly. I think this use case is easier to analyze and fix.
Comment 5 Julien Gouesse 2012-03-15 22:56:08 CET
Hi

It's a nice patch but please put only the relevant source code into it and call your trick only when the variable "closed" is set to true.
Comment 6 Daniel Balog 2012-03-16 10:19:51 CET
Are you referring to the closeDisplay field in X11GraphicsDevice? That is checked in the close() method itself, and doesn't need to be checked externally.
Comment 7 Julien Gouesse 2012-03-16 10:30:09 CET
I thought about this:
boolean closed = awtConfig.getScreen().getDevice().close();

but if you really know what you're doing, that's ok for me.
Comment 8 Daniel Balog 2012-03-16 10:40:28 CET
Ah I see. The thing is that close() doesn't doe anything for AWT besides check if it has already been closed(). I guess I would need a new local variable and throw a debug statement if the native closing did not work.
Comment 9 Daniel Balog 2012-03-16 16:57:56 CET
Created attachment 342 [details]
Modified potential fix

I updated the patch. 

Still working on fixing the offscreen view though.
Comment 10 Sven Gothel 2012-03-18 16:06:04 CET
Thank you Daniel, I will look at this one after fixing bug 564
Comment 11 Sven Gothel 2012-03-19 10:48:20 CET
Already fixed:

2011-12-22 Commit d225d0a8a16e362ddb14cb93c124eb06cf0ff05e,
fixed a OO 'typo' (using the wrong device), which hindered actual closing of the device.
  <http://jogamp.org/git/?p=jogl.git;a=commit;h=d225d0a8a16e362ddb14cb93c124eb06cf0ff05e>

GLCanvas.dispose():
 1) PostDisposeAction destroys the JAWTWindow
 2) DisposeAbstractGraphicsDeviceAction closes the AbstractGraphicsDevice 
     which is additionally being owned.

GLPBufferImpl.destroy():
  - closes the AbstractGraphicsDevice 

X11Util.Display: Shutdown (close open / pending Displays: false, open (no close attempt): 0/0, pending (not closed, marked uncloseable): 2)
X11Util: Pending X11 Display Connections: 2
X11Util: Pending[0]: NamedX11Display[:0.0, 0x41a63430, refCount 0, unCloseable true]
X11Util: Pending[1]: NamedX11Display[:0.0, 0x414b5c60, refCount 0, unCloseable true]

<http://jogamp.org/git/?p=jogl.git;a=commit;h=fcd0aa56a368adefbb516fcd710bec38560a2054>
Comment 12 Daniel Balog 2012-03-19 12:57:15 CET
Sounds great. Thanks a bunch!