Instrumenting EventTarget.prototype.addEventListener leads to problems in Chrome

[ffxsam]

• [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
• [ ] N/A: Provide a link to the affected event from your Sentry account

Package + Version

• [X] @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...

[kamilogorek]

Awesome, thanks! And in the meantime https://github.com/getsentry/sentry-javascript/pull/2078

[futpib]

EDIT: found a cleaner workaround:

Sentry.init({
    ...
    // XXX: Workaround for https://github.com/getsentry/sentry-javascript/issues/2074
    defaultIntegrations: Sentry.defaultIntegrations.filter(({ name }) => name !== 'TryCatch'),
});

old stuff

I'm opting for this workaround for now. ```js EventTarget.prototype.addEventListener = EventTarget.prototype.addEventListener.__sentry_original__; EventTarget.prototype.removeEventListener = EventTarget.prototype.removeEventListener.__sentry_original__; ```

[javan]

This issue is causing a fair amount of noise for us: Tens of thousands of events per hour, ~1.8 million total. Looking forward to a fix. 😬

[kamilogorek]

@javan fixed pending PR tests, we will release patch this week.

[futpib]

I'm still seeing this on @sentry/browser@5.3.0.

Running this snippet in the console still throws when sentry is enabled.

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

[javan]

Same. Issue is still present in v5.3.0.

Example: https://sentry-browser-chrome-bug.glitch.me Source: https://glitch.com/edit/#!/sentry-browser-chrome-bug?path=src/main.js:18:0

Screenshot


[javan]

It appears that using apply() instead of call() works around the issue. Not sure if it's a viable solution for Sentry's implementation.

For example, this snippet tickles the Chrome bug:

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

let count = 0
while (count++ < 10000) addEventListener("click", console.log)

This one does not:

const original = EventTarget.prototype.addEventListener
EventTarget.prototype.addEventListener = function() {
  "use strict"
  original.apply(this, arguments)
}

let count = 0
while (count++ < 10000) addEventListener("click", console.log)

[javan]

Also, @kamilogorek, I think this issue should be reopened.

[kamilogorek]

Done. Although I'll need some time to take a look at it, as I'm busy with other work. Any help appreciated :)

[rhcarvalho]

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

May 23, 2019 The Chromium issue has been marked as a duplicate of https://bugs.chromium.org/p/chromium/issues/detail?id=961199, which has been marked as fixed on the same day.

March 13, 2020 I wonder if this is still an issue with recent versions of Chrome/Chromium and the Sentry JS SDK?

[rhcarvalho]

I haven't found any relevant uses of "use strict" in master, I imagine the implementation might have changed quite a bit since the problem was reported. Marking this a needs-repro for the time being.

[Fonger]

@rhcarvalho

We still see many errors coming from Chrome 74 Mobile.

Interestingly, there's no issue with bundle.min.js (from sentrycdn) We start to see this issue when we switch to bundle.es6.min.js

I guess babel transpile somehow eliminate the usage of buggy function call in Chrome 74 somehow

Our hole website only targets es6 (es2015) so we hope to use es6 build

[kamilogorek]

@Fonger can you provide a link to the affected event ever here or directly to kamil@sentry.io? Thanks

[Fonger]

@kamilogorek sure. just emailed

We had rollbacked to use bundle.min.js several minutes ago though.

[SaraVieira]

Hey!

Sorry to do a plus one on this but seems we are also encountering this at codesandbox and its been also in the bunches:

Interesting thing is that it seems to only happen on chrome on a mac

You can see the user created crash report here: https://github.com/codesandbox/codesandbox-client/issues/5326

Let me know if you need anything else or any help in this

[kamilogorek]

Cross-posting for the reference: https://github.com/codesandbox/codesandbox-client/issues/5406#issuecomment-768339564