Merge lp:~alan-griffiths/compiz-core/init-sequence-fix into lp:compiz-core/0.9.5

[Alan Griffiths]

CompManager::init() creates a CompScreen object (and will, after construction, store it's address in ::screen).
In the constructor of CompScreen a PrivateScreen object is constructed. This has a base subobject of type CoreOptions.
The constructor of CoreOptions NO LONGER dereferences the ::screen variable (to call CompScreen::addAction).
Calling a member function via an invalid (0 in this case) pointer was "undefined behaviour".

[Sam Spilsbury (smspillaz)]

29 - <xsl:text> screen->addAction (&amp;</xsl:text>
30 + <xsl:text> if (screen) screen->addAction (&amp;</xsl:text>
31 <xsl:text>mOptions[</xsl:text>

Hrmm. Not sure if I like the idea of doing nul checks just to handle the startup case in CoreOptions.

To be honest, the option code shouldn't really be initialized until a plugin which is in charge of doing all the settings is loaded (eg, ccp). I would say it might be more appropriate here to allow *Options classes to have a default parameter in their constructor which indicates that they would rather have the options code initialized lazily, eg, in the bcop generated files, have a static bool initialized = false; variable and then atomically initialize the options whenever they are first written to, accessed or an action is bound.

That gains us a bit of startup time too, if we do it like that. Not really sure what the cost of having a once-only section optionGet* is, though I'm pretty sure there are compiler hints to deal with that no ?

[Alan Griffiths (alan-griffiths)]

Sam, I didn't want to change the runtime behaviour in the way you suggest.

The result is a workaround true, but the prior workaround was to test a flag inside the addAction member function (which was already in "undefined behaviour" land).

There are several issues that could be addressed in a larger fix:

1. I'd prefer to avoid the need for tests by ensuring the initialisation happens in a sane sequence (i.e. "screen" is intialised before CoreOptions is constructed).

2. I don't like having the global "screen" variable at all - objects that need a pointer to the CompScreen object should have it supplied on construction.

[Sam Spilsbury (smspillaz)]

> Sam, I didn't want to change the runtime behaviour in the way you suggest.
>
> The result is a workaround true, but the prior workaround was to test a flag
> inside the addAction member function (which was already in "undefined
> behaviour" land).

So on the count of "its undefined behaviour", I'd be happy to merge this anyways.

>
> There are several issues that could be addressed in a larger fix:
>
> 1. I'd prefer to avoid the need for tests by ensuring the initialisation
> happens in a sane sequence (i.e. "screen" is intialised before CoreOptions is
> constructed).
>
> 2. I don't like having the global "screen" variable at all - objects that need
> a pointer to the CompScreen object should have it supplied on construction.

This is probably a lot more feasible than it looks - all plugins are already passed a CompScreen on construction (DI), it is trivial for them to use that. As for the other objects - all windows can store a pointer to the CompScreen object and then the other stuff is also easy, although we really, for things like actions we should just be passing them interfaces for ActionStorage or whatever and the implementation is CompScreen (for now).

[Alan Griffiths (alan-griffiths)]

> > 2. I don't like having the global "screen" variable at all - objects that
> need
> > a pointer to the CompScreen object should have it supplied on construction.
>
> This is probably a lot more feasible than it looks - all plugins are already
> passed a CompScreen on construction (DI), it is trivial for them to use that.
> As for the other objects - all windows can store a pointer to the CompScreen
> object and then the other stuff is also easy, although we really, for things
> like actions we should just be passing them interfaces for ActionStorage or
> whatever and the implementation is CompScreen (for now).

Yeah, I'll get to that in a later fix.

=== modified file 'src/screen.cpp'
--- src/screen.cpp	2012-01-29 16:28:45 +0000
+++ src/screen.cpp	2012-02-01 10:20:28 +0000
@@ -3196,7 +3196,7 @@
 bool
 CompScreen::addAction (CompAction *action)
 {
-    if (!screenInitalized || !priv->initialized)
+    if (!priv->initialized)
 	return false;
 
     if (action->active ())
@@ -4333,7 +4333,6 @@
     CompPlugin  *corePlugin;
 
     priv = new PrivateScreen (this);
-    assert (priv);
 
     screenInitalized = true;
 

=== modified file 'xslt/bcop.xslt'
--- xslt/bcop.xslt	2012-01-24 08:24:18 +0000
+++ xslt/bcop.xslt	2012-02-01 10:20:28 +0000
@@ -1272,7 +1272,7 @@
 			</xsl:with-param>
                     </xsl:call-template>
                     <xsl:if test="not (./passive_grab/text() = 'false')">
-                        <xsl:text>    screen->addAction (&amp;</xsl:text>
+                        <xsl:text>    if (screen) screen->addAction (&amp;</xsl:text>
                         <xsl:text>mOptions[</xsl:text>
 	                <xsl:call-template name="printOptionsEnumName"/>
 	                <xsl:text>].value ().action ());
@@ -1288,7 +1288,7 @@
 			</xsl:with-param>
                     </xsl:call-template>
                     <xsl:if test="not (./passive_grab/text() = 'false')">
-                        <xsl:text>    screen->addAction (&amp;</xsl:text>
+                        <xsl:text>    if (screen) screen->addAction (&amp;</xsl:text>
                         <xsl:text>mOptions[</xsl:text>
 	                <xsl:call-template name="printOptionsEnumName"/>
                         <xsl:text>].value ().action ());
@@ -1304,7 +1304,7 @@
 			</xsl:with-param>
                     </xsl:call-template>
                     <xsl:if test="not (./passive_grab/text() = 'false')">
-                        <xsl:text>    screen->addAction (&amp;</xsl:text>
+                        <xsl:text>    if (screen) screen->addAction (&amp;</xsl:text>
                         <xsl:text>mOptions[</xsl:text>
 	                <xsl:call-template name="printOptionsEnumName"/>
                         <xsl:text>].value ().action ());
@@ -1320,7 +1320,7 @@
 			</xsl:with-param>
                     </xsl:call-template>
                     <xsl:if test="not (./passive_grab/text() = 'false')">
-                        <xsl:text>    screen->addAction (&amp;</xsl:text>
+                        <xsl:text>    if (screen) screen->addAction (&amp;</xsl:text>
                         <xsl:text>mOptions[</xsl:text>
 	                <xsl:call-template name="printOptionsEnumName"/>
                         <xsl:text>].value ().action ());