Compiz Core

Merge lp://qastaging/~vanvugt/compiz-core/fix-startup-plugins into lp://qastaging/compiz-core

fix-startup-plugins
Merge into 0.9.7

Proposed by Daniel van Vugt on 2012-03-29

Status:	Rejected
Rejected by:	Daniel van Vugt on 2012-03-30
Proposed branch:	lp://qastaging/~vanvugt/compiz-core/fix-startup-plugins
Merge into:	lp://qastaging/compiz-core
Diff against target:	181 lines (+27/-71) 2 files modified src/privatescreen.h (+5/-0) src/screen.cpp (+22/-71)
To merge this branch:	bzr merge lp://qastaging/~vanvugt/compiz-core/fix-startup-plugins
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
Daniel van Vugt		Disapprove on 2012-03-30
Alan Griffiths		Needs Fixing on 2012-03-29
Sam Spilsbury	2012-03-29	Approve on 2012-03-29
Didier Roche-Tolomelli	2012-03-29	Pending
Review via email: mp+99886@code.qastaging.launchpad.net

Description of the change

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 subsquent updates check the blacklist
and know they should never include those failed plugins in testing whether
the active plugin lists have changed.

The second part of the fix is to ensure updatePlugins no longer calls
screen->setOptionForPlugin to change the active_plugins setting based on what
can and is loaded. That's not only redundant and wasteful but might cause
further callbacks to updatePlugins unnecessarily.

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".

Revision history for this message

Tim Penhey (thumper) wrote on 2012-03-29:

std::set has a count method.

Ideally I was looking for something like "contains", but a non-zero
count means that it exists in the set.

Perhaps
blacklist.count(name)
is nicer than:
blacklist.find(name) != blacklist.end()

Revision history for this message

Sam Spilsbury (smspillaz) wrote on 2012-03-29:

Looks good to me.

I would also say there's another case this branch doesn't handle, which is the case where a plugin fails to load because it either a) can't be loaded or b) can't be loaded because of a missing plugin dep. In that case, I think we should put such plugins at the back of the list at the end of every new push. Dependencies will in that case be automatically resolved if the user tries to load the dependend plugin A as A will load and dependent plugin B will load thereafter. It also avoids the mass push/pop if a plugin was failing to init and in the middle of the list.

review: Approve

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2012-03-29:

Tim: count() would have to visit all elements in the set. Whereas find() only needs to visit 50% on average.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2012-03-29:

Ignore my previous comment. The performance is the same. A set can only contain 0 or 1 of each unique value.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2012-03-29:

An improvement. But...

Important:

You also need to update src/privatescreen/tests/test-privatescreen.cpp

Less important:

It doesn't handle cases where a blacklisted plugin would load on a subsequent attempt - e.g. because it got installed while compiz was running.

I also have a preference for giving better names to a many of the variables here.

PS I do agree about using find() not count() - but both are O(log(n)) in an RB-tree, not liner.

review: Needs Fixing

Revision history for this message

Didier Roche-Tolomelli (didrocks) wrote on 2012-03-29:

Nice work Daniel!
Once the branch will land to trunk, I'll make the necessary change in the ubuntu package, we can push it to a ppa and ask people to try from it.

Note to myself: override the gconf "default" profile with the plugin set once this change is done.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2012-03-30:

Superseded by: https://code.launchpad.net/~alan-griffiths/compiz-core/rework-updatePlugins/+merge/99981

review: Disapprove

Unmerged revisions

3072. By Daniel van Vugt on 2012-03-29

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 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.