GNOME Bugzilla – Bug 634510
Don't create GtkTextBuffer in notify_buffer, when view is being destroyed.
Last modified: 2010-11-13 11:34:47 UTC
Created attachment 174195 [details] Test case. I'm attaching small test case reproducing it and in next post a patch that alleviates it. Upon a destruction of GtkSourceView, gtk_text_view_destroy sets its buffer to NULL and sends a notification, which runs notify_buffer. And notify_buffer recreates a buffer in GtkTextView, so in gtk_text_view_finalize an assertion fails: Gtk:ERROR:gtktextview.c:3002:gtk_text_view_finalize: assertion failed: (priv->buffer == NULL) Aborted (core dumped)
Created attachment 174196 [details] [review] If view is being destroyed then do nothing in notify_buffer. This makes view not calling gtk_text_view_get_buffer which in turn can create a new buffer.
This seems like a bug introduced due to gseal as we were using the direct accessor to avoid this problem before. The patch looks good to me, but I want pbor to give it another check.
thanks for testing this and proving the bug. Indeed this is something that sneaked back during the gtk seal work. As a matter of fact in gtksourceview 2 we had static void notify_buffer (GtkSourceView *view) { /* we use view->buffer directly instead of get_buffer() * since the latter causes the buffer to be recreated and that * is not a good idea when finalizing */ set_source_buffer (view, GTK_TEXT_VIEW (view)->buffer); } and in the current master we have /* notify_buffer() would recreate the buffer if it is set to null, * and we don't want that to happen when finalizing */ g_signal_handlers_disconnect_by_func (view, notify_buffer, NULL); set_source_buffer (view, NULL); So either your patch should remove the old comment and code and comment (since according to the text is not working, or it should fix the current workaround. If we understand why it does not work, I think I find the current workaround slightly cleaner, but I am open to both strategies as long as the clear comment is preserved. If then you also turn your testcase in a unit test, that would be super :)
(In reply to comment #3) > thanks for testing this and proving the bug. Indeed this is something that > sneaked back during the gtk seal work. > > As a matter of fact in gtksourceview 2 we had > > static void > notify_buffer (GtkSourceView *view) > { > /* we use view->buffer directly instead of get_buffer() > * since the latter causes the buffer to be recreated and that > * is not a good idea when finalizing */ > set_source_buffer (view, GTK_TEXT_VIEW (view)->buffer); > } > > and in the current master we have > > /* notify_buffer() would recreate the buffer if it is set to null, > * and we don't want that to happen when finalizing */ > g_signal_handlers_disconnect_by_func (view, notify_buffer, NULL); > set_source_buffer (view, NULL); > > So either your patch should remove the old comment and code and comment (since > according to the text is not working, or it should fix the current workaround. > > If we understand why it does not work, I think I find the current workaround > slightly cleaner, but I am open to both strategies as long as the clear comment > is preserved. I just started wondering where the fix should be made now. Looking at code in gtktextview.c:3002: 3002:g_assert (priv->buffer == NULL); 3002: 3003:gtk_text_view_destroy_layout (text_view); 3004:gtk_text_view_set_buffer (text_view, NULL); This made me thinking whether the assertion shouldn't be moved below the gtk_text_view_set_buffer () to catch if some notification didn't set the buffer again. If this is a bug in Gtk+ then nothing in GtkSourceView needs to be done, because when gtk_text_view_finalize () is called, the buffer notification is already disconnected. If this is not a bug in Gtk+, then there are two options: 1. Disconnect the notification in gtk_source_view_dispose (), not in gtk_source_view_finalize (). 2. Use the patch I provided earlier, with additional comment of course. This is because that notify_buffer will do nothing in disposal stage. But in finalize stage, a flag about widget being in destruction is cleared, so notify_buffer will recreate the buffer again. That's why it needs to be disconnected before finalizing GtkTextView. > If then you also turn your testcase in a unit test, that would be super :) Sure, will do if above doubts are cleared.
Created attachment 174252 [details] [review] Unitized test case from first post. I'm not silencing stderr in fork test.
Created attachment 174253 [details] [review] Unitized test case from first post with corrected whitespaces. Changed double spaces to tabs.
I'll try to read your comment more carefully during the week end. But in general I think I like the approach of disconnecting in dispose() For the test case maybe add a comment explaining what is testing and even a reference to this bug number. Thanks again for the early adoption in bindings and for contributing feedback and patches, it is really appreciated.
Created attachment 174359 [details] [review] Disconnect the notify buffer in dispose. Adding just in case. Instead of changing notify_buffer body, a disconnection is moved to gtk_source_view_dispose ().
Created attachment 174360 [details] [review] Unitized test case from first post with corrected whitespaces and added comment. Added a comment in test case.
Review of attachment 174359 [details] [review]: ::: gtksourceview/gtksourceview.c @@ +1702,3 @@ + /* notify_buffer() would recreate the buffer if it is set to null, + * and we don't want that to happen when destroying/finalizing */ + g_signal_handlers_disconnect_by_func (view, notify_buffer, NULL); note that dispose may run multiple times... I guess we could save the connection id and then check that is different from 0 and use g_signal_disconnect @@ -1745,1 @@ set_source_buffer (view, NULL); shouldn't set_source_buffer be moved too?
Review of attachment 174360 [details] [review]: ::: tests/test-getbuffer.c @@ +42,3 @@ + gtk_test_init (&argc, &argv); + + g_test_add_func ("/Buffer/get-buffer", test_get_buffer); I think it is usually a good idea have very descriptive names for the unit test... in fact I think it is a good idea to have the bug number in the test name. What about g_test_add_func ("/Buffer/bug-XXXXX", test_get_buffer);
Created attachment 174367 [details] [review] Disconnect the notify buffer in dispose once. set_source_buffer () is now moved to dispose () too. Probably putting it in 'if' block instead should be safe...
Created attachment 174368 [details] [review] Unitized test case from first post with corrected whitespaces and added comment and changed name.
Review of attachment 174367 [details] [review]: Patch now looks good to me (though I have not tested it). I am assuming it actually fixes the problem you were observing and that your unit test passes
Review of attachment 174368 [details] [review]: I did not test, I am assuming the test actually passes after your patch :)
Yes, now tests in both gtksourceview and its C++ bindings pass. :) I was testing test-widget in C library and test-completion in both libraries and none were misbehaving. But note that I filed a bug to Gtk+ as well - bug 634677. But probably it is unlikely to be fixed soon, so it is good that bug got fixed here for now. Anyway, patches are pushed.
Oh, and I assume that it is known that one test in language manager fails: /LanguageManager/guess-language: ** GtkSourceView:ERROR:test-languagemanager.c:109:test_guess_language: assertion failed (gtk_source_language_get_id (l) == "xslt"): ("xml" == "xslt") /bin/sh: line 5: 11883 Aborted (core dumped) ${dir}$tst FAIL: test-languagemanager If not, maybe another bug report should be filed.
You need to make sure shared-mime-info is updated and installed in your prefix for that test to work
I see, now it works, thanks. Maybe gtksourceview should depend on shared-mime-info in jhbuild?
good idea. I pushed that change.