Rework the idle callback to have a safer interface

It turns out that the uv implementation would cause use-after-free if the idle
callback was used after the call to `close`, and additionally nothing would ever
really work that well if `start()` were called twice. To change this, the
`start` and `close` methods were removed in favor of specifying the callback at
creation, and allowing destruction to take care of closing the watcher.

3 files changed, 11 insertions, 21 deletions

diff --git a/src/libstd/rt/basic.rs b/src/libstd/rt/basic.rs
index 0c8d192d89a..322c58bc2b8 100644
--- a/src/libstd/rt/basic.rs
+++ b/src/libstd/rt/basic.rs
@@ -107,7 +107,7 @@ impl BasicLoop {
             match self.idle {
                 Some(idle) => {
                     if (*idle).active {
-                        (*idle).work.get_mut_ref().call();
+                        (*idle).work.call();
                     }
                 }
                 None => {}
@@ -150,8 +150,8 @@ impl EventLoop for BasicLoop {
     }
 
     // XXX: Seems like a really weird requirement to have an event loop provide.
-    fn pausible_idle_callback(&mut self) -> ~PausibleIdleCallback {
-        let callback = ~BasicPausible::new(self);
+    fn pausible_idle_callback(&mut self, cb: ~Callback) -> ~PausibleIdleCallback {
+        let callback = ~BasicPausible::new(self, cb);
         rtassert!(self.idle.is_none());
         unsafe {
             let cb_ptr: &*mut BasicPausible = cast::transmute(&callback);
@@ -204,36 +204,27 @@ impl Drop for BasicRemote {
 
 struct BasicPausible {
     eloop: *mut BasicLoop,
-    work: Option<~Callback>,
+    work: ~Callback,
     active: bool,
 }
 
 impl BasicPausible {
-    fn new(eloop: &mut BasicLoop) -> BasicPausible {
+    fn new(eloop: &mut BasicLoop, cb: ~Callback) -> BasicPausible {
         BasicPausible {
             active: false,
-            work: None,
+            work: cb,
             eloop: eloop,
         }
     }
 }
 
 impl PausibleIdleCallback for BasicPausible {
-    fn start(&mut self, f: ~Callback) {
-        rtassert!(!self.active && self.work.is_none());
-        self.active = true;
-        self.work = Some(f);
-    }
     fn pause(&mut self) {
         self.active = false;
     }
     fn resume(&mut self) {
         self.active = true;
     }
-    fn close(&mut self) {
-        self.active = false;
-        self.work = None;
-    }
 }
 
 impl Drop for BasicPausible {
diff --git a/src/libstd/rt/rtio.rs b/src/libstd/rt/rtio.rs
index 96ba5123456..1e12da8645c 100644
--- a/src/libstd/rt/rtio.rs
+++ b/src/libstd/rt/rtio.rs
@@ -31,7 +31,7 @@ pub trait Callback {
 pub trait EventLoop {
     fn run(&mut self);
     fn callback(&mut self, proc());
-    fn pausible_idle_callback(&mut self) -> ~PausibleIdleCallback;
+    fn pausible_idle_callback(&mut self, ~Callback) -> ~PausibleIdleCallback;
     fn remote_callback(&mut self, ~Callback) -> ~RemoteCallback;
 
     /// The asynchronous I/O services. Not all event loops may provide one
@@ -226,10 +226,8 @@ pub trait RtioTTY {
 }
 
 pub trait PausibleIdleCallback {
-    fn start(&mut self, f: ~Callback);
     fn pause(&mut self);
     fn resume(&mut self);
-    fn close(&mut self);
 }
 
 pub trait RtioSignal {}
diff --git a/src/libstd/rt/sched.rs b/src/libstd/rt/sched.rs
index f84e10fe989..c2e665f4903 100644
--- a/src/libstd/rt/sched.rs
+++ b/src/libstd/rt/sched.rs
@@ -169,7 +169,8 @@ impl Scheduler {
     pub fn bootstrap(mut ~self, task: ~Task) {
 
         // Build an Idle callback.
-        self.idle_callback = Some(self.event_loop.pausible_idle_callback());
+        let cb = ~SchedRunner as ~Callback;
+        self.idle_callback = Some(self.event_loop.pausible_idle_callback(cb));
 
         // Initialize the TLS key.
         local_ptr::init_tls_key();
@@ -184,7 +185,7 @@ impl Scheduler {
         // Before starting our first task, make sure the idle callback
         // is active. As we do not start in the sleep state this is
         // important.
-        self.idle_callback.get_mut_ref().start(~SchedRunner as ~Callback);
+        self.idle_callback.get_mut_ref().resume();
 
         // Now, as far as all the scheduler state is concerned, we are
         // inside the "scheduler" context. So we can act like the
@@ -202,7 +203,7 @@ impl Scheduler {
 
         // Close the idle callback.
         let mut sched: ~Scheduler = Local::take();
-        sched.idle_callback.get_mut_ref().close();
+        sched.idle_callback.take();
         // Make one go through the loop to run the close callback.
         sched.run();