Max call stack on objects with circular reference

[scripter-co]

• Sinon version : 4.4.2
• Environment : node
• Example URL : https://github.com/scripter-co/sinon-circular-reference
• Other libraries you are using: mocha

What did you expect to happen?

Sinon does not throw an error when it comes across a circular reference

What actually happens

Sinon fails with the following:

➜  circular-reference git:(master) ✗ npm t

> circular-reference@1.0.0 test /Users/mahdan01/workspace/circular-reference
> mocha ./tests



  1) throws a "Maximum call stack size exceeded" error in sinon deepEqual

  0 passing (28ms)
  1 failing

  1) throws a "Maximum call stack size exceeded" error in sinon deepEqual:
     RangeError: Maximum call stack size exceeded
      at isElement (node_modules/sinon/lib/sinon/util/core/deep-equal.js:28:19)
      at deepEqual (node_modules/sinon/lib/sinon/util/core/deep-equal.js:37:9)
      at deepEqual$matcher (node_modules/sinon/lib/sinon/util/core/deep-equal.js:111:16)
      at deepEqual (node_modules/sinon/lib/sinon/util/core/deep-equal.js:84:45)
      at deepEqual$matcher (node_modules/sinon/lib/sinon/util/core/deep-equal.js:111:16)
      at deepEqual (node_modules/sinon/lib/sinon/util/core/deep-equal.js:84:45)
      at deepEqual$matcher (node_modules/sinon/lib/sinon/util/core/deep-equal.js:111:16)
      at deepEqual (node_modules/sinon/lib/sinon/util/core/deep-equal.js:84:45)
      at deepEqual$matcher (node_modules/sinon/lib/sinon/util/core/deep-equal.js:111:16)

How to reproduce

https://github.com/scripter-co/sinon-circular-reference

simply:

const sinon = require('sinon');

it('throws a "Maximum call stack size exceeded" error in sinon deepEqual', () => {
  const spy = sinon.spy();

  const firstObj = {};
  firstObj['aKeyName'] = firstObj;

  const secondObj = {};
  secondObj['aKeyName'] = secondObj;

  spy(firstObj);

  sinon.assert.calledWith(spy, secondObj);
});

Additional information:

I have tracked it down to here: https://github.com/sinonjs/sinon/blob/master/lib/sinon/util/core/deep-equal.js#L84. The problem is that when you are comparing the two objects, you will always fall into this condition and continue to send a[prop], b[prop] to arguments[2]. It's worth nothing that if the circular reference key names are different, this problem does not exists due to prop not existing on both a and b.

The use case that this is a problem is when using React within something like JSDOM. React adds a __reactInternalInstance$ID_HERE (e.g. __reactInternalInstance$upckzz9n7m) key to elements that it renders on the page. Within this key you get a circular reference and therefore under the correct conditions, you will get this error.

[fatso83]

Any idea on how we should approach this issue? I would think that adding a cache of identifiers would be
a) a massive memory hog
b) a total performance killer (if constantly having to check the cache to see if we have already traversed this route)

We could add an exception handler that we could explicitly configure to ignore such errors, but I think that would make for unexpected results and should affect performance negatively as well.

[mroderick]

We could use the deepEqual function from samsam, which supports comparison of cyclic objects.

https://github.com/sinonjs/samsam/blob/master/lib/samsam.js#L166-L181

[scripter-co]

samsam.deepEqual doesnt cover all the cases we need so we'll end up with a half way house where we implement some checks, then fall back to samsam. At that point, you may as well just implement everything within sinon.

for instance, the following is a problem:

console.log(samsam.deepEqual({}, null)); // undefined
console.log(samsam.deepEqual({}, { a: true })); // false

we would do all the previous checks in sinon's deepEqual and then, if both types are object, we use samsam.

as a side note, both of @fatso83's points would be introduced by using samsam's deepEqual as it uses a path cache. that being said, this does fix the problem, from the test cases i've written.

[mroderick]

I agree that the first line of your example is a problem.

I don't understand why the second line would be considered a problem. Could you help me understand it?

Recently, the sinonjs GitHub organisation has taken over a number of projects from the busterjs organisation, including samsam.

We are working towards re-releasing samsam as @sinonjs/samsam on npm and will do a major version bump when we do.

There's nothing to stop us from fixing issues in samsam.deepEqual and ensure that it works well with sinon.

Since you understand the problem well, perhaps you could create issues on https://github.com/sinonjs/samsam for where deepEqual fails to live up to expectations?

[scripter-co]

@mroderick apologies for the confusion, the second line was just there for comparative reasons.

Interesting about the breaking change. Would be a good time to get samsam.deepEqual to return a boolean at all times (which it probably should always have done).

I'll create the issue over on samsam to get some feedback.

[mroderick]

Would be a good time to get samsam.deepEqual to return a boolean at all times (which it probably should always have done).

I'll create the issue over on samsam to get some feedback.

Thanks 🍨

[mroderick]

@scripter-co do you want to do the honours of importing @sinonjs/samsam@2.0.0, to address this issue?

Would you mind writing a test that shows that it can now deal with circular structures?

[scripter-co]

@mroderick of course 👍

[scripter-co]

@mroderick I've added a PR. It's a bit of a weird one, samsam can handle most of the cases, however with Error it can't compare the two so I've had to leave it in. I'm not 100% that all use cases are covered by the unit tests (although can't think of any at the moment) so would be great to get you to take a thorough look over it as, although its only small, it's rather a big change.

Edit: In fact, still seems a case where it can get into a loop when there is a matcher provided. Going to take a further look.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[mroderick]

@scripter-co this is a rather long running issue. Would you mind if someone else finishes it?

[scripter-co]

I'll try take a look in the next few days but sure, if someone else can look into it I don't mind.

[mroderick]

This has been fixed with #1853 and published to NPM as sinon@6.1.3