servo: Merge #8168 - XHR timeouts use same abstraction as scripts timers. (fixes #3396) (from benschulz:xhr-timeout-ordering); r=jdm

Alright, this is it. Finally the fix for #3396. :D

I'll add two comments via reviewable in a second.

Source-Repo: https://github.com/servo/servo
Source-Revision: df81cd7ce9de4823ad966f873639ed06ca368e4c

servo/components/script/dom/bindings/global.rs

@@ -22,8 +22,9 @@ use msg::constellation_msg::{ConstellationChan, PipelineId, WorkerId};
 use net_traits::ResourceTask;
 use profile_traits::mem;
 use script_task::{CommonScriptMsg, ScriptChan, ScriptPort, ScriptTask};
-use script_traits::TimerEventRequest;
+use script_traits::{MsDuration, TimerEventRequest};
 use std::sync::mpsc::Sender;
+use timers::{ScheduledCallback, TimerHandle};
 use url::Url;
 use util::mem::HeapSizeOf;
 
@@ -197,6 +198,23 @@ impl<'a> GlobalRef<'a> {
         }
     }
 
+    /// Schedule the given `callback` to be invoked after at least `duration` milliseconds have
+    /// passed.
+    pub fn schedule_callback(&self, callback: Box<ScheduledCallback>, duration: MsDuration) -> TimerHandle {
+        match *self {
+            GlobalRef::Window(window) => window.schedule_callback(callback, duration),
+            GlobalRef::Worker(worker) => worker.schedule_callback(callback, duration),
+        }
+    }
+
+    /// Unschedule a previously-scheduled callback.
+    pub fn unschedule_callback(&self, handle: TimerHandle) {
+        match *self {
+            GlobalRef::Window(window) => window.unschedule_callback(handle),
+            GlobalRef::Worker(worker) => worker.unschedule_callback(handle),
+        }
+    }
+
     /// Returns the receiver's reflector.
     pub fn reflector(&self) -> &Reflector {
         match *self {

servo/components/script/dom/window.rs

@@ -55,7 +55,7 @@ use profile_traits::mem;
 use rustc_serialize::base64::{FromBase64, STANDARD, ToBase64};
 use script_task::{ScriptChan, ScriptPort, MainThreadScriptMsg, RunnableWrapper};
 use script_task::{SendableMainThreadScriptChan, MainThreadScriptChan, MainThreadTimerEventChan};
-use script_traits::{TimerEventChan, TimerEventId, TimerEventRequest, TimerSource};
+use script_traits::{MsDuration, TimerEventChan, TimerEventId, TimerEventRequest, TimerSource};
 use selectors::parser::PseudoElement;
 use std::ascii::AsciiExt;
 use std::borrow::ToOwned;
@@ -71,7 +71,7 @@ use std::sync::mpsc::TryRecvError::{Disconnected, Empty};
 use std::sync::mpsc::{Sender, channel};
 use string_cache::Atom;
 use time;
-use timers::{ActiveTimers, IsInterval, TimerCallback};
+use timers::{ActiveTimers, IsInterval, ScheduledCallback, TimerCallback, TimerHandle};
 use url::Url;
 use util::geometry::{self, MAX_RECT};
 use util::str::{DOMString, HTML_SPACE_CHARACTERS};
@@ -1083,6 +1083,16 @@ impl Window {
         self.scheduler_chan.clone()
     }
 
+    pub fn schedule_callback(&self, callback: Box<ScheduledCallback>, duration: MsDuration) -> TimerHandle {
+        self.timers.schedule_callback(callback,
+                                      duration,
+                                      TimerSource::FromWindow(self.id.clone()))
+    }
+
+    pub fn unschedule_callback(&self, handle: TimerHandle) {
+        self.timers.unschedule_callback(handle);
+    }
+
     pub fn windowproxy_handler(&self) -> WindowProxyHandler {
         WindowProxyHandler(self.dom_static.windowproxy_handler.0)
     }

servo/components/script/dom/workerglobalscope.rs

@@ -24,12 +24,12 @@ use msg::constellation_msg::{ConstellationChan, PipelineId, WorkerId};
 use net_traits::{ResourceTask, load_whole_resource};
 use profile_traits::mem;
 use script_task::{CommonScriptMsg, ScriptChan, ScriptPort};
-use script_traits::{TimerEventChan, TimerEventId, TimerEventRequest, TimerSource};
+use script_traits::{MsDuration, TimerEventChan, TimerEventId, TimerEventRequest, TimerSource};
 use std::cell::Cell;
 use std::default::Default;
 use std::rc::Rc;
 use std::sync::mpsc::{Receiver, Sender};
-use timers::{ActiveTimers, IsInterval, TimerCallback};
+use timers::{ActiveTimers, IsInterval, ScheduledCallback, TimerCallback, TimerHandle};
 use url::{Url, UrlParser};
 use util::str::DOMString;
 
@@ -143,6 +143,16 @@ impl WorkerGlobalScope {
         self.scheduler_chan.clone()
     }
 
+    pub fn schedule_callback(&self, callback: Box<ScheduledCallback>, duration: MsDuration) -> TimerHandle {
+        self.timers.schedule_callback(callback,
+                                      duration,
+                                      TimerSource::FromWorker)
+    }
+
+    pub fn unschedule_callback(&self, handle: TimerHandle) {
+        self.timers.unschedule_callback(handle);
+    }
+
     pub fn get_cx(&self) -> *mut JSContext {
         self.runtime.cx()
     }

servo/components/script/dom/xmlhttprequest.rs

@@ -30,6 +30,7 @@ use dom::xmlhttprequestupload::XMLHttpRequestUpload;
 use encoding::all::UTF_8;
 use encoding::label::encoding_from_whatwg_label;
 use encoding::types::{DecoderTrap, EncoderTrap, Encoding, EncodingRef};
+use euclid::length::Length;
 use hyper::header::Headers;
 use hyper::header::{Accept, ContentLength, ContentType, qitem};
 use hyper::http::RawStatus;
@@ -50,14 +51,13 @@ use std::ascii::AsciiExt;
 use std::borrow::ToOwned;
 use std::cell::{Cell, RefCell};
 use std::default::Default;
-use std::sync::mpsc::{Sender, TryRecvError, channel};
+use std::sync::mpsc::channel;
 use std::sync::{Arc, Mutex};
-use std::thread::sleep_ms;
 use time;
+use timers::{ScheduledCallback, TimerHandle};
 use url::{Url, UrlParser};
 use util::mem::HeapSizeOf;
 use util::str::DOMString;
-use util::task::spawn_named;
 
 pub type SendParam = StringOrURLSearchParams;
 
@@ -137,8 +137,7 @@ pub struct XMLHttpRequest {
     send_flag: Cell<bool>,
 
     global: GlobalField,
-    #[ignore_heap_size_of = "Defined in std"]
-    timeout_cancel: DOMRefCell<Option<Sender<()>>>,
+    timeout_cancel: DOMRefCell<Option<TimerHandle>>,
     fetch_time: Cell<i64>,
     #[ignore_heap_size_of = "Cannot calculate Heap size"]
     timeout_target: DOMRefCell<Option<Box<ScriptChan + Send>>>,
@@ -974,36 +973,49 @@ impl XMLHttpRequest {
             }
         }
 
-        // Sets up the object to timeout in a given number of milliseconds
-        // This will cancel all previous timeouts
-        let timeout_target = (*self.timeout_target.borrow().as_ref().unwrap()).clone();
-        let global = self.global.root();
-        let xhr = Trusted::new(global.r().get_cx(), self, global.r().script_chan());
-        let gen_id = self.generation_id.get();
-        let (cancel_tx, cancel_rx) = channel();
-        *self.timeout_cancel.borrow_mut() = Some(cancel_tx);
-        spawn_named("XHR:Timer".to_owned(), move || {
-            sleep_ms(duration_ms);
-            match cancel_rx.try_recv() {
-                Err(TryRecvError::Empty) => {
-                    timeout_target.send(CommonScriptMsg::RunnableMsg(XhrEvent, box XHRTimeout {
-                        xhr: xhr,
-                        gen_id: gen_id,
-                    })).unwrap();
-                },
-                Err(TryRecvError::Disconnected) | Ok(()) => {
-                    // This occurs if xhr.timeout_cancel (the sender) goes out of scope (i.e, xhr went out of scope)
-                    // or if the oneshot timer was overwritten. The former case should not happen due to pinning.
-                    debug!("XHR timeout was overwritten or canceled")
+        #[derive(JSTraceable, HeapSizeOf)]
+        struct ScheduledXHRTimeout {
+            #[ignore_heap_size_of = "Cannot calculate Heap size"]
+            target: Box<ScriptChan + Send>,
+            #[ignore_heap_size_of = "Because it is non-owning"]
+            xhr: Trusted<XMLHttpRequest>,
+            generation_id: GenerationId,
+        }
+
+        impl ScheduledCallback for ScheduledXHRTimeout {
+            fn invoke(self: Box<Self>) {
+                let s = *self;
+                s.target.send(CommonScriptMsg::RunnableMsg(XhrEvent, box XHRTimeout {
+                    xhr: s.xhr,
+                    gen_id: s.generation_id,
+                })).unwrap();
+            }
+
+            fn box_clone(&self) -> Box<ScheduledCallback> {
+                box ScheduledXHRTimeout {
+                    target: self.target.clone(),
+                    xhr: self.xhr.clone(),
+                    generation_id: self.generation_id,
                 }
             }
         }
-    );
+
+        // Sets up the object to timeout in a given number of milliseconds
+        // This will cancel all previous timeouts
+        let global = self.global.root();
+        let callback = ScheduledXHRTimeout {
+            target: (*self.timeout_target.borrow().as_ref().unwrap()).clone(),
+            xhr: Trusted::new(global.r().get_cx(), self, global.r().script_chan()),
+            generation_id: self.generation_id.get(),
+        };
+        let duration = Length::new(duration_ms as u64);
+        *self.timeout_cancel.borrow_mut() = Some(global.r().schedule_callback(box callback, duration));
     }
 
     fn cancel_timeout(&self) {
-        if let Some(cancel_tx) = self.timeout_cancel.borrow_mut().take() {
-            let _ = cancel_tx.send(());
+        if let Some(handle) = self.timeout_cancel.borrow_mut().take() {
+            let global = self.global.root();
+            global.r().unschedule_callback(handle);
         }
     }
 

servo/components/script/timers.rs

@@ -7,6 +7,7 @@ use dom::bindings::cell::DOMRefCell;
 use dom::bindings::codegen::Bindings::FunctionBinding::Function;
 use dom::bindings::global::global_object_for_js_object;
 use dom::bindings::reflector::Reflectable;
+use dom::bindings::trace::JSTraceable;
 use dom::window::ScriptHelpers;
 use euclid::length::Length;
 use js::jsapi::{HandleValue, Heap, RootedValue};
@@ -106,6 +107,7 @@ pub enum TimerCallback {
 enum InternalTimerCallback {
     StringTimerCallback(DOMString),
     FunctionTimerCallback(Rc<Function>, Rc<Vec<Heap<JSVal>>>),
+    InternalCallback(Box<ScheduledCallback>),
 }
 
 impl HeapSizeOf for InternalTimerCallback {
@@ -115,6 +117,18 @@ impl HeapSizeOf for InternalTimerCallback {
     }
 }
 
+pub trait ScheduledCallback: JSTraceable + HeapSizeOf {
+    fn invoke(self: Box<Self>);
+
+    fn box_clone(&self) -> Box<ScheduledCallback>;
+}
+
+impl Clone for Box<ScheduledCallback> {
+    fn clone(&self) -> Box<ScheduledCallback> {
+        self.box_clone()
+    }
+}
+
 impl ActiveTimers {
     pub fn new(timer_event_chan: Box<TimerEventChan + Send>,
                scheduler_chan: Sender<TimerEventRequest>)
@@ -139,15 +153,6 @@ impl ActiveTimers {
                                is_interval: IsInterval,
                                source: TimerSource)
                                -> i32 {
-        // step 3
-        let TimerHandle(new_handle) = self.next_timer_handle.get();
-        self.next_timer_handle.set(TimerHandle(new_handle + 1));
-
-        let timeout = cmp::max(0, timeout);
-        // step 7
-        let duration = self.clamp_duration(Length::new(timeout as u64));
-        let next_call = self.base_time() + duration;
-
         let callback = match callback {
             TimerCallback::StringTimerCallback(code_str) =>
                 InternalTimerCallback::StringTimerCallback(code_str),
@@ -165,6 +170,38 @@ impl ActiveTimers {
             }
         };
 
+        let timeout = cmp::max(0, timeout);
+        // step 7
+        let duration = self.clamp_duration(Length::new(timeout as u64));
+
+        let TimerHandle(handle) = self.schedule_internal_callback(callback, duration, is_interval, source);
+        handle
+    }
+
+    pub fn schedule_callback(&self,
+                             callback: Box<ScheduledCallback>,
+                             duration: MsDuration,
+                             source: TimerSource) -> TimerHandle {
+        self.schedule_internal_callback(InternalTimerCallback::InternalCallback(callback),
+                                        duration,
+                                        IsInterval::NonInterval,
+                                        source)
+    }
+
+    // see https://html.spec.whatwg.org/multipage/#timer-initialisation-steps
+    fn schedule_internal_callback(&self,
+                                  callback: InternalTimerCallback,
+                                  duration: MsDuration,
+                                  is_interval: IsInterval,
+                                  source: TimerSource) -> TimerHandle {
+        assert!(self.suspended_since.get().is_none());
+
+        // step 3
+        let TimerHandle(new_handle) = self.next_timer_handle.get();
+        self.next_timer_handle.set(TimerHandle(new_handle + 1));
+
+        let next_call = self.base_time() + duration;
+
         let timer = Timer {
             handle: TimerHandle(new_handle),
             source: source,
@@ -184,11 +221,14 @@ impl ActiveTimers {
         }
 
         // step 10
-        new_handle
+        TimerHandle(new_handle)
     }
 
     pub fn clear_timeout_or_interval(&self, handle: i32) {
-        let handle = TimerHandle(handle);
+        self.unschedule_callback(TimerHandle(handle));
+    }
+
+    pub fn unschedule_callback(&self, handle: TimerHandle) {
         let was_next = self.is_next_timer(handle);
 
         self.timers.borrow_mut().retain(|t| t.handle != handle);
@@ -258,7 +298,10 @@ impl ActiveTimers {
                     }).collect();
 
                     let _ = function.Call_(this, arguments, Report);
-                }
+                },
+                InternalTimerCallback::InternalCallback(callback) => {
+                    callback.invoke();
+                },
             };
 
             self.nesting_level.set(0);