fix crash when closing a group chat with spellchk plugin enabled

Review Request #1951 — Created Oct. 23, 2022 and submitted

Information

pidgin/pidgin
release-2.x.y

Reviewers

fix crash when closing a group chat with spellchk plugin enabled

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.

Summary ID
fix crash when closing a group chat with spellchk plugin enabled
45afca0be177e1b9a6e21cc2c60d66aa0e703faa
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 …

grimgrim

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.

grimgrim
grim
  1. 
      
  2. 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.

    1. 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.

    2. 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?

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

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

    5. 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.

  3. 
      
grim
  1. Ship It!
  2. Nice work, thanks!!

  3. 
      
grim
  1. 
      
  2. 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.

  3. 
      
ivanhoe
grim
  1. Ship It!
  2. Awesome work! Thanks for knocking this one out!!

  3. 
      
grim
Review request changed
Status:
Completed