Merge lp:~vanvugt/unity/fix-918329-trunk into lp:unity

[Daniel van Vugt]

Fix SIGSEGV when a window is minimized. (LP: #918329)

The crash was caused by 'minimizedWindows' leaking pointers to windows that had been deleted, and then later trying to dereference those pointers. The reason for the leak appears to be a reference leak in the boost::shared_ptr's to CompizMinimizedWindowHandler. Reverting to regular pointers avoids the reference leak, avoids the crash, and makes the code easier to understand (and debug).

Detailed valgrind log showing the error:
https://bugs.launchpad.net/ubuntu/+source/unity/+bug/918329/comments/6

[Sam Spilsbury (smspillaz)]

Whoops: https://code.launchpad.net/~smspillaz/unity/unity.fix_881190_918360/+merge/89181

[Sam Spilsbury (smspillaz)]

Alright - so what do you think then? I'm in favor of using smart pointers where possible to do so, obviously we must use raw pointers in the list, but the list is managed by the destructor of the minimized window handler.

(I really need to clean up that file, but then again, the file probably represents how much of a hack fake minimization really is, which is the only way to get live previews from minimized windows *sigh*)

[Daniel van Vugt (vanvugt)]

I tried an in-between solution as you suggest, where the list is raw pointers and the share_ptrs stay in unityshell. It worked, however that necessitated some ugly dynamic typecasts. I think this version is better because the types are cleanly matched without casting required.

Smart pointers probably have their place. But in this case they caused leaks and made the code much harder to read and maintain.

[Daniel van Vugt (vanvugt)]

P.S. One of us just wasted a day because you forgot to mark the bug in progress or even assign it to yourself :(

[Sam Spilsbury (smspillaz)]

On Fri, 20 Jan 2012, Daniel van Vugt wrote:

> I tried an in-between solution as you suggest, where the list is raw pointers and the share_ptrs stay in unityshell. It worked, however that necessitated some ugly dynamic typecasts. I think this version is better because the types are cleanly matched without casting required.
>
> Smart pointers probably have their place. But in this case they caused leaks and made the code much harder to read and maintain.

There is no need for dynamic casting as there is only a single line of
inheritance.

I'll merge this one though.

> --
> https://code.launchpad.net/~vanvugt/unity/fix-918329-trunk/+merge/89391
> You are requested to review the proposed merge of lp:~vanvugt/unity/fix-918329-trunk into lp:unity.
>

[Sam Spilsbury (smspillaz)]

review: Approve

[Daniel van Vugt (vanvugt)]

Thanks.

And you're right about static casts instead of dynamic. But this version avoids all casts which is even better I think.

[Sam Spilsbury (smspillaz)]

On Fri, 20 Jan 2012, Daniel van Vugt wrote:

> Thanks.
>
> And you're right about static casts instead of dynamic. But this version avoids all casts which is even better I think.

Yes, boost::static_pointer_cast has the cost of the boost::shared_ptr
constructor (which is not that much, but something to dislike anyways).

I don't know why the static pointer casts were changed to dynamic casts.
That seems excessively silly, especially considering that one of them was
in a paint function (!). However, use of downcasting usually indicates a
problem with the design, one that should be revisited later (also related
to testing, since the base class exists for testing purposes)

> --
> https://code.launchpad.net/~vanvugt/unity/fix-918329-trunk/+merge/89391
> You are reviewing the proposed merge of lp:~vanvugt/unity/fix-918329-trunk into lp:unity.
>

=== modified file 'plugins/unityshell/src/compizminimizedwindowhandler.h'
--- plugins/unityshell/src/compizminimizedwindowhandler.h	2011-10-05 12:20:19 +0000
+++ plugins/unityshell/src/compizminimizedwindowhandler.h	2012-01-20 07:01:19 +0000
@@ -63,9 +63,8 @@
   static void handleEvent (XEvent *event);
   static std::list<CompWindow *> minimizingWindows;
 
-  typedef CompizMinimizedWindowHandler<Screen, Window> CompizMinimizedWindowHandler_complete;
-  typedef boost::shared_ptr<CompizMinimizedWindowHandler_complete> Ptr;
-  typedef std::list <Ptr> List;
+  typedef CompizMinimizedWindowHandler<Screen, Window> Type;
+  typedef std::list <Type *> List;
 protected:
 
   virtual std::vector<unsigned int> getTransients ();
@@ -74,10 +73,14 @@
 
   PrivateCompizMinimizedWindowHandler *priv;
   static bool handleEvents;
-  static std::list<Ptr> minimizedWindows;
+  static List minimizedWindows;
 };
 }
 
+/* XXX minimizedWindows should be removed because it is dangerous to keep
+ *     a list of windows separate to compiz-core. The list could get out of
+ *     sync and cause more crashes like LP: #918329, LP: #864758.
+ */
 template <typename Screen, typename Window>
 typename compiz::CompizMinimizedWindowHandler<Screen, Window>::List compiz::CompizMinimizedWindowHandler<Screen, Window>::minimizedWindows;
 
@@ -100,12 +103,7 @@
 template <typename Screen, typename Window>
 compiz::CompizMinimizedWindowHandler<Screen, Window>::~CompizMinimizedWindowHandler ()
 {
-  typedef compiz::CompizMinimizedWindowHandler<Screen, Window> minimized_window_handler_full;
-
-  compiz::CompizMinimizedWindowHandler<Screen, Window>::Ptr compizMinimizeHandler =
-        boost::dynamic_pointer_cast <minimized_window_handler_full> (Window::get (priv->mWindow)->mMinimizeHandler);
-
-  minimizedWindows.remove (compizMinimizeHandler);
+  minimizedWindows.remove (this);
 }
 
 template <typename Screen, typename Window>
@@ -144,18 +142,13 @@
   Atom          wmState = XInternAtom (screen->dpy (), "WM_STATE", 0);
   unsigned long data[2];
 
-  typedef compiz::CompizMinimizedWindowHandler<Screen, Window> minimized_window_handler_full;
-
   std::vector<unsigned int> transients = getTransients ();
 
   handleEvents = true;
   priv->mWindow->windowNotify (CompWindowNotifyMinimize);
   priv->mWindow->changeState (priv->mWindow->state () | CompWindowStateHiddenMask);
 
-  compiz::CompizMinimizedWindowHandler<Screen, Window>::Ptr compizMinimizeHandler =
-        boost::dynamic_pointer_cast <minimized_window_handler_full> (Window::get (priv->mWindow)->mMinimizeHandler);
-
-  minimizedWindows.push_back (compizMinimizeHandler);
+  minimizedWindows.push_back (this);
 
   for (unsigned int &w : transients)
   {
@@ -163,8 +156,10 @@
 
     if (win)
     {
-      Window::get (win)->mMinimizeHandler = MinimizedWindowHandler::Ptr (new CompizMinimizedWindowHandler (win));
-      Window::get (win)->mMinimizeHandler->minimize ();
+      Window *w = Window::get (win);
+      if (!w->mMinimizeHandler)
+        w->mMinimizeHandler = new Type (win);
+      w->mMinimizeHandler->minimize ();
     }
   }
 
@@ -226,14 +221,9 @@
   Atom          wmState = XInternAtom (screen->dpy (), "WM_STATE", 0);
   unsigned long data[2];
 
-  typedef compiz::CompizMinimizedWindowHandler<Screen, Window> minimized_window_handler_full;
-
   std::vector<unsigned int> transients = getTransients ();
 
-  compiz::CompizMinimizedWindowHandler<Screen, Window>::Ptr compizMinimizeHandler =
-        boost::dynamic_pointer_cast <minimized_window_handler_full> (Window::get (priv->mWindow)->mMinimizeHandler);
-
-  minimizedWindows.remove (compizMinimizeHandler);
+  minimizedWindows.remove (this);
 
   priv->mWindow->focusSetEnabled (Window::get (priv->mWindow), true);
 
@@ -247,10 +237,13 @@
 
     if (win)
     {
-      if (Window::get (win)->mMinimizeHandler)
-        Window::get (win)->mMinimizeHandler->unminimize ();
-
-      Window::get (win)->mMinimizeHandler.reset ();
+      Window *w = Window::get (win);
+      if (w && w->mMinimizeHandler)
+      {
+        w->mMinimizeHandler->unminimize ();
+        delete w->mMinimizeHandler;
+        w->mMinimizeHandler = NULL;
+      }
     }
   }
 
@@ -330,10 +323,9 @@
 
     if (w)
     {
-      typedef compiz::CompizMinimizedWindowHandler<Screen, Window> plugin_handler;
       Window *pw = Window::get (w);
-      plugin_handler::Ptr compizMinimizeHandler =
-        boost::dynamic_pointer_cast <plugin_handler> (pw->mMinimizeHandler);
+      Type *compizMinimizeHandler = pw->mMinimizeHandler;
+
       /* Restore and re-save input shape and remove */
       if (compizMinimizeHandler)
       {

=== modified file 'plugins/unityshell/src/unityshell.cpp'
--- plugins/unityshell/src/unityshell.cpp	2012-01-19 15:13:35 +0000
+++ plugins/unityshell/src/unityshell.cpp	2012-01-20 07:01:19 +0000
@@ -1742,11 +1742,7 @@
 
   if (mMinimizeHandler)
   {
-    typedef compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow> minimized_window_handler_unity;
-
-    compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow>::Ptr compizMinimizeHandler =
-        boost::dynamic_pointer_cast <minimized_window_handler_unity> (mMinimizeHandler);
-    mask |= compizMinimizeHandler->getPaintMask ();
+    mask |= mMinimizeHandler->getPaintMask ();
   }
   else if (mShowdesktopHandler)
   {
@@ -1815,7 +1811,7 @@
 
   if (!mMinimizeHandler)
   {
-    mMinimizeHandler = compiz::MinimizedWindowHandler::Ptr (new compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow> (window));
+    mMinimizeHandler = new UnityMinimizedHandler (window);
     mMinimizeHandler->minimize ();
   }
 }
@@ -1826,14 +1822,15 @@
   if (mMinimizeHandler)
   {
     mMinimizeHandler->unminimize ();
-    mMinimizeHandler.reset ();
+    delete mMinimizeHandler;
+    mMinimizeHandler = nullptr;
   }
 }
 
 bool
 UnityWindow::focus ()
 {
-  if (!mMinimizeHandler.get ())
+  if (!mMinimizeHandler)
     return window->focus ();
 
   if (window->overrideRedirect ())
@@ -1865,7 +1862,7 @@
 bool
 UnityWindow::minimized ()
 {
-  return mMinimizeHandler.get () != NULL;
+  return mMinimizeHandler != nullptr;
 }
 
 gboolean
@@ -1928,16 +1925,12 @@
 
   window->windowNotify(n);
 
-  if (mMinimizeHandler.get () != NULL)
+  if (mMinimizeHandler)
   {
     /* The minimize handler will short circuit the frame
      * region update func and ensure that the frame
      * does not have a region */
-    typedef compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow> minimized_window_handler_unity;
-
-    compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow>::Ptr compizMinimizeHandler =
-        boost::dynamic_pointer_cast <minimized_window_handler_unity> (mMinimizeHandler);
-    compizMinimizeHandler->windowNotify (n);
+    mMinimizeHandler->windowNotify (n);
   }
   else if (mShowdesktopHandler)
   {
@@ -1976,13 +1969,9 @@
   /* The minimize handler will short circuit the frame
    * region update func and ensure that the frame
    * does not have a region */
-  typedef compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow> minimized_window_handler_unity;
-
-  compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow>::Ptr compizMinimizeHandler =
-      boost::dynamic_pointer_cast <minimized_window_handler_unity> (mMinimizeHandler);
-
-  if (compizMinimizeHandler)
-    compizMinimizeHandler->updateFrameRegion (region);
+
+  if (mMinimizeHandler)
+    mMinimizeHandler->updateFrameRegion (region);
   else if (mShowdesktopHandler)
     mShowdesktopHandler->updateFrameRegion (region);
   else
@@ -2453,6 +2442,7 @@
   , PluginClassHandler<UnityWindow, CompWindow>(window)
   , window(window)
   , gWindow(GLWindow::get(window))
+  , mMinimizeHandler(nullptr)
   , mShowdesktopHandler(nullptr)
   , focusdesktop_handle_(0)
 {
@@ -2519,8 +2509,10 @@
     window->minimizedSetEnabled (this, false);
     window->minimize ();
 
-    mMinimizeHandler.reset ();
+    delete mMinimizeHandler;
+    mMinimizeHandler = nullptr;
   }
+
   if (mShowdesktopHandler)
     delete mShowdesktopHandler;
     

=== modified file 'plugins/unityshell/src/unityshell.h'
--- plugins/unityshell/src/unityshell.h	2012-01-14 13:05:16 +0000
+++ plugins/unityshell/src/unityshell.h	2012-01-20 07:01:19 +0000
@@ -376,7 +376,10 @@
   void leaveShowDesktop ();
   bool handleAnimations (unsigned int ms);
 
-  compiz::MinimizedWindowHandler::Ptr mMinimizeHandler;
+  typedef compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow>
+          UnityMinimizedHandler;
+  UnityMinimizedHandler *mMinimizeHandler;
+
   UnityShowdesktopHandler             *mShowdesktopHandler;
   
 private: