fix crash when closing a group chat with spellchk plugin enabled

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

Information
ivanhoe
pidgin/pidgin
release-2.x.y
PIDGIN-17702
Reviewers
pidgin 
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 
fix crash when closing a group chat with spellchk plugin enabled
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
grim 
  1. 
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  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. 



    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. 



    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. 



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

      5. 



    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.
      
 
      

      6. 


 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 





 

  

   




grim




   

   







  

 



 

  


   

   

    
 
grim


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 







  2. 
 
    
  
  
    
  
   
    
    
    




    
    Nice work, thanks!!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 













  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 





 

  

   




grim




   

   







  

 



 

  


   

   

    
 
grim


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  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. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




ivanhoe




   

   

  

 



 















 





 

  

   




grim




   

   







  

 



 

  


   

   

    
 
grim


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 







  2. 
 
    
  
  
    
  
   
    
    
    




    
    Awesome work! Thanks for knocking this one out!!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 













  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




grim




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 







 


 










  

 









     

    

   

  

  

   
   Loading...