[geary/wip/ubuntu-1804-network-unreachable: 1/2] Prevent circular refs using idle and timeout manager when running

[Michael Gratton <mjog src gnome org>]

commit f48170ae3828fc145129b54ae632f23e8e773ebc
Author: Michael Gratton <mike vee net>
Date:   Sat Feb 16 15:58:08 2019 +1100

    Prevent circular refs using idle and timeout manager when running
    
    If an IdleManager or TimeoutManager had been scheduled, it would not get
    destroyed until it was executed by the main loop, causing criticals
    if the objects enclosed by its callback had been destroyed.
    
    This adds a weak reference to the manager object itself when scheduling
    on the main loop, so it can get safely dropped.

 src/engine/util/util-idle-manager.vala     | 53 ++++++++++++++++++++++++-----
 src/engine/util/util-timeout-manager.vala  | 54 +++++++++++++++++++++++-------
 test/engine/util-idle-manager-test.vala    | 40 ++++++++++++++++++++++
 test/engine/util-timeout-manager-test.vala | 45 +++++++++++++++++++++++++
 4 files changed, 171 insertions(+), 21 deletions(-)
---
diff --git a/src/engine/util/util-idle-manager.vala b/src/engine/util/util-idle-manager.vala
index d99431d9..bd1809af 100644
--- a/src/engine/util/util-idle-manager.vala
+++ b/src/engine/util/util-idle-manager.vala
@@ -32,6 +32,32 @@ public class Geary.IdleManager : BaseObject {
     /** The idle callback function prototype. */
     public delegate void IdleFunc(IdleManager manager);
 
+
+    // Instances of this are passed to GLib.Idle to execute the
+    // callback. This holds a weak ref to the manager itself allowing
+    // it to be destroyed if all of its references are broken, even if
+    // it is still queued via GLib.Idle.
+    private class HandlerRef : GLib.Object {
+
+        private GLib.WeakRef manager;
+
+
+        public HandlerRef(IdleManager manager) {
+            this.manager = GLib.WeakRef(manager);
+        }
+
+        public bool execute() {
+            bool ret = Source.REMOVE;
+            IdleManager? manager = this.manager.get() as IdleManager;
+            if (manager != null) {
+                ret = manager.execute();
+            }
+            return ret;
+        }
+
+    }
+
+
     /** Determines if the function will be re-scheduled after being run. */
     public Repeat repetition = Repeat.ONCE;
 
@@ -43,10 +69,8 @@ public class Geary.IdleManager : BaseObject {
         get { return this.source_id >= 0; }
     }
 
-    // Callback must be unowned to avoid reference loop with owner's
-    // class when a closure is used as the callback.
     private unowned IdleFunc callback;
-    private int source_id = -1;
+    private int64 source_id = -1;
 
 
     /**
@@ -70,7 +94,11 @@ public class Geary.IdleManager : BaseObject {
      */
     public void schedule() {
         reset();
-        this.source_id = (int) GLib.Idle.add_full(this.priority, on_trigger);
+
+        HandlerRef handler = new HandlerRef(this);
+        this.source_id = (int) GLib.Idle.add_full(
+            this.priority, handler.execute
+        );
     }
 
     /**
@@ -79,15 +107,14 @@ public class Geary.IdleManager : BaseObject {
      * @return `true` if function was already scheduled, else `false`
      */
     public bool reset() {
-        bool is_running = this.is_running;
-        if (is_running) {
-            Source.remove(this.source_id);
+        if (this.is_running) {
+            Source.remove((uint) this.source_id);
             this.source_id = -1;
         }
-        return is_running;
+        return this.is_running;
     }
 
-    private bool on_trigger() {
+    private bool execute() {
         bool ret = Source.CONTINUE;
         // If running only once, reset the source id now in case the
         // callback resets the timer while it is executing, so we
@@ -97,6 +124,14 @@ public class Geary.IdleManager : BaseObject {
             this.source_id = -1;
             ret = Source.REMOVE;
         }
+
+        unowned IdleFunc? callback = this.callback;
+        if (callback != null) {
+            callback(this);
+        } else {
+            ret = Source.REMOVE;
+        }
+
         callback(this);
         return ret;
     }
diff --git a/src/engine/util/util-timeout-manager.vala b/src/engine/util/util-timeout-manager.vala
index fef16179..24302617 100644
--- a/src/engine/util/util-timeout-manager.vala
+++ b/src/engine/util/util-timeout-manager.vala
@@ -33,6 +33,31 @@ public class Geary.TimeoutManager : BaseObject {
     public delegate void TimeoutFunc(TimeoutManager manager);
 
 
+    // Instances of this are passed to GLib.Timeout to execute the
+    // callback. This holds a weak ref to the manager itself allowing
+    // it to be destroyed if all of its references are broken, even if
+    // it is still queued via GLib.Timeout.
+    private class HandlerRef : GLib.Object {
+
+        private GLib.WeakRef manager;
+
+
+        public HandlerRef(TimeoutManager manager) {
+            this.manager = GLib.WeakRef(manager);
+        }
+
+        public bool execute() {
+            bool ret = Source.REMOVE;
+            TimeoutManager? manager = this.manager.get() as TimeoutManager;
+            if (manager != null) {
+                ret = manager.execute();
+            }
+            return ret;
+        }
+
+    }
+
+
     /** Determines if {@link interval} represent seconds. */
     public bool use_seconds;
 
@@ -50,10 +75,8 @@ public class Geary.TimeoutManager : BaseObject {
         get { return this.source_id >= 0; }
     }
 
-    // Callback must be unowned to avoid reference loop with owner's
-    // class when a closure is used as the callback.
     private unowned TimeoutFunc callback;
-    private int source_id = -1;
+    private int64 source_id = -1;
 
 
     /**
@@ -91,10 +114,16 @@ public class Geary.TimeoutManager : BaseObject {
      */
     public void start() {
         reset();
+
+        HandlerRef handler = new HandlerRef(this);
         this.source_id = (int) (
             (this.use_seconds)
-            ? GLib.Timeout.add_seconds(this.interval, on_trigger, this.priority)
-            : GLib.Timeout.add(this.interval, on_trigger, this.priority)
+            ? GLib.Timeout.add_seconds(
+                this.interval, handler.execute, this.priority
+            )
+            : GLib.Timeout.add(
+                this.interval, handler.execute, this.priority
+            )
         );
     }
 
@@ -107,25 +136,26 @@ public class Geary.TimeoutManager : BaseObject {
      * @return `true` if the timeout was already running, else `false`
      */
     public bool reset() {
-        bool is_running = this.is_running;
-        if (is_running) {
-            Source.remove(this.source_id);
+        if (this.is_running) {
+            Source.remove((uint) this.source_id);
             this.source_id = -1;
         }
-        return is_running;
+        return this.is_running;
     }
 
-    private bool on_trigger() {
+    private bool execute() {
         bool ret = Source.CONTINUE;
+
         // If running only once, reset the source id now in case the
         // callback resets the timer while it is executing, so we
         // avoid removing the source just before it would be removed
-        // after this call anyway
+        // after this call anyway.
         if (this.repetition == Repeat.ONCE) {
             this.source_id = -1;
             ret = Source.REMOVE;
         }
-        callback(this);
+
+        this.callback(this);
         return ret;
     }
 
diff --git a/test/engine/util-idle-manager-test.vala b/test/engine/util-idle-manager-test.vala
index 0d2b2aba..54dd4bc5 100644
--- a/test/engine/util-idle-manager-test.vala
+++ b/test/engine/util-idle-manager-test.vala
@@ -7,12 +7,52 @@
 
 class Geary.IdleManagerTest : TestCase {
 
+
+    private class WeakRefTest : GLib.Object {
+
+        public IdleManager test { get; private set; }
+
+        public WeakRefTest() {
+            // Pass in an arg to ensure the closure is non-trivial
+            string arg = "my hovercraft is full of eels";
+            this.test = new IdleManager(
+                () => {
+                    do_stuff(arg);
+                }
+            );
+
+            // Pass
+            this.test.schedule();
+        }
+
+        private void do_stuff(string arg) {
+            assert(false);
+        }
+
+    }
+
+
     public IdleManagerTest() {
         base("Geary.IdleManagerTest");
+        add_test("weak_ref", callback_weak_ref);
         add_test("start_reset", start_reset);
         add_test("test_run", test_run);
     }
 
+    public void callback_weak_ref() throws GLib.Error {
+        WeakRefTest? owner = new WeakRefTest();
+        GLib.WeakRef weak_ref = GLib.WeakRef(owner.test);
+
+        // Should make both objects null even though the even loop
+        // hasn't run and hence the callback hasn't been called.
+        owner = null;
+        assert_null(weak_ref.get());
+
+        // Pump the loop a few times so the callback can get called.
+        this.main_loop.iteration(false);
+        this.main_loop.iteration(false);
+    }
+
     public void start_reset() throws Error {
         IdleManager test = new IdleManager(() => { /* noop */ });
         assert(!test.is_running);
diff --git a/test/engine/util-timeout-manager-test.vala b/test/engine/util-timeout-manager-test.vala
index 1539082b..a9a01a64 100644
--- a/test/engine/util-timeout-manager-test.vala
+++ b/test/engine/util-timeout-manager-test.vala
@@ -12,8 +12,34 @@ class Geary.TimeoutManagerTest : TestCase {
 
     private const double MILLISECONDS_EPSILON = 0.1;
 
+
+    private class WeakRefTest : GLib.Object {
+
+        public TimeoutManager test { get; private set; }
+
+        public WeakRefTest() {            // Pass in an arg to ensure the closure is non-trivial
+            string arg = "my hovercraft is full of eels";
+            this.test = new TimeoutManager.milliseconds(
+                10, () => {
+                    do_stuff(arg);
+                }
+            );
+
+            // Pass
+            this.test.start();
+        }
+
+        private void do_stuff(string arg) {
+            // This should never get called
+            assert(false);
+        }
+
+    }
+
+
     public TimeoutManagerTest() {
         base("Geary.TimeoutManagerTest");
+        add_test("weak_ref", callback_weak_ref);
         add_test("start_reset", start_reset);
         if (Test.slow()) {
             add_test("seconds", seconds);
@@ -22,6 +48,25 @@ class Geary.TimeoutManagerTest : TestCase {
         }
     }
 
+    public void callback_weak_ref() throws GLib.Error {
+        WeakRefTest? owner = new WeakRefTest();
+        double duration = owner.test.interval;
+        GLib.WeakRef weak_ref = GLib.WeakRef(owner.test);
+
+        // Should make both objects null even though the even loop
+        // hasn't run and hence the callback hasn't been called.
+        owner = null;
+        assert_null(weak_ref.get());
+
+        // Pump the loop until the timeout has passed so that the
+        // callback can get called.
+        Timer timer = new Timer();
+        timer.start();
+        while (timer.elapsed() < (duration / 1000) * 2) {
+            this.main_loop.iteration(false);
+        }
+    }
+
     public void start_reset() throws Error {
         TimeoutManager test = new TimeoutManager.seconds(1, () => { /* noop */ });
         assert(!test.is_running);