aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorChristopher Vollick <0@psycoti.ca>2023-09-14 15:12:58 -0400
committerMarvin W <git@larma.de>2024-04-28 21:36:27 +0200
commitd473efcbfe3603fac62594bd9d8fee8b23ffcbe5 (patch)
treeec7d09a8ac92b3ab434b383ab02215aa2632055b
parentba83a4ba3d6886b711278b8804566dcbe8ad7621 (diff)
downloaddino-d473efcbfe3603fac62594bd9d8fee8b23ffcbe5.tar.gz
dino-d473efcbfe3603fac62594bd9d8fee8b23ffcbe5.zip
Add WeakTimeout Pattern to Prevent Leaks
While doing testing I noticed that skeletons were being leaked, and eventually tracked it down to the timer that updates the time label closing over "this" and then keeping the reference alive, potentially for 24 hours. I noticed a few other places in the code doing some version of this, and one of them had the "static and weak pointer" approach, which I pulled out into a util class. Now, we still have to make sure we're passing it a static method instead of a lambda, as that would also close over "this" and render the whole thing useless, but at least most of the annoying parts live in the util class now. Also the call_widget version was doing a weird thing where it was removing itself, but then returning "true"? I'm not sure what that accomplishes, because returning "false" means to not run this again. So I think my new version is the same in practice, but simpler... There are other timeouts in the code that I briefly looked over, but all of them seemed to be relatively short hard-coded durations, so I left them alone. But if any of them are long-lived, it's possible they could also benefit from this class in the future. Closes #1480 Co-Authored-By: Marvin W <git@larma.de>
-rw-r--r--libdino/CMakeLists.txt1
-rw-r--r--libdino/meson.build1
-rw-r--r--libdino/src/util/weak_timeout.vala40
-rw-r--r--main/src/ui/conversation_content_view/call_widget.vala13
-rw-r--r--main/src/ui/conversation_content_view/conversation_item_skeleton.vala10
-rw-r--r--main/src/ui/conversation_content_view/quote_widget.vala9
-rw-r--r--main/src/ui/widgets/date_separator.vala17
7 files changed, 63 insertions, 28 deletions
diff --git a/libdino/CMakeLists.txt b/libdino/CMakeLists.txt
index d52f9184..e4d786c9 100644
--- a/libdino/CMakeLists.txt
+++ b/libdino/CMakeLists.txt
@@ -70,6 +70,7 @@ SOURCES
src/util/display_name.vala
src/util/util.vala
src/util/weak_map.vala
+ src/util/weak_timeout.vala
CUSTOM_VAPIS
"${CMAKE_BINARY_DIR}/exports/xmpp-vala.vapi"
"${CMAKE_BINARY_DIR}/exports/qlite.vapi"
diff --git a/libdino/meson.build b/libdino/meson.build
index 356c15d3..17804d23 100644
--- a/libdino/meson.build
+++ b/libdino/meson.build
@@ -76,6 +76,7 @@ sources = files(
'src/util/display_name.vala',
'src/util/util.vala',
'src/util/weak_map.vala',
+ 'src/util/weak_timeout.vala',
)
sources += [version_vala]
c_args = [
diff --git a/libdino/src/util/weak_timeout.vala b/libdino/src/util/weak_timeout.vala
new file mode 100644
index 00000000..28894ed3
--- /dev/null
+++ b/libdino/src/util/weak_timeout.vala
@@ -0,0 +1,40 @@
+public class Dino.WeakTimeout {
+ // XXX: If you get an error saying your function doesn't match the delegate, make sure it's static!
+ // These are marked as "has_target=false" so you can't close over "this" and leak it in your lambda.
+ [CCode (has_target = false, instance_pos = 0)]
+ public delegate bool SourceFunc<T> (T object);
+
+ [CCode (has_target = false, instance_pos = 0)]
+ public delegate void SourceOnceFunc<T> (T object);
+
+ public static uint add<T>(uint interval, T object, owned SourceFunc<T> function, int priority = GLib.Priority.DEFAULT) {
+ var weak = WeakRef((Object)object);
+ return GLib.Timeout.add(interval, () => {
+ var strong = weak.get();
+ if (strong == null) return false;
+
+ return function(strong);
+ }, priority);
+ }
+
+ public static uint add_once<T>(uint interval, T object, owned SourceOnceFunc<T> function, int priority = GLib.Priority.DEFAULT) {
+ var weak = WeakRef((Object)object);
+ return GLib.Timeout.add(interval, () => {
+ var strong = weak.get();
+ if (strong == null) return false;
+
+ function(strong);
+ return false;
+ }, priority);
+ }
+
+ public static uint add_seconds<T>(uint interval, T object, owned SourceFunc<T> function, int priority = GLib.Priority.DEFAULT) {
+ return add(interval * 1000, object, (owned) function, priority);
+ }
+
+ // This one doesn't have an upstream equivalent, but it seems pretty obvious to me
+ public static uint add_seconds_once<T>(uint interval, T object, owned SourceOnceFunc<T> function, int priority = GLib.Priority.DEFAULT) {
+ return add_once(interval * 1000, object, (owned) function, priority);
+ }
+
+}
diff --git a/main/src/ui/conversation_content_view/call_widget.vala b/main/src/ui/conversation_content_view/call_widget.vala
index ab047196..3d6b250b 100644
--- a/main/src/ui/conversation_content_view/call_widget.vala
+++ b/main/src/ui/conversation_content_view/call_widget.vala
@@ -111,6 +111,10 @@ namespace Dino.Ui {
incoming_call_revealer.reveal_child = true;
}
+ private void on_time_update_timeout() {
+ if (time_update_handler_id != 0) update_call_state();
+ }
+
private void update_call_state() {
incoming_call_revealer.reveal_child = false;
incoming_call_revealer.remove_css_class("incoming");
@@ -156,14 +160,7 @@ namespace Dino.Ui {
string duration = get_duration_string((new DateTime.now_utc()).difference(call.local_time));
subtitle_label.label = _("Started %s ago").printf(duration);
- time_update_handler_id = Timeout.add_seconds(get_next_time_change() + 1, () => {
- if (time_update_handler_id != 0) {
- Source.remove(time_update_handler_id);
- time_update_handler_id = 0;
- update_call_state();
- }
- return true;
- });
+ time_update_handler_id = Dino.WeakTimeout.add_seconds_once(get_next_time_change() + 1, this, on_time_update_timeout);
break;
case Call.State.OTHER_DEVICE:
diff --git a/main/src/ui/conversation_content_view/conversation_item_skeleton.vala b/main/src/ui/conversation_content_view/conversation_item_skeleton.vala
index 5d86f6c7..3b818b44 100644
--- a/main/src/ui/conversation_content_view/conversation_item_skeleton.vala
+++ b/main/src/ui/conversation_content_view/conversation_item_skeleton.vala
@@ -173,14 +173,14 @@ public class ConversationItemSkeleton : Plugins.ConversationItemWidgetInterface,
set_widget(widget, Plugins.WidgetType.GTK4, 3);
}
+ private void on_time_update_timeout() {
+ if (main_grid.parent != null) update_time();
+ }
+
private void update_time() {
time_label.label = get_relative_time(item.time.to_local()).to_string();
- time_update_timeout = Timeout.add_seconds((int) get_next_time_change(item.time), () => {
- if (this.main_grid.parent == null) return false;
- update_time();
- return false;
- });
+ time_update_timeout = Dino.WeakTimeout.add_seconds_once((int) get_next_time_change(item.time), this, on_time_update_timeout);
}
private void update_name_label() {
diff --git a/main/src/ui/conversation_content_view/quote_widget.vala b/main/src/ui/conversation_content_view/quote_widget.vala
index 23b62e6a..b267e6bc 100644
--- a/main/src/ui/conversation_content_view/quote_widget.vala
+++ b/main/src/ui/conversation_content_view/quote_widget.vala
@@ -41,12 +41,13 @@ namespace Dino.Ui.Quote {
this.author_jid = content_item.jid;
}
+ private void on_display_time_timeout() {
+ if (display_time_timeout != 0) update_display_time();
+ }
+
private void update_display_time() {
this.display_time = ConversationItemSkeleton.get_relative_time(message_time.to_local());
- display_time_timeout = Timeout.add_seconds((int) ConversationItemSkeleton.get_next_time_change(message_time), () => {
- if (display_time_timeout != 0) update_display_time();
- return false;
- });
+ display_time_timeout = Dino.WeakTimeout.add_seconds_once((int) ConversationItemSkeleton.get_next_time_change(message_time), this, on_display_time_timeout);
}
public override void dispose() {
diff --git a/main/src/ui/widgets/date_separator.vala b/main/src/ui/widgets/date_separator.vala
index b5d84a5b..fcaa61d1 100644
--- a/main/src/ui/widgets/date_separator.vala
+++ b/main/src/ui/widgets/date_separator.vala
@@ -38,18 +38,13 @@ public class Dino.Ui.ViewModel.CompatDateSeparatorModel : DateSeparatorModel {
}
}
- private void update_time_label() {
- date_label = get_relative_time(date);
- time_update_timeout = set_update_time_label_timeout((int) get_next_time_change(), this);
+ private static void on_time_update_timeout(CompatDateSeparatorModel self) {
+ if (self.time_update_timeout != 0) self.update_time_label();
}
- private static uint set_update_time_label_timeout(int interval, CompatDateSeparatorModel model_) {
- WeakRef model_weak = WeakRef(model_);
- return Timeout.add_seconds(interval, () => {
- CompatDateSeparatorModel? model = (CompatDateSeparatorModel) model_weak.get();
- if (model != null && model.time_update_timeout != 0) model.update_time_label();
- return false;
- });
+ private void update_time_label() {
+ date_label = get_relative_time(date);
+ time_update_timeout = Dino.WeakTimeout.add_seconds_once(get_next_time_change(), this, on_time_update_timeout);
}
private int get_next_time_change() {
@@ -113,4 +108,4 @@ public class Dino.Ui.DateSeparator : Gtk.Widget {
}
base.dispose();
}
-} \ No newline at end of file
+}