Merge lp:~vanvugt/nux/fix-769957 into lp:nux

[Daniel van Vugt]

Fix an X11 Display leak, causing window leaks and graphics corruption (LP: #769957)

Fixed an X11 Display leak, which caused Window leaks of the XInputWindows for the Unity panel and launcher. This created graphics corruption each time the Unity plugin restarted (other plugins toggled).

Possibly also fixed other window/resource leaks in nux/unity which were side effects of the same display leak.

Technical notes:

1. The Window objects being leaked are the _window members of XInputWindow used by BaseWindow::m_input_window. While the destructor of XInputWindow does correctly call XDestroyWindow for these, they are still leaked because the DestroyWindow request never reaches the X server. That's because XDestroyWindow only queues the destroy request and does not guarantee it has or will be completed. The display parameter to XDestroyWindow is the display being leaked, and as such never flushes or completes any of its pending requests such as the destroy request.

2. Given technical note #1, it is likely that a simpler workaround for this window leak problem would be to add an XFlush call to the end of XInputWindow::~XInputWindow(). That would be a simpler, more benign and compatible fix for bug 769957, however that would not fix the display leak which is the root cause of the problem. This merge proposal fixes all related leaks by removing the Display leak, which then causes the leaking windows to be destroyed also.

[Jay Taoko (jaytaoko)]

Thank you for your patch! Before we can accept it, we require you to agree to inalogic's contributor agreement (http://inalogic.com/license). If you accept it, send and email to <email address hidden> and state that you agree.

[Jay Taoko (jaytaoko)]

I am ready to approve the branch. However, is this branch suppose to be an SRU for Unity in Natty? It is based on nux trunk (version 1.0) which is the next version of Nux for Oneiric. Let me know if you submitted this branch as and SRU for Natty and I will port it back to Nux 0.9 series.

[Daniel van Vugt (vanvugt)]

Ideally we should backport the fix as much as possible, yes. However there is a problem doing that too quickly. The change to WindowThread.h changes the nux::WindowThread object layout and breaks existing unity binaries. If you can ensure that unity is rebuilt and updated at the same time as the change to nux, then backport away...

Alternatively, my theory above about adding an innocent XFlush to XInputWindow::~XInputWindow() *might* sufficiently solve the bug in question even though it doesn't fix all the leaks. It will *definitely* keep the nux ABI compatible with existing unity releases for natty at least.

[Daniel van Vugt (vanvugt)]

Maybe I should test that alternate fix, and we can then recommend only the alternate fix for the 0.9 series if that works?

[Jay Taoko (jaytaoko)]

Indeed, your branch would break the API of Nux 0.9. However, I will still ask if it can be back ported. Let me know how it goes with the alternate fix.

[Daniel van Vugt (vanvugt)]

Confirmed. The XFlush hack alone is enough to fix the corruption bug in natty. Should be compatible with any series:
lp:~vanvugt/nux/fix-769957-natty

[Jay Taoko (jaytaoko)]

Approved. I am going to merge it into Nux trunk.

review: Approve

=== modified file 'Nux/Nux.cpp'
--- Nux/Nux.cpp	2011-04-06 21:54:09 +0000
+++ Nux/Nux.cpp	2011-05-27 09:43:27 +0000
@@ -151,7 +151,7 @@
     w->m_ExitData = 0;
     w->SetWindowStyle (WINDOWSTYLE_NORMAL);
     w->m_embedded_window = true;
-    w->ThreadCtor (XOpenDisplay (NULL), X11Window, OpenGLContext);
+    w->ThreadCtor (NULL, X11Window, OpenGLContext);
     return w;
   }
 #endif

=== modified file 'Nux/WindowThread.cpp'
--- Nux/WindowThread.cpp	2011-04-06 21:54:09 +0000
+++ Nux/WindowThread.cpp	2011-05-27 09:43:27 +0000
@@ -425,6 +425,10 @@
     m_GLibLoop      = 0;
     m_GLibContext   = 0;
 #endif
+#if defined(NUX_OS_LINUX)
+    _x11display = NULL;
+    _ownx11display = false;
+#endif
     
     _pending_wake_up_timer = false;
     _inside_main_loop = false;
@@ -455,6 +459,13 @@
     delete _Timelines;
     delete _async_wake_up_functor;
     delete _fake_event_call_back;
+
+#if defined(NUX_OS_LINUX)
+    if (_x11display && _ownx11display)
+    {
+      XCloseDisplay(_x11display);
+    }
+#endif
   }
 
   void WindowThread::SetFocusedArea (Area *focused_area)
@@ -1541,7 +1552,18 @@
       ParentWindow = 0;
     }
 
-    _graphics_display = gGLWindowManager.CreateFromForeignWindow (X11Display, X11Window, OpenGLContext);
+    if (X11Display)
+    {
+      _x11display = X11Display;
+      _ownx11display = false;
+    }
+    else
+    {
+      _x11display = XOpenDisplay(NULL);
+      _ownx11display = true;
+    }
+      
+    _graphics_display = gGLWindowManager.CreateFromForeignWindow (_x11display, X11Window, OpenGLContext);
 
     if (_graphics_display == 0)
     {

=== modified file 'Nux/WindowThread.h'
--- Nux/WindowThread.h	2011-04-06 21:54:09 +0000
+++ Nux/WindowThread.h	2011-05-27 09:43:27 +0000
@@ -265,6 +265,8 @@
 
     */
     virtual bool ThreadCtor (Display *X11Display, Window X11Window, GLXContext OpenGLContext);
+    Display *_x11display;
+    bool     _ownx11display;
 #endif
 
     /*!