GNOME Bugzilla – Bug 358514
crash when closing distant application running by ssh
Last modified: 2006-10-03 10:17:51 UTC
That bug has been opened on https://launchpad.net/distros/ubuntu/+source/metacity/+bug/62970 "when I remotely log on to a distant machine via ssh, using x-forwarding, when closing a programm (such as gedit) on the distant computer, metacity crashes ... DistroRelease: Ubuntu 6.10 ExecutablePath: /usr/bin/metacity Package: metacity 1:2.16.2-0ubuntu1 ProcCmdline: /usr/bin/metacity --sm-client-id=default0 ... > Thanks for your bug. What do you mean by "when closing a programm (such as gedit) on the distant computer"? How do you close it? From the client? or from the "distant computer"? ... sorry for the lack of clarity when I type " ssh [RemoteComputer] gedit foo.txt " X forwarding allows the display of gedit on my local computer. When closing this window, my local metacity crashes (and thus this repport bug was proposed to me)." It exits on a "Bug in window manager: Unexpected X error: BadWindow (invalid Window parameter) serial 12391 error_code 3 request_code 19 minor_code 0)" Backtrace: "Program received signal SIGABRT, Aborted.
+ Trace 73710
Thread 47625249272048 (LWP 1261)
Interesting; window-props.c:set_title_text() had a call to XDeleteProperty() that was not surrounded by a meta_error_trap_push/pull pair surrounding it. Don't know why we're getting a _NET_WM_NAME property notify for gedit when I close it (at least for version 2.8.x, since that was what was on the remote machine I could log into), but I do, and it triggers this bug. * src/window-props.c (set_title_text): surround the XDeleteProperty() call with a meta_error_trap_push/meta_error_trap_pop pair to prevent a crash when closing a remote instance of gedit (and perhaps other apps). #358514.
Created attachment 73781 [details] [review] possible missing traps in other places I looked over window.c and display.c for other possible missing trap push/pop pairs. I don't really understand why it's needed in some places and not others (e.g. XSync calls or calls in meta_display_new()). Maybe Havoc can look over it and comment for me?
A bit of background is that most window managers just ignore all X errors. Metacity tries to ignore only "unavoidable"/"expected"/"outside-our-control" errors - i.e. if an error could happen, but only if metacity is buggy, we don't push the error trap. This is more like how GDK works than like a traditional WM. The usual X error that metacity can't avoid in advance is BadWindow, i.e. the window has been destroyed by its owner. Probably the majority of error traps are intended to prevent this. Any X request on another client's window can result in BadWindow, unless you have the server grabbed, since the client could destroy the window before your request is processed. However, if metacity created the window, it should know that BadWindow is impossible, because only metacity could destroy the window. Pushing an error trap when messing with metacity's own windows could hide bugs where we destroy a window but still try to use it. Going through your patch: - XCheckIfEvent afaik just looks at the event queue, doesn't make any X requests, so it doesn't need error traps - XCreateFontCursor / XFreeCursor are not expected to fail for any reason I know of (also, XFreeCursor is not AFAIK a round trip so the boolean to pop should be FALSE) - XDeleteProperty on display->leader_window should not fail because metacity owns this window and won't get BadWindow - XcursorSetTheme etc. I don't know of a reason they'd fail - XChangeProperty on the root window to set metacity sentinel can't fail since the root window can't be destroyed - meta_dispaly_set_input_focus_window may need those traps, though the patch doesn't have enough context to know for sure (maybe there are already traps around a larger part of the code) - SetInputFocus on the no focus window does not need traps since we own the window In general the way metacity works requires knowing what's going on with all these X calls on the protocol level, so it's probably a little scary. A good rule of thumb I'd say is that anytime an argument to an X call is a client window, an error trap is required, and otherwise usually an error trap is not required. X calls generally do not involve a round trip unless they inherently have to have one, because they return information queried from the X server. Surprisingly, even the resource-creation calls (create window, create GC, etc.) do not do a round trip. If you get the round trip flag wrong (say there was a round trip and there wasn't) then the error trap won't work, because there won't be an XSync - which means the error reply may not have arrived yet.
Created attachment 73828 [details] [review] Fix meta_display_set_input_focus_window Cool, thanks for the explanation. We had push/pop pairs around one of the calls to meta_display_set_input_focus_window() but it was missing in another. I figure the best way to avoid forgetting is to just stick the push/pop into set_input_focus_window() near the X call. So here's a patch that does that, which I'll commit in a second. I think that closes out everything for this bug, so I'll mark it as fixed now.
works fine with 2.16.3, thank you for the quick fixing!