GNOME Bugzilla – Bug 629055
GtkSourceCompletion show-headers property: Critical messages and crash
Last modified: 2012-04-10 15:29:56 UTC
Created attachment 169760 [details] test-completion.c modified If we set show-headers to false, and if the proposals data change, there are critical messages and if we want to scroll in the proposals the program crashes. I've modified a bit test-completion.c (see attachment). There are two lists of proposals, test_provider_populate() switches between the two at each call. Some output messages: (test-completion:3867): Gtk-CRITICAL **: gtktreeview.c:4958 (gtk_tree_view_bin_expose): assertion `has_next' failed. There is a disparity between the internal view of the GtkTreeView, and the GtkTreeModel. This generally means that the model has changed without letting the view know. Any display from now on is likely to be incorrect. (test-completion:3867): GtkSourceView-CRITICAL **: gtk_source_completion_model_iter_is_header: assertion `iter->user_data != NULL' failed (test-completion:3867): Gtk-CRITICAL **: gtktreeview.c:6017 (validate_visible_area): assertion `has_next' failed. There is a disparity between the internal view of the GtkTreeView, and the GtkTreeModel. This generally means that the model has changed without letting the view know. Any display from now on is likely to be incorrect. (test-completion:3867): GtkSourceView-CRITICAL **: gtk_source_completion_model_iter_is_header: assertion `iter->user_data != NULL' failed (test-completion:3867): GtkSourceView-CRITICAL **: tree_model_get_value: assertion `iter->user_data != NULL' failed (test-completion:3867): GLib-GObject-WARNING **: gtype.c:4181: type id `0' is invalid (test-completion:3867): GLib-GObject-WARNING **: can't peek value table for type `<invalid>' which is not currently referenced Segmentation fault (core dumped)
I encountered similar errors and occasional crash while writing code completion for my python Go plugin (go-gedit-plugin). Setting show-headers to false seemed to fix the problem. sys:1: GtkWarning: gtktreeview.c:5011 (gtk_tree_view_bin_expose): assertion `has_next' failed. There is a disparity between the internal view of the GtkTreeView, and the GtkTreeModel. This generally means that the model has changed without letting the view know. Any display from now on is likely to be incorrect. example messages: (gedit:4419): GtkSourceView-CRITICAL **: gtk_source_completion_model_iter_is_header: assertion `iter->user_data != NULL' failed sys:1: GtkWarning: gtktreeview.c:6070 (validate_visible_area): assertion `has_next' failed. There is a disparity between the internal view of the GtkTreeView, and the GtkTreeModel. This generally means that the model has changed without letting the view know. Any display from now on is likely to be incorrect. sys:1: GtkWarning: gtktreeview.c:5011 (gtk_tree_view_bin_expose): assertion `has_next' failed. There is a disparity between the internal view of the GtkTreeView, and the GtkTreeModel. This generally means that the model has changed without letting the view know. Any display from now on is likely to be incorrect. (gedit:4502): GtkSourceView-CRITICAL **: gtk_source_completion_model_iter_is_header: assertion `iter->user_data != NULL' failed sys:1: GtkWarning: gtktreeview.c:6070 (validate_visible_area): assertion `has_next' failed. There is a disparity between the internal view of the GtkTreeView, and the GtkTreeModel. This generally means that the model has changed without letting the view know. Any display from now on is likely to be incorrect.
(In reply to comment #1) > Setting show-headers to false seemed to fix the problem. I suppose you meant 'true'.
(In reply to comment #2) > (In reply to comment #1) > > Setting show-headers to false seemed to fix the problem. > > I suppose you meant 'true'. Oops, you're right.
Created attachment 211647 [details] [review] test-completion: vary the proposals on populate
Created attachment 211648 [details] [review] CompletionModel: set correct flag
Created attachment 211649 [details] [review] CompletionModel: remove empty signals
Created attachment 211650 [details] [review] CompletionModel: fix the value of iter.user_data
Created attachment 211651 [details] [review] CompletionModel: fix provider visibility update
Created attachment 211652 [details] [review] CompletionModel: fix header visibility update
Created attachment 211653 [details] [review] CompletionModel: fix node removal
Created attachment 211654 [details] [review] CompletionModel: fix unmarked nodes removal
Review of attachment 211647 [details] [review]: While I completely agree that the test should cover the case of varying proposals, this seems a tad too arbitrary and I wonder if someone looking at the testcase in a couple of months would understand whats going on... at the very least we need a comment in the source code, but I wonder if we can come up with something that is a bit more predictable... to trigger the bug do we need to switch list on the same word or would it crash even if we showed different lists for different words? If the latter we could use a list for "normal words" and a different list for "special words (e.g. words made of digits or words in upper case)
Review of attachment 211648 [details] [review]: look good
Review of attachment 211649 [details] [review]: dunno... not strong opinion here, but I kind of like the idea that when implementing an interface we implement all its methods, even if they do nothing. Anyway, I guess this patch is just cleanup and not crucial for the bugfix Jesse what do you think?
Review of attachment 211650 [details] [review]: Hard to review without context... I am assuming it is correct, so go ahead and commit, but it would be cool if the commit message was a bit more articulate on the reason instead of just stating the obvious
(In reply to comment #12) > Review of attachment 211647 [details] [review]: > > While I completely agree that the test should cover the case of varying > proposals, this seems a tad too arbitrary and I wonder if someone looking at > the testcase in a couple of months would understand whats going on... at the > very least we need a comment in the source code, but I wonder if we can come up > with something that is a bit more predictable... to trigger the bug do we need > to switch list on the same word or would it crash even if we showed different > lists for different words? If the latter we could use a list for "normal words" > and a different list for "special words (e.g. words made of digits or words in > upper case) To trigger the bug, the completion window must not disappear when the proposals are varying. In a real use of completion, this typically occur when the proposals are removed to keep only those with a certain prefix, as the user types more characters. But I don't know why, the word completion doesn't trigger the bug.
(In reply to comment #14) > Review of attachment 211649 [details] [review]: > > dunno... not strong opinion here, but I kind of like the idea that when > implementing an interface we implement all its methods, even if they do > nothing. I looked at GtkListStore, and the signals are not implemented, so I think it's correct to remove them. > Anyway, I guess this patch is just cleanup and not crucial for the bugfix Yes indeed.
Review of attachment 211651 [details] [review]: Makes sense. some nitpicks below, but other than that it looks fine to commit ::: gtksourceview/gtksourcecompletionmodel.c @@ +887,3 @@ GtkTreePath *path = NULL; + g_return_if_fail (GTK_SOURCE_IS_COMPLETION_MODEL (model)); no need for the g_return_if_fail in internal functions (I know current code contradicts this rule, but let's not add to it ;) @@ +891,3 @@ + g_return_if_fail (info->first != NULL); + + if (info->filtered == FALSE) if (!info->filtered) @@ +901,1 @@ + while (item != NULL) not your code, but since you touch it, as a general rule I'd rather use a for loop, makes easy to not forget list_next() if we ever use "continue"
Review of attachment 211652 [details] [review]: looks good ::: gtksourceview/gtksourcecompletionmodel.c @@ +856,1 @@ + g_return_if_fail (info != NULL); no need
(In reply to comment #15) > Review of attachment 211650 [details] [review]: > > Hard to review without context... I am assuming it is correct, so go ahead and > commit, but it would be cool if the commit message was a bit more articulate on > the reason instead of just stating the obvious iter.user_data must contain a GList*. Here in the context, 'item' is the GList*, and 'node' is item.data.
Created attachment 211659 [details] [review] CompletionModel: fix provider visibility update
Review of attachment 211653 [details] [review]: I admit I do not have a full handle on this code, so if Jesse wants to jump in he's more than welcome to do so... From my understanding the patch looks sane and if it fixes the problem it can go in. One nitpick below... ::: gtksourceview/gtksourcecompletionmodel.c @@ +840,1 @@ model->priv->store = g_list_delete_link (model->priv->store, can the delete_link be moved up and the two "if" merged?
Review of attachment 211654 [details] [review]: Same as previous patch... I do not have a full understanding of this code without studying it in more detail. Since it seems to fix the bug and I do not see obvious problems, I am ok with committing it
Created attachment 211660 [details] [review] CompletionModel: fix header visibility update A g_return_if_fail is still there, for the others I understand why it was not needed, but for this one it's safer to keep it, because at some moments a ProviderInfo can have its 'first' field as NULL.
(In reply to comment #22) > Review of attachment 211653 [details] [review]: > > I admit I do not have a full handle on this code, so if Jesse wants to jump in > he's more than welcome to do so... > > From my understanding the patch looks sane and if it fixes the problem it can > go in. > > One nitpick below... > > ::: gtksourceview/gtksourcecompletionmodel.c > @@ +840,1 @@ > model->priv->store = g_list_delete_link (model->priv->store, > > can the delete_link be moved up and the two "if" merged? if (!node->filtered && path == NULL) { // Here 'item' must still be in the store to compute its path. // So the delete_link() must be done after. ppath = path_from_list (model, item); } if (node->proposal != NULL) { // This can be done after delete_link(), no problem. g_hash_table_remove (info->proposals, node->proposal); } model->priv->store = g_list_delete_link (model->priv->store, item);
Created attachment 211685 [details] [review] test-completion: a fixed and a random provider
(In reply to comment #24) > Created an attachment (id=211660) [details] [review] > CompletionModel: fix header visibility update > > A g_return_if_fail is still there, for the others I understand why it was not > needed, but for this one it's safer to keep it, because at some moments a > ProviderInfo can have its 'first' field as NULL. If it can, then there should be a proper check on NULL, not an assertion/warning kind of check. If the function can never be called with info->first is NULL, then no check is needed.
All patches look good to go for me, great work!
(In reply to comment #27) > (In reply to comment #24) > > Created an attachment (id=211660) [details] [review] [details] [review] > > CompletionModel: fix header visibility update > > > > A g_return_if_fail is still there, for the others I understand why it was not > > needed, but for this one it's safer to keep it, because at some moments a > > ProviderInfo can have its 'first' field as NULL. > > If it can, then there should be a proper check on NULL, not an > assertion/warning kind of check. What do you mean by a "proper check"? A simple if/return? The advantage of the g_return_if_fail is that there is a warning if for whatever reason it occurs, so we can see that there is a problem somewhere that should be fixed. > If the function can never be called with > info->first is NULL, then no check is needed. It's difficult to be 100% sure of that. Also, if the code is modified later and if the function is called somewhere else, it's always better to have a little warning than a segfault.