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


    

 There are no open issues


   

  

 







 






  

 








 














 





 

  

   




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:
   

   


    Completed

   

  

















  

 









     

    

   

  

  

   
   Loading...