Merge lp:~alan-griffiths/compiz-core/rework-updatePlugins into lp:compiz-core

[Alan Griffiths]

Avoid graphics corruption and hangs on compiz start-up by ensuring that
plugins don't get initialized, de-initialized and re-initialzed during
start-up. (LP: #963093) (LP: #963633)

The multiple init/fini/init calls occured when compiz was asked to load an
invalid plugin name. This occurred most recently as plugins "bailer" and
"detection" were left in some peoples' configs while the plugins themselves
no longer exist in the current compiz release.

The source of the graphics corruption and hangs has been found to be a
problem in the composite and/or opengl plugins. One or both of them are unsafe
to init/fini multiple times without a full compiz restart. So the root cause
is not exactly known yet. However composite and opengl are not alone; many
plugins have bugs with init/fini/init sequences, so it is valuable to fix
the start-up plugin ordering as this does.

Essentially this fix works by remembering which plugins don't exist at all
and putting them on a black list. Then subsequent updates check the blacklist
and know they should never include those failed plugins in testing whether
the active plugin lists have changed.

The final part of the fix is to remove a rendundant call to updatePlugins from
EventManager::init. It is not required as main tells PluginManager when to
load plugins on startup.

IMPORTANT NOTE FOR UBUNTU PACKAGING
In downstream ubuntu branches the DEFAULT_PLUGINS list in "debian/rules" also causes multiple plugin loads on start-up and prevents this fix from working! The solution is to change DEFAULT_PLUGINS to just "ccp".

ORIGINAL DESCRIPTION:
Some test cases to cover what we need.

Combined Daniel's "blacklist" with some cleanup of the code and ignoring any non-existent plugins entirely.

[Daniel van Vugt (vanvugt)]

Re: "Just because some plugins are "popped" to insert a new one, doesn't require them to be finalized."
This is untrue. Some plugins have loose dependencies on others and change their behaviour/features depending on what other plugins are loaded. Hence popped plugins must be fini'd and reinitialized.

Also, even if this branch works perfectly I would still be unhappy with the risk associated with such a large change in critical logic. Please don't hate me...

I'm looking at a much smaller/simpler solution. In fact I was designing it in my head as you designed this code. Will let you know if/when I think I have something that works.

In other news, the two critical bugs we're aiming to fix have had ZERO duplicates in the past two days. It sounds like the workarounds are working.

review: Abstain

[Daniel van Vugt (vanvugt)]

I've tested it now. The big startup bugs are avoided, however it causes a regression: Disabling plugins in ccsm has no effect. Those plugins remain active and running even after they are removed from the active_plugins list. That's one reason why I proposed the new logging last night. It's useful :)

Arguably, it's an acceptable compromise to fix the critical issues and introduce a less severe bug. However the the size of the new code in updatePlugins still makes me a little scared. Also, I managed an equally successful hack yesterday with just a few lines changed, but similar regressions.

[Daniel van Vugt (vanvugt)]

Replaced by: https://code.launchpad.net/~vanvugt/compiz-core/fix-startup-plugins/+merge/99886
Which has no such regressions. And is smaller, simpler.

review: Disapprove

[Daniel van Vugt (vanvugt)]

Manual testing in precise is all good. The bugs are fixed.

But...
1. Please don't change formatting unnecessarily from func (x) to func(x). I agree with the latter but it's not what compiz uses.
2. You alternative between both styles inconsistently: func (x) and func(x)
3. The critical bugs this branch fixes are not mentioned anywhere and we don't have a usable commit message. But I can add those.

review: Approve

[Daniel van Vugt (vanvugt)]

Also, please try to avoid renaming variables (even though you're right) this late in a stable release cycle. It increases the sizes of diffs and makes critical bug fixes harder to review.

[Daniel van Vugt (vanvugt)]

Updated the description and linked relevant bugs.

[Daniel van Vugt (vanvugt)]

Also, repeatedly calling: desiredPlugins[desireIndex].s ()
is inefficient and harder to read.

We should just do: const CompString &name = desiredPlugins[desireIndex].s ();
and then use "name".

[Daniel van Vugt (vanvugt)]

Damn. It did contain real conflicts, but I've fixed them.

[lp:~alan-griffiths/compiz-core/rework-updatePlugins]

3075. By Alan Griffiths on 2012-03-30

Avoid graphics corruption and hangs on compiz start-up by ensuring that
plugins don't get initialized, de-initialized and re-initialzed during
start-up. (LP: #963093) (LP: #963633)

The multiple init/fini/init calls occured when compiz was asked to load an
invalid plugin name. This occurred most recently as plugins "bailer" and
"detection" were left in some peoples' configs while the plugins themselves
no longer exist in the current compiz release.

The source of the graphics corruption and hangs has been found to be a
problem in the composite and/or opengl plugins. One or both of them are unsafe
to init/fini multiple times without a full compiz restart. So the root cause
is not exactly known yet. However composite and opengl are not alone; many
plugins have bugs with init/fini/init sequences, so it is valuable to fix
the start-up plugin ordering as this does.

Essentially this fix works by remembering which plugins don't exist at all
and putting them on a black list. Then subsequent updates check the blacklist
and know they should never include those failed plugins in testing whether
the active plugin lists have changed.

The final part of the fix is to remove a rendundant call to updatePlugins from
EventManager::init. It is not required as main tells PluginManager when to
load plugins on startup.

IMPORTANT NOTE FOR UBUNTU PACKAGING
In downstream ubuntu branches the DEFAULT_PLUGINS list in "debian/rules" also
causes multiple plugin loads on start-up and prevents this fix from working!
The solution is to change DEFAULT_PLUGINS to just "ccp".

=== modified file 'include/core/plugin.h'
--- include/core/plugin.h	2012-02-16 05:31:28 +0000
+++ include/core/plugin.h	2012-03-29 17:16:19 +0000
@@ -207,7 +207,7 @@
 	 * CompPlugin::pop()'d.
 	 */
 	static List & getPlugins ();
-	
+
 	/**
 	 * Gets a list of the names of all the known plugins, including plugins that may
 	 * have already been loaded.

=== modified file 'src/privatescreen.h'
--- src/privatescreen.h	2012-03-24 09:49:30 +0000
+++ src/privatescreen.h	2012-03-29 17:16:19 +0000
@@ -45,6 +45,8 @@
 
 #include "core_options.h"
 
+#include <set>
+
 /**
  * A wrapping of the X display screen. This takes care of communication to the
  * X server.
@@ -574,6 +576,10 @@
     private:
 	CompOption::Value plugin;
 	bool	          dirtyPluginList;
+	typedef std::set<CompString> CompStringSet;
+	CompStringSet blacklist;
+
+	CompOption::Value::Vector mergedPluginList();
 };
 
 class GrabList

=== modified file 'src/privatescreen/tests/test-privatescreen.cpp'
--- src/privatescreen/tests/test-privatescreen.cpp	2012-03-29 13:33:39 +0000
+++ src/privatescreen/tests/test-privatescreen.cpp	2012-03-29 17:16:19 +0000
@@ -175,6 +175,29 @@
     MOCK_CONST_METHOD0(createFailed, bool ());
 };
 
+class StubActivePluginsOption
+{
+public:
+    StubActivePluginsOption(CoreOptions& co) : co(co)
+    {
+	CompOption::Vector& mOptions = co.getOptions ();
+	CompOption::Value::Vector list;
+	CompOption::Value value;
+
+	// active_plugins
+	mOptions[CoreOptions::ActivePlugins].setName ("active_plugins", CompOption::TypeList);
+	list.clear ();
+	value.set(CompString ("core"));
+	list.push_back (value);
+    }
+
+    bool setActivePlugins(const char*, const char* key, CompOption::Value & value)
+    {
+	return co.setOption(key, value);
+    }
+private:
+    CoreOptions& co;
+};
 } // (anon) namespace
 
 namespace {
@@ -254,6 +277,17 @@
 	}
 	return true;
     }
+
+    CompStringList mockListPlugins (const char *path)
+    {
+	CompStringList list;
+	list.push_back("one");
+	list.push_back("two");
+	list.push_back("three");
+	list.push_back("four");
+
+	return list;
+    }
 };
 
 
@@ -349,19 +383,320 @@
 	WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
     EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("two"))).
 	WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
-    EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("three"))).
-	WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
-
-    EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(1);  // Once for "three" which doesn't load
-
-    EXPECT_CALL(comp_screen, _setOptionForPlugin(StrEq("core"), StrEq("active_plugins"), _)).
-	    WillOnce(Return(false));
-
-    ps.updatePlugins();
-
-    // TODO Some cleanup that probably ought to be automatic.
-    EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(2);
-    EXPECT_CALL(comp_screen, _finiPluginForScreen(Ne((void*)0))).Times(2);
+<<<<<<< TREE
+    EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("three"))).
+	WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
+
+    EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(1);  // Once for "three" which doesn't load
+
+    EXPECT_CALL(comp_screen, _setOptionForPlugin(StrEq("core"), StrEq("active_plugins"), _)).
+	    WillOnce(Return(false));
+
+    ps.updatePlugins();
+
+    // TODO Some cleanup that probably ought to be automatic.
+    EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(2);
+    EXPECT_CALL(comp_screen, _finiPluginForScreen(Ne((void*)0))).Times(2);
+=======
+    EXPECT_CALL(mockfs.mockVtableTwo, init()).WillOnce(Return(true));
+
+    EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("three"))).
+	WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
+    EXPECT_CALL(mockfs.mockVtableThree, init()).WillOnce(Return(false));
+
+    EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(1);  // Once for "three" which doesn't load
+
+    EXPECT_CALL(comp_screen, _setOptionForPlugin(StrEq("core"), StrEq("active_plugins"), _)).
+	    WillOnce(Return(false));
+
+    EXPECT_CALL(mockfs, ListPlugins(_)).
+	WillRepeatedly(Invoke(&mockfs, &MockPluginFilesystem::mockListPlugins));
+
+    ps.updatePlugins();
+
+    Mock::VerifyAndClearExpectations(&mockfs);
+    Mock::VerifyAndClearExpectations(&mockfs.mockVtableOne);
+    Mock::VerifyAndClearExpectations(&mockfs.mockVtableTwo);
+    Mock::VerifyAndClearExpectations(&mockfs.mockVtableThree);
+    Mock::VerifyAndClearExpectations(&mockfs.mockVtableFour);
+
+    // TODO Some cleanup that probably ought to be automatic.
+    EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(2);
+    EXPECT_CALL(comp_screen, _finiPluginForScreen(Ne((void*)0))).Times(2);
+    EXPECT_CALL(mockfs.mockVtableOne, finiScreen(Ne((void*)0))).Times(1);
+    EXPECT_CALL(mockfs.mockVtableOne, fini()).Times(1);
+    EXPECT_CALL(mockfs.mockVtableTwo, finiScreen(Ne((void*)0))).Times(1);
+    EXPECT_CALL(mockfs.mockVtableTwo, fini()).Times(1);
+
+    for (CompPlugin* p; (p = CompPlugin::pop ()) != 0; CompPlugin::unload (p));
+}
+
+TEST(privatescreen_PluginManagerTest, updating_when_failing_to_load_plugin_in_middle_of_list)
+{
+    using namespace testing;
+
+    MockCompScreen comp_screen;
+
+    cps::PluginManager ps(&comp_screen);
+    StubActivePluginsOption sapo(ps);
+
+    CompOption::Value::Vector values;
+    values.push_back ("core");
+    ps.setPlugins (values);
+    ps.setDirtyPluginList ();
+
+    std::list <CompString> plugins;
+    plugins.push_back ("one");
+    plugins.push_back ("three");
+    plugins.push_back ("four");
+    initialPlugins = plugins;
+
+    MockPluginFilesystem mockfs;
+
+    EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("one"))).
+	WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
+    EXPECT_CALL(mockfs.mockVtableOne, init()).WillOnce(Return(true));
+
+    EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("three"))).
+	WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
+    EXPECT_CALL(mockfs.mockVtableThree, init()).WillOnce(Return(false));
+
+    EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("four"))).
+	WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
+    EXPECT_CALL(mockfs.mockVtableFour, init()).Times(1).WillRepeatedly(Return(true));
+
+    EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(1);  // Once for "three" which doesn't load
+
+    EXPECT_CALL(comp_screen, _setOptionForPlugin(StrEq("core"), StrEq("active_plugins"), _)).
+	    WillOnce(Return(true));
+
+    EXPECT_CALL(mockfs, ListPlugins(_)).
+	WillRepeatedly(Invoke(&mockfs, &MockPluginFilesystem::mockListPlugins));
+
+    ps.updatePlugins();
+
+    Mock::VerifyAndClearExpectations(&mockfs);
+    Mock::VerifyAndClearExpectations(&mockfs.mockVtableOne);
+    Mock::VerifyAndClearExpectations(&mockfs.mockVtableTwo);
+    Mock::VerifyAndClearExpectations(&mockfs.mockVtableThree);
+    Mock::VerifyAndClearExpectations(&mockfs.mockVtableFour);
+
+    EXPECT_CALL(comp_screen, _setOptionForPlugin(StrEq("core"), StrEq("active_plugins"), _)).
+	    WillOnce(Invoke(&sapo, &StubActivePluginsOption::setActivePlugins));
+
+    EXPECT_CALL(mockfs, ListPlugins(_)).
+	WillRepeatedly(Invoke(&mockfs, &MockPluginFilesystem::mockListPlugins));
+
+    ps.updatePlugins();
+
+    Mock::VerifyAndClearExpectations(&mockfs);
+    Mock::VerifyAndClearExpectations(&mockfs.mockVtableOne);
+    Mock::VerifyAndClearExpectations(&mockfs.mockVtableTwo);
+    Mock::VerifyAndClearExpectations(&mockfs.mockVtableThree);
+    Mock::VerifyAndClearExpectations(&mockfs.mockVtableFour);
+
+    // TODO Some cleanup that probably ought to be automatic.
+    EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(2);
+    EXPECT_CALL(comp_screen, _finiPluginForScreen(Ne((void*)0))).Times(2);
+    EXPECT_CALL(mockfs.mockVtableOne, finiScreen(Ne((void*)0))).Times(1);
+    EXPECT_CALL(mockfs.mockVtableOne, fini()).Times(1);
+    EXPECT_CALL(mockfs.mockVtableFour, finiScreen(Ne((void*)0))).Times(1);
+    EXPECT_CALL(mockfs.mockVtableFour, fini()).Times(1);
+    for (CompPlugin* p; (p = CompPlugin::pop ()) != 0; CompPlugin::unload (p));
+}
+
+TEST(privatescreen_PluginManagerTest, calling_updatePlugins_with_fewer_plugins)
+{
+    using namespace testing;
+
+    MockCompScreen comp_screen;
+
+    cps::PluginManager ps(&comp_screen);
+
+    StubActivePluginsOption sapo(ps);
+
+    // Stuff that has to be done before calling updatePlugins()
+    initialPlugins = std::list <CompString>();
+    CompOption::Value::Vector values;
+    values.push_back ("core");
+    ps.setPlugins (values);
+    ps.setDirtyPluginList ();
+
+    {
+	CompOption::Value::Vector plugins;
+	plugins.push_back ("one");
+	plugins.push_back ("two");
+	plugins.push_back ("three");
+	CompOption::Value v(plugins);
+	sapo.setActivePlugins("core", "active_plugins", v);
+    }
+
+    MockPluginFilesystem mockfs;
+
+    EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("one"))).
+	WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
+    EXPECT_CALL(mockfs.mockVtableOne, init()).WillOnce(Return(true));
+
+    EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("two"))).
+	WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
+    EXPECT_CALL(mockfs.mockVtableTwo, init()).WillOnce(Return(true));
+
+    EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("three"))).
+	WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
+    EXPECT_CALL(mockfs.mockVtableThree, init()).WillOnce(Return(true));
+
+    EXPECT_CALL(comp_screen, _setOptionForPlugin(StrEq("core"), StrEq("active_plugins"), _)).
+	    WillOnce(Invoke(&sapo, &StubActivePluginsOption::setActivePlugins));
+
+    EXPECT_CALL(mockfs, ListPlugins(_)).
+	WillRepeatedly(Invoke(&mockfs, &MockPluginFilesystem::mockListPlugins));
+
+    ps.updatePlugins();
+
+    Mock::VerifyAndClearExpectations(&mockfs);
+    Mock::VerifyAndClearExpectations(&mockfs.mockVtableOne);
+    Mock::VerifyAndClearExpectations(&mockfs.mockVtableTwo);
+    Mock::VerifyAndClearExpectations(&mockfs.mockVtableThree);
+    Mock::VerifyAndClearExpectations(&mockfs.mockVtableFour);
+
+    EXPECT_CALL(comp_screen, _finiPluginForScreen(Ne((void*)0))).Times(2);
+    EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(1);
+    EXPECT_CALL(mockfs.mockVtableTwo, finiScreen(Ne((void*)0))).Times(1);
+    EXPECT_CALL(mockfs.mockVtableTwo, fini()).Times(1);
+    EXPECT_CALL(mockfs.mockVtableThree, fini()).Times(1);
+    EXPECT_CALL(mockfs.mockVtableThree, finiScreen(Ne((void*)0))).Times(1);
+    EXPECT_CALL(mockfs.mockVtableThree, init()).WillOnce(Return(true));
+    EXPECT_CALL(comp_screen, _setOptionForPlugin(StrEq("core"), StrEq("active_plugins"), _)).
+	    WillOnce(Invoke(&sapo, &StubActivePluginsOption::setActivePlugins));
+
+    {
+	CompOption::Value::Vector plugins;
+	plugins.push_back ("one");
+	plugins.push_back ("three");
+	CompOption::Value v(plugins);
+	sapo.setActivePlugins("core", "active_plugins", v);
+    }
+
+    EXPECT_CALL(mockfs, ListPlugins(_)).
+	WillRepeatedly(Invoke(&mockfs, &MockPluginFilesystem::mockListPlugins));
+
+    ps.updatePlugins();
+
+    Mock::VerifyAndClearExpectations(&mockfs);
+    Mock::VerifyAndClearExpectations(&mockfs.mockVtableOne);
+    Mock::VerifyAndClearExpectations(&mockfs.mockVtableTwo);
+    Mock::VerifyAndClearExpectations(&mockfs.mockVtableThree);
+    Mock::VerifyAndClearExpectations(&mockfs.mockVtableFour);
+
+    // TODO Some cleanup that probably ought to be automatic.
+    EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(2);
+    EXPECT_CALL(comp_screen, _finiPluginForScreen(Ne((void*)0))).Times(2);
+    EXPECT_CALL(mockfs.mockVtableOne, finiScreen(Ne((void*)0))).Times(1);
+    EXPECT_CALL(mockfs.mockVtableOne, fini()).Times(1);
+    EXPECT_CALL(mockfs.mockVtableThree, finiScreen(Ne((void*)0))).Times(1);
+    EXPECT_CALL(mockfs.mockVtableThree, fini()).Times(1);
+
+    for (CompPlugin* p; (p = CompPlugin::pop ()) != 0; CompPlugin::unload (p));
+}
+
+TEST(privatescreen_PluginManagerTest, calling_updatePlugins_with_additional_plugins)
+{
+    using namespace testing;
+
+    MockCompScreen comp_screen;
+
+    cps::PluginManager ps(&comp_screen);
+
+    StubActivePluginsOption sapo(ps);
+
+    // Stuff that has to be done before calling updatePlugins()
+    initialPlugins = std::list <CompString>();
+    CompOption::Value::Vector values;
+    values.push_back ("core");
+    ps.setPlugins (values);
+    ps.setDirtyPluginList ();
+
+    {
+	CompOption::Value::Vector plugins;
+	plugins.push_back ("one");
+	plugins.push_back ("two");
+	plugins.push_back ("four");
+	CompOption::Value v(plugins);
+	sapo.setActivePlugins("core", "active_plugins", v);
+    }
+
+    MockPluginFilesystem mockfs;
+
+    EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("one"))).
+	WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
+    EXPECT_CALL(mockfs.mockVtableOne, init()).WillOnce(Return(true));
+
+    EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("two"))).
+	WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
+    EXPECT_CALL(mockfs.mockVtableTwo, init()).WillOnce(Return(true));
+
+    EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("four"))).
+	WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
+    EXPECT_CALL(mockfs.mockVtableFour, init()).WillOnce(Return(true));
+
+    EXPECT_CALL(comp_screen, _setOptionForPlugin(StrEq("core"), StrEq("active_plugins"), _)).
+	    WillOnce(Invoke(&sapo, &StubActivePluginsOption::setActivePlugins));
+
+    EXPECT_CALL(mockfs, ListPlugins(_)).
+	WillRepeatedly(Invoke(&mockfs, &MockPluginFilesystem::mockListPlugins));
+
+    ps.updatePlugins();
+
+    Mock::VerifyAndClearExpectations(&mockfs);
+    Mock::VerifyAndClearExpectations(&mockfs.mockVtableOne);
+    Mock::VerifyAndClearExpectations(&mockfs.mockVtableTwo);
+    Mock::VerifyAndClearExpectations(&mockfs.mockVtableThree);
+    Mock::VerifyAndClearExpectations(&mockfs.mockVtableFour);
+
+    EXPECT_CALL(comp_screen, _finiPluginForScreen(Ne((void*)0))).Times(1);
+    EXPECT_CALL(mockfs.mockVtableFour, fini()).Times(1);
+    EXPECT_CALL(mockfs.mockVtableFour, finiScreen(Ne((void*)0))).Times(1);
+    EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("three"))).
+	WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
+    EXPECT_CALL(mockfs.mockVtableThree, init()).WillOnce(Return(true));
+    EXPECT_CALL(mockfs.mockVtableFour, init()).WillOnce(Return(true));
+    EXPECT_CALL(comp_screen, _setOptionForPlugin(StrEq("core"), StrEq("active_plugins"), _)).
+	    WillOnce(Invoke(&sapo, &StubActivePluginsOption::setActivePlugins));
+
+    {
+	CompOption::Value::Vector plugins;
+	plugins.push_back ("one");
+	plugins.push_back ("two");
+	plugins.push_back ("three");
+	plugins.push_back ("four");
+	CompOption::Value v(plugins);
+	sapo.setActivePlugins("core", "active_plugins", v);
+    }
+
+    EXPECT_CALL(mockfs, ListPlugins(_)).
+	WillRepeatedly(Invoke(&mockfs, &MockPluginFilesystem::mockListPlugins));
+
+    ps.updatePlugins();
+
+    Mock::VerifyAndClearExpectations(&mockfs);
+    Mock::VerifyAndClearExpectations(&mockfs.mockVtableOne);
+    Mock::VerifyAndClearExpectations(&mockfs.mockVtableTwo);
+    Mock::VerifyAndClearExpectations(&mockfs.mockVtableThree);
+    Mock::VerifyAndClearExpectations(&mockfs.mockVtableFour);
+
+    // TODO Some cleanup that probably ought to be automatic.
+    EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(4);
+    EXPECT_CALL(comp_screen, _finiPluginForScreen(Ne((void*)0))).Times(4);
+    EXPECT_CALL(mockfs.mockVtableFour, finiScreen(Ne((void*)0))).Times(1);
+    EXPECT_CALL(mockfs.mockVtableFour, fini()).Times(1);
+    EXPECT_CALL(mockfs.mockVtableThree, finiScreen(Ne((void*)0))).Times(1);
+    EXPECT_CALL(mockfs.mockVtableThree, fini()).Times(1);
+    EXPECT_CALL(mockfs.mockVtableTwo, finiScreen(Ne((void*)0))).Times(1);
+    EXPECT_CALL(mockfs.mockVtableTwo, fini()).Times(1);
+    EXPECT_CALL(mockfs.mockVtableOne, finiScreen(Ne((void*)0))).Times(1);
+    EXPECT_CALL(mockfs.mockVtableOne, fini()).Times(1);
+
+>>>>>>> MERGE-SOURCE
     for (CompPlugin* p; (p = CompPlugin::pop ()) != 0; CompPlugin::unload (p));
 }
 
@@ -398,5 +733,9 @@
     comp_screen.priv.reset(new PrivateScreen(&comp_screen));
     cps::EventManager& em(*comp_screen.priv.get());
 
+<<<<<<< TREE
+=======
+    em.setPlugins (values);
+>>>>>>> MERGE-SOURCE
     em.init(0);
 }

=== modified file 'src/screen.cpp'
--- src/screen.cpp	2012-03-24 09:49:30 +0000
+++ src/screen.cpp	2012-03-29 17:16:19 +0000
@@ -893,83 +893,45 @@
     }
 }
 
-void
-cps::PluginManager::updatePlugins ()
+CompOption::Value::Vector
+cps::PluginManager::mergedPluginList ()
 {
-    unsigned int pListCount = 1;
-
-    possibleTap = NULL;
-    dirtyPluginList = false;
-
-    CompOption::Value::Vector &list = optionGetActivePlugins ();
-
-    /* Determine the number of plugins, which is core +
-     * initial plugins + plugins in option list in addition
-     * to initial plugins */
-    foreach (CompString &pn, initialPlugins)
-    {
-	if (pn != "core")
-	    pListCount++;
-    }
-
-    CompString lpName;
-	
-    foreach (CompOption::Value &lp, list)
-    {
-	bool skip = false;
-	
-	lpName = lp.s();
-	
-	if (lpName == "core")
-	    continue;
-
-	foreach (CompString &p, initialPlugins)
-	{
-	    if (p == lpName)
-	    {
-		skip = true;
-		break;
-	    }
-	}
-
-	/* plugin not in initial list */
-	if (!skip)
-	    pListCount++;
-    }
-
-    /* pListCount is now the number of plugisn contained in both the
-     * initial and new plugins list */
-    CompOption::Value::Vector pList(pListCount);
-
-    if (pList.empty ())
-    {
-	screen->setOptionForPlugin ("core", "active_plugins", plugin);
-	return;
-    }
+    std::list<CompString> availablePlugins(CompPlugin::availablePlugins ());
+
+    CompOption::Value::Vector result;
+
+    CompOption::Value::Vector const& extraPluginsRequested = optionGetActivePlugins();
 
     /* Must have core as first plugin */
-    pList.at (0) = "core";
-    unsigned int j = 1;
+    result.push_back("core");
 
     /* Add initial plugins */
-    foreach (CompString &p, initialPlugins)
+    foreach(CompString & p, initialPlugins)
     {
 	if (p == "core")
 	    continue;
-	pList.at (j).set (p);
-	j++;
+
+	if (blacklist.find (p) != blacklist.end ())
+	    continue;
+
+	if (availablePlugins.end() != std::find(availablePlugins.begin(), availablePlugins.end(), p))
+	    result.push_back(p);
     }
 
     /* Add plugins not in the initial list */
-    foreach (CompOption::Value &opt, list)
+    foreach(CompOption::Value const& opt, extraPluginsRequested)
     {
-	std::list <CompString>::iterator it = initialPlugins.begin ();
-	bool				 skip = false;
-	
-	if (opt.s () == "core")
-	   continue;
-
-	for (; it != initialPlugins.end (); it++)
+	if (opt.s() == "core")
+	    continue;
+
+	if (blacklist.find (opt.s()) != blacklist.end ())
+	    continue;
+
+	typedef std::list<CompString>::iterator iterator;
+	bool skip = false;
+
+	for (iterator it = initialPlugins.begin(); it != initialPlugins.end();
+		it++)
 	{
 	    if ((*it) == opt.s())
 	    {
@@ -980,50 +942,71 @@
 
 	if (!skip)
 	{
-	    pList.at (j++).set (opt.s ());
+	    if (availablePlugins.end() != std::find(availablePlugins.begin(), availablePlugins.end(), opt.s()))
+		result.push_back(opt.s());
 	}
     }
-
-    assert (j == pList.size ());
-
-    /* j is initialized to 1 to make sure we never pop the core plugin */
-    unsigned int              i;
-    for (i = j = 1; j < plugin.list ().size () && i < pList.size (); i++, j++)
+    return result;
+}
+
+
+void
+cps::PluginManager::updatePlugins ()
+{
+    possibleTap = NULL;
+    dirtyPluginList = false;
+
+    CompOption::Value::Vector const desiredPlugins(mergedPluginList());
+
+    unsigned int pluginIndex;
+    for (pluginIndex = 1;
+	pluginIndex < plugin.list ().size () && pluginIndex < desiredPlugins.size ();
+	pluginIndex++)
     {
-	if (plugin.list ().at (j).s () != pList.at (i).s ())
+	if (plugin.list ().at (pluginIndex).s () != desiredPlugins.at (pluginIndex).s ())
 	    break;
     }
 
-    CompPlugin::List pop;
+    unsigned int desireIndex = pluginIndex;
 
-    if (unsigned int const nPop = plugin.list ().size () - j)
+    // We have pluginIndex pointing at first difference (or end).
+    // Now pop plugins off stack to this point, but keep track that they are loaded
+    CompPlugin::List alreadyLoaded;
+    if (const unsigned int nPop = plugin.list().size() - pluginIndex)
     {
-	for (j = 0; j < nPop; j++)
+	for (pluginIndex = 0; pluginIndex < nPop; pluginIndex++)
 	{
-	    pop.push_back (CompPlugin::pop ());
-	    plugin.list ().pop_back ();
+	    alreadyLoaded.push_back(CompPlugin::pop());
+	    plugin.list().pop_back();
 	}
     }
 
-    for (; i < pList.size (); i++)
+    // Now work forward through requested plugins
+    for (; desireIndex < desiredPlugins.size(); desireIndex++)
     {
 	CompPlugin *p = NULL;
 	bool failedPush = false;
 
-	foreach (CompPlugin *pp, pop)
+	// If already loaded, just try to push it...
+	foreach(CompPlugin * pp, alreadyLoaded)
 	{
-	    if (pList[i]. s () == pp->vTable->name ())
+	    if (desiredPlugins[desireIndex].s() == pp->vTable->name())
 	    {
 		if (CompPlugin::push (pp))
 		{
 		    p = pp;
-		    pop.erase (std::find (pop.begin (), pop.end (), pp));
+		    alreadyLoaded.erase(
+			    std::find(alreadyLoaded.begin(),
+				    alreadyLoaded.end(), pp));
 		    break;
 		}
 		else
 		{
-		    pop.erase (std::find (pop.begin (), pop.end (), pp));
-		    CompPlugin::unload (pp);
+		    alreadyLoaded.erase(
+			    std::find(alreadyLoaded.begin(),
+				    alreadyLoaded.end(), pp));
+		    blacklist.insert (desiredPlugins[desireIndex].s ());
+		    CompPlugin::unload(pp);
 		    p = NULL;
 		    failedPush = true;
 		    break;
@@ -1031,25 +1014,32 @@
 	    }
 	}
 
+	// ...otherwise, try to load and push
 	if (p == 0 && !failedPush)
 	{
-	    p = CompPlugin::load (pList[i].s ().c_str ());
-	    
+	    p = CompPlugin::load(desiredPlugins[desireIndex].s ().c_str ());
+
 	    if (p)
 	    {
-		if (!CompPlugin::push (p))
+		if (!CompPlugin::push(p))
 		{
-		    CompPlugin::unload (p);
+		    blacklist.insert (desiredPlugins[desireIndex].s ());
+		    CompPlugin::unload(p);
 		    p = 0;
 		}
 	    }
+	    else
+	    {
+		blacklist.insert (desiredPlugins[desireIndex].s ());
+	    }
 	}
 
 	if (p)
-	    plugin.list ().push_back (p->vTable->name ());
+	    plugin.list().push_back(p->vTable->name());
     }
 
-    foreach (CompPlugin *pp, pop)
+    // Any plugins that are loaded, but were not re-initialized can be unloaded.
+    foreach(CompPlugin * pp, alreadyLoaded)
 	CompPlugin::unload (pp);
 
     if (!dirtyPluginList)
@@ -4626,9 +4616,6 @@
 
     optionSetToggleWindowShadedKeyInitiate (CompScreenImpl::shadeWin);
 
-    if (isDirtyPluginList ())
-	updatePlugins ();
-
     return true;
 }