Merge lp:~smspillaz/compiz-core/compiz-core.fix_931927 into lp:compiz-core

[Sam Spilsbury]

Only call removeAction on the CompOption destructor and not the CompOption::Value
destructor, since plugins that make copies of CompOption::Value can cause actions to
be added through setOptionForPlugin and then those actions will be removed when the
value temporary goes away.

The action adding and removing only happens within the bounds of CompOption anyways, so
its its more appropriate to have it in its destructor.

Of course, this brings up another issue, which is that CompOption should be noncopyable
but this opens up a whole another can of worms.

How this even worked before is beyond me...

bug 931927

(See also lp:~smspillaz/compiz-expo-plugin/compiz-expo-plugin.api_change_931927)

[Daniel van Vugt (vanvugt)]

The noOptions API change makes me nervous. Especially the part about returning a reference to a static local:

203 +CompOption::Vector &
204 +noOptions ()
205 +{
206 + static CompOption::Vector v (0);
207 + return v;
208 +}

Why not just change the global:
    CompOption::Vector noOptions (0);
to:
    const CompOption::Vector noOptions (0);
??

review: Needs Information

[Daniel van Vugt (vanvugt)]

And change:
    extern CompOption::Vector noOptions;
to:
    extern const CompOption::Vector noOptions;

[Daniel van Vugt (vanvugt)]

And the same for all of:
164 -CompOption::Vector noOptions (0);
165 -CompOption::Value::Vector emptyList;
166 -CompString emptyString;
167 -CompMatch emptyMatch;
168 -CompAction emptyAction;
169 -unsigned short emptyColor[4];

[Daniel van Vugt (vanvugt)]

All the globals should remain globals instead of functions. Perhaps convert them to const for safety.

review: Needs Fixing

[Sam Spilsbury (smspillaz)]

Static initialization and destruction, basically.

There's a case where the destructor would be called on the global when a plugin was loaded (compiler, linker bug?) before it was actually fully constructed.

I would have made them const, but a lot of the Option* code requires a non const reference. It worked fine even when the globals weren't const, so I don't think this is an issue.

[Sam Spilsbury (smspillaz)]

(See http://www.parashift.com/c++-faq-lite/ctors.html#faq-10.14)

[Daniel van Vugt (vanvugt)]

OK, that's not a compiler/linker/loader bug at all. That is correct behaviour. I have seen many such static ctor/dtor DSO bugs on various Unixes before, but not Linux, at least not for ~10 years.

The real problem must be globals using other globals during their construction/destruction (again). That's what really needs fixing I think.

[Sam Spilsbury (smspillaz)]

Its not worth fixing in this branch :)

The best way to get around this fiasco, is like the article says, to use the construct-upon-access idiom which is what I have done here.

[Sam Spilsbury (smspillaz)]

> OK, that's not a compiler/linker/loader bug at all. That is correct behaviour.
> I have seen many such static ctor/dtor DSO bugs on various Unixes before, but
> not Linux, at least not for ~10 years.

To clarify, I guess the reason why I said linker bug was because of this

#0 CompAction::active
#1 CompOption::~CompOption // wtf ?
#2 __static_initialization_and_destruction

The use of uninitialized globals within the constructors of other globals is fine, but calling the destructor to a static object before we hit main () is very strange indeed.

(And yes, I've hit compiler bugs before where destructors were incorrectly called :P)

[Daniel van Vugt (vanvugt)]

OK, I agree it's a quick fix that should work, even though it impacts the public API in a less than ideal way.

Alan already started logging bugs describing the removal of globals such as screen (in the distant future)...

review: Abstain

[Daniel van Vugt (vanvugt)]

And I do believe you now :)

But you will probably find some obscure DSO-related reason for the apparently premature destruction.

[Sam Spilsbury (smspillaz)]

> OK, I agree it's a quick fix that should work, even though it impacts the
> public API in a less than ideal way.
>
> Alan already started logging bugs describing the removal of globals such as
> screen (in the distant future)...

The good news is that only one plugin was impacted by this.

It would be so awesome if const references could be NULL. I guess that's what exceptions are for.

[Tim Penhey (thumper)]

I have to agree with Daniel on a number of points. This is kinda ick, but
better than nothing as far as I can tell.

Might as well remove the boost/noncopyable header since you are not using it.

You do hit a couple of differences here.

Static initialisation initially zeros things out. So...

unsigned short emptyColor[4];
would be zero initialised.

When you have a static variable in a function, I don't think it is zero
initialised. Worth checking with Alan about that. So, inside the emptyColor
function use:
  static unsigned short v[4] = {0,0,0,0};

We need to file bugs about the catch (...) bits, that's just ick.

I can't comment on whether this fix actually works. Ideally we'd have test
coverage :-(

review: Needs Fixing

[Tim Penhey (thumper)]

> And I do believe you now :)
>
> But you will probably find some obscure DSO-related reason for the apparently
> premature destruction.

What do you mean by DSO-related?

And I have to say that everytime I've come across a weird destruction bug, whether mis-timed or double delete, it has been a coding error, not the compiler.

[Daniel van Vugt (vanvugt)]

I mean that if this destruction issue /is/ a bug then it is most likely triggered by using dynamic shared objects. I've seen many such loader/construction/destruction bugs before. But none in Linux since at least before Ubuntu existed.

[Daniel van Vugt (vanvugt)]

Re: "Static initialisation initially zeros things out"

Is that in the C++ spec? As far as I know, basic types like;
    unsigned short emptyColor[4];
will not be initialized at all, because they don't have a constructor.

[Tim Penhey (thumper)]

> Re: "Static initialisation initially zeros things out"
>
> Is that in the C++ spec? As far as I know, basic types like;
> unsigned short emptyColor[4];
> will not be initialized at all, because they don't have a constructor.

Normally no, but for statics / globals, yes.

[Daniel van Vugt (vanvugt)]

If that's true I would still recommend explicit initialization for the sake of readability :)

[Tim Penhey (thumper)]

review: Approve

[Alan Griffiths (alan-griffiths)]

> Re: "Static initialisation initially zeros things out"
>
> Is that in the C++ spec? As far as I know, basic types like;
> unsigned short emptyColor[4];
> will not be initialized at all, because they don't have a constructor.

Local statics have a special case (don't you just love C++) - they are zero initialized (the same as namespace scope variables - which is what they are to the link loader).

[Alan Griffiths (alan-griffiths)]

Oh, and you don't need

"static unsigned short v[4] = {0,0,0,0};"

either:

"static unsigned short v[] = {0,0,0,0};"

or

"static unsigned short v[4] = {0};"

is fine

[Daniel van Vugt (vanvugt)]

*** Found the problem ***

CompOption::Vector noOptions (0);
causes a reserve of at least one CompOption to be constructed (and then quickly destructed)

The fix is to change:
    CompOption::Vector noOptions (0);
to:
    CompOption::Vector noOptions;

review: Needs Fixing

[Sam Spilsbury (smspillaz)]

> *** Found the problem ***
>
> CompOption::Vector noOptions (0);
> causes a reserve of at least one CompOption to be constructed (and then
> quickly destructed)
>
> The fix is to change:
> CompOption::Vector noOptions (0);
> to:
> CompOption::Vector noOptions;

It is still better to use the on-demand construction, for reasons specified above. I'll fix this though.

[Daniel van Vugt (vanvugt)]

This means we don't need to change the public API of "noOptions" or need to change any plugins. You're now doing it for stylistic reasons only. :)

[Sam Spilsbury (smspillaz)]

There is still a risk involved. I'd much prefer to have less risky code then to be tripped up later.

This isn't "purely stylistic"

[Sam Spilsbury (smspillaz)]

(Also as evidenced by the fact that PrivateAction was not even created during static initialization and destruction makes me more inclined to make it like this)

[Alan Griffiths (alan-griffiths)]

170 + CompString & emptyString ()

Rather invites:

emptyString () = CompString("Murphy rules!");

(The same applies to a lot of the functions, and the original [non-]constants.)

So, we should add const to the "constants" while addressing this problem.

On the comment discussion: Global data, monostate & singleton are all likely causes for issues. So this code could do with some cleanup regardless of "on demand" or "global data".

As Sam says, it is slightly safer to use "on demand" than global variables - as it ensures they have actually been initialized before use. OTOH, if we "know" there is no init-sequence issue that only makes the design marginally better.

So, I wouldn't have done the "wrapping" if the problem can be fixed more simply. But, if we had the code ready to go I'd take it, it make the codebase a safer place to live.

review: Needs Fixing

[Sam Spilsbury (smspillaz)]

I can remove all the global data, but I can tell you know that the diff is going to get a *lot* bigger (ditto for adding constness)

[Tim Penhey (thumper)]

Sam, if the change to the vector constructor is enough to fix this bug, we should go with the simplest possible branch.

[Tim Penhey (thumper)]

I have to admit to preferring to use the default constructor for vectors rather than trying to be clever with the size. It also means that people don't have to go and look for what the integer parameter constructor does.

[Alan Griffiths (alan-griffiths)]

"Don't initialize to list size zero since that creates a temporary and destroys it
(for gosh knows what reasons)"

That's because it calls a constructor with a default parameter. Vis:

CompOption::Vector(o, CompOption());

[Sam Spilsbury (smspillaz)]

I'd really like to know what all the push back on delaying initialization till use is, its a far safer way of doing things and doesn't have any obvious drawbacks.

[Tim Penhey (thumper)]

Please also change the constructor in noOptions too:

  static CompOption::Vector v (0);

should be:

  static CompOption::Vector v;

Given that there were also issues around the ordering of the static object initialisation that Sam said he saw, I'm in favour of landing this branch. However, we should look to clean the whole mess up soonish.

I want to know though what the impact is of making these constants.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Yes indeed on demand construction is more than stylistic. I think my clumsy words missed the point there. My point should have been that on demand construction (while being good for preventative maintenance), is causing a lot more code to change just before a release. And that's a risk.

[Daniel van Vugt (vanvugt)]

Conditionally approved, pending the 1-line fix Tim mentioned:
210 + static CompOption::Vector v (0);
becomes:
210 + static CompOption::Vector v;

I've also tested for further API breakage in libcompizconfig, compiz-plugins-main and compiz-plugins-extra. Only main (expo) failed, which Sam has already proposed a fix for.

review: Approve

[Sam Spilsbury (smspillaz)]

Impact of removing the const is that we need to remove these

CompAction &
CompOption::Value::action ()
{
    try
    {
 return boost::get<CompAction>(mValue);
    }
    catch (...)
    {
 return emptyAction ();
    }
}

CompMatch &
CompOption::Value::match ()
{
    try
    {
 return boost::get<CompMatch>(mValue);
    }
    catch (...)
    {
 return emptyMatch ();
    }
}

CompString &
CompOption::Value::s ()
{
    try
    {
 return boost::get < CompString > (mValue);
    }
    catch (...)
    {
 return emptyString ();
    }
}

which I am +1 for but I'm not sure how much client code uses these

[Daniel van Vugt (vanvugt)]

Satisfies my previous comment.

review: Approve

[Alan Griffiths (alan-griffiths)]

Reads OK.

Still concerned about the lack of const-safety, But willing to leave that for posterity.

review: Approve

=== modified file 'include/core/abiversion.h'
--- include/core/abiversion.h	2012-01-25 13:46:33 +0000
+++ include/core/abiversion.h	2012-02-20 08:16:21 +0000
@@ -5,6 +5,6 @@
 #  error Conflicting definitions of CORE_ABIVERSION
 #endif
 
-#define CORE_ABIVERSION 20120125
+#define CORE_ABIVERSION 20120216
 
 #endif // COMPIZ_ABIVERSION_H

=== modified file 'include/core/option.h'
--- include/core/option.h	2012-01-31 14:52:20 +0000
+++ include/core/option.h	2012-02-20 08:16:21 +0000
@@ -47,7 +47,8 @@
  * A configuration option with boolean, int, float, String, Color, Key, Button,
  * Edge, Bell, or List.
  */
-class CompOption {
+class CompOption
+{
 	/**
 	 * Option data types
 	 */
@@ -375,7 +376,6 @@
     set (t);
 }
 
-
-extern CompOption::Vector noOptions;
+CompOption::Vector & noOptions ();
 
 #endif

=== modified file 'include/core/plugin.h'
--- include/core/plugin.h	2012-02-01 17:49:07 +0000
+++ include/core/plugin.h	2012-02-20 08:16:21 +0000
@@ -268,7 +268,7 @@
 {
     CompOption::Class *oc = dynamic_cast<CompOption::Class *> (T::get (screen));
     if (!oc)
-	return noOptions;
+	return noOptions ();
     return oc->getOptions ();
 }
 
@@ -306,7 +306,7 @@
 {
     CompOption::Class *oc = dynamic_cast<CompOption::Class *> (T::get (screen));
     if (!oc)
-	return noOptions;
+	return noOptions ();
     return oc->getOptions ();
 }
 

=== modified file 'plugins/move/src/move.cpp'
--- plugins/move/src/move.cpp	2012-02-01 14:03:38 +0000
+++ plugins/move/src/move.cpp	2012-02-20 08:16:21 +0000
@@ -511,7 +511,7 @@
 		    {
 			moveTerminate (&optionGetInitiateButton (),
 				       CompAction::StateTermButton,
-				       noOptions);
+				       noOptions ());
 		    }
 		}
 	    }
@@ -606,9 +606,9 @@
 		    if (ms->w->id () == event->xclient.window)
 		    {
 			moveTerminate (&optionGetInitiateButton (),
-				       CompAction::StateCancel, noOptions);
+				       CompAction::StateCancel, noOptions ());
 			moveTerminate (&optionGetInitiateKey (),
-				       CompAction::StateCancel, noOptions);
+				       CompAction::StateCancel, noOptions ());
 
 		    }
 		}
@@ -617,15 +617,15 @@
 	case DestroyNotify:
 	    if (w && w->id () == event->xdestroywindow.window)
 	    {
-		moveTerminate (&optionGetInitiateButton (), 0, noOptions);
-		moveTerminate (&optionGetInitiateKey (), 0, noOptions);
+		moveTerminate (&optionGetInitiateButton (), 0, noOptions ());
+		moveTerminate (&optionGetInitiateKey (), 0, noOptions ());
 	    }
 	    break;
 	case UnmapNotify:
 	    if (w && w->id () == event->xunmap.window)
 	    {
-		moveTerminate (&optionGetInitiateButton (), 0, noOptions);
-		moveTerminate (&optionGetInitiateKey (), 0, noOptions);
+		moveTerminate (&optionGetInitiateButton (), 0, noOptions ());
+		moveTerminate (&optionGetInitiateKey (), 0, noOptions ());
 	    }
 	default:
 	    break;

=== modified file 'plugins/resize/src/resize.cpp'
--- plugins/resize/src/resize.cpp	2012-02-09 07:48:57 +0000
+++ plugins/resize/src/resize.cpp	2012-02-20 08:16:21 +0000
@@ -1259,7 +1259,7 @@
 			CompAction *action = &optionGetInitiateButton ();
 
 			resizeTerminate (action, CompAction::StateTermButton,
-					 noOptions);
+					 noOptions ());
 		    }
 		}
 	    }
@@ -1356,9 +1356,9 @@
 		    if (rs->w->id () == event->xclient.window)
 		    {
 			resizeTerminate (&optionGetInitiateButton (),
-					 CompAction::StateCancel, noOptions);
+					 CompAction::StateCancel, noOptions ());
 			resizeTerminate (&optionGetInitiateKey (),
-					 CompAction::StateCancel, noOptions);
+					 CompAction::StateCancel, noOptions ());
 		    }
 		}
 	    }
@@ -1366,15 +1366,15 @@
 	case DestroyNotify:
 	    if (w && w->id () == event->xdestroywindow.window)
 	    {
-		resizeTerminate (&optionGetInitiateButton (), 0, noOptions);
-		resizeTerminate (&optionGetInitiateKey (), 0, noOptions);
+		resizeTerminate (&optionGetInitiateButton (), 0, noOptions ());
+		resizeTerminate (&optionGetInitiateKey (), 0, noOptions ());
 	    }
 	    break;
 	case UnmapNotify:
 	    if (w && w->id () == event->xunmap.window)
 	    {
-		resizeTerminate (&optionGetInitiateButton (), 0, noOptions);
-		resizeTerminate (&optionGetInitiateKey (), 0, noOptions);
+		resizeTerminate (&optionGetInitiateButton (), 0, noOptions ());
+		resizeTerminate (&optionGetInitiateKey (), 0, noOptions ());
 	    }
 	default:
 	    break;

=== modified file 'src/event.cpp'
--- src/event.cpp	2012-02-17 09:17:31 +0000
+++ src/event.cpp	2012-02-20 08:16:21 +0000
@@ -306,7 +306,7 @@
 	    {
 		if (!o.value ().action ().terminate ().empty ())
 		    o.value ().action ().terminate () (&o.value ().action (),
-						       state, noOptions);
+						       state, noOptions ());
 	    }
 	}
 

=== modified file 'src/option.cpp'
--- src/option.cpp	2012-02-08 15:31:32 +0000
+++ src/option.cpp	2012-02-20 08:16:21 +0000
@@ -35,12 +35,47 @@
 #include <core/option.h>
 #include "privateoption.h"
 
-CompOption::Vector noOptions (0);
-CompOption::Value::Vector emptyList;
-CompString         emptyString;
-CompMatch          emptyMatch;
-CompAction         emptyAction;
-unsigned short     emptyColor[4];
+namespace
+{
+    CompOption::Value::Vector & emptyList ()
+    {
+	static CompOption::Value::Vector v;
+	return v;
+    }
+
+    CompString & emptyString ()
+    {
+	static CompString v;
+	return v;
+    }
+
+    CompMatch & emptyMatch ()
+    {
+	static CompMatch v;
+	return v;
+    }
+
+    CompAction & emptyAction ()
+    {
+	static CompAction v;
+	return v;
+    }
+
+    unsigned short * emptyColor ()
+    {
+	static unsigned short v[4] = { 0, 0, 0, 0 };
+	return v;
+    }
+}
+
+CompOption::Vector &
+noOptions ()
+{
+    static CompOption::Vector v;
+    return v;
+}
+
+
 
 static bool
 checkIsAction (CompOption::Type type)
@@ -59,32 +94,8 @@
     return false;
 }
 
-static void
-finiOptionValue (CompOption::Value &v)
-{
-    switch (v.type()) {
-	case CompOption::TypeAction:
-	case CompOption::TypeKey:
-	case CompOption::TypeButton:
-	case CompOption::TypeEdge:
-	case CompOption::TypeBell:
-	    if (v.action ().state () & CompAction::StateAutoGrab && screen)
-		screen->removeAction (&v.action ());
-	    break;
-
-	case CompOption::TypeList:
-	    foreach (CompOption::Value &val, v.list ())
-		finiOptionValue (val);
-	    break;
-
-	default:
-	    break;
-    }
-}
-
 CompOption::Value::~Value()
 {
-    finiOptionValue(*this);
 }
 
 void
@@ -142,7 +153,7 @@
     }
     catch (...)
     {
-	return emptyColor;
+	return emptyColor ();
     }
 }
 
@@ -155,7 +166,7 @@
     }
     catch (...)
     {
-	return emptyString;
+	return emptyString ();
     }
 }
 
@@ -168,7 +179,7 @@
     }
     catch (...)
     {
-	return emptyString;
+	return emptyString ();
     }
 }
 
@@ -181,7 +192,7 @@
     }
     catch (...)
     {
-	return emptyMatch;
+	return emptyMatch ();
     }
 }
 
@@ -194,7 +205,7 @@
     }
     catch (...)
     {
-	return emptyMatch;
+	return emptyMatch ();
     }
 }
 
@@ -207,7 +218,7 @@
     }
     catch (...)
     {
-	return emptyAction;
+	return emptyAction ();
     }
 }
 
@@ -220,7 +231,7 @@
     }
     catch (...)
     {
-	return emptyAction;
+	return emptyAction ();
     }
 }
 
@@ -235,7 +246,7 @@
     }
     catch (...)
     {
-	return emptyList;
+	return emptyList ();
     }
 }
 
@@ -248,7 +259,7 @@
     }
     catch (...)
     {
-	return emptyList;
+	return emptyList ();
     }
 }
 
@@ -416,33 +427,20 @@
     setName (name, type);
 }
 
-static void
-finiScreenOptionValue (CompScreen        *s,
-		       CompOption::Value &v)
-{
-    switch (v.type()) {
-	case CompOption::TypeAction:
-	case CompOption::TypeKey:
-	case CompOption::TypeButton:
-	case CompOption::TypeEdge:
-	case CompOption::TypeBell:
-	    if (v.action ().state () & CompAction::StateAutoGrab)
-		s->removeAction (&v.action ());
-	    break;
-
-	case CompOption::TypeList:
-	    foreach (CompOption::Value &val, v.list ())
-		finiScreenOptionValue (s, val);
-	    break;
-
-	default:
-	    break;
-    }
-}
-
 CompOption::~CompOption ()
 {
-    finiOptionValue (priv->value);
+    /* Remove any added actions */
+    try
+    {
+	CompAction &action = value ().action ();
+
+	if (action.active () && screen)
+	    screen->removeAction (&action);
+    }
+    catch (...)
+    {
+    }
+
     delete priv;
 }
 

=== modified file 'src/plugin.cpp'
--- src/plugin.cpp	2012-01-31 17:01:32 +0000
+++ src/plugin.cpp	2012-02-20 08:16:21 +0000
@@ -661,7 +661,7 @@
 CompOption::Vector &
 CompPlugin::VTable::getOptions ()
 {
-    return noOptions;
+    return noOptions ();
 }
 
 bool

=== modified file 'src/session.cpp'
--- src/session.cpp	2012-01-19 18:12:31 +0000
+++ src/session.cpp	2012-02-20 08:16:21 +0000
@@ -220,7 +220,7 @@
 dieCallback (SmcConn   connection,
 	     SmPointer clientData)
 {
-    screen->sessionEvent (CompSession::EventDie, noOptions);
+    screen->sessionEvent (CompSession::EventDie, noOptions ());
 
     CompSession::close ();
     exit (0);
@@ -230,14 +230,14 @@
 saveCompleteCallback (SmcConn	connection,
 		      SmPointer clientData)
 {
-    screen->sessionEvent (CompSession::EventSaveComplete, noOptions);
+    screen->sessionEvent (CompSession::EventSaveComplete, noOptions ());
 }
 
 static void
 shutdownCancelledCallback (SmcConn   connection,
 			   SmPointer clientData)
 {
-    screen->sessionEvent (CompSession::EventShutdownCancelled, noOptions);
+    screen->sessionEvent (CompSession::EventShutdownCancelled, noOptions ());
 }
 
 void