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

Proposed by Sam Spilsbury on 2012-01-19
Status: Superseded
Proposed branch: lp:~smspillaz/unity/unity.fix_881190_918360
Merge into: lp:unity
Diff against target: 281 lines (+60/-43) 5 files modified
To merge this branch: bzr merge lp:~smspillaz/unity/unity.fix_881190_918360
Reviewer Review Type Date Requested Status
Unity Team 2012-01-19 Pending
Review via email: mp+89181@code.launchpad.net

This proposal has been superseded by a proposal from 2012-01-20.

Description of the Change

    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.

To post a comment you must log in.
Daniel van Vugt (vanvugt) 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.

Sam Spilsbury (smspillaz) wrote :

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.

Preview Diff

1=== modified file 'plugins/unityshell/src/compizminimizedwindowhandler.h'
2--- plugins/unityshell/src/compizminimizedwindowhandler.h 2011-10-05 12:20:19 +0000
3+++ plugins/unityshell/src/compizminimizedwindowhandler.h 2012-01-19 03:18:24 +0000
4@@ -45,9 +45,12 @@
5 {
6 public:
7
8- CompizMinimizedWindowHandler (CompWindow *w);
9 ~CompizMinimizedWindowHandler ();
10
11+ typedef CompizMinimizedWindowHandler<Screen, Window> CompizMinimizedWindowHandler_complete;
12+ typedef boost::shared_ptr<CompizMinimizedWindowHandler_complete> Ptr;
13+ typedef std::list <CompizMinimizedWindowHandler *> List;
14+
15 void setVisibility (bool visible);
16 unsigned int getPaintMask ();
17
18@@ -58,23 +61,23 @@
19
20 void windowNotify (CompWindowNotify n);
21
22+ static MinimizedWindowHandler::Ptr create (CompWindow *w);
23+
24 static void setFunctions (bool keepMinimized);
25 static void handleCompizEvent (const char *, const char *, CompOption::Vector &);
26 static void handleEvent (XEvent *event);
27 static std::list<CompWindow *> minimizingWindows;
28-
29- typedef CompizMinimizedWindowHandler<Screen, Window> CompizMinimizedWindowHandler_complete;
30- typedef boost::shared_ptr<CompizMinimizedWindowHandler_complete> Ptr;
31- typedef std::list <Ptr> List;
32 protected:
33
34- virtual std::vector<unsigned int> getTransients ();
35+ std::vector<unsigned int> getTransients ();
36
37 private:
38
39+ CompizMinimizedWindowHandler (CompWindow *w);
40+
41 PrivateCompizMinimizedWindowHandler *priv;
42 static bool handleEvents;
43- static std::list<Ptr> minimizedWindows;
44+ static std::list<CompizMinimizedWindowHandler *> minimizedWindows;
45 };
46 }
47
48@@ -88,6 +91,14 @@
49 bool compiz::CompizMinimizedWindowHandler<Screen, Window>::handleEvents = true;
50
51 template <typename Screen, typename Window>
52+compiz::MinimizedWindowHandler::Ptr
53+compiz::CompizMinimizedWindowHandler<Screen, Window>::create(CompWindow *w)
54+{
55+ Ptr p (new compiz::CompizMinimizedWindowHandler<Screen, Window> (w));
56+ return boost::static_pointer_cast <MinimizedWindowHandler> (p);
57+}
58+
59+template <typename Screen, typename Window>
60 compiz::CompizMinimizedWindowHandler<Screen, Window>::CompizMinimizedWindowHandler(CompWindow *w) :
61 MinimizedWindowHandler (screen->dpy (), w->id ())
62 {
63@@ -100,12 +111,19 @@
64 template <typename Screen, typename Window>
65 compiz::CompizMinimizedWindowHandler<Screen, Window>::~CompizMinimizedWindowHandler ()
66 {
67- typedef compiz::CompizMinimizedWindowHandler<Screen, Window> minimized_window_handler_full;
68-
69- compiz::CompizMinimizedWindowHandler<Screen, Window>::Ptr compizMinimizeHandler =
70- boost::dynamic_pointer_cast <minimized_window_handler_full> (Window::get (priv->mWindow)->mMinimizeHandler);
71-
72- minimizedWindows.remove (compizMinimizeHandler);
73+ if (!priv->mWindow->destroyed ())
74+ {
75+ priv->mWindow->unminimize ();
76+ priv->mWindow->focusSetEnabled (Window::get (priv->mWindow), false);
77+ priv->mWindow->minimizeSetEnabled (Window::get (priv->mWindow), false);
78+ priv->mWindow->unminimizeSetEnabled (Window::get (priv->mWindow), false);
79+ priv->mWindow->minimizedSetEnabled (Window::get (priv->mWindow), false);
80+ priv->mWindow->minimize ();
81+ }
82+ else
83+ {
84+ minimizedWindows.remove (this);
85+ }
86 }
87
88 template <typename Screen, typename Window>
89@@ -152,10 +170,7 @@
90 priv->mWindow->windowNotify (CompWindowNotifyMinimize);
91 priv->mWindow->changeState (priv->mWindow->state () | CompWindowStateHiddenMask);
92
93- compiz::CompizMinimizedWindowHandler<Screen, Window>::Ptr compizMinimizeHandler =
94- boost::dynamic_pointer_cast <minimized_window_handler_full> (Window::get (priv->mWindow)->mMinimizeHandler);
95-
96- minimizedWindows.push_back (compizMinimizeHandler);
97+ minimizedWindows.push_back (this);
98
99 for (unsigned int &w : transients)
100 {
101@@ -163,7 +178,7 @@
102
103 if (win)
104 {
105- Window::get (win)->mMinimizeHandler = MinimizedWindowHandler::Ptr (new CompizMinimizedWindowHandler (win));
106+ Window::get (win)->mMinimizeHandler = CompizMinimizedWindowHandler::create (win);
107 Window::get (win)->mMinimizeHandler->minimize ();
108 }
109 }
110@@ -230,10 +245,7 @@
111
112 std::vector<unsigned int> transients = getTransients ();
113
114- compiz::CompizMinimizedWindowHandler<Screen, Window>::Ptr compizMinimizeHandler =
115- boost::dynamic_pointer_cast <minimized_window_handler_full> (Window::get (priv->mWindow)->mMinimizeHandler);
116-
117- minimizedWindows.remove (compizMinimizeHandler);
118+ minimizedWindows.remove (this);
119
120 priv->mWindow->focusSetEnabled (Window::get (priv->mWindow), true);
121
122@@ -333,7 +345,7 @@
123 typedef compiz::CompizMinimizedWindowHandler<Screen, Window> plugin_handler;
124 Window *pw = Window::get (w);
125 plugin_handler::Ptr compizMinimizeHandler =
126- boost::dynamic_pointer_cast <plugin_handler> (pw->mMinimizeHandler);
127+ boost::static_pointer_cast <plugin_handler> (pw->mMinimizeHandler);
128 /* Restore and re-save input shape and remove */
129 if (compizMinimizeHandler)
130 {
131
132=== modified file 'plugins/unityshell/src/comptransientfor.cpp'
133--- plugins/unityshell/src/comptransientfor.cpp 2011-08-23 14:47:38 +0000
134+++ plugins/unityshell/src/comptransientfor.cpp 2012-01-19 03:18:24 +0000
135@@ -55,13 +55,18 @@
136 bool
137 compiz::CompTransientForReader::isTransientFor (unsigned int ancestor)
138 {
139+ if (!ancestor ||
140+ !priv->mWindow->id ())
141+ return false;
142+
143 return priv->mWindow->transientFor () == ancestor;
144 }
145
146 bool
147 compiz::CompTransientForReader::isGroupTransientFor (unsigned int clientLeader)
148 {
149- if (!clientLeader)
150+ if (!clientLeader ||
151+ !priv->mWindow->id ())
152 return false;
153
154 if (priv->mWindow->transientFor () == None || priv->mWindow->transientFor () == screen->root ())
155
156=== modified file 'plugins/unityshell/src/minimizedwindowhandler.h'
157--- plugins/unityshell/src/minimizedwindowhandler.h 2011-10-05 12:20:19 +0000
158+++ plugins/unityshell/src/minimizedwindowhandler.h 2012-01-19 03:18:24 +0000
159@@ -27,6 +27,7 @@
160 #include "transientfor.h"
161 #include "inputremover.h"
162 #include <boost/shared_ptr.hpp>
163+#include <boost/weak_ptr.hpp>
164 #include <boost/bind.hpp>
165
166 // Will be merged back into compiz
167
168=== modified file 'plugins/unityshell/src/transientfor.cpp'
169--- plugins/unityshell/src/transientfor.cpp 2011-08-26 10:37:35 +0000
170+++ plugins/unityshell/src/transientfor.cpp 2012-01-19 03:18:24 +0000
171@@ -51,9 +51,9 @@
172 {
173 if (actualType == XA_WINDOW && actualFormat == 32 && nLeft == 0 && nItems == 1)
174 {
175- Window *data = (Window *) prop;
176+ Window *data = (Window *) prop;
177
178- serverAncestor = *data;
179+ serverAncestor = *data;
180 }
181
182 XFree (prop);
183@@ -65,6 +65,10 @@
184 bool
185 compiz::X11TransientForReader::isTransientFor (unsigned int ancestor)
186 {
187+ if (!ancestor ||
188+ !priv->mXid)
189+ return false;
190+
191 return ancestor == getAncestor ();
192 }
193
194@@ -80,14 +84,18 @@
195 std::vector<std::string> strings;
196 std::list<Atom> atoms;
197
198+ if (!clientLeader ||
199+ !priv->mXid)
200+ return false;
201+
202 if (XGetWindowProperty (priv->mDpy, priv->mXid, wmClientLeader, 0L, 2L, false,
203 XA_WINDOW, &actualType, &actualFormat, &nItems, &nLeft, (unsigned char **)&prop) == Success)
204 {
205 if (actualType == XA_WINDOW && actualFormat == 32 && nLeft == 0 && nItems == 1)
206 {
207- Window *data = (Window *) prop;
208+ Window *data = (Window *) prop;
209
210- serverClientLeader = *data;
211+ serverClientLeader = *data;
212 }
213
214 XFree (prop);
215
216=== modified file 'plugins/unityshell/src/unityshell.cpp'
217--- plugins/unityshell/src/unityshell.cpp 2012-01-18 18:47:55 +0000
218+++ plugins/unityshell/src/unityshell.cpp 2012-01-19 03:18:24 +0000
219@@ -1745,7 +1745,7 @@
220 typedef compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow> minimized_window_handler_unity;
221
222 compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow>::Ptr compizMinimizeHandler =
223- boost::dynamic_pointer_cast <minimized_window_handler_unity> (mMinimizeHandler);
224+ boost::static_pointer_cast <minimized_window_handler_unity> (mMinimizeHandler);
225 mask |= compizMinimizeHandler->getPaintMask ();
226 }
227 else if (mShowdesktopHandler)
228@@ -1815,7 +1815,7 @@
229
230 if (!mMinimizeHandler)
231 {
232- mMinimizeHandler = compiz::MinimizedWindowHandler::Ptr (new compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow> (window));
233+ mMinimizeHandler = compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow>::create (window);
234 mMinimizeHandler->minimize ();
235 }
236 }
237@@ -1936,7 +1936,7 @@
238 typedef compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow> minimized_window_handler_unity;
239
240 compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow>::Ptr compizMinimizeHandler =
241- boost::dynamic_pointer_cast <minimized_window_handler_unity> (mMinimizeHandler);
242+ boost::static_pointer_cast <minimized_window_handler_unity> (mMinimizeHandler);
243 compizMinimizeHandler->windowNotify (n);
244 }
245 else if (mShowdesktopHandler)
246@@ -1979,7 +1979,7 @@
247 typedef compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow> minimized_window_handler_unity;
248
249 compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow>::Ptr compizMinimizeHandler =
250- boost::dynamic_pointer_cast <minimized_window_handler_unity> (mMinimizeHandler);
251+ boost::static_pointer_cast <minimized_window_handler_unity> (mMinimizeHandler);
252
253 if (compizMinimizeHandler)
254 compizMinimizeHandler->updateFrameRegion (region);
255@@ -2449,7 +2449,7 @@
256
257 /* Window init */
258 UnityWindow::UnityWindow(CompWindow* window)
259- : BaseSwitchWindow (dynamic_cast<BaseSwitchScreen *> (UnityScreen::get (screen)), window)
260+ : BaseSwitchWindow (static_cast<BaseSwitchScreen *> (UnityScreen::get (screen)), window)
261 , PluginClassHandler<UnityWindow, CompWindow>(window)
262 , window(window)
263 , gWindow(GLWindow::get(window))
264@@ -2510,17 +2510,8 @@
265
266 UnityShowdesktopHandler::animating_windows.remove (window);
267
268- if (mMinimizeHandler)
269- {
270- unminimize ();
271- window->focusSetEnabled (this, false);
272- window->minimizeSetEnabled (this, false);
273- window->unminimizeSetEnabled (this, false);
274- window->minimizedSetEnabled (this, false);
275- window->minimize ();
276+ mMinimizeHandler.reset ();
277
278- mMinimizeHandler.reset ();
279- }
280 if (mShowdesktopHandler)
281 delete mShowdesktopHandler;
282