assert: fix deepEqual RangeError: Maximum call stack size exceeded

Fixes: #13314 Refs: #6416 This commit changes semantics of the memos cycles tracker. Before the change it was used to track all currently wisited nodes of an object tree, which is a bit shifted from its original intention of tracking cycles. The change brings intended semantics, by tracking only objects of the current branch of the object tree. PR-URL: #13318 Fixes: #13314 Ref: #6416 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
nodejs · Jun 5, 2017 · 47c9de9 · 47c9de9
1 parent d1b39d9
commit 47c9de9
Show file tree

Hide file tree

Showing 3 changed files with 38 additions and 13 deletions.
diff --git a/lib/assert.js b/lib/assert.js
@@ -262,24 +262,27 @@ function _deepEqual(actual, expected, strict, memos) {
   // Use memos to handle cycles.
   if (!memos) {
     memos = {
-      actual: { map: new Map(), position: 0 },
-      expected: { map: new Map(), position: 0 }
+      actual: new Map(),
+      expected: new Map(),
+      position: 0
     };
-  }
-
-  const actualPosition = memos.actual.map.get(actual);
-  if (actualPosition !== undefined) {
-    if (actualPosition === memos.expected.map.get(expected)) {
-      return true;
-    }
   } else {
-    memos.actual.map.set(actual, memos.actual.position++);
+    memos.position++;
   }
-  if (!memos.expected.map.has(expected)) {
-    memos.expected.map.set(expected, memos.expected.position++);
+
+  if (memos.actual.has(actual)) {
+    return memos.actual.get(actual) === memos.expected.get(expected);
   }
 
-  return objEquiv(actual, expected, strict, memos);
+  memos.actual.set(actual, memos.position);
+  memos.expected.set(expected, memos.position);
+
+  const areEq = objEquiv(actual, expected, strict, memos);
+
+  memos.actual.delete(actual);
+  memos.expected.delete(expected);
+
+  return areEq;
 }
 
 function setHasSimilarElement(set, val1, strict, memo) {

diff --git a/test/parallel/test-assert-deep.js b/test/parallel/test-assert-deep.js
@@ -158,6 +158,16 @@ assertNotDeepOrStrict(new Set([1, 2, 3, 4]), new Set([1, 2, 3]));
 assertDeepAndStrictEqual(new Set(['1', '2', '3']), new Set(['1', '2', '3']));
 assertDeepAndStrictEqual(new Set([[1, 2], [3, 4]]), new Set([[3, 4], [1, 2]]));
 
+const a = [ 1, 2 ];
+const b = [ 3, 4 ];
+const c = [ 1, 2 ];
+const d = [ 3, 4 ];
+
+assertDeepAndStrictEqual(
+  { a: a, b: b, s: new Set([a, b]) },
+  { a: c, b: d, s: new Set([d, c]) }
+);
+
 assertDeepAndStrictEqual(new Map([[1, 1], [2, 2]]), new Map([[1, 1], [2, 2]]));
 assertDeepAndStrictEqual(new Map([[1, 1], [2, 2]]), new Map([[2, 2], [1, 1]]));
 assertNotDeepOrStrict(new Map([[1, 1], [2, 2]]), new Map([[1, 2], [2, 1]]));

diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js
@@ -535,6 +535,18 @@ a.throws(makeBlock(thrower, TypeError), function(err) {
 
   a.throws(makeBlock(a.deepEqual, d, e), /AssertionError/);
   a.throws(makeBlock(a.deepStrictEqual, d, e), /AssertionError/);
+
+  // https://github.com/nodejs/node/issues/13314
+  const f = {};
+  f.ref = f;
+
+  const g = {};
+  g.ref = g;
+
+  const h = {ref: g};
+
+  a.throws(makeBlock(a.deepEqual, f, h), /AssertionError/);
+  a.throws(makeBlock(a.deepStrictEqual, f, h), /AssertionError/);
 }
 // GH-7178. Ensure reflexivity of deepEqual with `arguments` objects.
 const args = (function() { return arguments; })();