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. Fixes: https://bugs.launchpad.net/bugs/918329. Appoved by Sam Spilsbury.
kyegupov · Jan 20, 2012 · b036fd6 · b036fd6
2 parents 5f4745d + a2799ba
commit b036fd6
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 54 deletions.
diff --git a/plugins/unityshell/src/compizminimizedwindowhandler.h b/plugins/unityshell/src/compizminimizedwindowhandler.h
@@ -63,9 +63,8 @@ class CompizMinimizedWindowHandler:
   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 @@ class CompizMinimizedWindowHandler:
 
   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 @@ compiz::CompizMinimizedWindowHandler<Screen, Window>::CompizMinimizedWindowHandl
 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,27 +142,24 @@ compiz::CompizMinimizedWindowHandler<Screen, Window>::minimize ()
   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)
   {
     CompWindow *win = screen->findWindow (w);
 
     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 @@ compiz::CompizMinimizedWindowHandler<Screen, Window>::unminimize ()
   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 @@ compiz::CompizMinimizedWindowHandler<Screen, Window>::unminimize ()
 
     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 @@ compiz::CompizMinimizedWindowHandler<Screen, Window>::handleEvent (XEvent *event
 
     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)
       {

diff --git a/plugins/unityshell/src/unityshell.cpp b/plugins/unityshell/src/unityshell.cpp
@@ -1742,11 +1742,7 @@ bool UnityWindow::glPaint(const GLWindowPaintAttrib& attrib,
 
   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 @@ UnityWindow::minimize ()
 
   if (!mMinimizeHandler)
   {
-    mMinimizeHandler = compiz::MinimizedWindowHandler::Ptr (new compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow> (window));
+    mMinimizeHandler = new UnityMinimizedHandler (window);
     mMinimizeHandler->minimize ();
   }
 }
@@ -1826,14 +1822,15 @@ UnityWindow::unminimize ()
   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 @@ UnityWindow::focus ()
 bool
 UnityWindow::minimized ()
 {
-  return mMinimizeHandler.get () != NULL;
+  return mMinimizeHandler != nullptr;
 }
 
 gboolean
@@ -1928,16 +1925,12 @@ void UnityWindow::windowNotify(CompWindowNotify n)
 
   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 @@ void UnityWindow::updateFrameRegion(CompRegion &region)
   /* 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 @@ UnityWindow::UnityWindow(CompWindow* window)
   , PluginClassHandler<UnityWindow, CompWindow>(window)
   , window(window)
   , gWindow(GLWindow::get(window))
+  , mMinimizeHandler(nullptr)
   , mShowdesktopHandler(nullptr)
   , focusdesktop_handle_(0)
 {
@@ -2519,8 +2509,10 @@ UnityWindow::~UnityWindow()
     window->minimizedSetEnabled (this, false);
     window->minimize ();
 
-    mMinimizeHandler.reset ();
+    delete mMinimizeHandler;
+    mMinimizeHandler = nullptr;
   }
+
   if (mShowdesktopHandler)
     delete mShowdesktopHandler;
 

diff --git a/plugins/unityshell/src/unityshell.h b/plugins/unityshell/src/unityshell.h
@@ -376,7 +376,10 @@ class UnityWindow :
   void leaveShowDesktop ();
   bool handleAnimations (unsigned int ms);
 
-  compiz::MinimizedWindowHandler::Ptr mMinimizeHandler;
+  typedef compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow>
+          UnityMinimizedHandler;
+  UnityMinimizedHandler *mMinimizeHandler;
+
   UnityShowdesktopHandler             *mShowdesktopHandler;
 
 private: