Bug 740892 - Deadlock in on_screen_monitors_changed
Summary: Deadlock in on_screen_monitors_changed
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Linux 6
Classification: Red Hat
Component: gnome-screensaver
Version: 6.2
Hardware: Unspecified
OS: Unspecified
medium
medium
Target Milestone: rc
: ---
Assignee: Ray Strode [halfline]
QA Contact: Desktop QE
URL:
Whiteboard:
Depends On: 743876
Blocks: 740581 747120
TreeView+ depends on / blocked
 
Reported: 2011-09-23 16:49 UTC by Adam Jackson
Modified: 2011-12-06 16:28 UTC (History)
2 users (show)

Fixed In Version: gnome-screensaver-2.28.3-15.el6
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-12-06 16:28:24 UTC


Attachments (Terms of Use)
Describe the attachment briefly. (1.34 KB, patch)
2011-09-23 16:49 UTC, Adam Jackson
no flags Details | Diff
The simple patch (2.23 KB, patch)
2011-10-05 14:18 UTC, Ray Strode [halfline]
no flags Details | Diff


Links
System ID Priority Status Summary Last Updated
Red Hat Product Errata RHEA-2011:1652 normal SHIPPED_LIVE gnome-screensaver bug fix and enhancement update 2011-12-06 00:50:26 UTC

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@redhat.com>
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@redhat.com>
---
 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 Product and 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


Note You need to log in before you can comment on or make changes to this bug.