Bug 1794078 - [marionette] Make "seen" list of objects in "clone an object" compliant to the WebDriver spec. r=webdriver-reviewers,jgraham,jdescottes

Differential Revision: https://phabricator.services.mozilla.com/D166034

remote/marionette/element.sys.mjs

@@ -451,11 +451,11 @@ element.findClosest = function(startNode, selector) {
  *
  * @param {ElementIdentifier} id
  *     The WebElement reference identifier for a DOM element.
+ * @param {NodeCache} nodeCache
+ *     Node cache that holds already seen WebElement and ShadowRoot references.
  * @param {WindowProxy} win
  *     Current window, which may differ from the associated
  *     window of <var>el</var>.
- * @param {NodeCache} seenEls
- *     Known element store to look up Element instances from.
  *
  * @return {Element|null} The DOM element that the identifier was generated
  *     for, or null if the element does not still exist.
@@ -467,8 +467,8 @@ element.findClosest = function(startNode, selector) {
  *     If the element has gone stale, indicating its node document is no
  *     longer the active document or it is no longer attached to the DOM.
  */
-element.resolveElement = function(id, win, seenEls) {
-  const el = seenEls.resolve(id);
+element.resolveElement = function(id, nodeCache, win) {
+  const el = nodeCache.resolve(id);
 
   // For WebDriver classic only elements from the same browsing context
   // are allowed to be accessed.

remote/marionette/evaluate.sys.mjs

@@ -7,7 +7,6 @@ import { clearTimeout, setTimeout } from "resource://gre/modules/Timer.sys.mjs";
 const lazy = {};
 
 ChromeUtils.defineESModuleGetters(lazy, {
-  element: "chrome://remote/content/marionette/element.sys.mjs",
   error: "chrome://remote/content/shared/webdriver/Errors.sys.mjs",
 });
 
@@ -20,30 +19,6 @@ const FINISH = "finish";
 /** @namespace */
 export const evaluate = {};
 
-/**
- * Asserts that an arbitrary object is not cyclic.
- *
- * @param {Object} obj
- *     Object to test.  This assertion is only meaningful if passed
- *     an actual object or array.
- * @param {String=} msg
- *     Custom message to use for `error` if assertion fails.
- * @param {Error=} [error=JavaScriptError] error
- *     Error to throw if assertion fails.
- *
- * @throws {JavaScriptError}
- *     If the object is cyclic.
- */
-evaluate.assertAcyclic = function(
-  obj,
-  msg = "",
-  err = lazy.error.JavaScriptError
-) {
-  if (evaluate.isCyclic(obj)) {
-    throw new err(msg || "Cyclic object value");
-  }
-};
-
 /**
  * Evaluate a script in given sandbox.
  *
@@ -190,71 +165,6 @@ evaluate.sandbox = function(
     });
 };
 
-/**
- * Tests if an arbitrary object is cyclic.
- *
- * Element prototypes are by definition acyclic, even when they
- * contain cyclic references.  This is because `evaluate.cloneJSON`
- * ensures they are marshaled as web elements.
- *
- * @param {*} value
- *     Object to test for cyclical references.
- *
- * @return {boolean}
- *     True if object is cyclic, false otherwise.
- */
-evaluate.isCyclic = function(value, stack = []) {
-  let t = Object.prototype.toString.call(value);
-
-  // null
-  if (t == "[object Undefined]" || t == "[object Null]") {
-    return false;
-
-    // primitives
-  } else if (
-    t == "[object Boolean]" ||
-    t == "[object Number]" ||
-    t == "[object String]"
-  ) {
-    return false;
-
-    // HTMLElement, SVGElement, XULElement, et al.
-  } else if (lazy.element.isElement(value)) {
-    return false;
-
-    // Array, NodeList, HTMLCollection, et al.
-  } else if (lazy.element.isCollection(value)) {
-    if (stack.includes(value)) {
-      return true;
-    }
-    stack.push(value);
-
-    for (let i = 0; i < value.length; i++) {
-      if (evaluate.isCyclic(value[i], stack)) {
-        return true;
-      }
-    }
-
-    stack.pop();
-    return false;
-  }
-
-  // arbitrary objects
-  if (stack.includes(value)) {
-    return true;
-  }
-  stack.push(value);
-
-  for (let prop in value) {
-    if (evaluate.isCyclic(value[prop], stack)) {
-      return true;
-    }
-  }
-
-  stack.pop();
-  return false;
-};
-
 /**
  * `Cu.isDeadWrapper` does not return true for a dead sandbox that
  * was assosciated with and extension popup.  This provides a way to

remote/marionette/json.sys.mjs

@@ -9,7 +9,6 @@ const lazy = {};
 ChromeUtils.defineESModuleGetters(lazy, {
   element: "chrome://remote/content/marionette/element.sys.mjs",
   error: "chrome://remote/content/shared/webdriver/Errors.sys.mjs",
-  evaluate: "chrome://remote/content/marionette/evaluate.sys.mjs",
   Log: "chrome://remote/content/shared/Log.sys.mjs",
   pprint: "chrome://remote/content/shared/Format.sys.mjs",
   ShadowRoot: "chrome://remote/content/marionette/element.sys.mjs",
@@ -29,8 +28,8 @@ export const json = {};
  *
  * @param {Object} value
  *     Object to be cloned.
- * @param {NodeCache} seenEls
- *     Known node cache to look up WebElement and ShadowRoot instances from.
+ * @param {Set} seen
+ *     List of objects already processed.
  * @param {Function} cloneAlgorithm
  *     The clone algorithm to invoke for individual list entries or object
  *     properties.
@@ -38,28 +37,35 @@ export const json = {};
  * @return {Object}
  *     The cloned object.
  */
-function cloneObject(value, seenEls, cloneAlgorithm) {
-  if (lazy.element.isCollection(value)) {
-    lazy.evaluate.assertAcyclic(value);
-    return [...value].map(entry => cloneAlgorithm(entry, seenEls));
+function cloneObject(value, seen, cloneAlgorithm) {
+  // Only proceed with cloning an object if it hasn't been seen yet.
+  if (seen.has(value)) {
+    throw new lazy.error.JavaScriptError("Cyclic object value");
   }
+  seen.add(value);
 
-  // arbitrary objects
-  let result = {};
-  for (let prop in value) {
-    lazy.evaluate.assertAcyclic(value[prop]);
+  let result;
 
-    try {
-      result[prop] = cloneAlgorithm(value[prop], seenEls);
-    } catch (e) {
-      if (e.result == Cr.NS_ERROR_NOT_IMPLEMENTED) {
-        lazy.logger.debug(`Skipping ${prop}: ${e.message}`);
-      } else {
-        throw e;
+  if (lazy.element.isCollection(value)) {
+    result = [...value].map(entry => cloneAlgorithm(entry, seen));
+  } else {
+    // arbitrary objects
+    result = {};
+    for (let prop in value) {
+      try {
+        result[prop] = cloneAlgorithm(value[prop], seen);
+      } catch (e) {
+        if (e.result == Cr.NS_ERROR_NOT_IMPLEMENTED) {
+          lazy.logger.debug(`Skipping ${prop}: ${e.message}`);
+        } else {
+          throw e;
+        }
       }
     }
   }
 
+  seen.delete(value);
+
   return result;
 }
 
@@ -82,13 +88,12 @@ function cloneObject(value, seenEls, cloneAlgorithm) {
  *   a callable `toJSON` function, are returned verbatim.  This means
  *   their internal integrity _are not_ checked.  Be careful.
  *
- * -  Other arbitrary objects are first tested for cyclic references
- *    and then recursed into.
+ * - If a cyclic references is detected a JavaScriptError is thrown.
  *
  * @param {Object} value
  *     Object to be cloned.
- * @param {NodeCache} seenEls
- *     Known node cache to look up WebElement and ShadowRoot instances from.
+ * @param {NodeCache} nodeCache
+ *     Node cache that holds already seen WebElement and ShadowRoot references.
  *
  * @return {Object}
  *     Same object as provided by `value` with the WebDriver specific
@@ -100,8 +105,12 @@ function cloneObject(value, seenEls, cloneAlgorithm) {
  *     If the element has gone stale, indicating it is no longer
  *     attached to the DOM.
  */
-json.clone = function(value, seenEls) {
-  function cloneJSON(value, seenEls) {
+json.clone = function(value, nodeCache) {
+  function cloneJSON(value, seen) {
+    if (seen === undefined) {
+      seen = new Set();
+    }
+
     const type = typeof value;
 
     if ([undefined, null].includes(value)) {
@@ -129,7 +138,7 @@ json.clone = function(value, seenEls) {
         );
       }
 
-      const sharedId = seenEls.add(el);
+      const sharedId = nodeCache.add(value);
       return lazy.WebReference.from(el, sharedId).toJSON();
     } else if (typeof value.toJSON == "function") {
       // custom JSON representation
@@ -139,14 +148,14 @@ json.clone = function(value, seenEls) {
       } catch (e) {
         throw new lazy.error.JavaScriptError(`toJSON() failed with: ${e}`);
       }
-      return cloneJSON(unsafeJSON, seenEls);
+      return cloneJSON(unsafeJSON, seen);
     }
 
     // Collections and arbitrary objects
-    return cloneObject(value, seenEls, cloneJSON);
+    return cloneObject(value, seen, cloneJSON);
   }
 
-  return cloneJSON(value, seenEls);
+  return cloneJSON(value, new Set());
 };
 
 /**
@@ -154,8 +163,8 @@ json.clone = function(value, seenEls) {
  *
  * @param {Object} value
  *     Arbitrary object.
- * @param {NodeCache} seenEls
- *     Known node cache to look up WebElement and ShadowRoot instances from.
+ * @param {NodeCache} nodeCache
+ *     Node cache that holds already seen WebElement and ShadowRoot references.
  * @param {WindowProxy} win
  *     Current window.
  *
@@ -168,8 +177,12 @@ json.clone = function(value, seenEls) {
  * @throws {StaleElementReferenceError}
  *     If the element is stale, indicating it is no longer attached to the DOM.
  */
-json.deserialize = function(value, seenEls, win) {
-  function deserializeJSON(value, seenEls) {
+json.deserialize = function(value, nodeCache, win) {
+  function deserializeJSON(value, seen) {
+    if (seen === undefined) {
+      seen = new Set();
+    }
+
     if (value === undefined || value === null) {
       return value;
     }
@@ -190,16 +203,16 @@ json.deserialize = function(value, seenEls, win) {
             webRef instanceof lazy.WebElement ||
             webRef instanceof lazy.ShadowRoot
           ) {
-            return lazy.element.resolveElement(webRef.uuid, win, seenEls);
+            return lazy.element.resolveElement(webRef.uuid, nodeCache, win);
           }
 
           // WebFrame and WebWindow not supported yet
           throw new lazy.error.UnsupportedOperationError();
         }
 
-        return cloneObject(value, seenEls, deserializeJSON);
+        return cloneObject(value, seen, deserializeJSON);
     }
   }
 
-  return deserializeJSON(value, seenEls);
+  return deserializeJSON(value, new Set());
 };

remote/marionette/test/xpcshell/test_evaluate.js

@@ -1,71 +0,0 @@
-const { evaluate } = ChromeUtils.importESModule(
-  "chrome://remote/content/marionette/evaluate.sys.mjs"
-);
-
-add_test(function test_acyclic() {
-  evaluate.assertAcyclic({});
-
-  Assert.throws(() => {
-    const obj = {};
-    obj.reference = obj;
-    evaluate.assertAcyclic(obj);
-  }, /JavaScriptError/);
-
-  // custom message
-  const cyclic = {};
-  cyclic.reference = cyclic;
-  Assert.throws(
-    () => evaluate.assertAcyclic(cyclic, "", RangeError),
-    RangeError
-  );
-  Assert.throws(
-    () => evaluate.assertAcyclic(cyclic, "foo"),
-    /JavaScriptError: foo/
-  );
-  Assert.throws(
-    () => evaluate.assertAcyclic(cyclic, "bar", RangeError),
-    /RangeError: bar/
-  );
-
-  run_next_test();
-});
-
-add_test(function test_isCyclic_noncyclic() {
-  for (const type of [true, 42, "foo", [], {}, null, undefined]) {
-    ok(!evaluate.isCyclic(type));
-  }
-
-  run_next_test();
-});
-
-add_test(function test_isCyclic_object() {
-  const obj = {};
-  obj.reference = obj;
-  ok(evaluate.isCyclic(obj));
-
-  run_next_test();
-});
-
-add_test(function test_isCyclic_array() {
-  const arr = [];
-  arr.push(arr);
-  ok(evaluate.isCyclic(arr));
-
-  run_next_test();
-});
-
-add_test(function test_isCyclic_arrayInObject() {
-  const arr = [];
-  arr.push(arr);
-  ok(evaluate.isCyclic({ arr }));
-
-  run_next_test();
-});
-
-add_test(function test_isCyclic_objectInArray() {
-  const obj = {};
-  obj.reference = obj;
-  ok(evaluate.isCyclic([obj]));
-
-  run_next_test();
-});

remote/marionette/test/xpcshell/test_json.js

@@ -121,6 +121,38 @@ add_test(function test_clone_objects() {
   run_next_test();
 });
 
+add_test(function test_clone_сyclicReference() {
+  // object
+  Assert.throws(() => {
+    const obj = {};
+    obj.reference = obj;
+    json.clone(obj, nodeCache);
+  }, /JavaScriptError/);
+
+  // array
+  Assert.throws(() => {
+    const array = [];
+    array.push(array);
+    json.clone(array, nodeCache);
+  }, /JavaScriptError/);
+
+  // array in object
+  Assert.throws(() => {
+    const array = [];
+    array.push(array);
+    json.clone({ array }, nodeCache);
+  }, /JavaScriptError/);
+
+  // object in array
+  Assert.throws(() => {
+    const obj = {};
+    obj.reference = obj;
+    json.clone([obj], nodeCache);
+  }, /JavaScriptError/);
+
+  run_next_test();
+});
+
 add_test(function test_deserialize_generalTypes() {
   // null
   equal(json.deserialize(undefined, nodeCache, win), undefined);

remote/marionette/test/xpcshell/xpcshell.ini

@@ -12,7 +12,6 @@ skip-if = appname == "thunderbird"
 [test_cookie.js]
 [test_dom.js]
 [test_element.js]
-[test_evaluate.js]
 [test_json.js]
 [test_message.js]
 [test_modal.js]