After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 634510 - Don't create GtkTextBuffer in notify_buffer, when view is being destroyed.
Don't create GtkTextBuffer in notify_buffer, when view is being destroyed.
Status: RESOLVED FIXED
Product: gtksourceview
Classification: Platform
Component: General
2.91.x
Other Linux
: Normal normal
: ---
Assigned To: GTK Sourceview maintainers
GTK Sourceview maintainers
Depends on: 634677
Blocks:
 
 
Reported: 2010-11-10 13:14 UTC by Krzesimir Nowak
Modified: 2010-11-13 11:34 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Test case. (557 bytes, text/plain)
2010-11-10 13:14 UTC, Krzesimir Nowak
  Details
If view is being destroyed then do nothing in notify_buffer. (887 bytes, patch)
2010-11-10 13:18 UTC, Krzesimir Nowak
none Details | Review
Unitized test case from first post. (1.86 KB, patch)
2010-11-11 14:41 UTC, Krzesimir Nowak
none Details | Review
Unitized test case from first post with corrected whitespaces. (1.85 KB, patch)
2010-11-11 14:46 UTC, Krzesimir Nowak
none Details | Review
Disconnect the notify buffer in dispose. (1.47 KB, patch)
2010-11-12 20:35 UTC, Krzesimir Nowak
none Details | Review
Unitized test case from first post with corrected whitespaces and added comment. (2.09 KB, patch)
2010-11-12 20:36 UTC, Krzesimir Nowak
none Details | Review
Disconnect the notify buffer in dispose once. (2.15 KB, patch)
2010-11-12 22:11 UTC, Krzesimir Nowak
accepted-commit_now Details | Review
Unitized test case from first post with corrected whitespaces and added comment and changed name. (2.11 KB, patch)
2010-11-12 22:12 UTC, Krzesimir Nowak
accepted-commit_now Details | Review

Description Krzesimir Nowak 2010-11-10 13:14:18 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)
Comment 1 Krzesimir Nowak 2010-11-10 13:18:21 UTC
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.
Comment 2 Ignacio Casal Quinteiro (nacho) 2010-11-10 14:35:54 UTC
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.
Comment 3 Paolo Borelli 2010-11-10 23:36:14 UTC
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 :)
Comment 4 Krzesimir Nowak 2010-11-11 12:27:58 UTC
(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.
Comment 5 Krzesimir Nowak 2010-11-11 14:41:36 UTC
Created attachment 174252 [details] [review]
Unitized test case from first post.

I'm not silencing stderr in fork test.
Comment 6 Krzesimir Nowak 2010-11-11 14:46:02 UTC
Created attachment 174253 [details] [review]
Unitized test case from first post with corrected whitespaces.

Changed double spaces to tabs.
Comment 7 Paolo Borelli 2010-11-12 12:59:51 UTC
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.
Comment 8 Krzesimir Nowak 2010-11-12 20:35:51 UTC
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 ().
Comment 9 Krzesimir Nowak 2010-11-12 20:36:35 UTC
Created attachment 174360 [details] [review]
Unitized test case from first post with corrected whitespaces and added comment.

Added a comment in test case.
Comment 10 Paolo Borelli 2010-11-12 20:56:02 UTC
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?
Comment 11 Paolo Borelli 2010-11-12 20:56:02 UTC
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?
Comment 12 Paolo Borelli 2010-11-12 20:58:58 UTC
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);
Comment 13 Krzesimir Nowak 2010-11-12 22:11:01 UTC
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...
Comment 14 Krzesimir Nowak 2010-11-12 22:12:04 UTC
Created attachment 174368 [details] [review]
Unitized test case from first post with corrected whitespaces and added comment and changed name.
Comment 15 Paolo Borelli 2010-11-13 02:00:22 UTC
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
Comment 16 Paolo Borelli 2010-11-13 02:01:39 UTC
Review of attachment 174368 [details] [review]:

I did not test, I am assuming the test actually passes after your patch :)
Comment 17 Krzesimir Nowak 2010-11-13 10:53:41 UTC
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.
Comment 18 Krzesimir Nowak 2010-11-13 11:05:07 UTC
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.
Comment 19 Paolo Borelli 2010-11-13 11:10:34 UTC
You need to make sure shared-mime-info is updated and installed in your prefix for that test to work
Comment 20 Krzesimir Nowak 2010-11-13 11:26:42 UTC
I see, now it works, thanks. Maybe gtksourceview should depend on shared-mime-info in jhbuild?
Comment 21 Paolo Borelli 2010-11-13 11:34:47 UTC
good idea. I pushed that change.