Sentry-javascript: JSON Serialization error with circular reference

[cgeorg]

• [X] Review the documentation: https://docs.sentry.io/
• [X] Search for existing issues: https://github.com/getsentry/sentry-javascript/issues
• [X] Use the latest release: https://github.com/getsentry/sentry-javascript/releases
• [X] Provide a link to the affected event from your Sentry account

Package + Version

• [X] @sentry/browser
• [ ] @sentry/node
• [ ] raven-js
• [ ] raven-node _(raven for node)_
• [ ] other:

Version:

5.4.3,5.5.0

Description

When serializing an event/error that contains a circular reference, a browser error occurs, and then that error is logged, losing the original error.

Repro steps: open a console with sentry SDK loaded and initialized, run:

var obj = {};
obj.repro = obj;
Sentry.captureEvent(obj);

Logged stack trace in Sentry from the above:

Converting circular structure to JSON
    --> starting at object with constructor 'Object'
    --- property 'repro' closes the circle
<anonymous> in JSON.stringify
../src/transports/fetch.ts in sendEvent at line 15:18
../../core/src/basebackend.ts in sendEvent at line 96:21
../../core/src/baseclient.ts in onfulfilled at line 353:32
../../utils/src/syncpromise.ts in onSuccess at line 127:21
../../utils/src/syncpromise.ts at line 86:52
<anonymous> in Array.forEach
../../utils/src/syncpromise.ts in _executeHandlers at line 86:22
../../utils/src/syncpromise.ts in _attachHandler at line 96:10
../../utils/src/syncpromise.ts in callback at line 106:12
../../utils/src/syncpromise.ts in new n at line 42:7
../../utils/src/syncpromise.ts in then at line 105:12
../../core/src/baseclient.ts in callback at line 342:46
../../utils/src/syncpromise.ts in new n at line 42:7
../../core/src/baseclient.ts in _processEvent at line 341:12
../../core/src/baseclient.ts in method at line 135:10
../../hub/src/hub.ts in _invokeClient at line 81:27
../../hub/src/hub.ts in method at line 223:10
../../minimal/src/index.ts in callOnHub at line 13:17
../../minimal/src/index.ts in Object.n.captureEvent at line 64:10
<anonymous> at line 3:8

Link to affected production issue in Sentry (my guess is that this is an error involving the Datatables JS library, not sure though): https://sentry.io/organizations/grasp-and-grow/issues/1107636848/

I see similar issues, but they seem to involve memory consumption and crashes.

[kamilogorek]

captureException and captureMessage methods are guarding this behavior. However, captureEvent is a method that should (and do) enable user to send a raw event directly to Sentry, without any data alteration. It's not event mentioned in our documentation because of that as it's meant to be used in very specific scenarios and has to be as light-weight as possible.

You can either use @sentry/utils and our normalize function or any other library to serialize it. Or you can write your own Transport that does that, it's up to you really. However, we need a public method that allows things like this.

[cgeorg]

@kamilogorek if you look at the linked issue, it is happening in sentry code - a global handler is attempting to captureEvent.

<anonymous> in JSON.stringify ../src/transports/fetch.ts in sendEvent at line 15:18 ../../core/src/basebackend.ts in sendEvent at line 96:21 ../../core/src/baseclient.ts in onfulfilled at line 351:32 ../../utils/src/syncpromise.ts in onSuccess at line 127:21 ../../utils/src/syncpromise.ts at line 86:52 <anonymous> in Array.forEach ../../utils/src/syncpromise.ts in _executeHandlers at line 86:22 ../../utils/src/syncpromise.ts in _attachHandler at line 96:10 ../../utils/src/syncpromise.ts in callback at line 106:12 ../../utils/src/syncpromise.ts in new n at line 42:7 ../../utils/src/syncpromise.ts in then at line 105:12 ../../core/src/baseclient.ts in callback at line 340:46 ../../utils/src/syncpromise.ts in new n at line 42:7 ../../core/src/baseclient.ts in _processEvent at line 339:12 ../../core/src/baseclient.ts in method at line 138:10 ../../hub/src/hub.ts in _invokeClient at line 81:27 ../../hub/src/hub.ts in captureEvent at line 185:10 ../src/integrations/globalhandlers.ts in i at line 69:25 ../src/tracekit.ts in _notifyHandlers at line 182:20 ../src/tracekit.ts in _traceKitWindowOnError at line 255:7 /bundles/webappjs in Object.trigger at line 1:246952 /bundles/webappjs in HTMLTableElement.<anonymous> at line 1:247498 /bundles/webappjs in Function.each at line 1:190462 /bundles/webappjs in i.fn.init.each at line 1:189122 /bundles/webappjs in i.fn.init.trigger at line 1:247474 /bundles/webappjs in o at line 1:630391 /bundles/webappjs in nt at line 1:629537 /bundles/webappjs in Object.error at line 1:609920 /bundles/webappjs in l at line 1:214888 /bundles/webappjs in Object.fireWith [as rejectWith] at line 1:215633 /bundles/webappjs in b at line 1:250661 /bundles/webappjs in XMLHttpRequest.<anonymous> at line 1:255476

[kamilogorek]

@cgeorg is there a chance that you can upload /bundles/webappjs sourcemaps and retrigger this error? I'm trying to find a way to reproduce this issue (other than manually calling captureEvent) but I'm not able to. It looks like there's something with an XHR wrapper that adds HTMLTableElement as part of it's data payload.

[cgeorg]

We're using the ASP.NET bundling for that, it looks like it doesn't support source mapping out of the box. I'm trying to put together a codepen but can't get it to trigger the same issue. Having trouble getting around CORS things with codepen to hit the exact code path - I will update if I can get a simple repro for you.

[nie7321]

I'm hitting this as well. It looks like something DataTables.net is doing is causing it. I'm getting into a bad state cuz my login session is expired, so when DT.n asks the server for more JSON, it's instead getting a 401.

TypeError: Converting circular structure to JSON
    --> starting at object with constructor 'Object'
    |     property '_buttons' -> object with constructor 'Array'
    |     index 0 -> object with constructor 'Object'
    |     ...
    |     property 'contex...
  at JSON.stringify(<anonymous>)
  at sendEvent(../src/transports/fetch.ts:15:18)
  at sendEvent(../../core/src/basebackend.ts:96:21)
  at onfulfilled(../../core/src/baseclient.ts:355:32)
  at onSuccess(../../utils/src/syncpromise.ts:127:21)
  at ? (../../utils/src/syncpromise.ts:86:52)
  at Array.forEach(<anonymous>)
  at _executeHandlers(../../utils/src/syncpromise.ts:86:22)
  at _attachHandler(../../utils/src/syncpromise.ts:96:10)
  at callback(../../utils/src/syncpromise.ts:106:12)
  at new n(../../utils/src/syncpromise.ts:42:7)
  at then(../../utils/src/syncpromise.ts:105:12)
  at callback(../../core/src/baseclient.ts:344:46)
  at new n(../../utils/src/syncpromise.ts:42:7)
  at _processEvent(../../core/src/baseclient.ts:343:12)
  at method(../../core/src/baseclient.ts:137:10)
  at _invokeClient(../../hub/src/hub.ts:81:27)
  at captureEvent(../../hub/src/hub.ts:223:10)
  at i(../src/integrations/globalhandlers.ts:65:25)
  at _notifyHandlers(../src/tracekit.ts:183:20)
  at _traceKitWindowOnError(../src/tracekit.ts:256:7)
  at trigger(./node_modules/jquery/dist/jquery.js:8477:27)
  at call(./node_modules/jquery/dist/jquery.js:8549:17)
  at each(./node_modules/jquery/dist/jquery.js:367:19)
  at each(./node_modules/jquery/dist/jquery.js:202:17)
  at trigger(./node_modules/jquery/dist/jquery.js:8548:15)
  at _fnCallbackFire(./node_modules/datatables.net/js/jquery.dataTables.js:6643:23)
  at _fnLog(./node_modules/datatables.net/js/jquery.dataTables.js:6470:5)
  at apply(./node_modules/datatables.net/js/jquery.dataTables.js:3907:7)
  at fire(./node_modules/jquery/dist/jquery.js:3291:31)
  at rejectWith(./node_modules/jquery/dist/jquery.js:3421:7)
  at complete(./node_modules/jquery/dist/jquery.js:9535:14)
  at XMLHttpRequest.<anonymous>(./node_modules/jquery/dist/jquery.js:9785:9)

Here's my app.js & app.js.map:

sentry-javascript_issues_2190.zip

Hope this helps!

[kamilogorek]

@nie7321 what version are you using? I had no reports of any similar issue is quite a long time.

[nie7321]

@kamilogorek 5.6.3, which if afaik the latest:

<script src="https://browser.sentry-cdn.com/5.6.3/bundle.min.js" integrity="sha384-/Cqa/8kaWn7emdqIBLk3AkFMAHBk0LObErtMhO+hr52CntkaurEnihPmqYj3uJho" crossorigin="anonymous"></script>

[kamilogorek]

Thanks. Is there a way that you could provide a reproducible example? Just loading your app.js doesn't trigger anything, so I'm not able to retrigger it.

[nie7321]

@kamilogorek have a look at https://codepen.io/nie7321/pen/bGGdLxE?editors=1111

I have a nonsense DSN in there, but I can see it trying to send the stack trace in question if I look at the POST:

Hopefully that's useful!

[kamilogorek]

Umm... I get a completely different message 😅

[nie7321]

That's odd -- I am still getting the right message from that codepen.

I've tested on Chrome Version 76.0.3809.132 (Official Build) (64-bit) & Version 77.0.3865.90 (Official Build) (64-bit), both on OS X.

[cgeorg]

I also get the circular JSON error in recent Chromes on OSX and Windows
(once I add a valid dsn).

On Wed, Oct 9, 2019 at 10:19 AM Nick Evans [e-mail] wrote:

That's odd -- I am still getting the right message from that codepen.

I've tested on Chrome Version 76.0.3809.132 (Official Build) (64-bit) &
Version 77.0.3865.90 (Official Build) (64-bit), both on OS X.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/getsentry/sentry-javascript/issues/2190?email_source=notifications&email_token=ABMJAPBWDJNQWMHNIPSPNC3QNXR5JA5CNFSM4IJOEBGKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAYBMTI#issuecomment-540022349,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABMJAPHOA5MVWANUNUFAHWTQNXR5JANCNFSM4IJOEBGA
.

[schmod]

I'm seeing this in a node.js server. Here's a minimal reproduction example:

const Sentry = require('@sentry/node');
const app = require('express')();

const dsn = 'https://[email protected]/000000';

Sentry.init({ dsn });

app.use(Sentry.Handlers.requestHandler());
app.use('/', () => {
  console.log(
    JSON.stringify(new Promise((resolve) => setTimeout(resolve, 50)))
  );
});

const port = 3000;
app.listen(port, () => console.log(`App listening on port ${port}!`));

$ curl localhost:3000

TypeError: Converting circular structure to JSON
    at JSON.stringify (<anonymous>)
    at app.use (/Users/andrew/source/sentry-circular/index.js:11:10)
    at Layer.handle [as handle_request] (/Users/andrew/source/sentry-circular/node_modules/express/lib/router/layer.js:95:5)
    at trim_prefix (/Users/andrew/source/sentry-circular/node_modules/express/lib/router/index.js:317:13)
    at /Users/andrew/source/sentry-circular/node_modules/express/lib/router/index.js:284:7
    at Function.process_params (/Users/andrew/source/sentry-circular/node_modules/express/lib/router/index.js:335:12)
    at next (/Users/andrew/source/sentry-circular/node_modules/express/lib/router/index.js:275:10)
    at Domain.<anonymous> (/Users/andrew/source/sentry-circular/node_modules/@sentry/node/dist/handlers.js:192:13)
    at Domain.run (domain.js:349:14)
    at sentryRequestMiddleware (/Users/andrew/source/sentry-circular/node_modules/@sentry/node/dist/handlers.js:188:15)

[kamilogorek]

@schmod did some research around your issue, and there are two things. One bad, it's no-fix in our codebase, and one good, if you update node to v11.12.0+, your issue will solve itself.

The problem here, is that when you use requestHandler, you create an instance of domain that has to be there in order to create an execution context for all the requests.

Once it's there, node internally attaches it to all the promises that are created inside the lifecycle of a given domain. What happens then, is your promise has a domain key, which in itself contains a reference to req and res of the middleware.

When you try to call JSON.stringify on it, it's trying to serialize promise.domain.req, which references res and other way around.

promise.domain.req.res === promise.domain.res;
promise.domain.res.req === promise.domain.req

The same thing would happen if you try to serialize them directly:

app.use('/', (req, res) => {
  // both calls would break
  JSON.stringify(req);
  JSON.stringify(res);
});

Thankfully, it has been fixed in version 11.12 of Node, so it shouldn't be an issue after upgrade. Till then, Promises are simply non-serializable objects, especially when done manually like you did above, as we cannot patch JSON.stringify.

Reference:

https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V11.md#2019-03-15-version-11120-current-bridgear

[56adebf789] - domain: set .domain non-enumerable on resources (Jordan Harband) #26210

[cgeorg]

I don't think this should be closed, there is still a valid issue that can be seen here: https://github.com/getsentry/sentry-javascript/issues/2190#issuecomment-540005201

[kamilogorek]

@cgeorg true, missed that one, as there were too many things in one thread.

Debugging second issue now.

😒

[cgeorg]

I just ran the codepen example again, substituting my own valid DSN, and I got a circular stack trace error logged in my project.

The event ID is f18cfba0991d498096041515e29158b4

Converting circular structure to JSON
--> starting at object with constructor 'Object'
| property '_buttons' -> object with constructor 'Array'
| index 0 -> object with constructor 'Object'
| ...
| property 'contex...
in JSON.stringify
../src/transports/fetch.ts in sendEvent at line 15:18
../../core/src/basebackend.ts in sendEvent at line 96:21
../../core/src/baseclient.ts in onfulfilled at line 355:32
../../utils/src/syncpromise.ts in onSuccess at line 127:21
../../utils/src/syncpromise.ts at line 86:52
in Array.forEach
../../utils/src/syncpromise.ts in _executeHandlers at line 86:22
../../utils/src/syncpromise.ts in _attachHandler at line 96:10
../../utils/src/syncpromise.ts in callback at line 106:12
../../utils/src/syncpromise.ts in new n at line 42:7
../../utils/src/syncpromise.ts in then at line 105:12
../../core/src/baseclient.ts in callback at line 344:46
../../utils/src/syncpromise.ts in new n at line 42:7
../../core/src/baseclient.ts in _processEvent at line 343:12
../../core/src/baseclient.ts in method at line 137:10
../../hub/src/hub.ts in _invokeClient at line 81:27
../../hub/src/hub.ts in captureEvent at line 223:10
../src/integrations/globalhandlers.ts in i at line 65:25
../src/tracekit.ts in _notifyHandlers at line 183:20
../src/tracekit.ts in HTMLBodyElement.c at line 256:7
/ajax/libs/jquery/3.4.1/jquery.min.js in Object.trigger at line 2:70678
/ajax/libs/jquery/3.4.1/jquery.min.js in HTMLTableElement. at line 2:71222
/ajax/libs/jquery/3.4.1/jquery.min.js in Function.each at line 2:2777
/ajax/libs/jquery/3.4.1/jquery.min.js in k.fn.init.each at line 2:1419
/ajax/libs/jquery/3.4.1/jquery.min.js in k.fn.init.trigger at line 2:71198
/1.10.20/js/jquery.dataTables.min.js in A at line 89:439
/1.10.20/js/jquery.dataTables.min.js in O at line 88:91
/1.10.20/js/jquery.dataTables.min.js in Object.error at line 49:394
/ajax/libs/jquery/3.4.1/jquery.min.js in c at line 2:27742
/ajax/libs/jquery/3.4.1/jquery.min.js in Object.fireWith [as rejectWith] at line 2:28487
/ajax/libs/jquery/3.4.1/jquery.min.js in l at line 2:78814
/ajax/libs/jquery/3.4.1/jquery.min.js in XMLHttpRequest. at line 2:81117

[kamilogorek]

@nie7321 @cgeorg this is some very broken piece of code. Even onerror handler doesn't provide correct data for it to be any use and I don't see any sensible way patching it in the SDK itself. The best I can do here, is to make sure that it won't break.

[cgeorg]

I think that is reasonable - for instance, you could try stringifying the properties being added from the error to the sentry event, and if an exception is thrown replace them with some generic message about an error serializing that part of the message. This way we'd still get as much information about the original error as possible.

[kamilogorek]

We already do this for Events, but it requires onerror data to be in place. Here we'd have to assume that a message can be either string or this event object, which is non-standard in any format. This would simply require too much additional boilerplate to handle, and it's the first time I see such behaviour in the wild.

It won't be the most beautiful error report you'll ever see, but when paired with $.fn.dataTable.ext.errMode = 'throw'; you'll get necessary information. See: https://datatables.net/manual/tech-notes/7

Patched here: https://github.com/getsentry/sentry-javascript/pull/2302

[kamilogorek]

Fixed and merged into master. 5.8.0 will be released on Monday morning. #nodeployfridays