Bug 1128997 - Support indefinite script timeout r=ato
authorreimu <r2hkri@gmail.com>
Sun, 06 Jan 2019 23:04:41 +0000
changeset 509797 b97e2750407e3e98d7165c5605732ce2a7b0e68c
parent 509796 cd0f006ea4b311f6223c6d151f09aa9758c7ad92
child 509798 0a3a8b70b8bacd012d6165c3e909b0df1f95ba86
push id10547
push userffxbld-merge
push dateMon, 21 Jan 2019 13:03:58 +0000
treeherdermozilla-beta@24ec1916bffe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersato
bugs1128997
milestone66.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1128997 - Support indefinite script timeout r=ato Differential Revision: https://phabricator.services.mozilla.com/D13181
testing/geckodriver/src/marionette.rs
testing/marionette/capabilities.js
testing/marionette/evaluate.js
testing/marionette/test/unit/test_capabilities.js
testing/web-platform/tests/webdriver/tests/new_session/timeouts.py
testing/web-platform/tests/webdriver/tests/set_timeouts/set.py
testing/webdriver/src/capabilities.rs
testing/webdriver/src/command.rs
testing/webdriver/src/response.rs
--- a/testing/geckodriver/src/marionette.rs
+++ b/testing/geckodriver/src/marionette.rs
@@ -476,25 +476,28 @@ impl MarionetteSession {
             | GetElementTagName(_)
             | IsEnabled(_)
             | ExecuteScript(_)
             | ExecuteAsyncScript(_)
             | GetAlertText
             | TakeScreenshot
             | TakeElementScreenshot(_) => WebDriverResponse::Generic(resp.to_value_response(true)?),
             GetTimeouts => {
-                let script = try_opt!(
-                    try_opt!(
+                let script = match try_opt!(
                         resp.result.get("script"),
                         ErrorStatus::UnknownError,
                         "Missing field: script"
-                    ).as_u64(),
-                    ErrorStatus::UnknownError,
-                    "Failed to interpret script timeout duration as u64"
-                );
+                    ) {
+                        Value::Null => None,
+                        n => try_opt!(
+                            Some(n.as_u64()),
+                            ErrorStatus::UnknownError,
+                            "Failed to interpret script timeout duration as u64"
+                        ),
+                };
                 // Check for the spec-compliant "pageLoad", but also for "page load",
                 // which was sent by Firefox 52 and earlier.
                 let page_load = try_opt!(
                     try_opt!(
                         resp.result.get("pageLoad").or(resp.result.get("page load")),
                         ErrorStatus::UnknownError,
                         "Missing field: pageLoad"
                     ).as_u64(),
--- a/testing/marionette/capabilities.js
+++ b/testing/marionette/capabilities.js
@@ -62,18 +62,21 @@ class Timeouts {
     for (let [type, ms] of Object.entries(json)) {
       switch (type) {
         case "implicit":
           t.implicit = assert.positiveInteger(ms,
               pprint`Expected ${type} to be a positive integer, got ${ms}`);
           break;
 
         case "script":
-          t.script = assert.positiveInteger(ms,
-              pprint`Expected ${type} to be a positive integer, got ${ms}`);
+          if (ms !== null) {
+            assert.positiveInteger(ms,
+                pprint`Expected ${type} to be a positive integer, got ${ms}`);
+          }
+          t.script = ms;
           break;
 
         case "pageLoad":
           t.pageLoad = assert.positiveInteger(ms,
               pprint`Expected ${type} to be a positive integer, got ${ms}`);
           break;
 
         default:
@@ -433,17 +436,19 @@ class Capabilities extends Map {
   toString() { return "[object Capabilities]"; }
 
   /**
    * JSON serialisation of capabilities object.
    *
    * @return {Object.<string, ?>}
    */
   toJSON() {
-    return marshal(this);
+    let marshalled = marshal(this);
+    marshalled.timeouts = super.get("timeouts");
+    return marshalled;
   }
 
   /**
    * Unmarshal a JSON object representation of WebDriver capabilities.
    *
    * @param {Object.<string, *>=} json
    *     WebDriver capabilities.
    *
@@ -544,17 +549,17 @@ function getWebDriverPlatformName() {
 
     default:
       return name.toLowerCase();
   }
 }
 
 // Specialisation of |JSON.stringify| that produces JSON-safe object
 // literals, dropping empty objects and entries which values are undefined
-// or null.  Objects are allowed to produce their own JSON representations
+// or null. Objects are allowed to produce their own JSON representations
 // by implementing a |toJSON| function.
 function marshal(obj) {
   let rv = Object.create(null);
 
   function* iter(mapOrObject) {
     if (mapOrObject instanceof Map) {
       for (const [k, v] of mapOrObject) {
         yield [k, v];
--- a/testing/marionette/evaluate.js
+++ b/testing/marionette/evaluate.js
@@ -87,22 +87,21 @@ this.evaluate = {};
  */
 evaluate.sandbox = function(sb, script, args = [],
     {
       async = false,
       file = "dummy file",
       line = 0,
       timeout = DEFAULT_TIMEOUT,
     } = {}) {
-  let scriptTimeoutID, timeoutHandler, unloadHandler;
+  let scriptTimeoutID, unloadHandler;
 
   let promise = new Promise((resolve, reject) => {
     let src = "";
     sb[COMPLETE] = resolve;
-    timeoutHandler = () => reject(new ScriptTimeoutError(`Timed out after ${timeout} ms`));
     unloadHandler = sandbox.cloneInto(
         () => reject(new JavaScriptError("Document was unloaded")),
         sb);
 
     if (async) {
       sb[CALLBACK] = sb[COMPLETE];
     }
     sb[ARGUMENTS] = sandbox.cloneInto(args, sb);
@@ -115,17 +114,20 @@ evaluate.sandbox = function(sb, script, 
       src += `${ARGUMENTS}.push(rv => ${CALLBACK}(rv));`;
     }
 
     src += `(function() {
       ${script}
     }).apply(null, ${ARGUMENTS})`;
 
     // timeout and unload handlers
-    scriptTimeoutID = setTimeout(timeoutHandler, timeout);
+    if (timeout !== null) {
+      scriptTimeoutID = setTimeout(() => reject(new ScriptTimeoutError(
+          `Timed out after ${timeout} ms`)), timeout);
+    }
     sb.window.onunload = unloadHandler;
 
     let res;
     try {
       res = Cu.evalInSandbox(src, sb, "1.8", file, line);
     } catch (e) {
       reject(new JavaScriptError(e));
     }
--- a/testing/marionette/test/unit/test_capabilities.js
+++ b/testing/marionette/test/unit/test_capabilities.js
@@ -63,17 +63,17 @@ add_test(function test_Timeouts_fromJSON
     equal(e.message, "Unrecognised timeout: sessionId");
   }
 
   run_next_test();
 });
 
 add_test(function test_Timeouts_fromJSON_invalid_types() {
   for (let value of [null, [], {}, false, "10", 2.5]) {
-    Assert.throws(() => Timeouts.fromJSON({"script": value}), /InvalidArgumentError/);
+    Assert.throws(() => Timeouts.fromJSON({"implicit": value}), /InvalidArgumentError/);
   }
 
   run_next_test();
 });
 
 add_test(function test_Timeouts_fromJSON_bounds() {
   for (let value of [-1, Number.MAX_SAFE_INTEGER + 1]) {
     Assert.throws(() => Timeouts.fromJSON({"script": value}), /InvalidArgumentError/);
--- a/testing/web-platform/tests/webdriver/tests/new_session/timeouts.py
+++ b/testing/web-platform/tests/webdriver/tests/new_session/timeouts.py
@@ -1,22 +1,32 @@
 import pytest
 
-from tests.support.asserts import assert_success
+from tests.support.asserts import assert_success, assert_error
 
 
 def test_default_values(session):
     timeouts = session.capabilities["timeouts"]
 
     assert timeouts["implicit"] == 0
     assert timeouts["pageLoad"] == 300000
     assert timeouts["script"] == 30000
 
 
 @pytest.mark.parametrize("timeouts", [
     {"implicit": 444, "pageLoad": 300000,"script": 30000},
     {"implicit": 0, "pageLoad": 444,"script": 30000},
     {"implicit": 0, "pageLoad": 300000,"script": 444},
+    {"implicit": 0, "pageLoad": 300000,"script": None},
 ])
 def test_timeouts(new_session, add_browser_capabilities, timeouts):
     response, _ = new_session({"capabilities": {"alwaysMatch": add_browser_capabilities({"timeouts": timeouts})}})
     value = assert_success(response)
     assert value["capabilities"]["timeouts"] == timeouts
+
+@pytest.mark.parametrize("timeouts", [
+    {"implicit": None, "pageLoad": 300000,"script": 30000},
+    {"implicit": 0, "pageLoad": None,"script": 30000},
+    {"implicit": None, "pageLoad": None,"script": None}
+])
+def test_invalid_timeouts(new_session, add_browser_capabilities, timeouts):
+    response, _ = new_session({"capabilities": {"alwaysMatch": add_browser_capabilities({"timeouts": timeouts})}})
+    assert_error(response, "invalid argument")
--- a/testing/web-platform/tests/webdriver/tests/set_timeouts/set.py
+++ b/testing/web-platform/tests/webdriver/tests/set_timeouts/set.py
@@ -34,44 +34,66 @@ def test_parameters_empty_no_change(sess
     original = session.timeouts._get()
 
     response = set_timeouts(session, {})
     assert_success(response)
 
     assert session.timeouts._get() == original
 
 
+def test_script_parameter_empty_no_change(session):
+    original = session.timeouts._get()
+
+    response = set_timeouts(session, {"implicit": 100})
+    assert_success(response)
+
+    assert session.timeouts._get()["script"] == original["script"]
+
+
 def test_key_invalid(session):
     response = set_timeouts(session, {"foo": 1000})
     assert_error(response, "invalid argument")
 
 
 @pytest.mark.parametrize("typ", ["implicit", "pageLoad", "script"])
 @pytest.mark.parametrize("value", [0, 2.0, 2**53 - 1])
 def test_positive_integer(session, typ, value):
     response = set_timeouts(session, {typ: value})
     assert_success(response)
 
     assert session.timeouts._get(typ) == value
 
 
-@pytest.mark.parametrize("typ", ["implicit", "pageLoad", "script"])
+@pytest.mark.parametrize("typ", ["implicit", "pageLoad"])
 @pytest.mark.parametrize("value", [None, [], {}, False, "10"])
 def test_value_invalid_types(session, typ, value):
     response = set_timeouts(session, {typ: value})
     assert_error(response, "invalid argument")
 
 
+@pytest.mark.parametrize("value", [[], {}, False, "10"])
+def test_value_invalid_types_for_script(session, value):
+    response = set_timeouts(session, {"script": value})
+    assert_error(response, "invalid argument")
+
+
 @pytest.mark.parametrize("typ", ["implicit", "pageLoad", "script"])
 @pytest.mark.parametrize("value", [-1, 2.5, 2**53])
 def test_value_positive_integer(session, typ, value):
     response = set_timeouts(session, {typ: value})
     assert_error(response, "invalid argument")
 
 
 def test_set_all_fields(session):
     timeouts = {"implicit": 10, "pageLoad": 20, "script": 30}
     response = set_timeouts(session, timeouts)
     assert_success(response)
 
     assert session.timeouts.implicit == 10
     assert session.timeouts.page_load == 20
     assert session.timeouts.script == 30
+
+
+def test_script_value_null(session):
+    response = set_timeouts(session, {"script": None})
+    assert_success(response)
+
+    assert session.timeouts.script == None
--- a/testing/webdriver/src/capabilities.rs
+++ b/testing/webdriver/src/capabilities.rs
@@ -326,16 +326,18 @@ impl SpecNewSessionParameters {
         let obj = try_opt!(
             value.as_object(),
             ErrorStatus::InvalidArgument,
             "timeouts capability is not an object"
         );
 
         for (key, value) in obj {
             match &**key {
+                _x @ "script" if value.is_null() => { }
+
                 x @ "script" | x @ "pageLoad" | x @ "implicit" => {
                     let timeout = try_opt!(
                         value.as_f64(),
                         ErrorStatus::InvalidArgument,
                         format!("{} timeouts value is not a number: {}", x, value)
                     );
                     if timeout < 0.0 || timeout.fract() != 0.0 {
                         return Err(WebDriverError::new(
--- a/testing/webdriver/src/command.rs
+++ b/testing/webdriver/src/command.rs
@@ -499,19 +499,48 @@ pub struct TimeoutsParameters {
     #[serde(
         default,
         rename = "pageLoad",
         skip_serializing_if = "Option::is_none",
         deserialize_with = "deserialize_to_u64"
     )]
     pub page_load: Option<u64>,
     #[serde(
-        default, skip_serializing_if = "Option::is_none", deserialize_with = "deserialize_to_u64"
+        default,
+        skip_serializing_if = "Option::is_none",
+        deserialize_with = "deserialize_to_nullable_u64"
     )]
-    pub script: Option<u64>,
+    pub script: Option<Option<u64>>,
+}
+
+fn deserialize_to_nullable_u64<'de, D>(deserializer: D) -> Result<Option<Option<u64>>, D::Error>
+where
+    D: Deserializer<'de>,
+{
+    let opt = Option::deserialize(deserializer)?.map(|value: f64| value);
+    let value = match opt {
+        Some(n) => {
+            if n < 0.0 || n.fract() != 0.0 {
+                return Err(de::Error::custom(format!(
+                    "{} is not a positive Integer",
+                    n
+                )));
+            }
+            if (n as u64) > MAX_SAFE_INTEGER {
+                return Err(de::Error::custom(format!(
+                    "{} is greater than maximum safe integer",
+                    n
+                )));
+            }
+            Some(Some(n as u64))
+        }
+        None => Some(None),
+    };
+
+    Ok(value)
 }
 
 fn deserialize_to_u64<'de, D>(deserializer: D) -> Result<Option<u64>, D::Error>
 where
     D: Deserializer<'de>,
 {
     let opt = Option::deserialize(deserializer)?.map(|value: f64| value);
     let value = match opt {
@@ -1022,17 +1051,17 @@ mod tests {
     }
 
     #[test]
     fn test_json_timeout_parameters_with_values() {
         let json = r#"{"implicit":0,"pageLoad":2.0,"script":9007199254740991}"#;
         let data = TimeoutsParameters {
             implicit: Some(0u64),
             page_load: Some(2u64),
-            script: Some(9007199254740991u64),
+            script: Some(Some(9007199254740991u64)),
         };
 
         check_deserialize(&json, &data);
     }
 
     #[test]
     fn test_json_timeout_parameters_with_invalid_values() {
         let json = r#"{"implicit":-1,"pageLoad":2.5,"script":9007199254740992}"#;
--- a/testing/webdriver/src/response.rs
+++ b/testing/webdriver/src/response.rs
@@ -79,24 +79,24 @@ impl NewSessionResponse {
             capabilities,
             session_id,
         }
     }
 }
 
 #[derive(Debug, PartialEq, Serialize)]
 pub struct TimeoutsResponse {
-    pub script: u64,
+    pub script: Option<u64>,
     #[serde(rename = "pageLoad")]
     pub page_load: u64,
     pub implicit: u64,
 }
 
 impl TimeoutsResponse {
-    pub fn new(script: u64, page_load: u64, implicit: u64) -> TimeoutsResponse {
+    pub fn new(script: Option<u64>, page_load: u64, implicit: u64) -> TimeoutsResponse {
         TimeoutsResponse {
             script,
             page_load,
             implicit,
         }
     }
 }
 
@@ -255,17 +255,17 @@ mod tests {
         ));
 
         check_serialize(&json, &data);
     }
 
     #[test]
     fn test_json_timeouts_response() {
         let json = r#"{"value":{"script":1,"pageLoad":2,"implicit":3}}"#;
-        let data = WebDriverResponse::Timeouts(TimeoutsResponse::new(1, 2, 3));
+        let data = WebDriverResponse::Timeouts(TimeoutsResponse::new(Some(1), 2, 3));
 
         check_serialize(&json, &data);
     }
 
     #[test]
     fn test_json_void_response() {
         let json = r#"{"value":null}"#;
         let data = WebDriverResponse::Void;