Merge lp:~smspillaz/unity/unity.fix_881190_918360 into lp:unity

[Sam Spilsbury]

    Fix LP #881190 and a condition that could happen after: #918360 due to
    a incorrect refcounting.

    The transient for reader would report every window as a "transient" if it
    matched the w->transientFor () definition on the window - at destruction time
    this will be zero, so it will match every single window. That would cause
    all windows to be unminimized.

    However, it makes no sense to do the unminimize/minimize normally dance when
    the window has been destroyed.

    Additionaly, there was a cyclic reference in CompizMinimizedWindowHandler,
    which would cause those objects to never be destroyed. A condition could
    happen where CompizMinimizedWindowHandler could loop its own list and dereference
    a CompWindow that was gone.

[Daniel van Vugt (vanvugt)]

Conflicts with: https://code.launchpad.net/~vanvugt/unity/fix-918329-trunk/+merge/89391

Also, this proposal attempts to fix 2 separate bugs. I think you should remove the fix for 918360 which is redundant with my proposal and re-propose just a fix for bug 881190.

[Sam Spilsbury (smspillaz)]

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

> Conflicts with: https://code.launchpad.net/~vanvugt/unity/fix-918329-trunk/+merge/89391
>
> Also, this proposal attempts to fix 2 separate bugs. I think you should remove the fix for 918360 which is redundant with my proposal and re-propose just a fix for bug 881190.

Ack, removing the cycle code and keeping the fix for the unminimize

> --
> https://code.launchpad.net/~smspillaz/unity/unity.fix_881190_918360/+merge/89181
> You are the owner of lp:~smspillaz/unity/unity.fix_881190_918360.
>

Unmerged revisions

1843. By Sam Spilsbury on 2012-01-19

Fix LP #881190 and a condition that could happen after: #918360 due to
a incorrect refcounting.

The transient for reader would report every window as a "transient" if it
matched the w->transientFor () definition on the window - at destruction time
this will be zero, so it will match every single window. That would cause
all windows to be unminimized.

However, it makes no sense to do the unminimize/minimize normally dance when
the window has been destroyed.

Additionaly, there was a cyclic reference in CompizMinimizedWindowHandler,
which would cause those objects to never be destroyed. A condition could
happen where CompizMinimizedWindowHandler could loop its own list and dereference
a CompWindow that was gone.

=== 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-19 03:18:24 +0000
@@ -45,9 +45,12 @@
 {
 public:
 
-  CompizMinimizedWindowHandler (CompWindow *w);
   ~CompizMinimizedWindowHandler ();
 
+  typedef CompizMinimizedWindowHandler<Screen, Window> CompizMinimizedWindowHandler_complete;
+  typedef boost::shared_ptr<CompizMinimizedWindowHandler_complete> Ptr;
+  typedef std::list <CompizMinimizedWindowHandler *> List;
+
   void setVisibility (bool visible);
   unsigned int getPaintMask ();
 
@@ -58,23 +61,23 @@
 
   void windowNotify (CompWindowNotify n);
 
+  static MinimizedWindowHandler::Ptr create (CompWindow *w);
+
   static void setFunctions (bool keepMinimized);
   static void handleCompizEvent (const char *, const char *, CompOption::Vector &);
   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;
 protected:
 
-  virtual std::vector<unsigned int> getTransients ();
+  std::vector<unsigned int> getTransients ();
 
 private:
 
+  CompizMinimizedWindowHandler (CompWindow *w);
+
   PrivateCompizMinimizedWindowHandler *priv;
   static bool handleEvents;
-  static std::list<Ptr> minimizedWindows;
+  static std::list<CompizMinimizedWindowHandler *> minimizedWindows;
 };
 }
 
@@ -88,6 +91,14 @@
 bool compiz::CompizMinimizedWindowHandler<Screen, Window>::handleEvents = true;
 
 template <typename Screen, typename Window>
+compiz::MinimizedWindowHandler::Ptr
+compiz::CompizMinimizedWindowHandler<Screen, Window>::create(CompWindow *w)
+{
+  Ptr p (new compiz::CompizMinimizedWindowHandler<Screen, Window> (w));
+  return boost::static_pointer_cast <MinimizedWindowHandler> (p);
+}
+
+template <typename Screen, typename Window>
 compiz::CompizMinimizedWindowHandler<Screen, Window>::CompizMinimizedWindowHandler(CompWindow *w) :
   MinimizedWindowHandler (screen->dpy (), w->id ())
 {
@@ -100,12 +111,19 @@
 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);
+  if (!priv->mWindow->destroyed ())
+  {
+    priv->mWindow->unminimize ();
+    priv->mWindow->focusSetEnabled (Window::get (priv->mWindow), false);
+    priv->mWindow->minimizeSetEnabled (Window::get (priv->mWindow), false);
+    priv->mWindow->unminimizeSetEnabled (Window::get (priv->mWindow), false);
+    priv->mWindow->minimizedSetEnabled (Window::get (priv->mWindow), false);
+    priv->mWindow->minimize ();
+  }
+  else
+  {
+    minimizedWindows.remove (this);
+  }
 }
 
 template <typename Screen, typename Window>
@@ -152,10 +170,7 @@
   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,7 +178,7 @@
 
     if (win)
     {
-      Window::get (win)->mMinimizeHandler = MinimizedWindowHandler::Ptr (new CompizMinimizedWindowHandler (win));
+      Window::get (win)->mMinimizeHandler = CompizMinimizedWindowHandler::create (win);
       Window::get (win)->mMinimizeHandler->minimize ();
     }
   }
@@ -230,10 +245,7 @@
 
   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);
 
@@ -333,7 +345,7 @@
       typedef compiz::CompizMinimizedWindowHandler<Screen, Window> plugin_handler;
       Window *pw = Window::get (w);
       plugin_handler::Ptr compizMinimizeHandler =
-        boost::dynamic_pointer_cast <plugin_handler> (pw->mMinimizeHandler);
+        boost::static_pointer_cast <plugin_handler> (pw->mMinimizeHandler);
       /* Restore and re-save input shape and remove */
       if (compizMinimizeHandler)
       {

=== modified file 'plugins/unityshell/src/comptransientfor.cpp'
--- plugins/unityshell/src/comptransientfor.cpp	2011-08-23 14:47:38 +0000
+++ plugins/unityshell/src/comptransientfor.cpp	2012-01-19 03:18:24 +0000
@@ -55,13 +55,18 @@
 bool
 compiz::CompTransientForReader::isTransientFor (unsigned int ancestor)
 {
+  if (!ancestor ||
+      !priv->mWindow->id ())
+    return false;
+
   return priv->mWindow->transientFor () == ancestor;
 }
 
 bool
 compiz::CompTransientForReader::isGroupTransientFor (unsigned int clientLeader)
 {
-  if (!clientLeader)
+  if (!clientLeader ||
+      !priv->mWindow->id ())
     return false;
 
   if (priv->mWindow->transientFor () == None || priv->mWindow->transientFor () == screen->root ())

=== modified file 'plugins/unityshell/src/minimizedwindowhandler.h'
--- plugins/unityshell/src/minimizedwindowhandler.h	2011-10-05 12:20:19 +0000
+++ plugins/unityshell/src/minimizedwindowhandler.h	2012-01-19 03:18:24 +0000
@@ -27,6 +27,7 @@
 #include "transientfor.h"
 #include "inputremover.h"
 #include <boost/shared_ptr.hpp>
+#include <boost/weak_ptr.hpp>
 #include <boost/bind.hpp>
 
 // Will be merged back into compiz

=== modified file 'plugins/unityshell/src/transientfor.cpp'
--- plugins/unityshell/src/transientfor.cpp	2011-08-26 10:37:35 +0000
+++ plugins/unityshell/src/transientfor.cpp	2012-01-19 03:18:24 +0000
@@ -51,9 +51,9 @@
   {
     if (actualType == XA_WINDOW && actualFormat == 32 && nLeft == 0 && nItems == 1)
     {
-	    Window *data = (Window *) prop;
+      Window *data = (Window *) prop;
 
-	    serverAncestor = *data;
+      serverAncestor = *data;
     }
 
     XFree (prop);
@@ -65,6 +65,10 @@
 bool
 compiz::X11TransientForReader::isTransientFor (unsigned int ancestor)
 {
+  if (!ancestor ||
+      !priv->mXid)
+    return false;
+
   return ancestor == getAncestor ();
 }
 
@@ -80,14 +84,18 @@
   std::vector<std::string> strings;
   std::list<Atom>   atoms;
 
+  if (!clientLeader ||
+      !priv->mXid)
+    return false;
+
   if (XGetWindowProperty (priv->mDpy, priv->mXid, wmClientLeader, 0L, 2L, false,
                           XA_WINDOW, &actualType, &actualFormat, &nItems, &nLeft, (unsigned char **)&prop) == Success)
   {
     if (actualType == XA_WINDOW && actualFormat == 32 && nLeft == 0 && nItems == 1)
     {
-	    Window *data = (Window *) prop;
+      Window *data = (Window *) prop;
 
-	    serverClientLeader = *data;
+      serverClientLeader = *data;
     }
 
     XFree (prop);

=== modified file 'plugins/unityshell/src/unityshell.cpp'
--- plugins/unityshell/src/unityshell.cpp	2012-01-18 18:47:55 +0000
+++ plugins/unityshell/src/unityshell.cpp	2012-01-19 03:18:24 +0000
@@ -1745,7 +1745,7 @@
     typedef compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow> minimized_window_handler_unity;
 
     compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow>::Ptr compizMinimizeHandler =
-        boost::dynamic_pointer_cast <minimized_window_handler_unity> (mMinimizeHandler);
+        boost::static_pointer_cast <minimized_window_handler_unity> (mMinimizeHandler);
     mask |= compizMinimizeHandler->getPaintMask ();
   }
   else if (mShowdesktopHandler)
@@ -1815,7 +1815,7 @@
 
   if (!mMinimizeHandler)
   {
-    mMinimizeHandler = compiz::MinimizedWindowHandler::Ptr (new compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow> (window));
+    mMinimizeHandler = compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow>::create (window);
     mMinimizeHandler->minimize ();
   }
 }
@@ -1936,7 +1936,7 @@
     typedef compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow> minimized_window_handler_unity;
 
     compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow>::Ptr compizMinimizeHandler =
-        boost::dynamic_pointer_cast <minimized_window_handler_unity> (mMinimizeHandler);
+        boost::static_pointer_cast <minimized_window_handler_unity> (mMinimizeHandler);
     compizMinimizeHandler->windowNotify (n);
   }
   else if (mShowdesktopHandler)
@@ -1979,7 +1979,7 @@
   typedef compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow> minimized_window_handler_unity;
 
   compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow>::Ptr compizMinimizeHandler =
-      boost::dynamic_pointer_cast <minimized_window_handler_unity> (mMinimizeHandler);
+      boost::static_pointer_cast <minimized_window_handler_unity> (mMinimizeHandler);
 
   if (compizMinimizeHandler)
     compizMinimizeHandler->updateFrameRegion (region);
@@ -2449,7 +2449,7 @@
 
 /* Window init */
 UnityWindow::UnityWindow(CompWindow* window)
-  : BaseSwitchWindow (dynamic_cast<BaseSwitchScreen *> (UnityScreen::get (screen)), window)
+  : BaseSwitchWindow (static_cast<BaseSwitchScreen *> (UnityScreen::get (screen)), window)
   , PluginClassHandler<UnityWindow, CompWindow>(window)
   , window(window)
   , gWindow(GLWindow::get(window))
@@ -2510,17 +2510,8 @@
 
   UnityShowdesktopHandler::animating_windows.remove (window);
 
-  if (mMinimizeHandler)
-  {
-    unminimize ();
-    window->focusSetEnabled (this, false);
-    window->minimizeSetEnabled (this, false);
-    window->unminimizeSetEnabled (this, false);
-    window->minimizedSetEnabled (this, false);
-    window->minimize ();
+  mMinimizeHandler.reset ();
 
-    mMinimizeHandler.reset ();
-  }
   if (mShowdesktopHandler)
     delete mShowdesktopHandler;