Commit 91cb3780 authored by Michael Gratton's avatar Michael Gratton 🤞 Committed by Michael Gratton

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.
parent c693ef07
......@@ -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;
}
......
......@@ -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;
}
......
......@@ -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);
......
......@@ -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);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment