GNOME Bugzilla – Bug 655122
Crash when resizing window on MacOS Lion
Last modified: 2012-01-09 15:25:06 UTC
When resizing a window on macOS Lion, GTK crashes hard.
+ Trace 227844
I experimented with it a bit this afternoon: It does re-size, but from the top edge and top right corner instead of the bottom right corner. It's supposed to re-size from any edge or corner on Lion. Unfortunately, after a varying number of resizes, it crashes, as Michael noted.
Some additional debug info: rogram received signal EXC_BAD_ACCESS, Could not access memory. Reason: KERN_PROTECTION_FAILURE at address: 0x0000000c 0x00053c46 in find_window_for_ns_event (nsevent=0xeb8910, x=0xbffff750, y=0xbffff74c, x_root=0xbffff748, y_root=0xbffff744) at gdkevents-quartz.c:650 650 grab_nswindow = ((GdkWindowImplQuartz *)grab_private->impl)->toplevel; (gdb) bt
+ Trace 227857
Cannot access memory at address 0xc (gdb) p grab_private $1 = (GdkWindowObject *) 0x0 (gdb) p grab_toplevel $2 = (GdkWindow *) 0x0 (gdb) p display $3 = (GdkDisplay *) 0x10d9000 (gdb) p view $4 = (GdkQuartzView *) 0x217ae70 (gdb) p toplevel $5 = (GdkWindow *) 0x10e5170 (gdb) p grab $6 = (GdkPointerGrabInfo *) 0x217da40 (gdb) p toplevel_under_pointer No symbol "toplevel_under_pointer" in current context. (gdb) p *grab $7 = { window = 0x0, native_window = 0x10e5170, serial_start = 0, serial_end = 4294967295, owner_events = 0, event_mask = 0, implicit = 1, time = 683779, activated = 1, implicit_ungrab = 0 } (gdb) p grab_event_mask_from_ns_event(nsevent) No symbol "grab_event_mask_from_ns_event" in current context. (gdb) p get_event_mask_from_ns_event(nsevent) $8 = 316 (gdb) p /x get_event_mask_from_ns_event(nsevent) $9 = 0x13c (gdb) This explains the crash: Since grab->window is NULL, the following tries to dereference a NULL pointer: grab_toplevel = gdk_window_get_effective_toplevel (grab->window); grab_private = (GdkWindowObject *)grab_toplevel; grab_nswindow = ((GdkWindowImplQuartz *)grab_private->impl)->toplevel; So, a potential fix is: GdkWindow* grab_win = grab->window ? grab->window : grab->native_wndow; if (!grab_win) return NULL; That will at least prevent the crash, though it won't do anything for the goofy resize behavior. Kris, you have a much better handle on how grabs work than I do. Will this work?
Here's the backtrace for gtk+-3. Same crash, the code has just been extracted to a new function: Program received signal EXC_BAD_ACCESS, Could not access memory. Reason: KERN_PROTECTION_FAILURE at address: 0x0000000c 0x00665c3b in find_toplevel_for_mouse_event (nsevent=0x56abec0, x=0xbffff6c0, y=0xbffff6bc) at gdkevents-quartz.c:610 610 grab_nswindow = ((GdkWindowImplQuartz *)grab_toplevel->impl)->toplevel; (gdb) bt
+ Trace 227858
Using grab->native_window when the grab-window == NULL works to prevent the crash. Digging a bit into the resizing, I find that the reason that resizing works along the top edge is that first, there's no grab on a titlebar (so the grab bug is avoided) and second, a mouse-move event on the titlebar is ignored, leaving quartz free to resize the window. So I added a line to return NULL just before the one that was causing the crashes, and presto!, resizing now works on all edges and corners. That's not the fix, obviously, but it does point the way.
Created attachment 192547 [details] [review] Fix the crash and allow resizing on all window edges in Lion This works. It's not perfect, because it doesn't get to the root cause of the grab having native window set but not window, but it seems to be otherwise correct.
Fantastic! I ported the patch to vanilla gtk-2-24 (it didn't apply as-is), and I had to change a couple of grab_window references to grab_private, which I *hope* is correct: https://github.com/mhutch/bockbuild/blob/master/packages/patches/gtk/gtklionresizecrash.patch It mostly works for me (on Lion). Often GTK doesn't seem to track the grab or the grab release correctly and strange things happen, but it's not clear to me how to reproduce this reliably.
Ah, I meant change grab_toplevel references to grab_private. I figured out one of the issues. You can drag faster than the window resizes, and therefore get out of the 5px border, at which point GTK interrupts the resize.
Yes, the patch is against Gtk3. I ran out of time yesterday before I got to backporting it. Makes sense. We really want the grab to register the border region and set a resizing flag. I'll have a look at that. Maybe in the process I'll see why the grab loses (or doesn't get) the window pointer.
gdkwindow.c:proxy_button_event() is sometimes failing to get the pointer_window from the toplevel_window using _gdk_window_find_descendant_at(). A simple fix seems to be: if (pointer_window == NULL) pointer_window = toplevel_window; I'll attach a patch soon.
Created attachment 192576 [details] [review] Don't allow pointer_window to ever be NULL
Er, I probably shouldn't have marked John's patch as obsoleted by mine, since my patch really only obsoletes the grab->window ? grab->window : grab->native_window bit of his patch.
Easy enough to fix: Just select "details" of the obsoleted patch and uncheck the box.
Created attachment 192587 [details] [review] Patch for Gtk2 This patch goes on Gtk2. It uses Jeff's fix in gdkwindow.c instead of selecting window or native_window as the other patch does. It still checks grab->window to protect against a possible crash. (I changed Jeff's fix a bit, moving the setting of pointer_window to after the parent check.) It also addresses Michael's complaint that the resize can get stopped by overrunning the edge detection by adding a new gboolean to GdkPointerGrabInfo and setting it if the first post-grab event is inside the 5-pixel border, then returning NULL if we find it set. I haven't been able to reproduce the problem, so Michael, please test it. I'm not sure if this affects ABI... Kris, is it acceptable?
Created attachment 192592 [details] [review] Revised Gtk2 Patch Ah, now I see a problem with the patch. This might be what Michael noticed earlier and diagnosed as running out of the border: The first time after restoring focus to the window, it takes a moment for the grab to be recognized. If you click and immediately start to drag, nothing happens; if you click and wait, then drag, resizing works as expected. After the first attempt, subsequent attempts work instantly until the window loses focus. So this patch adds another border handler for the ungrabbed case. It also fixes an omission of checking the new grabinfo->resize parameter and returning NULL if it's set, and replaces the "magic number" 5 with a const gint GDK_LION_RESIZE.
Works well, and fixes all the GTK type assertions that happened when resizing too. Thanks! There's one remaining issue I can find - clicking once in the resize border (i.e.. not dragging) seems to take a grab which is not released. It can only be reset by clicking on the window titlebar.
I hesitate to display my ignorance, but does anyone have an idea on how long it may take for these patches to be compiled into the SourceForge DMG package? Thanks! -Ed
What SF DMG package?
Here is the link I'm referring to, which is also in the purple "Downloads" box at the top-right of www.gnucash.org (for "Mac OSX Intel"): http://downloads.sourceforge.net/sourceforge/gnucash/Gnucash-Intel-2.4.7.dmg Thanks!
Ah. This is a Gtk+ bug, please don't post user questions about applications here. In the case of Gnucash, you should ask questions on gnucash-user@gnucash.org. I've already answered your question there for the bugs which we have solutions to. This isn't one of them yet.
(In reply to comment #7) > I figured out one of the issues. You can drag faster than the window resizes, > and therefore get out of the 5px border, at which point GTK interrupts the > resize. Is this really happening? When I test with the old corner-only resize grip on Tiger, Cocoa will swallow all events once a resize has started until the mouse button has been released. Therefore, we did not have a need for any code tracking the resize before. I would really expect Lion to behave the same when a resize has been started on the border -- otherwise I would almost assume this a bug in Lion's resize logic. I will check the AppKit release notes to see what's mentioned about this.
This article has some interesting details on window edge resizing: http://arstechnica.com/apple/reviews/2011/07/mac-os-x-10-7.ars/4
I *might* have been wrong, but it looked that way to me. Something certainly wasn't right, and it seems better with the new patch. One subtlety of the new resizing is that Cocoa does pass through click events in the resize area - this patch isn't doing that.
Created attachment 192641 [details] [review] Updated patch to fix bug mentioned in comment #15 This patch is basically just an update to John Ralls' patch but fixes the bug Michael Hutchinson mentions in comment #15 about how clicking in the resize area and then releasing the mouse button keeps the pointer grab active thus disallowing further clicks in the window until you click the title bar. Basically the fix is to not set grab->resizing to TRUE if the event type is NSLeftMouseUp.
(In reply to comment #22) > I *might* have been wrong, but it looked that way to me. Something certainly > wasn't right, and it seems better with the new patch. > > One subtlety of the new resizing is that Cocoa does pass through click events > in the resize area - this patch isn't doing that. I think what's really going on is that Gdk takes a moment to establish the grab, and if you start moving before then the event handler goes to the non-grab part of the logic. I fixed that with a second set of edge detection, but I think the final patch should move that to an edge (for Lion) and lower-right-corner (everything before) resize handler for NSLeftDrag that falls through to the other handlers. If I'm right, the grab->resizing thing can go away.
(In reply to comment #21) > This article has some interesting details on window edge resizing: > > http://arstechnica.com/apple/reviews/2011/07/mac-os-x-10-7.ars/4 Well, I've got that mostly covered. I need to figure out how to see if the scrollbars are visible and enable that part when appropriate. The extra border region outside of the window might be the reason we were getting a grab->native_window and no grab->window, too... if he's right about that. It's not obvious to me where the active point on those resize cursors is, but it appears to me that the resize works for every starting point that the resize cursor is set.
(In reply to comment #25) > The extra border region outside of the window might be the reason we were > getting a grab->native_window and no grab->window, too... if he's right about > that. It's not obvious to me where the active point on those resize cursors is, > but it appears to me that the resize works for every starting point that the > resize cursor is set. Yes, if I start the resize outside the window I still get the unreleased grab, even with Jeff's patch. When the unreleased grab is in place, resizing from outside the window works.
Michael: ugh, really? My patch for the grab->resize thing seemed to fix it for me :-(
(In reply to comment #22) > I *might* have been wrong, but it looked that way to me. Something certainly > wasn't right, and it seems better with the new patch. Ah, so the "GTK+ interrupts the resize" is without a patched GTK+ on Lion. That likely makes sense, because then both the window manager and GTK+ are processing the event. > One subtlety of the new resizing is that Cocoa does pass through click events > in the resize area - this patch isn't doing that. What I need to know is whether Cocoa also passes through any other/subsequent events. How the resizing logic for GTK+ is working for older OS X releases is: 1. GTK+ gets a button down event. If we see that this is in the lower-right resize area, we will ignore this event. There is no need to create an artificial grab. 2. Cocoa will restart the resizing and no mouse events are received by GTK+. 3. When the resizing ends, everything is back to normal. So in 2, Cocoa essentially grabs all events. If the Lion resizing behaves similar, but only with all of the window border as starting point, then we simply need to replace/extend the lower-right resize area with the entire window border and we're done. If the application continues to receive mouse dragged events while the window is in resize, how can the application distinguish between mouse dragged events handled by the window manager and other events?
Hmm. I'm not sure I agree that under previous versions CF isn't sending Gtk mouse events; the code appears to handle any mouse events in that corner as resize events regardless of what kind of events they are. I am sure that in Lion, CF does send NSLeftDrag events while resizing. That's not really a problem, we just need to recognize that an NSLeftDrag in the border region is a resize and let Cocoa do its thing, while handling all other mouse events in the border region normally. I suspect that we can do the same with the lower-right-corner on earlier versions and when scrollbars are enabled on Lion. find_window_for_ns_event() is perhaps not the best place to be doing this. It rather makes the function name misleading, and I don't really like idea of renaming it to "find_window_for_ns_event_and_oh_by_the_way_handle_window_resizing_too()". ;-)
Gtk+ is definitely getting NSLeftMouseDragged events on Lion when resizing a window.
Created attachment 192697 [details] [review] different approach to fixing this bug Here's a different approach which does not need the grab->resizing hack and also doesn't seem to suffer from any of the pointer grab bugs noticed above. We might even be able to trim out at least 1 more of the lion checks: /* If we're on Lion and within 5 pixels of an edge, then * assume that the user wants to resize, and return NULL to * let Quartz get on with it. We check the selector * isRestorable to see if we're on 10.7. */ if (([grab_nswindow respondsToSelector:@selector(isRestorable)]) && (*x < GDK_LION_RESIZE || *x > grab_private->width - GDK_LION_RESIZE || *y > grab_private->height - GDK_LION_RESIZE)) { return NULL; } I can't seem to get this one to hit anymore, so we can probably just drop it.
This seems to work well, I can't find any problems with it. It still doesn't allow clicking on widgets within the 5px border but that's very minor and probably not worth worrying about.
(In reply to comment #31) > Here's a different approach which does not need the grab->resizing hack and > also doesn't seem to suffer from any of the pointer grab bugs noticed above. Great! I was not very happy with the resizing hack ... > We might even be able to trim out at least 1 more of the lion checks: > > > /* If we're on Lion and within 5 pixels of an edge, then > * assume that the user wants to resize, and return NULL to > * let Quartz get on with it. We check the selector > * isRestorable to see if we're on 10.7. > */ > if (([grab_nswindow > respondsToSelector:@selector(isRestorable)]) && > (*x < GDK_LION_RESIZE || > *x > grab_private->width - GDK_LION_RESIZE || > *y > grab_private->height - GDK_LION_RESIZE)) > { > return NULL; > } > > I can't seem to get this one to hit anymore, so we can probably just drop it. This is the check in the grabbed case, right? It is a good sign that you cannot hit this one, because I think it should go. Once GTK+ is in a grab, it should actually not be interrupted by a resize (if that's possible to achieve). My other two questions at the moment are: 1) Is the patch: + if (!pointer_window) + pointer_window = toplevel_window; still needed? I rather not add that to gdkwindow.c 2) In another bug, John has indicated that respondsToSelector: can be quite expensive. I think we should do this check once and then cache the value in the Window or Display object.
(In reply to comment #33) > (In reply to comment #31) > > Here's a different approach which does not need the grab->resizing hack and > > also doesn't seem to suffer from any of the pointer grab bugs noticed above. > > Great! I was not very happy with the resizing hack ... > > > > We might even be able to trim out at least 1 more of the lion checks: > > > > > > /* If we're on Lion and within 5 pixels of an edge, then > > * assume that the user wants to resize, and return NULL to > > * let Quartz get on with it. We check the selector > > * isRestorable to see if we're on 10.7. > > */ > > if (([grab_nswindow > > respondsToSelector:@selector(isRestorable)]) && > > (*x < GDK_LION_RESIZE || > > *x > grab_private->width - GDK_LION_RESIZE || > > *y > grab_private->height - GDK_LION_RESIZE)) > > { > > return NULL; > > } > > > > I can't seem to get this one to hit anymore, so we can probably just drop it. > > This is the check in the grabbed case, right? It is a good sign that you > cannot hit this one, because I think it should go. Once GTK+ is in a grab, it > should actually not be interrupted by a resize (if that's possible to achieve). > > My other two questions at the moment are: > > 1) Is the patch: > > + if (!pointer_window) > + pointer_window = toplevel_window; > > still needed? I rather not add that to gdkwindow.c > > 2) In another bug, John has indicated that respondsToSelector: can be quite > expensive. I think we should do this check once and then cache the value in > the Window or Display object. Yes, it's the instance in the grabbed case. If we can keep a resize from creating the grab, that would indeed be best. For safety we need to either prevent pointer_window from being NULL or make sure that we gracefully handle when it is -- or better yet, both. Segfaults are never acceptable. Detecting and caching the OSX version somewhere would be helpful in a lot of places. It would be nice if there was a cleaner way of doing it than running respondsToSelector on a series of methods introduced in each version, but I haven't been able to find one. Ideally done while initializing the backend, it needs to be stored somewhere that's easy to retrieve without passing around an extra parameter.
(In reply to comment #33) > 2) In another bug, John has indicated that respondsToSelector: can be quite > expensive. I think we should do this check once and then cache the value in > the Window or Display object. Since we're only really doing it for version detection, presumably a "static gboolean macos_at_least_lion" somewhere would do the trick?
(In reply to comment #35) > (In reply to comment #33) > > 2) In another bug, John has indicated that respondsToSelector: can be quite > > expensive. I think we should do this check once and then cache the value in > > the Window or Display object. > > Since we're only really doing it for version detection, presumably a "static > gboolean macos_at_least_lion" somewhere would do the trick? Either that prefixed with _gdk_ or putting it in Display should work. Currently there is only one display to deal with in the quartz backend (it is even defined globally as _gdk_display...), however, if you compile for both quartz and x11 at the same time you might get into trouble. So a static gboolean might be safest.
Head-slap time. We should just parse the release element from uname (3). I'd rather have a guint with the actual version than a gboolean. Might be useful in other places (ATSUI vs. CoreText comes to mind), and it's more future-proof. A global function gdk_quartz_getversion() to retrieve it would be great.
Created attachment 192841 [details] [review] Test for resizing in a separate function I did a little experimentation, printing out the mouse event types in the resize area. It turns out that Kris is right: Cocoa emits an NSLeftMouseDown at the beginning of the resize and then eats all of the NSLeftMouseDragged events so that we never see them. As I've said, I don't really like overloading the "find-the-window" function with the resize detection, so this patch pulls it out into its own function. Since there's no grab it should also avoid the segfault from grab->window being NULL, and it doesn't do anything about that (though I think that we should). It's also using a respondsToSelector to detect Lion. I'll write up a separate patch for using uname for that. This is against Gtk2 so that Michael and Jeffrey can test it; it should port over to master pretty easily.
Created attachment 192844 [details] [review] Functions for setting and retrieving the running version of OSX Creates an enum type for the version and a function which runs from gdk_display_quartz_init to set a static variable with the appropriate value of GdkOSXVersion and a getter function, gdk_quartz_osx_version() to retrieve it. The test in test_resize() becomes: if (gdk_quartz_osx_version () >= GDK_OSX_LION && (x < GDK_LION_RESIZE || x > toplevel_private->width - GDK_LION_RESIZE || y > toplevel_private->height - GDK_LION_RESIZE)) and we can use the same in gdk_window_set_decorations. It will be faster, but it will still emit the compiler warning when built with older SDKs.
(In reply to comment #39) > Created an attachment (id=192844) [details] [review] > Functions for setting and retrieving the running version of OSX > I like using an enumeration and function call. However, perhaps we can better use Gestalt instead of uname? This is suggested in: http://www.cocoadev.com/index.pl?DeterminingOSVersion
(In reply to comment #38) > Created an attachment (id=192841) [details] [review] > Test for resizing in a separate function > > I did a little experimentation, printing out the mouse event types in the > resize area. It turns out that Kris is right: Cocoa emits an NSLeftMouseDown at > the beginning of the resize and then eats all of the NSLeftMouseDragged events > so that we never see them. Great! I knew it would have to work like this, otherwise it becomes pretty problematic. Right now the patch pretty much looks like what I would expect. > As I've said, I don't really like overloading the "find-the-window" function > with the resize detection, so this patch pulls it out into its own function. > Since there's no grab it should also avoid the segfault from grab->window being > NULL, and it doesn't do anything about that (though I think that we should). I agree. > This is against Gtk2 so that Michael and Jeffrey can test it; it should port > over to master pretty easily. I am eager to hear how this works for Michael and Jeffrey.
Created attachment 192882 [details] [review] New version patch using Gestalt() instead of uname() Nice find. In addition to being the Apple-sanctioned way, it saves the sscanf. Since it's now basically a lookup-and-translate function, there's no need to cache the result, so I've collapsed it to a single function and no static variable.
I'll try to test these latest updates soon. Just swamped in other bugs atm.
On my queue too. I've been stuck bootcamped into Windows handing some other urgent stuff.
Comment on attachment 192841 [details] [review] Test for resizing in a separate function For the lion check, I gather there is no y < check because we don't handle the title bar? Should this be commented? If the lion check is replaced with the code from comment 42, I am fine with accepting this for 2-24 and master.
Comment on attachment 192882 [details] [review] New version patch using Gestalt() instead of uname() My only worry here was whether calling Gestalt() on each event would introduce a performance problem. I would propose we commit it like this and if Gestalt() calls show up in profiles at some point, we can always add a cache for it.
Committed. Master: http://git.gnome.org/browse/gtk+/commit/?id=fc7dfd7246d31c4007ab64957345c113492f6f5c and http://git.gnome.org/browse/gtk+/commit/?id=f84c787be4fae777eeaf6e99538284582ad4ed50 2-24: http://git.gnome.org/browse/gtk+/commit/?h=gtk-2-24&id=9edc6ca045499b49bf48cff5fdd36aa6cea25c86 and http://git.gnome.org/browse/gtk+/commit/?h=gtk-2-24&id=a2a85a194c9a1fd3bf27e1087589e0d6c6ee9d48
*** Bug 656493 has been marked as a duplicate of this bug. ***
*** Bug 662732 has been marked as a duplicate of this bug. ***