Bug 1447977 - Handle cyclic references in element prototypes. r=automatedtester
authorAndreas Tolfsen <ato@sny.no>
Fri, 06 Jul 2018 20:08:37 +0100
changeset 425710 b4cd5a640564cee2dda926724bf2faeb8934d702
parent 425709 cbb958819d32f25d41d0e6fe61105d57a882b3eb
child 425757 12f49235dfd8d8b7330bf6e722ffdc122752ec0d
push id66200
push useratolfsen@mozilla.com
push dateTue, 10 Jul 2018 21:22:45 +0000
treeherderautoland@b4cd5a640564 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersautomatedtester
bugs1447977
milestone63.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 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 {*} value
  *     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"])