GNOME Bugzilla – Bug 168207
[PATCH] Metacity crashes with ssh-askpass
Last modified: 2005-02-23 17:43:14 UTC
Steps to reproduce: 1. Start GNOME 2. Launch ssh-askpass (from the XFree86 distribution) 3. Stack trace:
+ Trace 56024
Other information: I think the problem is with meta_workspace_add_window() in workspace.c. The g_return_if_fail() at the top of the function checks to make sure window->workspace is NULL. However, right below that there is an if clause that checks for the same thing. However, if window->workspace is not NULL, the function has already returned, so something is fishy. Since the workspace argument is definitely NULL, I changed the g_return_if_fail() to check the workspace is not NULL. That fixed the crash, and everything seems to be working okay. However, you gues should probably review this.
Created attachment 37805 [details] [review] Fix the crash with ssh-askpass (maybe correctly)
I should point out that ssh-askpass worked fine with metacity-2.9.13, but fails with 2.9.21.
The check you use should be added instead of replacing the previous one. But I think a better change would be to make the if statement that allowed this call in meta_window_new_with_attrs to read if (parent && parent->workspace) rather than if (parent) However, I'd first like to understand how it's possible for parent->workspace to be NULL... Could you create a verbose debugging log? To do so: 1) Reduce your desktop to as few windows as possible to reproduce the bug 2) Run METACITY_VERBOSE=1 METACITY_USE_LOGFILE=1 metacity --replace 3) On stdout metacity will print the name of the logfile 4) Reproduce the bug as quickly as possible 5) Kill the metacity you started above to stop the logfile from growing any longer 6) Compress the logfile (if needed) and attach it here
About the check being added. I thought about that, but I was confused by the line below that checks if window->workspace was NULL. If we assert that it must be NULL, shouldn't that check go away? As for the debug, I'll try this tonight over VNC, and attach the log.
Created attachment 37819 [details] Metacity debug log showing crash caused by ssh-askpass
A window is it's own parent?!? Window manager: Window 0x340000c (OpenSSH Au) transient for 0x340000c (root = 0) ... PLACEMENT: Putting window 0x340000c (OpenSSH Au) on same workspace as parent 0x340000c (OpenSSH Au) How stupid is that? That app is broken, or something else is fundamentally messed up. What gets me is that you say this worked under metacity 2.9.13. I don't see how that's possible, because the relevant code should crash under this case above with any metacity version from at least the last year and perhaps since the beginning of time. Did you upgrade OpenSSH or toggle some option in it?
Nothing changed in openssh-askpass between metacity-2.9.13 and 2.9.21. The app itself is not SSH, but an X11 front-end to it. You can download it from ftp://ftp.FreeBSD.org/pub/FreeBSD/ports/local-distfiles/kris/OpenSSH-askpass-1.2.2.2001.02.24.tar.gz. As for 2.9.13, I took for granted that the user that reported this to me said it worked in the previous version. I tried 2.9.13, and it crashed. I then tried 2.8.13, and it worked. He must have upgraded from GNOME 2.8 to 2.9.91, and not from 2.9.90 to 2.9.91.
Okay, binary search trial-and-error shows that the version of metacity obtained with cvs -q -z3 update -Pd -D "2004-12-22 16:00" does work while cvs -q -z3 update -Pd -D "2004-12-22 17:00" doesn't. That changes was the enforce-windows-being-on-only-one-workspace. Running a diff on those two versions shows: - tmp_list = parent->workspaces; - while (tmp_list != NULL) - { - /* this will implicitly add to the appropriate MRU lists - */ - meta_workspace_add_window (tmp_list->data, window); - - tmp_list = tmp_list->next; - } + /* this will implicitly add to the appropriate MRU lists + */ + meta_workspace_add_window (parent->workspace, window); Thus the list version (i.e. the old version) operates on anywhere from 0 to N workspaces. I was assuming 1 to N workspaces when I wrote the patch, for no reason other than 0 didn't occur to me. So the new version works on exactly 1 workspace, when it should work on 0 or 1. I'll attach a patch in a minute that fixes this. Oh, and I still think that openssh-askpass is braindead. Making a window be a parent of itself is pretty dumb, plus while running it I get the warning: "Window manager warning: Broken client! Window 0xc00009 (OpenSSH Au) changed client leader window or SM client ID".
Created attachment 37820 [details] [review] Don't try the parents' workspace if the parent doesn't have one This fixes the crash in my testing...
Confirmed here as well. With your patch, I no longer see the crash. Thanks!
committed. 2005-02-23 Elijah Newren <newren@gmail.com> * src/window.c: (meta_window_new_with_attrs): Fix crash that occurs when stupid apps claim that a window is its own parent. #168207