GNOME Bugzilla – Bug 588119
Should set RestartStyleHint to RestartIfRunning when replaced
Last modified: 2011-01-25 14:27:56 UTC
Please describe the problem: Some Ubuntu users are reporting with 2.27.0 that when they run "compiz --replace" after starting their session with Metacity as the window manager (/desktop/gnome/session/required_components/windowmanager = "metacity"), gnome-session continually tries to respawn Metacity once Compiz is running. Gnome-session will then use 100% CPU for the remainder of the session when doing this. The reason for this is Metacity sets the RestartStyleHint to RestartImmediately when it first runs If another window manager replaces it later on, it exits without changing it back to RestartIfRunning. gnome-session will then immediately respawn Metacity, which will connect to the session manager, set the RestartStyleHint to RestartImmediately, try to control the display, then exit again when it realises it can't (because Compiz is running by now). This cycle then continues indefinately for the remainder of the session. This used to work in 2.26 (I checked the debug output from gnome-session, which showed that Metacity always sets the RestartStyleHint back to RestartIfRunning when replaced with another WM). The bug is introduced by this change: --- metacity-2.25.144/src/core/session.c 2009-02-01 20:33:15.000000000 +0000 +++ metacity-2.27.0/src/core/session.c 2009-02-20 15:26:32.000000000 +0000 @@ -376,6 +376,14 @@ meta_session_shutdown (void) SmProp *props[1]; char hint = SmRestartIfRunning; + if (!meta_get_display ()) + { + meta_verbose ("Cannot close session because there is no display"); + return; + } + + warn_about_lame_clients_and_finish_interact (FALSE); + if (session_connection == NULL) return; What is happening is meta_session_shutdown() is being called after meta_display_close(). Because the display is already closed at this point, meta_session_shutdown() returns without setting the RestartStyleHint. Steps to reproduce: 1. Log in with metacity as the default WM (/desktop/gnome/session/required_components/windowmanager = "metacity") 2. Run "compiz --replace" Actual results: Metacity exits and then gnome-session respawns a new one, which exits immediately. This cycle repeats for the remainder of the session. Expected results: Metacity should exit and not be respawned Does this happen every time? Yes - but you must have Metacity autostarted at the start of the session by setting /desktop/gnome/session/required_components/windowmanager = "metacity", otherwise there is no restart command and gnome-session won't try to restart it. Other information:
Created attachment 138075 [details] [review] Ubuntu patch This is the patch I proposed in Ubuntu. It moves the check for the display and displaying the warning dialog until after the RestartStyleHint is changed.
Here's the Ubuntu bug for reference: https://bugs.launchpad.net/metacity/+bug/389686
*** Bug 591642 has been marked as a duplicate of this bug. ***
I don't think attachment 138075 [details] [review] is quite right. I think the original commit introducing the problem should just get reverted. See 591642 comment 0 for my rationale.
I don't think entirely reverting abbd057 is right; as I read the code, there is an access to freed memory in the old code, since we g_free() the MetaDisplay structure but don't null out the global 'the_display'. Then when we call meta_display_close() we access display->closing. But I otherwise agree with Ray. I'll attach a patch that implements how I think makes sense.
Created attachment 141446 [details] [review] Should set RestartStyleHint to RestartIfRunning when replaced This reverts most of commit abbd057eb967e6ab462ffe305f41b2b04d417b25; - It's fine to call meta_session_shutdown() after the display is closed, since it's talking over the ICE connection - We should not call warn_about_lame_clients_and_finish_interact() unless we are interacting with the window manager in a session save. However, the part of abbd057 that fixed accessing freed memory was fixing a real problem; this patches does the same thing in a simpler way by fixing an obvious type in meta_display_close() where it was NULL'ing out the local variable 'display' rather than the global variable 'the_display' and adding keeping the check in meta_finalize() that was added in abbd057. The order of calling meta_session_shutdown() and calling meta_display_close() is reverted back to the old order to make it clear that it's OK if the display way already closed previously.
*** Bug 597654 has been marked as a duplicate of this bug. ***
Review of attachment 141446 [details] [review]: Looks like a very good idea to me. Committed.
-> FIXED