GNOME Bugzilla – Bug 673795
async bookmarks icon handling cannot work in gtk tree model get value for a cell area
Last modified: 2012-09-03 10:26:56 UTC
Created attachment 211671 [details] [review] bookmarks: remove async call in handler for gtk tree model get value gtk cell area apply cell attributes calls gtk tree model get value then unset the gvalue. Passing the gvalue to an async callback in the gtk tree model get value ephy handle calls for trouble and segfault here . Ie the gvalue is unset before use. This probably requires waiting for the async calls to returns. Here I remove the async call completely. Remove async icon_loaded_cb call in handler ephy_tree_model_node_get_value for gtk tree model get value as gtk cell area apply_cell_attributes calls synchroneously gtk_tree_model_get_value with a gvalue passed. This gvalue is passed to ephy_tree_model_node_get_value then provide_favicon which in the async webkit_favicon_database_get_favicon_pixbuf with the icon_loaded_cb handler. The latter takes the former gvalue as user_data. The async calls returns immediatly then we return to apply_cell_attributes which gets the gvalue and unset it. Then the gvalue is reused for other data type. Though webkit_favicon_database_get_favicon_pixbuf then executes the icon_loaded_cb callback passing it the then unset and reused gvalue as user_data. This leads to G_VALUE_HOLDS_OBJECT critical error from glib then segfault from ephy bookmarks icon_loaded_cb code.
*** Bug 681839 has been marked as a duplicate of this bug. ***
Review of attachment 211671 [details] [review]: The patch fixes the critical warnings, and a potencial crash because of the stack allocated memory usage, but of course it doesn't fix the real issue. We need a way to update the model with the icon when it's loaded. In any case I would apply this patch for now, because current code is broken.
Sergio wrote this originally, he should have a look at it.
So, I am not an expert on EphyNode but I guess the right fix in this case is to get a GtkTreeRowReference in provide_favicon() if webkit_favicon_database_try_get_favicon_pixbuf() does not return anything. That row reference is passed as user_data to the asynchronous call. Once the callback is invoked, we just need to force a repaint using gtk_tree_model_row_changed(). Does it make sense? Is it possible to get that row reference from an EphyNode?
(In reply to comment #4) > So, I am not an expert on EphyNode but I guess the right fix in this case is to > get a GtkTreeRowReference in provide_favicon() if > webkit_favicon_database_try_get_favicon_pixbuf() does not return anything. That > row reference is passed as user_data to the asynchronous call. > > Once the callback is invoked, we just need to force a repaint using > gtk_tree_model_row_changed(). Does it make sense? Is it possible to get that > row reference from an EphyNode? As far as I remember we use an EphyNodeView here, which *is* a GtkTreeView, so it should be possible to get a GtkTreeRowReference. You can also translate back and forth from EphyNodes to GtkTreeIters with the methods in EphyTreeModelNode, in case you need to do that.
So I faced a crasher after launching ephy with the bookmarks window open. The stacktrace is this:
+ Trace 230688
In order to reproduce this I'd say it's enough to open the bookmarks window and close it (or just break it) and then launch it again. EphySession will try to open the bookmarks window and boom!
(In reply to comment #5) > (In reply to comment #4) > > Once the callback is invoked, we just need to force a repaint using > > gtk_tree_model_row_changed(). Does it make sense? Is it possible to get that > > row reference from an EphyNode? > > As far as I remember we use an EphyNodeView here, which *is* a GtkTreeView, so > it should be possible to get a GtkTreeRowReference. You can also translate back > and forth from EphyNodes to GtkTreeIters with the methods in EphyTreeModelNode, > in case you need to do that. You know that code better than me, but gtk_tree_view_get_model(ephy-node-view) will return a GtkTreeModel which is not an EphyTreeModelNode (because the EphyNodeView wraps it with a filter and a sort model). So if I am not wrong, I cannot do anything with the EphyNode if I do not have access to the EphyTreeModelNode.
(In reply to comment #7) > You know that code better than me, but gtk_tree_view_get_model(ephy-node-view) > will return a GtkTreeModel which is not an EphyTreeModelNode (because the > EphyNodeView wraps it with a filter and a sort model). So if I am not wrong, I > cannot do anything with the EphyNode if I do not have access to the > EphyTreeModelNode. Both GtkTreeModelSort and GtkTreeModelFilter have methods to access the underlying model, no?
(In reply to comment #8) > (In reply to comment #7) > > You know that code better than me, but gtk_tree_view_get_model(ephy-node-view) > > will return a GtkTreeModel which is not an EphyTreeModelNode (because the > > EphyNodeView wraps it with a filter and a sort model). So if I am not wrong, I > > cannot do anything with the EphyNode if I do not have access to the > > EphyTreeModelNode. > > Both GtkTreeModelSort and GtkTreeModelFilter have methods to access the > underlying model, no? Sure, I was not sure that you wanted to break the encapsulation as the tree models structure is an internal detail.
Created attachment 222297 [details] [review] Added ephy_node_view_get_iter_for_node() We need this function to create a proper GtkTreeRowReference from a EphyNode.
Created attachment 222298 [details] [review] EphyBookmarksEditor: force a repaint of the row when we get a favicon Instead of trying to set the favicon in an already released GValue we just force a repaint of the row and let provide_favicon set the GValue properly.
Review of attachment 222298 [details] [review]: LGTM ::: src/bookmarks/ephy-bookmarks-editor.c @@ +1486,3 @@ + /* Force repaint. */ + if (gtk_tree_model_get_iter (model, &iter, path)) + gtk_tree_model_row_changed (model, path, &iter); So, this will call provide_favicon again?
(In reply to comment #12) > Review of attachment 222298 [details] [review]: > > LGTM > > ::: src/bookmarks/ephy-bookmarks-editor.c > @@ +1486,3 @@ > + /* Force repaint. */ > + if (gtk_tree_model_get_iter (model, &iter, path)) > + gtk_tree_model_row_changed (model, path, &iter); > > So, this will call provide_favicon again? Yes because that's the callback registered to provide the icon, I mean, I have not deeply followed the code, but as EphyNodeView is a GtkTreeView I assumed that it should work the same way.
Review of attachment 222297 [details] [review]: OK.
Review of attachment 222298 [details] [review]: OK. ::: src/bookmarks/ephy-bookmarks-editor.c @@ +1492,3 @@ + + gtk_tree_row_reference_free (reference); + g_clear_object (&favicon); Using clear_object seems over the top here, since it's a local variable.
Comment on attachment 211671 [details] [review] bookmarks: remove async call in handler for gtk tree model get value This patch is obsolete now, removing from review queue.
All changes have landed.