Summary

fix crash when closing a group chat with spellchk plugin enabled

Review Request #1951 — Created Oct. 23, 2022 and submitted Oct. 28, 2022, 1:34 a.m.

Information

Owner

ivanhoe

Repository

pidgin/pidgin

Branch

release-2.x.y

Bugs

PIDGIN-17702

Depends On

Reviewers

Groups

pidgin

People

Description

fix crash when closing a group chat with spellchk plugin enabled

Testing Done

Followed steps to reproduce from the bug report and neither experienced a crash or saw invalid reads related to this in valgrind.

Additionally tested that g_signal_handlers_disconnect_matched actually disconnects signal handlers when unloading the spellchk plugin.

Also tested following test case: * connect an IRC account * enable spellchk plugin * join a chat * disable spellchk plugin * close chat window

which led to an invalid read before I added the g_object_remove_weak_pointer call. Now, I don't see any invalid reads related to this in valgrind anymore.

Commits

Summary	ID
fix crash when closing a group chat with spellchk plugin enabled	45afca0be177e1b9a6e21cc2c60d66aa0e703faa

Issues

Description	From	Last Updated
I don't understand how this fixes the crash. There's nothing here that should remove a reference which doesn't explain why …	grim	Oct. 25, 2022, 6:53 a.m.
if you add g_object_add_weak_pointer(view, &spell->view); spell-view will be NULL in spellchk_free if the view is already dead.	grim	Oct. 28, 2022, 1:33 a.m.

pidgin/plugins/spellchk.c (Diff revision 1)

Show all issues

I don't understand how this fixes the crash. There's nothing here that should remove a reference which doesn't explain why adding a reference fixes it.

ivanhoe Oct. 23, 2022, 2:10 p.m.

I originally thought that g_signal_handlers_disconnect_matched would require that reference but it looks like it's the gtk_text_view_get_buffer call that needs it. As the valgrind output shows (see my comment on the YT issue) gtk_text_view_get_buffer calls gtk_text_view_set_buffer so it's actually running that internal get_buffer function which creates a new buffer for the text view and then removes a reference to that buffer (but not the text view). So I'm at a loss right now why adding a reference to the text view fixes the problem.

grim Oct. 24, 2022, 6:59 a.m.

It should only be calling set_buffer if it doesn't have a buffer yet. And in this case, everything should be a GtkIMHTML that should be managing the buffer. So maybe that's the real problem?

ivanhoe Oct. 24, 2022, 6:34 p.m.

From what I see in gdb gtk_imhtml_finalize is called before spellchk_free which would explain why it hasn't a buffer anymore.

grim Oct. 25, 2022, 6:53 a.m.

Ok that would make a LOT of sense. That "widget" is shady af...

grim Oct. 25, 2022, 6:56 a.m.

We just discussed on twitch, that if the imhtml is gone, it's creating a buffer, to remove the signal which means there should be a way to determine how to remove the signal if necessary.

Ship it!

```
Ship It!
```
```
Nice work, thanks!!
```

pidgin/plugins/spellchk.c (Diff revision 1)

Show all issues

if you add g_object_add_weak_pointer(view, &spell->view); spell-view will be NULL in spellchk_free if the view is already dead.

Change Summary:

Added weak pointer as discussed

Testing Done:

		Followed steps to reproduce from the bug report and neither experienced a crash or saw invalid reads related to this in valgrind.
	+
	+	Additionally tested that `g_signal_handlers_disconnect_matched` actually disconnects signal handlers when unloading the spellchk plugin.
	+
	+	Also tested following test case:
	+	* connect an IRC account
	+	* enable spellchk plugin
	+	* join a chat
	+	* disable spellchk plugin
	+	* close chat window
	+
	+	which led to an invalid read before I added the `g_object_remove_weak_pointer` call. Now, I don't see any invalid reads related to this in valgrind anymore.

Commits:

	Summary	ID
	fix crash when closing a group chat with spellchk plugin enabled	121ea3820627bbd18d598e74fc7e036d92a8f649
	fix crash when closing a group chat with spellchk plugin enabled	45afca0be177e1b9a6e21cc2c60d66aa0e703faa