From e87c275de02c2d96851f7e637d55cb7076f35224 Mon Sep 17 00:00:00 2001 From: Andreas Tolfsen Date: Thu, 26 Jan 2017 18:40:03 +0000 Subject: [PATCH 1/2] webdriver_server: associate timeouts with session The WebDriver timeout configuration durations should be associated with the session so that they do not bleed across sessions. Since they are currently stored on the WebDriverHandler, a call to the SetTimeouts command will cause them to affect the subsequent session. --- components/webdriver_server/lib.rs | 94 ++++++++++++++++++++---------- 1 file changed, 62 insertions(+), 32 deletions(-) diff --git a/components/webdriver_server/lib.rs b/components/webdriver_server/lib.rs index 98c250f85b5..514407020cb 100644 --- a/components/webdriver_server/lib.rs +++ b/components/webdriver_server/lib.rs @@ -96,18 +96,39 @@ pub fn start_server(port: u16, constellation_chan: Sender) { }).expect("Thread spawning failed"); } +/// Represents the current WebDriver session and holds relevant session state. struct WebDriverSession { id: Uuid, - frame_id: Option + frame_id: Option, + + /// Time to wait for injected scripts to run before interrupting them. + script_timeout: u32, + + /// Time to wait for a page to finish loading upon navigation. + load_timeout: u32, + + /// Time to wait for the element location strategy when retrieving elements, and when + /// waiting for an element to become interactable. + implicit_wait_timeout: u32, +} + +impl WebDriverSession { + pub fn new() -> WebDriverSession { + WebDriverSession { + id: Uuid::new_v4(), + frame_id: None, + + script_timeout: 30_000, + load_timeout: 300_000, + implicit_wait_timeout: 0, + } + } } struct Handler { session: Option, constellation_chan: Sender, - script_timeout: u32, - load_timeout: u32, resize_timeout: u32, - implicit_wait_timeout: u32 } #[derive(Clone, Copy, PartialEq)] @@ -230,24 +251,12 @@ impl ToJson for SetPrefsParameters { } } -impl WebDriverSession { - pub fn new() -> WebDriverSession { - WebDriverSession { - id: Uuid::new_v4(), - frame_id: None - } - } -} - impl Handler { pub fn new(constellation_chan: Sender) -> Handler { Handler { session: None, constellation_chan: constellation_chan, - script_timeout: 30_000, - load_timeout: 300_000, resize_timeout: 500, - implicit_wait_timeout: 0 } } @@ -355,19 +364,25 @@ impl Handler { fn wait_for_load(&self, sender: IpcSender, - receiver: IpcReceiver) -> WebDriverResult { - let timeout = self.load_timeout; + receiver: IpcReceiver) + -> WebDriverResult { + let session = try!(self.session + .as_ref() + .ok_or(WebDriverError::new(ErrorStatus::SessionNotCreated, ""))); + + let timeout = session.load_timeout; let timeout_chan = sender; thread::spawn(move || { thread::sleep(Duration::from_millis(timeout as u64)); let _ = timeout_chan.send(LoadStatus::LoadTimeout); }); - //Wait to get a load event + // wait to get a load event match receiver.recv().unwrap() { LoadStatus::LoadComplete => Ok(WebDriverResponse::Void), - LoadStatus::LoadTimeout => Err(WebDriverError::new(ErrorStatus::Timeout, - "Load timed out")) + LoadStatus::LoadTimeout => { + Err(WebDriverError::new(ErrorStatus::Timeout, "Load timed out")) + } } } @@ -683,15 +698,24 @@ impl Handler { } } - fn handle_set_timeouts(&mut self, parameters: &TimeoutsParameters) -> WebDriverResult { - //TODO: this conversion is crazy, spec should limit these to u32 and check upstream + fn handle_set_timeouts(&mut self, + parameters: &TimeoutsParameters) + -> WebDriverResult { + let mut session = try!(self.session + .as_mut() + .ok_or(WebDriverError::new(ErrorStatus::SessionNotCreated, ""))); + + // TODO(ato): Conversation is wrong. The script timeout can be null, and the numeric + // values should be limited to u32 in the standard. let value = parameters.ms as u32; match ¶meters.type_[..] { - "implicit" => self.implicit_wait_timeout = value, - "page load" => self.load_timeout = value, - "script" => self.script_timeout = value, - x => return Err(WebDriverError::new(ErrorStatus::InvalidSelector, - format!("Unknown timeout type {}", x))) + "script" => session.script_timeout = value, + "page load" => session.load_timeout = value, + "implicit" => session.implicit_wait_timeout = value, + x => { + return Err(WebDriverError::new(ErrorStatus::InvalidSelector, + format!("Unknown timeout type {}", x))) + } } Ok(WebDriverResponse::Void) } @@ -712,13 +736,19 @@ impl Handler { } fn handle_execute_async_script(&self, - parameters: &JavascriptCommandParameters) -> WebDriverResult { + parameters: &JavascriptCommandParameters) + -> WebDriverResult { + let session = try!(self.session + .as_ref() + .ok_or(WebDriverError::new(ErrorStatus::SessionNotCreated, ""))); + let func_body = ¶meters.script; let args_string = "window.webdriverCallback"; - let script = format!( - "setTimeout(webdriverTimeout, {}); (function(callback) {{ {} }})({})", - self.script_timeout, func_body, args_string); + let script = format!("setTimeout(webdriverTimeout, {}); (function(callback) {{ {} }})({})", + session.script_timeout, + func_body, + args_string); let (sender, receiver) = ipc::channel().unwrap(); let command = WebDriverScriptCommand::ExecuteAsyncScript(script, sender); From 90d1ee7602aabb41db1fee607bad16cb771d4cd9 Mon Sep 17 00:00:00 2001 From: Andreas Tolfsen Date: Thu, 26 Jan 2017 18:53:46 +0000 Subject: [PATCH 2/2] webdriver_server: allow script timeout to be optional The script timeout duration may be set to null, indicating that an injected script should run indefinitely without getting interrupted. This change fixes the internal data structure, but does not address the parsing of the input arguments to handle_set_timeouts. --- components/webdriver_server/lib.rs | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/components/webdriver_server/lib.rs b/components/webdriver_server/lib.rs index 514407020cb..0264197fb6c 100644 --- a/components/webdriver_server/lib.rs +++ b/components/webdriver_server/lib.rs @@ -101,8 +101,9 @@ struct WebDriverSession { id: Uuid, frame_id: Option, - /// Time to wait for injected scripts to run before interrupting them. - script_timeout: u32, + /// Time to wait for injected scripts to run before interrupting them. A [`None`] value + /// specifies that the script should run indefinitely. + script_timeout: Option, /// Time to wait for a page to finish loading upon navigation. load_timeout: u32, @@ -118,7 +119,7 @@ impl WebDriverSession { id: Uuid::new_v4(), frame_id: None, - script_timeout: 30_000, + script_timeout: Some(30_000), load_timeout: 300_000, implicit_wait_timeout: 0, } @@ -705,11 +706,10 @@ impl Handler { .as_mut() .ok_or(WebDriverError::new(ErrorStatus::SessionNotCreated, ""))); - // TODO(ato): Conversation is wrong. The script timeout can be null, and the numeric - // values should be limited to u32 in the standard. + // TODO: this conversion is crazy, spec should limit these to u32 and check upstream let value = parameters.ms as u32; match ¶meters.type_[..] { - "script" => session.script_timeout = value, + "script" => session.script_timeout = Some(value), "page load" => session.load_timeout = value, "implicit" => session.implicit_wait_timeout = value, x => { @@ -745,10 +745,15 @@ impl Handler { let func_body = ¶meters.script; let args_string = "window.webdriverCallback"; - let script = format!("setTimeout(webdriverTimeout, {}); (function(callback) {{ {} }})({})", - session.script_timeout, - func_body, - args_string); + let script = match session.script_timeout { + Some(timeout) => { + format!("setTimeout(webdriverTimeout, {}); (function(callback) {{ {} }})({})", + timeout, + func_body, + args_string) + } + None => format!("(function(callback) {{ {} }})({})", func_body, args_string), + }; let (sender, receiver) = ipc::channel().unwrap(); let command = WebDriverScriptCommand::ExecuteAsyncScript(script, sender);