Created attachment 309895 [details]
Crash Report

Can you rebuild gtk+ with -g in CFLAGS and crash it again? It would help a lot to have the actual line number of the crash. Even better would be to crash it in lldb so that you can find out exactly what variable is a bad pointer.

Created attachment 309977 [details]
Output of crash run in lldb with debugging symbols.

Tantalizingly close. After the crash, use the bt command to get a stack trace and paste that in.

Created attachment 309979 [details]
Output of crash run in lldb with debugging symbols and backtrace!

I've got there eventually, sorry for being so slow with all the debugging information!

Not an OS X developer, but the code in imquartz.c looks questionable to me - in several places, it uses the GDK_IS_WINDOW macro on the object returned by gdk_quartz_window_get_nsview(), which is not a GObject, so things are expected to crash...

Created attachment 311677 [details] [review]
Check that parent window is a GdkWindowQuartz.

(In reply to Matthias Clasen from comment #7)
> Not an OS X developer, but the code in imquartz.c looks questionable to me -
> in several places, it uses the GDK_IS_WINDOW macro on the object returned by
> gdk_quartz_window_get_nsview(), which is not a GObject, so things are
> expected to crash...

Indeed. The guy who implemented that was convinced that gdk_quartz_window_get_nsview() sometimes returns a GdkWindow*, but I don't see how that could be unless the GdkWindow is sometimes not a GdkWindowQuartz.

This patch instead checks if the parent window is really a GdkWindowQuartz; as long as that's true the returned NSView should be either a GdkQuartzNSView or nil.

OP, please test to see if that resolves your crash.

Created attachment 312565 [details] [review]
Patch for gtk+ 2.24

I came across the same crash in in gtk+ 2.24 (and also in quartz_set_cursor_location). gtk2 doesn't have the GDK_IS_QUARTZ_WINDOW macro, so I just left that part out.

I have tested that the fix does indeed work in discard_preedit() and quartz_set_cursor_location().

Is there a reason why this patch didn't make it into 3.18.2? It looks like it was committed in master several weeks before this version was released.

It's a pretty important one since it seems to break a lot of gtk apps running on El Capitan.

That's because Matthias didn't backport it to the gtk-3-18 branch. I just did so, so it will be in Gtk-3.18.3.

Thanks John!

(In reply to David Lechner from comment #9)
> Created attachment 312565 [details] [review] [review]
> Patch for gtk+ 2.24
> 
> I came across the same crash in in gtk+ 2.24 (and also in
> quartz_set_cursor_location). gtk2 doesn't have the GDK_IS_QUARTZ_WINDOW
> macro, so I just left that part out.

These checks cannot just be left out. While upgrading GTK+ 2.x for GIMP, we found that this commit is causing crashes.

I will be attaching two patches that I would like to commit to gtk-2-24 to fix this issue. The fix is to re-introduce the checks in a different way.

Created attachment 331745 [details] [review]
[PATCH 1/2] quartz: introduce gdk_quartz_window_is_quartz function

Created attachment 331746 [details] [review]
[PATCH 2/2] imquartz: fix regression introduced by commit 4ba1fb

Wouldn't it be cleaner and simpler to just put that check in gdk_quartz_window_get_nsview() and return NULL if it it fails? Is there a case somewhere where calling gdk_quartz_window_get_nsview on a GdkWindow* that isn't a GdkWindowImplQuartz* would somehow return a usable value?

 NSView *
 gdk_quartz_window_get_nsview (GdkWindow *window)
 {
-  if (GDK_WINDOW_DESTROYED (window))
+  if (!GDK_WINDOW_IS_QUARTZ (window) || GDK_WINDOW_DESTROYED (window))
     return NULL;
 
   return ((GdkWindowImplQuartz *)window->impl)->view;
 }

Then imquartz.c would need only add two checks for nsview == NULL.

Just make the macro public, instead of adding oddd function wrappers; look at what the x11 and wayland backends do. At least, thats my opinion.

(In reply to Matthias Clasen from comment #17)
> Just make the macro public, instead of adding oddd function wrappers; look
> at what the x11 and wayland backends do. At least, thats my opinion.

The macro is already public in 3.x and there are no problems there. In 2.x, the macro involves GdkWindowImpl which is all kept private.

I chose for a function wrapper, because the win32 backend has that too, probably to work around the same problem:

gboolean      gdk_win32_window_is_win32 (GdkWindow *window);

gboolean
gdk_win32_window_is_win32 (GdkWindow *window)
{
  return GDK_WINDOW_IS_WIN32 (window);
}

(In reply to John Ralls from comment #16)
> Wouldn't it be cleaner and simpler to just put that check in
> gdk_quartz_window_get_nsview() and return NULL if it it fails? Is there a
> case somewhere where calling gdk_quartz_window_get_nsview on a GdkWindow*
> that isn't a GdkWindowImplQuartz* would somehow return a usable value?

Simpler yes, cleaner I am not sure. I guess in theory the _get_nsview function needs a g_return_val_if_fail check for GDK_WINDOW_IS_QUARTZ(). If you want a silent check, which is what we want, we need the separate macro or function.

(In reply to Kristian Rietveld from comment #19)
> (In reply to John Ralls from comment #16)
> > Wouldn't it be cleaner and simpler to just put that check in
> > gdk_quartz_window_get_nsview() and return NULL if it it fails? Is there a
> > case somewhere where calling gdk_quartz_window_get_nsview on a GdkWindow*
> > that isn't a GdkWindowImplQuartz* would somehow return a usable value?
> 
> Simpler yes, cleaner I am not sure. I guess in theory the _get_nsview
> function needs a g_return_val_if_fail check for GDK_WINDOW_IS_QUARTZ(). If
> you want a silent check, which is what we want, we need the separate macro
> or function.

What I proposed is silent, though I agree that it would be better to log the error. It's cleaner because returning NULL instead of some undefined pointer is surely correct for _get_nsview(), as that will force the inevitable crash to be closer to its source.

Created attachment 332889 [details] [review]
Updated [PATCH 2/2] imquartz: fix regression introduced by commit 4ba1fb

As suggested in the latest comment, put an assertion in gdk_quartz_window_get_nsview() so we also avoid returning a garbled pointer.

Comment on attachment 332889 [details] [review]
Updated [PATCH 2/2] imquartz: fix regression introduced by commit 4ba1fb

Looks good, thanks.

Pushed to gtk-2-24. I leave it up to you whether you also want to have it pushed to master.


commit 899147d6d06d375d992ae28d250329b7b35a3ab4
Author: Kristian Rietveld <kris@loopnest.org>
Date:   Mon Jul 18 22:00:48 2016 +0200

    imquartz: fix regression introduced by commit 4ba1fb

    In the preceding fix, the checks involving GDK_IS_QUARTZ_WINDOW macros
    were left out. These checks are in fact crucial, because these functions
    are sometimes called with non-quartz functions as the original comments
    in the code do indicate. Therefore, reintroduce these checks. This
    fixes a crash in GIMP.

commit 6b8e20a05cde19500c0fb97576cc22c3ef1088dc
Author: Kristian Rietveld <kris@loopnest.org>
Date:   Mon Jul 18 21:59:28 2016 +0200

    quartz: introduce gdk_quartz_window_is_quartz function

    This function can be used to check whether a GdkWindow is a quartz
    window. It is equivalent to it's win32 counterpart. The function
    is necessary because the macro necessary for this check is private.