Fix the infinite resizing freeze

Review Request #1342 — Created March 13, 2022 and submitted

Information

pidgin/pidgin
release-2.x.y

Reviewers

This appears related to libpango somehow, as in Debian
Bullseye, libpango splits URLs with dashes at the end,
but in Debian Bookworm, the URLs are not split with dashes
at the end, and the bug does not appear to be triggered
in Bookworm.

This patch makes the assumption that the gtkimhtml widgets
stored in a PidginConversation normally resize in an
alternating manner. However, when the bug is triggered,
only the "entry" gtkimhtml member of PidginConversation
resizes, so we allow "entry" to resize only up to 3 times
in a row.

Compiled and tested on several desktop environments on a few
GNU/Linux distros by pasting the link mentioned
here https://issues.imfreedom.org/issue/PIDGIN-17413 moving
the cursor at the beginning of the buffer, and holding
the spacebar pressed.

Summary ID
Fix the infinite resizing freeze
This appears related to libpango somehow, as in Debian Bullseye, libpango splits URLs with dashes at the end, but in Debian Bookworm, the URLs are not split with dashes at the end, and the bug does not appear to be triggered in Bookworm. This patch makes the assumption that the gtkimhtml widgets stored in a PidginConversation normally resize in an alternating manner. However, when the bug is triggered, only the "entry" gtkimhtml member of PidginConversation resizes, so we allow "entry" to resize only up to 3 times in a row. Testing Done: Compiled and tested on several desktop environments on a few GNU/Linux distros by pasting the link mentioned here https://issues.imfreedom.org/issue/PIDGIN-17413 moving the cursor at the beginning of the buffer, and holding the spacebar pressed. Bugs closed: PIDGIN-16753, PIDGIN-16999, PIDGIN-17287, PIDGIN-17413, PIDGIN-17430, PIDGIN-17568, PIDGIN-17602 Reviewed at https://reviews.imfreedom.org/r/1342/
dfbe88566f90ddd415bb990c6be0f0a0982362d8
Description From Last Updated

I'd have preferred if this could have been done on the PidginConversation instead of leaking into the GtkIMHtml, but it …

QuLogicQuLogic

The imhtml is the widget that's being resized so you don't need to check it against anything else. So the …

grimgrim

Add space between ) and { in all ifs.

QuLogicQuLogic

Add surrounding braces.

QuLogicQuLogic
grim
  1. 
      
  2. This looks good, but it can be simplified quite a bit. You can just store an integer on the entry with g_object_set_data and GPOINTER_TO_INT and GINT_TO_POINTER. Also g_object_get_data will return NULL if it doesn't find the data, so that'll be fine for the counter since GPOINTER_TO_INT(NULL) returns 0.

    That also elimiates the need for the struct member in PidginConversation which we can't do because it breaks abi.

    1. Thank you for the review. I have amended the patch, but upon further testing, I found that when the conversation window is resized a lot, it still freezes under some conditions. I will try to investigate further.

  3. 
      
belgin
grim
  1. 
      
    1. Thank you for the review. I tried it the way you mention, by resetting resize-count to 0 right before returning,
      but that doesn't fix the problem because the infinite resizing is caused by two things:
      1. the scollbars insinde the entry imhtml appearing and disappearing
      2. the message imhtml (the one above the entry imhtml) resizes which causes the entry imhtml to resize again, which causes the message imhtml to resize, looping forever

      If only point 1 happened, the method you suggest would work, however, since resizing the entry imhtml causes
      a resize in the message imhtml which causes the entry imhtml to resize again, etc, it is not enough.

      Even with my patch, if you actively try to trigger the hang by furiously resizing the window, you will still succeed sometimes. I am still investigating this

  2. pidgin/gtkimhtml.c (Diff revision 2)
     
     

    The imhtml is the widget that's being resized so you don't need to check it against anything else. So the check against entry shouldn't be necessary.

    You should be able to get away with something like

    ```c
    gpointer raw_resize_count = NULL;
    gint resize_count = 0;

    raw_resize_count = g_object_get_data(G_OBJECT(imhtml), "resize-count");
    resize_count = GPOINTER_TO_INT(raw_resize_count);

    if(resize_count > MAX_RESIZE_COUNT) {
    return;
    }

    resize_count++;

    g_object_set_data(G_OBJECT(imhtml), "resize-count", GINT_TO_POINTER(resize_count);

    1. Something needs to reset resize-count at some point though, or else the entry size will be stuck.

    2. yep sorry i missed that bit before the return. I also forgot to close my code block, sorry about that.. I can't edit after the fact :-/

  3. 
      
belgin
belgin
QuLogic
  1. 
      
  2. I'd have preferred if this could have been done on the PidginConversation instead of leaking into the GtkIMHtml, but it seems you can't get a signal in before the size-allocate object handler, so that might not be possible. And I don't want to spend much more time than necessary on GtkIMHtml.

    1. I tried to avoid touching gtkimhtml.c, but that doesn't work because, as you said, gtk_imhtml_size_allocate executes before entering any of the functions in gtkconv.c.

  3. pidgin/gtkimhtml.c (Diff revision 4)
     
     

    Add space between ) and { in all ifs.

  4. pidgin/gtkimhtml.c (Diff revision 4)
     
     
     

    Add surrounding braces.

  5. 
      
belgin
grim
grim
belgin
belgin
belgin
belgin
belgin
grim
  1. Ship It!
  2. Amazing work, thank you so much for going through so many iterations and seeing this through until the end!!

  3. 
      
grim
Review request changed

Status: Closed (submitted)

Loading...