GNOME Bugzilla – Bug 760012
Segfault when closing app right after window is shown
Last modified: 2016-02-25 19:40:19 UTC
If I run the app and then immediately close the window, the app segfaults. The segfault occurs if I close the app with Alt+F4 or by clicking the close button in the headerbar. (org.gnome.Maps:31332): Clutter-CRITICAL **: clutter_canvas_set_size: assertion 'CLUTTER_IS_CANVAS (canvas)' failed Program received signal SIGSEGV, Segmentation fault. 0x00007fffe45b1d09 in clutter_actor_set_size (self=0xcb1aa0, width=256, height=256) at clutter-actor.c:10716 10716 g_return_if_fail (CLUTTER_IS_ACTOR (self)); (gdb) bt
+ Trace 235859
I think this is a champlain matter. Thanks for filing!
Created attachment 318162 [details] [review] path-layer: Add guards to invalidate callback We can get race condition when disposing a ChamplainPathLayer. Make sure we do not end up with a segmentation fault.
Hmm, I don't think this is the right way to fix the problem - there are lots of idle calls like this and if things the idle calls are properly reffed, they shouldn't get destroyed before the idle is called. First of all we should know why this happens - the invalidate_canvas() function is always called from schedule_redraw() which ref()s the layer so it shouldn't get destroyed before the idle function is called. The path_actor is a child of the layer so it shouldn't be destroyed either. It's true that path_actor itself isn't explicitly reffed for the priv->path_actor variable (and then unreffed in dispose) but that shouldn't matter IMO as if the layer isn't destroyed, the child actor shouldn't get destroyed either. I don't seem to be able to reproduce the problem with the libchamplain demos - could you investigate more when this happens?
(In reply to Jiri Techet from comment #3) > Hmm, I don't think this is the right way to fix the problem - there are lots > of idle calls like this and if things the idle calls are properly reffed, > they shouldn't get destroyed before the idle is called. Agreed. Trying more it seems that with only the guard for IS_PATH_LAYER ... it still segfaults, it is the guard against canvas that helps... so seems layer is properly reffed but the canvas is what has failed ... which is really weird. > > First of all we should know why this happens - the invalidate_canvas() > function is always called from schedule_redraw() which ref()s the layer so > it shouldn't get destroyed before the idle function is called. The > path_actor is a child of the layer so it shouldn't be destroyed either. Agreed. I wonder tho if it might be the canvas_set_size that triggers this. > > It's true that path_actor itself isn't explicitly reffed for the > priv->path_actor variable (and then unreffed in dispose) but that shouldn't > matter IMO as if the layer isn't destroyed, the child actor shouldn't get > destroyed either. > > I don't seem to be able to reproduce the problem with the libchamplain demos > - could you investigate more when this happens? I will try! Thanks!
(In reply to Jonas Danielsson from comment #4) > (In reply to Jiri Techet from comment #3) > > Hmm, I don't think this is the right way to fix the problem - there are lots > > of idle calls like this and if things the idle calls are properly reffed, > > they shouldn't get destroyed before the idle is called. > > Agreed. Trying more it seems that with only the guard for IS_PATH_LAYER ... > it still segfaults, it is the guard against canvas that helps... so seems > layer is properly reffed but the canvas is what has failed ... which is > really weird. > > > > > First of all we should know why this happens - the invalidate_canvas() > > function is always called from schedule_redraw() which ref()s the layer so > > it shouldn't get destroyed before the idle function is called. The > > path_actor is a child of the layer so it shouldn't be destroyed either. > > Agreed. I wonder tho if it might be the canvas_set_size that triggers this. > > > > > It's true that path_actor itself isn't explicitly reffed for the > > priv->path_actor variable (and then unreffed in dispose) but that shouldn't > > matter IMO as if the layer isn't destroyed, the child actor shouldn't get > > destroyed either. > > > > I don't seem to be able to reproduce the problem with the libchamplain demos > > - could you investigate more when this happens? > > I will try! Thanks! So I added this print to invalidate_canvas: g_print ("width: %f, height: %f, canvas: %p\n", width, height, priv->canvas); And got these while reproducing: width: 870,000000, height: 622,000000, canvas: 0x26cc660 width: 256,000000, height: 256,000000, canvas: (nil) *segfault* So canvas is NULL when it gets here, and since width/height is 256 priv->view is also NULL. Which means this is after dispose has run, setting view to NULL. And dispose also does priv->canvas = NULL; So it seems that invalidate_canvas is being called after dispose has run. So something is off with the reffing.
It does not happen all the time, so it is some kind of race somewhere. I could not reproduce with a modified launcher-js or with standard launcher-gtk yet.
*** Bug 760643 has been marked as a duplicate of this bug. ***
I do not see this happening on my systems Maps, only the jhbuild one. So this might be a candidate for git bisect. Hashem: Up for it? Dunno which component though, I guess trying with an older champlain, if that does not reproduce, then git bisect with champlain, but it might also be Maps (but I do not really think so we haven't changed how we use routeLayer in a long time), GTK+, or GJS I think...
Couldn't it be an introspection annotations problem somewhere so e.g. the layer gets passed with (transfer none) to some C code and then it's unreffed when the object that contains it gets destroyed? This doesn't seem to happen on the libchamplain side though - the champlain_view_add_layer() uses default (transfer full) and I don't see other place where this could happen.
The problem occurs after closing Maps once you searched for a route using the sidebar. I tried to use downgraded versions of libchamplain, clutter and clutter-gtk (the ones for F22), but it throws segfaults there too. I tried to test with downgraded versions of glib and gjs too, but I had problems when trying to compile glib and the gjs version was incompatible. Anyway, there are more tests to be done, like playing with different versions of gobject-introspection. Are you sure this doesn't happen in F22/F23?
Created attachment 321351 [details] [review] champlain-view: Remove children from user_layers in dispose
(In reply to Jonas Danielsson from comment #11) > Created attachment 321351 [details] [review] [review] > champlain-view: Remove children from user_layers in dispose Does this solve it for others? I could not reproduce when this is used.
Jonas, can you explain why this is needed? The whole ClutterActor hierarchy should be destroyed together with ChamplainView. If it is needed to destroy the actor explicitly, it would be good to add some comment in the patch why - right now it's not completely obvious.
(In reply to Jiri Techet from comment #13) > Jonas, can you explain why this is needed? The whole ClutterActor hierarchy > should be destroyed together with ChamplainView. If it is needed to destroy > the actor explicitly, it would be good to add some comment in the patch why > - right now it's not completely obvious. no :/ I thought as you that some annotation might be wrong triggering a refcount error. But didn't find one. And I saw this in dispose and got suspicous since user_layers are alloced but never explicitly de-allocated. And doing this seem to fix the issue. But might be by accident since this should be cleaned up by the view cleaning up it's children and so on and so on, right? Might be the gjs gc kicking in after setting the user_layers to NULL? No idea. Btw: Any chance for a release soon? Maps 3.19.90 depends on that enum fix that is unreleased.
Yes, can make a release by the end of the week. Does the patch fix the issue for you? If so maybe it's a good idea to apply it together with some comment saying it's actually a hack and we don't know what's going on but that it fixes this particular issue.
(In reply to Jiri Techet from comment #15) > Yes, can make a release by the end of the week. > Awesome! > Does the patch fix the issue for you? If so maybe it's a good idea to apply > it together with some comment saying it's actually a hack and we don't know > what's going on but that it fixes this particular issue. I think it did, I tried "a bunch of times" at least :) A question: champlain_view_add_layer does not have a "(transfer full):" on the layer so I guess that means we think that the caller owns the layer? Shouldn't we then make sure that it does not get destroyed here? Or should we ref it in in champlain_view_add_layer?
> A question: champlain_view_add_layer does not have a "(transfer full):" on > the layer so I guess that means we think that the caller owns the layer? > Shouldn't we then make sure that it does not get destroyed here? Or should > we ref it in in champlain_view_add_layer? Isn't (transfer full) the default? At least that's how I understand the "default annotations" part here https://wiki.gnome.org/Projects/GObjectIntrospection/Annotations#Transfer I don't know what exactly happens during ClutterActor destruction - I'd suppose children are disposed before parents in which case it should work correctly even without your patch but maybe something else happens before which breaks thing. Anyway, I think your patch is fine - just maybe add some comment explaining why we do it.
Created attachment 322160 [details] [review] champlain-view: Remove children from user_layers in dispose
Applied, thanks.