Instrumenting EventTarget.prototype.addEventListener leads to problems in Chrome

[ffxsam]

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

Package + Version

• @sentry/browser 5.2.0

Description

We're getting this error:

Cannot convert undefined or null to object

It seems to be happening here, according to Chrome:

https://github.com/getsentry/sentry-javascript/blob/master/packages/browser/src/integrations/trycatch.ts#L88

Here's the stack trace:

Here's a snapshot of the error in Chrome DevTools:

Seems like OpenLayers is throwing an error, but Sentry is not handling it properly and throwing an error itself.

[BenoitZugmeyer]

I'm stumbling across this error as well. This occurs sometimes when calling the global function addEventListener or removeEventListener, ex:

addEventListener("someevent", callback)

Calling it with a "this parameter" resolves the issue:

window.addEventListener("someevent", callback)

Sadly, I don't reproduce it consistently. The only thing I can think of is that in the first case, this is undefined (because of the "use strict" mode), and in the later case, this would be window.

I am using @sentry/browser@4.4.2. It seems to occur only on recent Chrome / Chromium versions (74.0.3729 and 75.0.3770, see details below). The "fun" fact is: this error is reported quite often so this is the main reason my Sentry event quota is exhausted.

EDIT: joining a screenshot of the detailed list of impacted browsers
As you can see, this seems to start from Chrome 74.0.3729, which has been out since april 23.

[ffxsam]

This is only happening for us when we're using OpenLayers to create a new map. As a result, I've decided to ditch OL for Leaflet (probably a good move in general), and I'm just hoping this solves my issue.

[BenoitZugmeyer]

I managed to write a minimal program to reproduce this consistently. Just run it in your console devtools:

const original = EventTarget.prototype.addEventListener
EventTarget.prototype.addEventListener = function (eventName, fn, options) {
  "use strict"
  original.call(this, eventName, fn, options)
}

for (var i = 0; i < 100000; i += 1) {
  if (i % 100 === 0) console.log(i)
  addEventListener("mouseup", () => {})
}

In my case, it throws after the 4600th iteration.

[ffxsam]

On a semi-related note, when I remove Sentry, LogRocket trips up in a very similar way.

[kamilogorek]

@BenoitZugmeyer @ffxsam thanks for the repro case. After further investigation, I can tell that it's not our SDK issue, it's just Chrome's limit for event listeners per object.

When you call the repro above on any page (be it http://example.com) it will break as well. No SDK required.

[BenoitZugmeyer]

Yes, this is a Chrome issue. But incuding your SDK triggers that issue, breaking our code. Lucky for me this is easily fixable in my codebase, but I would be a bit annoyed if it occured in a dependency like @ffxsam

[kamilogorek]

@BenoitZugmeyer how so? We don't add our own listeners. We only intercept user's calls to addEventListener. If you don't call it, we won't as well.
And the issue is not triggered by us, but it's reported from our codebase, as this is the place where original invocation ends up.

[BenoitZugmeyer]

Basically, in your SDK, you are doing this to intercept addEventListener calls:

const original = EventTarget.prototype.addEventListener
EventTarget.prototype.addEventListener = function (eventName, fn, options) {
  "use strict"
  original.call(this, eventName, fn, options)
}

Removing those lines from the snippet I gave you (meaning: removing your SDK in my code) fixes the issue: we can add as many event listeners as needed. So of course you don't add listeners on you own, but the Chrome bug is triggered by the way you are intercepting addEventListener calls.

[kamilogorek]

@BenoitZugmeyer you are right here, my bad. It's because of mentioned strict mode. The only thing we can do here is disable, and then it all works just fine 🤷‍♂

const original = EventTarget.prototype.addEventListener
EventTarget.prototype.addEventListener = function (eventName, fn, options) {
  original.call(this, eventName, fn, options)
}

for (var i = 0; i < 100000; i += 1) {
  if (i % 100 === 0) console.log(i)
  addEventListener("mouseup", () => {})
}

[BenoitZugmeyer]

I guess you could also do something like original.call(this || window, ...). Strict mode is not added in the function like in my snippet, but in the dist files generated by your build setup.

[kamilogorek]

this || window would effectively alter developers code behavior, so I'm not sure about this one.
use strict is function scoped, and because bundle.js defines it at the very top (well, TypeScript does that for us), it also affects wrapping method mentioned above:

/*! @sentry/browser 5.2.1 (a5b87c1e) | https://github.com/getsentry/sentry-javascript */
var Sentry = (function (exports) {
    'use strict';

Although this || window fixes the issue so... :S and it's like 99.9% usecase that it's coming from window's proto anyway.

[BenoitZugmeyer]

I reported the issue here: https://bugs.chromium.org/p/chromium/issues/detail?id=964249

I think it wouldn't alter the code behavior because calling the original addEventListener function with undefined seems equivalent to calling it with the global object. But I may not think about all the cases there...