Merge lp:~brandontschaefer/unity/mt-grab-mem-leak-reference-counting-crash-fix into lp:unity

[Brandon Schaefer]

Fix reference counting problem which leads to memory leaks and a crash in MT grab handles.

=== Problem ===
Reference counting problem causes Window/memory leaks which causes a map to not free XIDs causing a crash (hard to reproduce)

The problem is there is a vector mGrabHandles which stores a shard_ptr then the shared_ptr is pushed inside of a map mInputHandle. So when a window closes there is more then 1 things references each grab handle window so it never gets cleaned up.

=== Fix ===
Change the map to store weak ptrs, now when a window is closed everything gets cleaned up.

=== Test ===
Working on coming up with something, a manual test wouldn't work well here since it is hard to reproduce the crash.

[Łukasz Zemczak (sil2100)]

Ok, tested, working, not causing any regressions - but is there a known manual test to reproduce this issue?

review: Approve

[Marco Trevisan (Treviño) (3v1n0)]

Looks good for me, not sure it is easily possible to add an unit test here, though.

review: Approve

[Brandon Schaefer (brandontschaefer)]

@Lukaz

The only way I could get the crash was to do the following:

repeat 20-30 times
  open terminal
  toggle grab handles
  close window

Then open 2 terminal windows and click back and fourth between them (clicking on the boarders as well because those are different windows.)

The crash comes when on of the windows your click is in map (from one of the closed windows). So it is slightly hard to reproduce....

@Marco
I was trying to look at a unit test but the only way I can think to test this would be to:
Open a window/ toggle grab handles on/ close window.
Then assert the map size that holds all the grab handle windows gets smaller (as before it was not)

Ill see what I can do.

=== modified file 'plugins/unity-mt-grab-handles/src/unity-mt-grab-handle-group.cpp'
--- plugins/unity-mt-grab-handles/src/unity-mt-grab-handle-group.cpp	2011-12-22 09:56:31 +0000
+++ plugins/unity-mt-grab-handles/src/unity-mt-grab-handle-group.cpp	2012-10-13 20:28:20 +0000
@@ -40,7 +40,7 @@
 }
 
 void
-unity::MT::GrabHandleGroup::raiseHandle(const boost::shared_ptr <const unity::MT::GrabHandle> &h)
+unity::MT::GrabHandleGroup::raiseHandle(const std::shared_ptr <const unity::MT::GrabHandle> &h)
 {
   mOwner->raiseGrabHandle (h);
 }

=== modified file 'plugins/unity-mt-grab-handles/src/unity-mt-grab-handle-group.h'
--- plugins/unity-mt-grab-handles/src/unity-mt-grab-handle-group.h	2011-12-21 08:18:47 +0000
+++ plugins/unity-mt-grab-handles/src/unity-mt-grab-handle-group.h	2012-10-13 20:28:20 +0000
@@ -22,9 +22,7 @@
 #include <Nux/Nux.h>
 #include <glib.h>
 #include <boost/noncopyable.hpp>
-#include <boost/shared_ptr.hpp>
-#include <boost/enable_shared_from_this.hpp>
-#include <boost/weak_ptr.hpp>
+#include <memory>
 
 #include "unity-mt-grab-handle.h"
 #include "unity-mt-texture.h"
@@ -37,12 +35,12 @@
 extern unsigned int FADE_MSEC;
 
 class GrabHandleGroup :
-  public boost::enable_shared_from_this <GrabHandleGroup>,
+  public std::enable_shared_from_this <GrabHandleGroup>,
   boost::noncopyable
 {
 public:
 
-  typedef boost::shared_ptr <GrabHandleGroup> Ptr;
+  typedef std::shared_ptr <GrabHandleGroup> Ptr;
 
   static GrabHandleGroup::Ptr create (GrabHandleWindow *owner,
                                       std::vector<TextureSize> &textures);
@@ -59,7 +57,7 @@
 
   void hide();
   void show(unsigned int handles = ~0);
-  void raiseHandle (const boost::shared_ptr <const unity::MT::GrabHandle> &);
+  void raiseHandle (const std::shared_ptr <const unity::MT::GrabHandle> &);
 
   std::vector <TextureLayout> layout(unsigned int handles);
 

=== modified file 'plugins/unity-mt-grab-handles/src/unity-mt-grab-handle-impl-factory.cpp'
--- plugins/unity-mt-grab-handles/src/unity-mt-grab-handle-impl-factory.cpp	2011-12-23 07:13:19 +0000
+++ plugins/unity-mt-grab-handles/src/unity-mt-grab-handle-impl-factory.cpp	2012-10-13 20:28:20 +0000
@@ -19,9 +19,9 @@
 #include "unity-mt-grab-handle-impl-factory.h"
 #include "unity-mt-grab-handle.h"
 
-boost::shared_ptr <unity::MT::GrabHandle::ImplFactory> unity::MT::GrabHandle::ImplFactory::mDefault;
+std::shared_ptr <unity::MT::GrabHandle::ImplFactory> unity::MT::GrabHandle::ImplFactory::mDefault;
 
-boost::shared_ptr <unity::MT::GrabHandle::ImplFactory>
+std::shared_ptr <unity::MT::GrabHandle::ImplFactory>
 unity::MT::GrabHandle::ImplFactory::Default()
 {
   return mDefault;

=== modified file 'plugins/unity-mt-grab-handles/src/unity-mt-grab-handle-impl-factory.h'
--- plugins/unity-mt-grab-handles/src/unity-mt-grab-handle-impl-factory.h	2011-12-23 07:13:19 +0000
+++ plugins/unity-mt-grab-handles/src/unity-mt-grab-handle-impl-factory.h	2012-10-13 20:28:20 +0000
@@ -22,9 +22,7 @@
 #include <Nux/Nux.h>
 #include <glib.h>
 #include <boost/noncopyable.hpp>
-#include <boost/shared_ptr.hpp>
-#include <boost/enable_shared_from_this.hpp>
-#include <boost/weak_ptr.hpp>
+#include <memory>
 
 #include "unity-mt-grab-handle.h"
 
@@ -38,7 +36,7 @@
 
     virtual ~ImplFactory() {};
 
-    static boost::shared_ptr <ImplFactory>
+    static std::shared_ptr <ImplFactory>
     Default();
 
     static void
@@ -48,7 +46,7 @@
 
   protected:
 
-    static boost::shared_ptr <ImplFactory> mDefault;
+    static std::shared_ptr <ImplFactory> mDefault;
 
     ImplFactory() {};
 };

=== modified file 'plugins/unity-mt-grab-handles/src/unity-mt-grab-handle-window.h'
--- plugins/unity-mt-grab-handles/src/unity-mt-grab-handle-window.h	2011-12-21 08:18:47 +0000
+++ plugins/unity-mt-grab-handles/src/unity-mt-grab-handle-window.h	2012-10-13 20:28:20 +0000
@@ -37,7 +37,7 @@
                                   int y,
                                   unsigned int direction,
 				  unsigned int button) = 0;
-    virtual void raiseGrabHandle (const boost::shared_ptr <const unity::MT::GrabHandle> &) = 0;
+    virtual void raiseGrabHandle (const std::shared_ptr <const unity::MT::GrabHandle> &) = 0;
 };
 
 };

=== modified file 'plugins/unity-mt-grab-handles/src/unity-mt-grab-handle.cpp'
--- plugins/unity-mt-grab-handles/src/unity-mt-grab-handle.cpp	2011-12-21 08:18:47 +0000
+++ plugins/unity-mt-grab-handles/src/unity-mt-grab-handle.cpp	2012-10-13 20:28:20 +0000
@@ -55,7 +55,7 @@
 unity::MT::GrabHandle::raise () const
 {
   unity::MT::GrabHandleGroup::Ptr ghg = mOwner.lock ();
-  boost::shared_ptr <const unity::MT::GrabHandle> gh = shared_from_this ();
+  std::shared_ptr <const unity::MT::GrabHandle> gh = shared_from_this ();
   ghg->raiseHandle (gh);
 }
 
@@ -98,7 +98,7 @@
 unity::MT::GrabHandle::GrabHandle(Texture::Ptr texture,
                                   unsigned int    width,
                                   unsigned int    height,
-                                  const boost::shared_ptr <GrabHandleGroup> &owner,
+                                  const std::shared_ptr <GrabHandleGroup> &owner,
 				  unsigned int    id) :
   mOwner(owner),
   mTexture (texture),
@@ -110,7 +110,7 @@
 
 unity::MT::GrabHandle::Ptr
 unity::MT::GrabHandle::create (Texture::Ptr texture, unsigned int width, unsigned int height,
-                               const boost::shared_ptr <GrabHandleGroup> &owner,
+                               const std::shared_ptr <GrabHandleGroup> &owner,
                                unsigned int id)
 {
   unity::MT::GrabHandle::Ptr p (new unity::MT::GrabHandle (texture, width, height, owner, id));

=== modified file 'plugins/unity-mt-grab-handles/src/unity-mt-grab-handle.h'
--- plugins/unity-mt-grab-handles/src/unity-mt-grab-handle.h	2011-12-21 08:18:47 +0000
+++ plugins/unity-mt-grab-handles/src/unity-mt-grab-handle.h	2012-10-13 20:28:20 +0000
@@ -21,10 +21,8 @@
 
 #include <Nux/Nux.h>
 #include <glib.h>
+#include <memory>
 #include <boost/noncopyable.hpp>
-#include <boost/shared_ptr.hpp>
-#include <boost/enable_shared_from_this.hpp>
-#include <boost/weak_ptr.hpp>
 
 #include "unity-mt-texture.h"
 #include "unity-mt-grab-handle-window.h"
@@ -79,17 +77,17 @@
 class GrabHandleGroup;
 
 class GrabHandle :
-  public boost::enable_shared_from_this <GrabHandle>,
+  public std::enable_shared_from_this <GrabHandle>,
   boost::noncopyable
 {
 public:
 
-  typedef boost::shared_ptr <GrabHandle> Ptr;
+  typedef std::shared_ptr <GrabHandle> Ptr;
 
   static GrabHandle::Ptr create (Texture::Ptr    texture,
                                  unsigned int    width,
                                  unsigned int    height,
-                                 const boost::shared_ptr <GrabHandleGroup> &owner,
+                                 const std::shared_ptr <GrabHandleGroup> &owner,
                                  unsigned int id);
   ~GrabHandle();
 
@@ -160,10 +158,10 @@
   GrabHandle(Texture::Ptr    texture,
              unsigned int    width,
              unsigned int    height,
-             const boost::shared_ptr <GrabHandleGroup> &owner,
+             const std::shared_ptr <GrabHandleGroup> &owner,
              unsigned int id);
 
-  boost::weak_ptr <GrabHandleGroup>      mOwner;
+  std::weak_ptr <GrabHandleGroup>      mOwner;
   Texture::Ptr                           mTexture;
   unsigned int                           mId;
   nux::Geometry                          mRect;

=== modified file 'plugins/unity-mt-grab-handles/src/unity-mt-grab-handles.cpp'
--- plugins/unity-mt-grab-handles/src/unity-mt-grab-handles.cpp	2012-09-13 10:56:42 +0000
+++ plugins/unity-mt-grab-handles/src/unity-mt-grab-handles.cpp	2012-10-13 20:28:20 +0000
@@ -35,7 +35,8 @@
 unity::MT::Texture::Ptr
 unity::MT::X11TextureFactory::create ()
 {
-  return boost::shared_static_cast <unity::MT::Texture> (unity::MT::X11Texture::Ptr (new unity::MT::X11Texture (mWrap)));
+  unity::MT::Texture::Ptr tp(static_cast<unity::MT::Texture*> (new unity::MT::X11Texture(mWrap)));
+  return tp;
 }
 
 unity::MT::X11Texture::X11Texture (const GLTexture::List &t)
@@ -151,7 +152,7 @@
 }
 
 void
-UnityMTGrabHandlesWindow::raiseGrabHandle (const boost::shared_ptr <const unity::MT::GrabHandle> &h)
+UnityMTGrabHandlesWindow::raiseGrabHandle (const std::shared_ptr <const unity::MT::GrabHandle> &h)
 {
   UnityMTGrabHandlesScreen::get (screen)->raiseHandle (h, window->frame ());
 }
@@ -202,12 +203,13 @@
 }
 
 void
-UnityMTGrabHandlesScreen::raiseHandle (const boost::shared_ptr <const unity::MT::GrabHandle> &h,
+UnityMTGrabHandlesScreen::raiseHandle (const std::shared_ptr <const unity::MT::GrabHandle> &h,
                                        Window                                                owner)
 {
   for (const auto &pair : mInputHandles)
   {
-    if (*pair.second == *h)
+    const unity::MT::GrabHandle::Ptr gh = pair.second.lock();
+    if (*gh == *h)
     {
       unsigned int mask = CWSibling | CWStackMode;
       XWindowChanges xwc;
@@ -322,10 +324,11 @@
 
       if (it != mInputHandles.end())
       {
-	if (it->second)
-          it->second->buttonPress (event->xbutton.x_root,
-                                   event->xbutton.y_root,
-                                   event->xbutton.button);
+        const unity::MT::GrabHandle::Ptr gh = it->second.lock();
+        if (gh)
+          gh->buttonPress (event->xbutton.x_root,
+                           event->xbutton.y_root,
+                           event->xbutton.button);
       }
 
       break;
@@ -346,8 +349,9 @@
 
       if (it != mInputHandles.end())
       {
-        if (it->second)
-          it->second->reposition (0, 0, unity::MT::PositionLock);
+        const unity::MT::GrabHandle::Ptr gh = it->second.lock();
+        if (gh)
+          gh->reposition (0, 0, unity::MT::PositionLock);
       }
 
       break;
@@ -457,7 +461,7 @@
        * region */
       CompRegion reg = CompRegion(layout.second.x, layout.second.y, layout.second.width, layout.second.height);
 
-      for(GLTexture * tex : boost::shared_static_cast <unity::MT::X11Texture> (layout.first)->get ())
+      for(GLTexture * tex : static_cast<unity::MT::X11Texture*>(layout.first.get())->get())
       {
         GLTexture::MatrixList matl;
         GLTexture::Matrix     mat = tex->matrix();
@@ -633,7 +637,7 @@
 void
 UnityMTGrabHandlesScreen::addHandleWindow(const unity::MT::GrabHandle::Ptr &h, Window w)
 {
-  mInputHandles.insert(std::pair <Window, const unity::MT::GrabHandle::Ptr> (w, h));
+  mInputHandles.insert(std::make_pair(w, h));
 }
 
 void
@@ -774,7 +778,7 @@
     GLTexture::List t = GLTexture::readImageToTexture(fname, pname,
                                                       size);
 
-    (boost::shared_static_cast <unity::MT::X11TextureFactory> (unity::MT::Texture::Factory::Default ()))->setActiveWrap (t);
+    (static_cast<unity::MT::X11TextureFactory*>(unity::MT::Texture::Factory::Default().get())->setActiveWrap(t));
 
     mHandleTextures.at(i).first = unity::MT::Texture::Factory::Default ()->create ();
     mHandleTextures.at (i).second.width = size.width ();

=== modified file 'plugins/unity-mt-grab-handles/src/unity-mt-grab-handles.h'
--- plugins/unity-mt-grab-handles/src/unity-mt-grab-handles.h	2012-09-13 10:56:42 +0000
+++ plugins/unity-mt-grab-handles/src/unity-mt-grab-handles.h	2012-10-13 20:28:20 +0000
@@ -22,9 +22,7 @@
 #include <composite/composite.h>
 #include <opengl/opengl.h>
 #include <boost/noncopyable.hpp>
-#include <boost/shared_ptr.hpp>
-#include <boost/enable_shared_from_this.hpp>
-#include <boost/weak_ptr.hpp>
+#include <memory>
 
 #include <core/atoms.h>
 
@@ -72,7 +70,7 @@
 {
   public:
 
-    typedef boost::shared_ptr <X11Texture> Ptr;
+    typedef std::shared_ptr <X11Texture> Ptr;
 
     X11Texture (const GLTexture::List &t);
 
@@ -124,7 +122,7 @@
 
 private:
 
-  boost::weak_ptr <unity::MT::GrabHandle>  mGrabHandle;
+  std::weak_ptr <unity::MT::GrabHandle>  mGrabHandle;
   Window                                   mIpw;
   Display                                  *mDpy;
 };
@@ -175,7 +173,7 @@
   void preparePaint(int);
   void donePaint();
 
-  void raiseHandle (const boost::shared_ptr <const unity::MT::GrabHandle> &,
+  void raiseHandle (const std::shared_ptr <const unity::MT::GrabHandle> &,
                     Window                      owner);
 
   std::vector <unity::MT::TextureSize>  & textures()
@@ -188,7 +186,7 @@
   std::list <unity::MT::GrabHandleGroup::Ptr> mGrabHandles;
   std::vector <unity::MT::TextureSize> mHandleTextures;
 
-  std::map <Window, const unity::MT::GrabHandle::Ptr> mInputHandles;
+  std::map <Window, const std::weak_ptr <unity::MT::GrabHandle> > mInputHandles;
   CompWindowVector         		    mLastClientListStacking;
   Atom             			    mCompResizeWindowAtom;
 
@@ -243,7 +241,7 @@
 
 protected:
 
-  void raiseGrabHandle (const boost::shared_ptr <const unity::MT::GrabHandle> &h);
+  void raiseGrabHandle (const std::shared_ptr <const unity::MT::GrabHandle> &h);
   void requestMovement (int x,
                         int y,
                         unsigned int direction,

=== modified file 'plugins/unity-mt-grab-handles/src/unity-mt-texture.cpp'
--- plugins/unity-mt-grab-handles/src/unity-mt-texture.cpp	2011-12-23 07:19:07 +0000
+++ plugins/unity-mt-grab-handles/src/unity-mt-texture.cpp	2012-10-13 20:28:20 +0000
@@ -18,7 +18,7 @@
 
 #include "unity-mt-texture.h"
 
-boost::shared_ptr <unity::MT::Texture::Factory> unity::MT::Texture::Factory::mDefault;
+std::shared_ptr <unity::MT::Texture::Factory> unity::MT::Texture::Factory::mDefault;
 
 unity::MT::Texture::Factory::Factory ()
 {
@@ -34,7 +34,7 @@
   mDefault.reset (f);
 }
 
-boost::shared_ptr <unity::MT::Texture::Factory>
+std::shared_ptr <unity::MT::Texture::Factory>
 unity::MT::Texture::Factory::Default ()
 {
   return mDefault;

=== modified file 'plugins/unity-mt-grab-handles/src/unity-mt-texture.h'
--- plugins/unity-mt-grab-handles/src/unity-mt-texture.h	2011-12-23 07:19:07 +0000
+++ plugins/unity-mt-grab-handles/src/unity-mt-texture.h	2012-10-13 20:28:20 +0000
@@ -21,7 +21,6 @@
 
 #include <Nux/Nux.h>
 #include <boost/noncopyable.hpp>
-#include <boost/shared_ptr.hpp>
 
 namespace unity
 {
@@ -31,7 +30,7 @@
 {
   public:
 
-    typedef boost::shared_ptr <Texture> Ptr;
+    typedef std::shared_ptr <Texture> Ptr;
 
     virtual ~Texture ();
 
@@ -48,7 +47,7 @@
       static void
       SetDefault (Factory *);
 
-      static boost::shared_ptr <Factory>
+      static std::shared_ptr <Factory>
       Default ();
 
     protected:
@@ -57,7 +56,7 @@
 
     private:
 
-      static boost::shared_ptr <unity::MT::Texture::Factory> mDefault;
+      static std::shared_ptr <unity::MT::Texture::Factory> mDefault;
     };
 
   protected:

=== modified file 'tests/test_grabhandle.cpp'
--- tests/test_grabhandle.cpp	2011-12-23 01:28:05 +0000
+++ tests/test_grabhandle.cpp	2012-10-13 20:28:20 +0000
@@ -7,7 +7,7 @@
 #include <unity-mt-grab-handle-group.h>
 #include <unity-mt-grab-handle-impl-factory.h>
 #include <unity-mt-texture.h>
-#include <boost/shared_ptr.hpp>
+#include <memory>
 
 unsigned int unity::MT::MaximizedHorzMask = (1 << 0);
 unsigned int unity::MT::MaximizedVertMask = (1 << 1);
@@ -95,7 +95,7 @@
 class MockGrabHandleTexture : public unity::MT::Texture
 {
 public:
-  typedef boost::shared_ptr <MockGrabHandleTexture> Ptr;
+  typedef std::shared_ptr <MockGrabHandleTexture> Ptr;
   MockGrabHandleTexture () : unity::MT::Texture () {};
 };
 
@@ -112,13 +112,14 @@
 public:
   MockGrabHandleWindow () : GrabHandleWindow () {};
   MOCK_METHOD4 (requestMovement, void (int, int, unsigned int, unsigned int));
-  MOCK_METHOD1 (raiseGrabHandle, void (const boost::shared_ptr <const unity::MT::GrabHandle> &));
+  MOCK_METHOD1 (raiseGrabHandle, void (const std::shared_ptr <const unity::MT::GrabHandle> &));
 };
 
 Texture::Ptr
 MockGrabHandleTextureFactory::create ()
 {
-  return boost::shared_static_cast <Texture> (MockGrabHandleTexture::Ptr (new MockGrabHandleTexture ()));
+  Texture::Ptr pt(static_cast<unity::MT::Texture*>(new MockGrabHandleTexture()));
+  return pt;
 }
 
 GrabHandle::Impl *