Bug 740892

Summary: Deadlock in on_screen_monitors_changed
Product: Red Hat Enterprise Linux 6 Reporter: Adam Jackson <ajax>
Component: gnome-screensaverAssignee: Ray Strode [halfline] <rstrode>
Status: CLOSED ERRATA QA Contact: Desktop QE <desktop-qa-list>
Severity: medium Docs Contact:
Priority: medium    
Version: 6.2CC: jkoten, tpelka
Target Milestone: rc   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: gnome-screensaver-2.28.3-15.el6 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2011-12-06 16:28:24 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On: 743876    
Bug Blocks: 740581, 747120    
Attachments:
Description Flags
Describe the attachment briefly.
none
The simple patch none

Description Adam Jackson 2011-09-23 16:49:21 UTC
Created attachment 524658 [details]
Describe the attachment briefly.

See attached patch for details.

Comment 1 Adam Jackson 2011-09-23 16:50:25 UTC
Oh for crap's sake, bugzilla, maybe try not mangling patches.

From d2c37810bb2cc05ecfabb377813faff5f872fd9e Mon Sep 17 00:00:00 2001
From: Adam Jackson <ajax>
Date: Fri, 23 Sep 2011 12:15:58 -0400
Subject: [PATCH] Fix server grab timing during monitor removal

gs_manager_request_unlock() calls g_signal_emit, so we can recurse.  If
we end up back in gs_window_real_show, then we can attempt to invoke the
GL helper to pick a visual.  Since we've just taken a server grab, that
helper will never complete, and we'll deadlock.

Signed-off-by: Adam Jackson <ajax>
---
 src/gs-manager.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/gs-manager.c b/src/gs-manager.c
index cb3f43d..3924e0a 100644
--- a/src/gs-manager.c
+++ b/src/gs-manager.c
@@ -1514,15 +1514,15 @@ on_screen_monitors_changed (GdkScreen *screen,
                         l = next;
                 }
 
+                gdk_flush ();
+                gdk_x11_ungrab_server ();
+
                 /* make sure there is a lock dialog on a connected monitor,
                  * and that the keyboard is still properly grabbed after all
                  * the windows above got destroyed*/
                 if (n_windows > n_monitors) {
                         gs_manager_request_unlock (manager);
                 }
-
-                gdk_flush ();
-                gdk_x11_ungrab_server ();
         }
 }
 
-- 
1.7.6

Comment 6 RHEL Program Management 2011-10-04 22:11:13 UTC
This request was evaluated by Red Hat Product Management for inclusion
in a Red Hat Enterprise Linux maintenance release. Product Management has 
requested further review of this request by Red Hat Engineering, for potential
inclusion in a Red Hat Enterprise Linux Update release for currently deployed 
products. This request is not yet committed for inclusion in an Update release.

Comment 8 Ray Strode [halfline] 2011-10-05 14:09:48 UTC
So reproducing this bug is bit timing dependent.

The fade out is always 1 second long, which isn't a lot of time.

The way I was able to reproduce the problem fairly reliably was to:

1) Attach an external display to a laptop
2) run this at a gnome terminal:

sudo pm-suspend & sh -c "sleep 1; gnome-screensaver-command --lock"

3) wait for it to suspend, then disconnect the external display
4) resume the laptop

the "sleep 1" there is to make sure the pm-suspend process gets a bit of time to do its thing before the fade starts. The idea is to make sure suspend completes in the middle of the 1 second fade. If suspend is faster on any given machine that sleep value may need to be reduced or eliminated.

At this point, the bug will manifest itself as a frozen looking screen that's only partially drawn.

Comment 9 Ray Strode [halfline] 2011-10-05 14:18:36 UTC
Created attachment 526506 [details]
The simple patch

Above is a minimal fix for the deadlock. A slightly nicer patch would drop the server grab entirely and rework the code to find an appropriate window to take the input grab before destroying the window holding the existing grab.  That would be more invasive, though, and so less appropriate for a rhel update.

Comment 12 Jiri Koten 2011-10-06 11:44:19 UTC
The patch caused an ugly regression - see bug 743876.

Comment 13 Ray Strode [halfline] 2011-10-06 13:56:20 UTC
I will investigate right away.

Comment 14 Ray Strode [halfline] 2011-10-06 16:41:31 UTC
The code was placing the lock windows on realize instead of map. This worked before because realize happened at the same time as map.  My change makes realize happen earlier, thus the regression.

New fix building now that fixes the code to place the windows from the right hook.

Comment 16 errata-xmlrpc 2011-12-06 16:28:24 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

http://rhn.redhat.com/errata/RHEA-2011-1652.html