GNOME Bugzilla – Bug 633278
Patch to fix recent hangs with GtkScrolledWindow
Last modified: 2010-11-18 16:00:50 UTC
Created attachment 173338 [details] [review] Patch to restore hang protection Since commit e85dad38e2c579598c142515c8b816ea6657e0c8 we've been experiencing hangs. This patch properly restores the logic that used to prevent these hangs.
Created attachment 173340 [details] [review] Less intrusive patch Last patch had issues still with nautilus, lets try a yet less intrusive approach.
Hmmm seems this patch still needs work. GtkToolPalette demo shows no vertical scrollbar until window height is very small...
Created attachment 173376 [details] [review] Better patch Last patch had issues with scrollbar visibility states. This patch fixes things in a new way. - The old allocation protection mechanism is left in tact almost verbatim - We add a flag to note we are inside an allocation loop - We avoid queueing extra resizes when adjustments change if we are already allocating - When we find both scrollbars flip, we perform the allocation inline instead of waiting for an extra allocation cycle. Patch contains reindentation of the hfw guessing code since we execute that unconditionally now.
Review of attachment 173376 [details] [review]: ::: gtk/gtkscrolledwindow.c @@ +1625,3 @@ * infinite recursion */ + priv->inside_allocate = TRUE; I think I would prefer to have the setting/unsetting of inside_allocate either directly around the recursive call, or at the top/bottom of the size_allocate function. Having them sit in the middle like this seems odd.
Created attachment 173378 [details] [review] Cleaned up patch This one does the same except adds a function gtk_scrolled_window_allocate_child() which allocates the child and marks the ->inside_allocation guard in the process. It also does the child_allocation math and returns the relative allocation, making the inner allocation loop a bit more clear and readable.
This still doesn't completely fix nautilus scrolling issues. - in Icon View, the vscrollbar appears only after you resize the window - in List View, the hscrollbar is displayed with the wrong size (see http://people.gnome.org/~cosimoc/scrollfoobars/listview2.png) - in Compact View, no hscrollbars are displayed at all when resizing (see http://people.gnome.org/~cosimoc/scrollfoobars/compactview2.png)
Alright, enough beating around the bush. The attached patch fixes the hangs and maintains correct scrollbar visibility states for anything inside tested GTK+ so far. Based on the fact that reverting the original patch on bug 629778 fixes hangs but leaves nautilus scrollbars still in a bad state, I have to deduce that that original patch for bug 629778 is not to blame. So I propose we go ahead and commit this fix and treat the screwy scrollbar state in nautilus as a separate issue. If it is indeed a bug in GTK+, it's not the same one. What should be done for that is: bisect the GTK+ history after the point where 2.91.1 was released and find the commit that introduces the bug for nautilus and work from there.
(In reply to comment #7) > Alright, enough beating around the bush. > > The attached patch fixes the hangs and maintains correct > scrollbar visibility states for anything inside tested GTK+ so far. > > Based on the fact that reverting the original patch on bug 629778 > fixes hangs but leaves nautilus scrollbars still in a bad state, I > have to deduce that that original patch for bug 629778 is not to > blame. > > So I propose we go ahead and commit this fix and treat the screwy > scrollbar state in nautilus as a separate issue. > > If it is indeed a bug in GTK+, it's not the same one. What should be > done for that is: bisect the GTK+ history after the point where > 2.91.1 was released and find the commit that introduces the bug > for nautilus and work from there. I did this bisection and I actually found out that things seem to work perfectly before the second commit for bug 629778, i.e. it's commit e85dad38e2c579598c142515c8b816ea6657e0c8 that breaks everything (it causes both the hang, which is fixed by this patch, and the screwy scrollbars appearance).
After some more tiring investigation, I *think* this might be related to GtkLayout returning 0 for minimum and natural size (it's hardcoded in GtkLayout). This way child_scroll_height and child_scroll_width in the hfw loop are always 0 (as returned by _get_preferred_height()), and so they're never greater than the allocation->width/height values. This should cause priv->hscrollbar_visible/priv->vcsrollbar_visible to be FALSE. I still don't know about why the nautilus treeview doesn't behave right as well.
Needless to say, I dont like how the story is changing. It's obvious that I don't have the time to build the 53 some odd packages that jhbuild says I need to build in order to check for myself so I'll leave you the benefit of the doubt. Ok how about this: a.) We commit the above patch and you provide a test-case for GTK+ that reproduces the problem in your app. b.) Everybody lives with the hangs in GTK+ and waits for a random day that I have time to go and build and fix nautilus. (In reply to comment #9) > After some more tiring investigation, I *think* this might be related to > GtkLayout returning 0 for minimum and natural size (it's hardcoded in > GtkLayout). > This way child_scroll_height and child_scroll_width in the hfw loop are always > 0 (as returned by _get_preferred_height()), and so they're never greater than > the allocation->width/height values. This should cause > priv->hscrollbar_visible/priv->vcsrollbar_visible to be FALSE. > I still don't know about why the nautilus treeview doesn't behave right as > well. It should not have anything to do with what is returned by height-for-width apis unless your code is relying on height-for-width apis to define the upper and lower values of the adjustment. The scrollbar will only be visible if (upper - lower) > page_size (i.e. if the content does not fit the page after the scrollable widget receives an allocation).
After Cosimo found a typo that was introduced in the scrollable interface patch (causing the child vadjustment to be set with the actual hadjustent) I'm finally committing this fix. commit ae71cf72090de41a47dd9989f390cb515179050b Author: Tristan Van Berkom <tristan.van.berkom@gmail.com> Date: Sat Oct 30 23:10:43 2010 +0900 Fixed hangs in TextView and ToolPalette Fixed the hangs by adding a ->inside_allocation flag and avoiding to queue resizes while inside the allocation loop. The extra queue'd resizes were causing the scrolled window size_allocate() to perform the guess again and again thus causing an infinite loop.
Created attachment 173669 [details] [review] Seems the last patch was incomplete Additional patch to take into account that showing/hiding the scrollbars in itself causes a resize to be queued, changed the ->inside_allocate() approach for an approach to blindly trust scrollbar visibility in the next incoming allocate after an allocate which cause any scrollbar visibility to flip/change state.
Reopening bug... the above patch does not work right for GTK+ (it leaves textviews hanging in the gtk-demo at some sizes). Perhaps we can ignore the queue'd resize triggered by showing/hiding the v/hscrollbar.
Created attachment 173682 [details] [review] New fixed patch This patch does exactly the same as the last patch was intended to do, except without the misplaced 'if' statement. It works as expected from the GTK+ demos etc.
Xan, can you please test this patch and see if it fixes the webkit gtk+ widget hangs you described ?
Created attachment 173684 [details] [review] patch on top of Tristan's Hi Tristan, this patch on top of yours fixes nautilus scrollbars not appearing before first resize for me; not sure if it's the right fix but I did not spot any regressions so far.
(In reply to comment #15) > Xan, can you please test this patch and see if it fixes > the webkit gtk+ widget hangs you described ? I seem to still get exactly the same hang, the trace seems extremely similar/identical.
I am still seeing hangs in textview with current gtk when opening a file in gedit and then trying to resize a window. Text wrapping needs to be active.
Created attachment 174483 [details] [review] Patch to make size-allocate the "last word" This patch is a more brutal approach to stop the hangs. In gtk_widget_size_allocate(), *after* invoking the internal "allocate" signal and performing the allocation, we set the ->alloc_needed ->width_request_needed and ->height_request_needed flags to FALSE. This effectively obsoletes any queued resizes on a widget that were effected as a result of size allocations (textview blindly does this regardless if it's layout changes because of content changes or because of size allocation changes, which causes looping and madness). The patch also reverts the scrolled_window ->inside_allocate flag approach which becomes unneeded with this more brutal approach. I've compiled gedit and found it to solve the problem, would be interested to know if it fixes the hangs in webkitgtk also...
Review of attachment 173684 [details] [review]: This was the wrong approach as it makes the scrolled window stop retrying the child layout with no scrollbars if ever the content size changes. However the guess needs to be done whether the internal content size changes or the allocation of the scrolled window changes... both changes can result in a situation where scrollbars are displayed needlessly (i.e. giving the width of the vertical scrollbar to the child can result in the child not needing a vertical scrollbar at all, in both cases).
Created attachment 174492 [details] [review] Combined fix for hangs and inconsistent visibility states This patch includes the patch to make size_allocate() nullify any size requests triggered by way of allocation. Furthermore, inside the child allocation loop, the scrollbar visibility updates are forced. This is because the child does not always update both adjustments and emit the "changed" signal if the adjustments in question do not change due to the allocation. This can happen for instance... when the width of the child changes but the height does not change, then the scrollbar dissapears during the guess phase and does not reappear because the adjustment_changed() handlers dont get called (because the adjustments did not change). This patch effectively fixes hangs remaining in gedit as long with fixing inconsistent scrollbar visibility states observed in goocanvas. Would be nice to have this patch tested for nautilus and webkitgtk as well.
Created attachment 174493 [details] [review] Final patch This one introduces another minor change in the inner child allocation loop. The loop currently settles for both scrollbars visible if both scrollbars flip visibility in a single iteration after the first iteration. However it does not protect against one scrollbar flipping visibility in each iteration indefinitely... I added a cap of 4 max iterations to protect against this. Without this final touch; icon view basic test can infinitely recurse inside the inner loop at some (difficult to find) sizes, notably when wide and shrinking the height.
Works for me; I agree that it would probably be good to document the new rule ('size-allocate is god') somewhere.
I just tested and it works fine with nautilus master too.
Committed fix. Closing this sucker... again.