Bug 1447977 - Handle cyclic references in element prototypes. r?automatedtester draft
authorAndreas Tolfsen <ato@sny.no>
Fri, 06 Jul 2018 20:08:37 +0100
changeset 816078 b42f3d93e0375205df2c0bae39fdccbc86ecabc2
parent 816077 4edf3ef4c78c6bb2ead534760a333d5cdd1e5b20
push id115749
push userbmo:ato@sny.no
push dateTue, 10 Jul 2018 15:56:00 +0000
reviewersautomatedtester
bugs1447977
milestone63.0a1
Bug 1447977 - Handle cyclic references in element prototypes. r?automatedtester JavaScript objects can be nested to any depth, and as such we must check that the value to be serialised contains a cyclic structure before attempting to marshaling it. We handle this correctly for collections and arbitrary objects by relying on JSON.stringify. For example with arrays: let arr = []; arr.push(arr); And for objects: let obj = {}; obj.reference = obj; However, members of the different element prototypes (HTMLElement, SVGElement, XULElement, et al.) may also contain cyclic references via user-defined own properties: let body = document.documentElement; body.reference = body; JSON.stringify enumerates an object's own properties, which means it picks up on body's "reference" property in the above example. Marionette needs to special case element prototypes because we want to marshal them as web elements. This patch replaces JSON.stringify with a custom function for testing if a value contains cyclic structures that special-cases elements. MozReview-Commit-ID: 1TQtHrjf401
testing/marionette/evaluate.js
testing/web-platform/meta/MANIFEST.json
testing/web-platform/tests/webdriver/tests/execute_script/cyclic.py
--- a/testing/marionette/evaluate.js
+++ b/testing/marionette/evaluate.js
@@ -281,29 +281,74 @@ evaluate.toJSON = function(obj, seenEls)
     }
   }
   return rv;
 };
 
 /**
  * Tests if an arbitrary object is cyclic.
  *
- * @param {*} obj
+ * Element prototypes are by definition acyclic, even when they
+ * contain cyclic references.  This is because `evaluate.toJSON`
+ * ensures they are marshaled as web elements.
+ *
+ * @param {*} val
  *     Object to test for cyclical references.
  *
  * @return {boolean}
  *     True if object is cyclic, false otherwise.
  */
-evaluate.isCyclic = function(obj) {
-  try {
-    JSON.stringify(obj);
+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 (element.isElement(value)) {
     return false;
-  } catch (e) {
+
+  // Array, NodeList, HTMLCollection, et al.
+  } else if (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
  * still test for a dead object.
  *
  * @param {Object} obj
--- a/testing/web-platform/meta/MANIFEST.json
+++ b/testing/web-platform/meta/MANIFEST.json
@@ -612417,17 +612417,17 @@
    "cd68cb3737f930193eef98fb61ed625b4c53d539",
    "testharness"
   ],
   "shadow-dom/untriaged/events/event-dispatch/test-002.html": [
    "d3538e40ac8cc4749d69cc0dcd35d42fd3ef778a",
    "testharness"
   ],
   "shadow-dom/untriaged/events/event-dispatch/test-003.html": [
-   "94ed940ccca11f9abc37a940ba5f5fc194ea2317",
+   "47b84ac9527ad9e51bdba5b0c0b3ecbdba2e3696",
    "testharness"
   ],
   "shadow-dom/untriaged/events/event-retargeting/test-001.html": [
    "0ac11ec3761be5e2102584167fcee2bbd5c5a018",
    "testharness"
   ],
   "shadow-dom/untriaged/events/event-retargeting/test-003.html": [
    "62381ee32583aa97adbb891211373c3fc2239796",
@@ -618941,17 +618941,17 @@
    "f3e48d8ddd42f1eecb36af2a8e1cfade6d0a02d4",
    "testharness"
   ],
   "web-animations/animation-model/animation-types/interpolation-per-property.html": [
    "ab09cd8b77d05a1036f9976c3f0e92a6d9e183f3",
    "testharness"
   ],
   "web-animations/animation-model/animation-types/property-list.js": [
-   "5a818163c3ddcb6e0901b4f0086d555e9d440e27",
+   "a6c524f515065db203ae5395f699b857eb279cd4",
    "support"
   ],
   "web-animations/animation-model/animation-types/property-types.js": [
    "ecfe1d54d687bc6d0541b4a8c5ca9cf82c4d129e",
    "support"
   ],
   "web-animations/animation-model/animation-types/visibility.html": [
    "da3370704ca9e83a1171a64320a240e3145fab2c",
@@ -619097,17 +619097,17 @@
    "c65dd7fd3c76ac1e5d6f22dbd36544f7900cd992",
    "testharness"
   ],
   "web-animations/interfaces/KeyframeEffect/iterationComposite.html": [
    "c5ce17faeb355f1e9efae516d6272a88c46daa1f",
    "testharness"
   ],
   "web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument-001.html": [
-   "f68c116e1da5ae8783187af22f00758d02b7a0e9",
+   "fe0f1f5f1b2066c71cc20495c96e6e89914788a8",
    "testharness"
   ],
   "web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument-002.html": [
    "e9237e244034845f6f902f8149a0e66e5b6164f2",
    "testharness"
   ],
   "web-animations/interfaces/KeyframeEffect/setKeyframes.html": [
    "c5c631ef4e728ea5f40f14ad5590e4836068f361",
@@ -620561,17 +620561,17 @@
    "da39a3ee5e6b4b0d3255bfef95601890afd80709",
    "support"
   ],
   "webdriver/tests/execute_script/collections.py": [
    "0ee4e340b38b6aa59043286755822460e76b2dbd",
    "wdspec"
   ],
   "webdriver/tests/execute_script/cyclic.py": [
-   "21bae43b3a6e966b8698b7c439b29a68029adc58",
+   "145a8a67226f31e0c1023aa0609947486be5c319",
    "wdspec"
   ],
   "webdriver/tests/execute_script/json_serialize_windowproxy.py": [
    "20db10d82ed2b28a22674fcdc37cac0323d33c95",
    "wdspec"
   ],
   "webdriver/tests/execute_script/user_prompts.py": [
    "d4f627cda9669efc7fb8197bf6adde5d65b0aa1f",
--- a/testing/web-platform/tests/webdriver/tests/execute_script/cyclic.py
+++ b/testing/web-platform/tests/webdriver/tests/execute_script/cyclic.py
@@ -1,9 +1,10 @@
-from tests.support.asserts import assert_error
+from tests.support.asserts import assert_error, assert_same_element, assert_success
+from tests.support.inline import inline
 
 
 def execute_script(session, script, args=None):
     if args is None:
         args = []
     body = {"script": script, "args": args}
 
     return session.transport.send(
@@ -41,8 +42,35 @@ def test_array_in_object(session):
 
 def test_object_in_array(session):
     response = execute_script(session, """
         let obj = {};
         obj.reference = obj;
         return [obj];
         """)
     assert_error(response, "javascript error")
+
+
+def test_element_in_collection(session):
+    session.url = inline("<div></div>")
+    divs = session.find.css("div")
+
+    response = execute_script(session, """
+        let div = document.querySelector("div");
+        div.reference = div;
+        return [div];
+        """)
+    value = assert_success(response)
+    for expected, actual in zip(divs, value):
+        assert_same_element(session, expected, actual)
+
+
+def test_element_in_object(session):
+    session.url = inline("<div></div>")
+    div = session.find.css("div", all=False)
+
+    response = execute_script(session, """
+        let div = document.querySelector("div");
+        div.reference = div;
+        return {foo: div};
+        """)
+    value = assert_success(response)
+    assert_same_element(session, div, value["foo"])