fix(browser): Use apply rather than call in try-catch integration

[lobsterkatie]

Users have reported running into a bug in Chrome wherein calling addEventListener or requestAnimationFrame too many times on window eventually throws an error when our try-catch integration is running, specifically because of how we wrap those functions. In the discussion of that bug, one user reported that replacing our call call with an apply call in our wrapping functions solved the problem for him.

This makes that change, in the hopes it will fix the problem for everyone.

Fixes #3388
Fixes #2074

Ref: https://getsentry.atlassian.net/browse/WEB-669

[AbhiPrasad]

Now that we're here, can we add a integration test for this?

[github-actions]

size-limit report

Path	Base Size (4e65cb7)	Current Size	Change
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.79 KB	19.79 KB	-0.01% 🔽
@sentry/browser - ES5 CDN Bundle (minified)	63.38 KB	63.38 KB	+0.01% 🔺
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.44 KB	18.44 KB	+0.01% 🔺
@sentry/browser - ES6 CDN Bundle (minified)	56.41 KB	56.41 KB	+0.02% 🔺
@sentry/browser - Webpack (gzipped + minified)	22.27 KB	22.27 KB	+0.01% 🔺
@sentry/browser - Webpack (minified)	76.34 KB	76.35 KB	+0.01% 🔺
@sentry/react - Webpack (gzipped + minified)	22.3 KB	22.3 KB	+0.01% 🔺
@sentry/nextjs Client - Webpack (gzipped + minified)	46.43 KB	46.43 KB	+0.01% 🔺
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	26.69 KB	26.69 KB	+0.01% 🔺
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	25.01 KB	25.02 KB	+0.01% 🔺

[kamilogorek]

cc @BYK

[lobsterkatie]

Now that we're here, can we add a integration test for this?

What test would you add, exactly? As far as I know, this is a workaround for a bug that only exists in Chrome 74 and 75, last current in mid-2019. Or do you just mean integration tests for addEventListener and requestAnimationFrame in general?

[kamilogorek]

LGTM. Agree that tests would be nice addition.

[BYK]

I am not sure if this will actually fix the issue. My guess is the this reference on that .call() call being undefined or null and Chrome complaining for some reason?

[AbhiPrasad]

Or do you just mean integration tests for addEventListener and requestAnimationFrame in general?

This, but we can skip for now if you want

[lobsterkatie]

I am not sure if this will actually fix the issue. My guess is the this reference on that .call() call being undefined or null and Chrome complaining for some reason?

To be honest, I'm not sure, either. I can confirm that using the repro provided in the older issue, this error shows up in Chrome 74 and 75, but a) not in Chrome 76, and b) not if you use apply rather than call. That said, this may be a totally separate problem which just happens to manifest the same way.

Before digging further, though, I'd like to try this, because it's a painless fix. If it doesn’t work, we can debug further. (If that does happen, because it turns out this is in fact a different problem, it'd be great if someone could figure out a way to reproduce the new problem.)

This fix should land in 6.18.3, which will be released before the end of the week.

[lobsterkatie]

Or do you just mean integration tests for addEventListener and requestAnimationFrame in general?

This, but we can skip for now if you want.

Let's get this out so folks can try it, and handle the tests separately. Also, FWIW, we do actually have integration tests for both of those, they're just not playwright tests:

sentry-javascript/packages/browser/test/integration/suites/builtins.js

Lines 2 to 24 in 008982f

	it("should capture exceptions from event listeners", function() {
	return runInSandbox(sandbox, function() {
	var div = document.createElement("div");
	document.body.appendChild(div);
	div.addEventListener(
	"click",
	function() {
	window.element = div;
	window.context = this;
	foo();
	},
	false
	);
	var click = new MouseEvent("click");
	div.dispatchEvent(click);
	}).then(function(summary) {
	// Make sure we preserve the correct context
	assert.equal(summary.window.element, summary.window.context);
	delete summary.window.element;
	delete summary.window.context;
	assert.match(summary.events[0].exception.values[0].value, /baz/);
	});
	});

and

sentry-javascript/packages/browser/test/integration/suites/builtins.js

Lines 87 to 99 in 008982f

	it("should capture exceptions inside callback", function() {
	// needs to be visible or requestAnimationFrame won't ever fire
	sandbox.style.display = "block";
	return runInSandbox(sandbox, { manual: true }, function() {
	requestAnimationFrame(function() {
	window.finalizeManualTest();
	foo();
	});
	}).then(function(summary) {
	assert.match(summary.events[0].exception.values[0].value, /baz/);
	});
	});

.

[BYK]

@lobsterkatie .apply() is significantly slower than .call() so this may cause you some headaches, especially with time-sensitive things like requestAnimationFrame().

EDIT: Seems like the slowness does not exist on Chromium-based browsers. Firefox showed a significant dip in .apply() here: https://www.measurethat.net/Benchmarks/Show/398/0/direct-call-vs-bind-vs-call-vs-apply (but MS Edge only showed a marginal drop)

[lobsterkatie]

@lobsterkatie .apply() is significantly slower than .call() so this may cause you some headaches, especially with time-sensitive things like requestAnimationFrame().

Interesting...

I hadn't heard that before, so I just went and did a little research. Most of what I found was from the early aughts, but I did find one site which will benchmark it directly, and will also let you see other users' results, so you get to see a wider variety of platforms, browsers, and versions. The upshot is that you're right when it comes to Firefox (and the difference is pretty drastic), but in Safari and Chrome they're nearly identical.

Firefox:

Safari:

Chrome:

That said, I am an OG FF lover and mean no offense to its userbase, but it is also down to ~4% market share. So I'd like to go ahead and see if this fixes the problem, and if it doesn't, we can always revert and try a different strategy.