After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 775074 - GtkScrolledWindow does not disconnect all GtkAdjustment signal handlers it connects
GtkScrolledWindow does not disconnect all GtkAdjustment signal handlers it co...
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: GtkScrolledWindow
3.22.x
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2016-11-25 10:06 UTC by Massimo
Modified: 2017-08-31 22:34 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
test case (1.42 KB, text/plain)
2016-11-25 10:06 UTC, Massimo
  Details
ScrolledWindow: Disconn. Adjustment::value-changed (2.83 KB, patch)
2017-08-31 19:42 UTC, Daniel Boles
none Details | Review
ScrolledWindow: Disconn. Adjustment::value-changed (4.53 KB, patch)
2017-08-31 21:27 UTC, Daniel Boles
none Details | Review
ScrolledWindow: Disconnect indicator_value_changed (4.03 KB, patch)
2017-08-31 21:28 UTC, Daniel Boles
none Details | Review
ScrolledWindow: Disconn. Adjustment::value-changed (4.53 KB, patch)
2017-08-31 21:32 UTC, Daniel Boles
committed Details | Review
ScrolledWindow: Disconnect indicator_value_changed (3.44 KB, patch)
2017-08-31 21:32 UTC, Daniel Boles
committed Details | Review

Description Massimo 2016-11-25 10:06:18 UTC
Created attachment 340743 [details]
test case

After I wrote in bug 774790 about the thoretical possibility of signal 
emission with stale user_data, I sketched a test case mimicking a diff
viewer with 2 text views scrolling together, after the user closes one.
It is attached to this report.

Executing it under valgrind and scrolling the mouse wheel over the
GtkTextView shows that a GtkScrolledWindow does not disconnect all 
handlers it connects to adjustment signals.

In gtk_scrolled_view_set_{h,v}adjustment (and gtk_scrolled_window_destroy):

https://git.gnome.org/browse/gtk+/tree/gtk/gtkscrolledwindow.c?h=gtk-3-22#n2280

it disconnects gtk_scrolled_window_adjustment_changed, but few rows below
it connected also gtk_scrolled_window_adjustment_value_changed, which is never
disconnected.


Also indicators connect to adjustments signal that are not disconnected during
destruction because at unrealize time indicator_reset is called which set
indicator->scrollbar = NULL;

https://git.gnome.org/browse/gtk+/tree/gtk/gtkscrolledwindow.c?h=gtk-3-22#n4497

and in gtk_scrolled_window_destroy the call to remove_indicator is useless

https://git.gnome.org/browse/gtk+/tree/gtk/gtkscrolledwindow.c?h=gtk-3-22#n2778

as it does nothing when indicator->scrollbar is NULL;

https://git.gnome.org/browse/gtk+/tree/gtk/gtkscrolledwindow.c?h=gtk-3-22#n4349
Comment 1 Daniel Boles 2017-08-31 19:42:05 UTC
Created attachment 358886 [details] [review]
ScrolledWindow: Disconn. Adjustment::value-changed

We connected to this when setting a new adjustment, but we never
disconnected when changing from that adjustment to a different one.
Comment 2 Daniel Boles 2017-08-31 19:59:13 UTC
It's a little less obvious how to handle this one:

(In reply to Massimo from comment #0)
> Also indicators connect to adjustments signal that are not disconnected
> during
> destruction because at unrealize time indicator_reset is called which set
> indicator->scrollbar = NULL;
> 
> https://git.gnome.org/browse/gtk+/tree/gtk/gtkscrolledwindow.c?h=gtk-3-
> 22#n4497
> 
> and in gtk_scrolled_window_destroy the call to remove_indicator is useless
> 
> https://git.gnome.org/browse/gtk+/tree/gtk/gtkscrolledwindow.c?h=gtk-3-
> 22#n2778
> 
> as it does nothing when indicator->scrollbar is NULL;
> 
> https://git.gnome.org/browse/gtk+/tree/gtk/gtkscrolledwindow.c?h=gtk-3-
> 22#n4349

(as the disconnection is probably not the only thing that gets missed out here)
Comment 3 Daniel Boles 2017-08-31 21:27:26 UTC
Created attachment 358891 [details] [review]
ScrolledWindow: Disconn. Adjustment::value-changed

We connected to this when setting a new adjustment, but we never
disconnected when changing from that adjustment to a different one.


--

missed one
Comment 4 Daniel Boles 2017-08-31 21:28:21 UTC
Created attachment 358892 [details] [review]
ScrolledWindow: Disconnect indicator_value_changed

…in gtk_scrolled_window_destroy(), as remove_indicator() does not.
Comment 5 Daniel Boles 2017-08-31 21:32:02 UTC
Created attachment 358893 [details] [review]
ScrolledWindow: Disconn. Adjustment::value-changed

We connected to this when setting a new adjustment, but we never
disconnected when changing from that adjustment to a different one.


one more time with less git fubar
Comment 6 Daniel Boles 2017-08-31 21:32:19 UTC
Created attachment 358894 [details] [review]
ScrolledWindow: Disconnect indicator_value_changed

…in gtk_scrolled_window_destroy(), as remove_indicator() does not.
Comment 7 Daniel Boles 2017-08-31 22:34:44 UTC
committed in squashed form, using disconnect_by_data() as that's more concise.

Thanks!