[regression r3729] Plugin classes not destructed properly on unload

Bug #1193596 reported by Sami Jaktholm
26
This bug affects 6 people
Affects Status Importance Assigned to Milestone
Compiz
Fix Released
Critical
Sam Spilsbury
compiz (Ubuntu)
Fix Released
Undecided
Unassigned

Bug Description

When a plugin is unloaded, the screen and window classes of that plugin aren't properly destructed. This is a regression caused by r3729.

Here's what I've figured out so far:
1) Plugin list changes, CompPlugin::pop is called for plugins
2) CompPlugin::pop calls CompManager::finiPlugin
3) CompManager::finiPlugin calls p->vTable->markNoFurtherInstantiation
4) markNoFurtherInstantiation calls disallowInstantiations for screen (and window)
5) disallowInstantiations sets mPluginLoaded to false
6) CompManager::finiPlugin calls p->vTable->finiScreen
7) CompPlugin::VTable::finiScreen calls PluginClassHandler::get
8) PluginClassHandler::get returns NULL because mPluginLoaded is false
9) CompPlugin::VTable::finiScreen destructs the returned NULL instead of the PluginScreen instance

I'm not sure what happens after (9), but the destructors of PluginClasses are never called.

A crash due to this bug occurs with following steps:
1) Enable Opacify plugin
2) Activate it if not automatically activated
3) Disable Opacify plugin
4) Crash.

Related branches

Revision history for this message
Sami Jaktholm (sjakthol) wrote :

A possible fix is to call markNoFurtherInstantiation after finiScreen has completed. However, I'm not sure if is's a good idea to let the destructors (possibly) create new instances of PluginClasses we're trying to destroy.

MC Return (mc-return)
Changed in compiz:
milestone: none → 0.9.10.0
MC Return (mc-return)
Changed in compiz:
importance: Undecided → High
Revision history for this message
Sam Spilsbury (smspillaz) wrote :

Actually, you're right - it needs to call markNoFurtherInstantiation after finiScreen is completed. Thanks for finding that.

Changed in compiz:
assignee: nobody → Sam Spilsbury (smspillaz)
importance: High → Critical
Revision history for this message
Sam Spilsbury (smspillaz) wrote :

Additional note: allowing the destructors to create new instances isn't a problem - a new instance will only be created if a plugin class instance didn't already exist for that particular core structure.

Calling PluginWindow::get (w) inside of a PluginScreen destructor is more or less undesired behavior anyways - previously it would just result in ::get returning NULL (which by convention, is never checked), now the behavior is undefined. The proper solution is probably to have a "mark no further instantiations" phase for each attachable class (eg, PluginWindow::markNoFurtherInstantiations, PluginScreen::markNoFurtherInstantiations), but just delaying the marking until the end of the unload process is good enough for now.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

Fix committed into lp:compiz at revision None, scheduled for release in compiz, milestone 0.9.10.0

Changed in compiz:
status: New → Fix Committed
Stephen M. Webb (bregma)
Changed in compiz:
status: Fix Committed → Fix Released
Revision history for this message
Launchpad Janitor (janitor) wrote :
Download full text (70.8 KiB)

This bug was fixed in the package compiz - 1:0.9.10+13.10.20130822-0ubuntu1

---------------
compiz (1:0.9.10+13.10.20130822-0ubuntu1) saucy; urgency=low

  [ Sam Spilsbury ]
  * Bump version to 0.9.10

  [ Łukasz 'sil2100' Zemczak ]
  * Remove debian/patches/unity_support_test.patch:
    - Running the support test from compiz has bad side effects, from now
      on we run it from Xsession.d
  * Automatic snapshot from revision 3644

  [ Iven Hsu ]
  * Opacify: Only dim the windows above the active window.(LP:
    #1189374). (LP: #1189374)
  * KWD: Fix compile errors with KDE 4.11. The KWin developers made
    kdecorationbridge.h private. See:
    http://lists.freedesktop.org/archives/compiz/2013-March/003479.html
    (LP: #1193792). (LP: #1193792)

  [ Nikolay Martynov ]
  * When static switcher is enabled and has an option to show
    application icon turned on the icons are expected to be ~1/3 of a
    thumbnail (48px). Instead they are displayed in 512px size and
    completely cover everything. This change addresses this issue. See
    LP #1173914. (LP: #1173914, #1186426)

  [ BryanFRitt ]
  * Fixed the non-working Annotate 'Clear' Button. Moved this option's
    CCSM position upwards to keep the button shortcuts together. (LP:
    #1202907). (LP: #1202907)

  [ Mehrdad Afshari ]
  * Added "move window to previous monitor" feature to compiz Put
    plugin. (LP: #1178581)

  [ Hu Kang ]
  * gtk-window-decorator: destroy action menu when any of the (close,
    min, max) buttons on the title bar is pressed. (LP: #1101648)
  * Remove redundant src/logmessage/include/core/logmessage.h (LP:
    #1067246). (LP: #1067246)

  [ Steve Langasek ]
  * Fix for bug #763148 (with added test cases): when the desktop is
    resized, windows should stay on their original workspace. (LP:
    #763148)

  [ Brandon Schaefer ]
  * Unrevert 3728, fix failing tests. Change the behaviour of
    undecorating windows. Previously when a window was undecorated, we
    would shift it back to an appropriate position according to its
    gravity member. That behaviour was problematic because in the
    StaticGravity case the window has to just stay in the same place.
    But then if you had a window with StaticGravity which then did get a
    decoration and later removed it, it would be placed as though it was
    decorated and appear to be in the wrong place. The correct behaviour
    is to place all windows as though they have decorations, and then
    when decorations are removed, to move the window back to the corner
    as indicated in its gravity and then expand its size to cover the
    obscured regions no longer hidden because the decorations went away.
    (LP: #1165343).   1. Completely remove decorOffsetMove and other
    related code from      decor.cpp. Put the logic to handle the
    window->input () - window->border ()      placement offset inside of
    setWindowFrameExtents instead. Now the window      will always be
    offset from its original non-decorated position to the new
         decorated position, rather than having to guess between
    decoration sizes.   2. Make saveGeometry and restoreGeometry work
    relative to window->border ()      a...

Changed in compiz (Ubuntu):
status: New → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.