GNOME Bugzilla – Bug 732784
Crashes when closing the 2nd window
Last modified: 2014-08-10 08:05:59 UTC
epiphany-3.12.1-2.fc21.x86_64 With 2 windows opened, I closed one of those and epiphany crashed:
+ Trace 233776
It happen to me just when I try to close the 2nd window with accels (i.e. with Ctrl+W I get crash, with the close button I didn't). (epiphany:24460): GLib-GObject-WARNING **: gsignal.c:2579: instance '0xa7d950' has no handler with id '7035' (epiphany:24460): GLib-GObject-CRITICAL **: g_object_unref: assertion 'G_IS_OBJECT (object)' failed Program received signal SIGSEGV, Segmentation fault. g_type_check_instance_is_fundamentally_a (type_instance=type_instance@entry=0xbb6990, fundamental_type=fundamental_type@entry=80) at gtype.c:3977 3977 return NODE_FUNDAMENTAL_TYPE(lookup_type_node_I (type_instance->g_class->g_type)) == fundamental_type; (gdb) bt
+ Trace 233777
I think the warning isn't new, just the crash it new (since some changes in glib from git master). It hard to me to understand where the leak, but it look like this is a leak in EphyTitleBox, but I not sure.
Created attachment 279974 [details] [review] ephy-title-box: fix crash when closing second window These binding could already have been destroyed. It might be safe to simply remove the calls to g_clear_object, since they are not needed for memory management, but then the bindings might not be unbound if a new web view is set before the old one is destroyed. This patch may be unnecessarily verbose, since when I simply removed the calls to g_clear_object I did not notice any regressions.
Review of attachment 279974 [details] [review]: Looks good to me. I make more patch to fix more a warning there. ::: src/ephy-title-box.c @@ +518,3 @@ +static void +title_binding_destroyed_cb (gpointer data) Please, added the ephy_title_box_ prefix before, as in the rest of the code in this file. @@ +531,3 @@ + +static void +uri_binding_destroyed_cb (gpointer data) Ditto. @@ +600,3 @@ + G_BINDING_SYNC_CREATE, + NULL, NULL, + title_box, title_binding_destroyed_cb); Ditto. @@ +606,3 @@ G_BINDING_SYNC_CREATE, + ephy_title_box_transform_uri_to_label, NULL, + title_box, uri_binding_destroyed_cb); Ditto.
Review of attachment 279974 [details] [review]: Sorry, I forgot more a trivial comment. ::: src/ephy-title-box.c @@ +525,3 @@ + title_box = EPHY_TITLE_BOX (data); + priv = ephy_title_box_get_instance_private (title_box); + Added here a log messgae (without the ephy_title_box_ prefix): LOG ("title_binding_destroyed_cb title-box %p", title_box); @@ +538,3 @@ + title_box = EPHY_TITLE_BOX (data); + priv = ephy_title_box_get_instance_private (title_box); + Same here: LOG ("uri_binding_destroyed_cb title-box %p", title_box);
Created attachment 279977 [details] [review] ephy-title-box: Fix a warning when closing window I'm not sure if my patch is right, so please look into it :-)
My patch above needs to fix this warning: (epiphany:9702): GLib-GObject-WARNING **: gsignal.c:2579: instance '0xca8170' has no handler with id '1732'
Created attachment 279985 [details] [review] ephy-title-box: fix crash when closing second window These binding could already have been destroyed. It might be safe to simply remove the calls to g_clear_object, since they are not needed for memory management, but then the bindings might not be unbound if a new web view is set before the old one is destroyed.
Review of attachment 279985 [details] [review]: It ok the fact I reviewing patches in epiphany's bugs? (please, make the first word in the title to title case i.e. fix -> Fix)
Review of attachment 279977 [details] [review]: Looks good to me; I'm not sure why the handler is being disconnected early, but this is cleaner anyway.
Created attachment 279986 [details] [review] ephy-title-box: Fix crash when closing second window
(In reply to comment #8) > Review of attachment 279985 [details] [review]: > > It ok the fact I reviewing patches in epiphany's bugs? I dunno, but since you wrote this file I'm not too worried.
Review of attachment 279986 [details] [review]: Pushed as 005ce53 - ephy-title-box: Fix crash when closing second window
Review of attachment 279977 [details] [review]: Pushed as 7eabfe4 - ephy-title-box: Fix a warning when closing window
Pushed to master and gnome-3-12.
(In reply to comment #8) > Review of attachment 279985 [details] [review]: > > It ok the fact I reviewing patches in epiphany's bugs? No, we have maintainers for that. Patches are indeed wrong. > (please, make the first word in the title to title case i.e. fix -> Fix) Patches here workaround the problem, instead of fixing. When you start a commit message with "For some reason" is because you haven't investigated enough the problem :-) The real problem is that the web view is destroyed and nobody has called ephy_title_box_set_web_view() with NULL before, so when it's called with a new web view, the old web view is already destroyed, but the pointer hasn't been invalidated. That's why you get warnings about signals not present in the object, a warning also when trying to unref the view, and the bindings are already disconnected, because g_binding uses weak references. So, the EphyWindow should call ephy_title_box_set_web_view() with NULL when it's destroyed, but in ephy_window_dispose it's too late, because the children have already been destroyed at that point. I think EphyTitleBox should not take a reference to the view, and use a weak reference instead.
Yosef, I know you're working on this; I'm just assigning this to myself since my patch was pushed and I don't want to lose track of it.
Created attachment 281319 [details] [review] Revert "ephy-title-box: Fix a type" This reverts commit 5c8005ec5adb1b9432d3390e73de5213899502cb.
Created attachment 281320 [details] [review] Revert "ephy-title-box: Fix a warning when closing window" This reverts commit 7eabfe44e9804717bfb6f0fa10e53f9c4d170e38.
Created attachment 281321 [details] [review] Revert "ephy-title-box: Fix crash when closing second window" This reverts commit 005ce536fd94db9fca3906a358ca555a3e557771.
Created attachment 281322 [details] [review] ephy-title-box: Fix crash when closing second window The web view can be destroyed out from under us, so we need to use a weak reference to determine if it is still alive.
Michael, I think we not must to revert the patches, they just do some unrelated cleanup, but it still cleanup.
My patch should be reverted since it's unnecessary and messy compared to using a weak reference. Your middle patch "Fix a warning when closing window" indeed does not need to be reverted, I just think it's nicer to store the signal ID.
Carlos, ping :)
Pushed all patches, thanks you!