[california] Crash due to altering stack model cache during iteration: Bug #732035

[Jim Nelson <jnelson src gnome org>]

commit bc977b558da6812d880736ee1f7c63671cc74037
Author: Jim Nelson <jim yorba org>
Date:   Tue Jun 24 13:08:33 2014 -0700

    Crash due to altering stack model cache during iteration: Bug #732035
    
    The stack model map was being altered via our own signal handler
    being fired due to destroying widgets while walking the cache map,
    causing an assertion in Gee.  Using a copy of the map's keys works
    around the issue.

 src/Makefile.am                      |    1 +
 src/toolkit/toolkit-stack-model.vala |   16 ++--
 src/util/util-scheduled.vala         |  177 ++++++++++++++++++++++++++++++++++
 3 files changed, 186 insertions(+), 8 deletions(-)
---
diff --git a/src/Makefile.am b/src/Makefile.am
index 7498275..5bed4ce 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -130,6 +130,7 @@ california_VALASOURCES = \
        util/util-gfx.vala \
        util/util-markup.vala \
        util/util-memory.vala \
+       util/util-scheduled.vala \
        util/util-string.vala \
        util/util-uri.vala \
        \
diff --git a/src/toolkit/toolkit-stack-model.vala b/src/toolkit/toolkit-stack-model.vala
index fe9c02d..5eb3afe 100644
--- a/src/toolkit/toolkit-stack-model.vala
+++ b/src/toolkit/toolkit-stack-model.vala
@@ -116,6 +116,7 @@ public class StackModel<G> : BaseObject {
     private Gee.HashMap<G, Gtk.Widget?> items;
     private bool in_balance_cache = false;
     private bool stack_destroyed = false;
+    private Scheduled? scheduled_balance_cache = null;
     
     public StackModel(Gtk.Stack stack,
         OrderedTransitionType ordered_transition_type,
@@ -256,10 +257,8 @@ public class StackModel<G> : BaseObject {
                 // which (apparently) it has not when this change is made (probably made at start
                 // of transition, not the end) ... "transition-running" property would be useful
                 // here, but that's not available until GTK 3.12
-                Idle.add(() => {
+                scheduled_balance_cache = new Scheduled.once_at_idle(() => {
                     balance_cache("on_stack_child_visible");
-                    
-                    return false;
                 }, Priority.LOW);
                 
                 return;
@@ -305,13 +304,14 @@ public class StackModel<G> : BaseObject {
         
         // trim existing widgets from cache
         if (trim_from_cache != null) {
-            Gee.MapIterator<G, Gtk.Widget?> iter = items.map_iterator();
-            while (iter.next()) {
-                Gtk.Widget? presentation = iter.get_value();
-                if (presentation != null && trim_from_cache(iter.get_key(), visible_item)) {
+            // use a copy of the keys to avoid iterating over the map while it's being altered
+            // (in particular, by one of our own signal handlers)
+            foreach (G key in items.keys.to_array()) {
+                Gtk.Widget? presentation = items[key];
+                if (presentation != null && trim_from_cache(key, visible_item)) {
                     // set_value before removing from stack to prevent our signal handler from
                     // unsetting underneath us and causing iterator stamp problems
-                    iter.set_value(null);
+                    items[key] = null;
                     presentation.destroy();
                 }
             }
diff --git a/src/util/util-scheduled.vala b/src/util/util-scheduled.vala
new file mode 100644
index 0000000..1fd0dc2
--- /dev/null
+++ b/src/util/util-scheduled.vala
@@ -0,0 +1,177 @@
+/* Copyright 2014 Yorba Foundation
+ *
+ * This software is licensed under the GNU Lesser General Public License
+ * (version 2.1 or later).  See the COPYING file in this distribution.
+ */
+
+namespace California {
+
+/**
+ * A reference-counted source ID to better control execution of Idle and Timeout callbacks in the
+ * event loop.
+ *
+ * Idle and Timeout provide "fire and forget" ways to schedule events to be executed later.
+ * Cancelling those calls after scheduling them is easy to get wrong, since the returned uint (the
+ * source ID) is invalid once the callback has been removed from the scheduler, which must be done
+ * manually by the calling code.
+ *
+ * Scheduled manages the validity of the source ID.  It also simplifies writing delegates for the
+ * callback (especially for the Idle case) to make it easier to write Vala anonoymous functions.
+ *
+ * Note that if the last reference to a Scheduled is dropped the callback will be cancelled if its
+ * not already executed and continuous callbacks will halt.
+ */
+
+public class Scheduled : BaseObject {
+    public const string PROP_IS_SCHEDULED = "is-scheduled";
+    public const string PROP_IS_CONTINUOUS = "is-continuous";
+    public const string PROP_IS_EXECUTING = "is-executing";
+    
+    /**
+     * Return value for { link ScheduleContinuous} indicating if the code should remain scheduled.
+     */
+    public enum Reschedule {
+        AGAIN,
+        HALT
+    }
+    
+    /**
+     * A callback when only scheduling code to execute once.
+     */
+    public delegate void ScheduleOnce();
+    
+    /**
+     * A callback when scheduling code to execute continuously (between timeouts).
+     */
+    public delegate Reschedule ScheduleContinuous();
+    
+    /**
+     * Returns true if the callback is still scheduled for execution.
+     */
+    public bool is_scheduled { get { return source_id != 0; } }
+    
+    /**
+     * Returns true if the code is scheduled for continuous execution.
+     *
+     * May return true even if the code is no longer scheduled for execution.
+     */
+    public bool is_continuous { get { return schedule_continuous != null; } }
+    
+    /**
+     * Returns true if the code is currently executing.
+     *
+     * Note: this is not thread-safe.
+     */
+    public bool is_executing { get; private set; default = false; }
+    
+    private uint source_id;
+    private ScheduleOnce? schedule_once = null;
+    private ScheduleContinuous? schedule_continuous = null;
+    
+    /**
+     * Schedule code to execute continuously when the event loop is idle.
+     */
+    public Scheduled.continuous_at_idle(ScheduleContinuous cb, int priority = Priority.DEFAULT_IDLE) {
+        schedule_continuous = cb;
+        
+        source_id = Idle.add(on_continuous, priority);
+    }
+    
+    /**
+     * Schedule code to execute once when the event loop is idle.
+     */
+    public Scheduled.once_at_idle(ScheduleOnce cb, int priority = Priority.DEFAULT_IDLE) {
+        schedule_once = cb;
+        
+        source_id = Idle.add(on_once, priority);
+    }
+    
+    /**
+     * Schedule code to execute every n milliseconds.
+     */
+    public Scheduled.continuous_every_msec(uint msec, ScheduleContinuous cb, int priority = 
Priority.DEFAULT) {
+        schedule_continuous = cb;
+        
+        source_id = Timeout.add(msec, on_continuous, priority);
+    }
+    
+    /**
+     * Schedule code to execute once after n milliseconds has elapsed.
+     */
+    public Scheduled.once_after_msec(uint msec, ScheduleOnce cb, int priority = Priority.DEFAULT) {
+        schedule_once = cb;
+        
+        source_id = Timeout.add(msec, on_once, priority);
+    }
+    
+    /**
+     * Schedule code to execute after n seconds.
+     */
+    public Scheduled.continuous_every_sec(uint sec, ScheduleContinuous cb, int priority = Priority.DEFAULT) {
+        schedule_continuous = cb;
+        
+        source_id = Timeout.add_seconds(sec, on_continuous, priority);
+    }
+    
+    /**
+     * Schedule code to execute once after n seconds have elapsed.
+     */
+    public Scheduled.once_after_sec(uint sec, ScheduleOnce cb, int priority = Priority.DEFAULT) {
+        schedule_once = cb;
+        
+        source_id = Timeout.add_seconds(sec, on_once, priority);
+    }
+    
+    ~Scheduled() {
+        cancel();
+    }
+    
+    /**
+     * Cancel executing of scheduled code.
+     *
+     * Dropping the last reference to this object will also cancel execution.
+     */
+    public void cancel() {
+        if (source_id == 0)
+            return;
+        
+        Source.remove(source_id);
+        source_id = 0;
+    }
+    
+    private bool on_once() {
+        is_executing = true;
+        schedule_once();
+        is_executing = false;
+        
+        // with once, this source func is never renewed
+        source_id = 0;
+        
+        return false;
+    }
+    
+    private bool on_continuous() {
+        is_executing = true;
+        Reschedule reschedule = schedule_continuous();
+        is_executing = false;
+        
+        switch (reschedule) {
+            case Reschedule.AGAIN:
+                return true;
+            
+            case Reschedule.HALT:
+                source_id = 0;
+                
+                return false;
+            
+            default:
+                assert_not_reached();
+        }
+    }
+    
+    public override string to_string() {
+        return "Scheduled %s:%u".printf(schedule_once != null ? "once" : "continuously", source_id);
+    }
+}
+
+}