"Non-Error exception captured with keys: [...]" due to invalid error instanceof check

[theofidry]

Is there an existing issue for this?

• I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
• I have reviewed the documentation https://docs.sentry.io/
• I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

How do you use Sentry?

Self-hosted/on-premise

Which package are you using?

@sentry/angular

SDK Version

7.7.0

Framework Version

Angular 13.3 & Ionic 5

Link to Sentry event

No response

Steps to Reproduce

// Case1: capture string
Sentry.captureException('foo);

// Case2: capture error-like object
Sentry.captureException({
  name: 'FooError',
  message: 'foo',
});

// Case3: throw string
throw 'foo';

// Case4: throw error-like object
throw {
  name: 'FooError',
  message: 'foo',
};

// Case5: capture string with the SentryAngular ErrorHandler
this.errorHandler.handleError('foo');

// Case6: capture error-like object with the SentryAngular ErrorHandler
this.errorHandler.handleError({
  name: 'FooError',
  message: 'foo',
});

Expected Result

The first problem I identify here is the incorrect handling of error like objects. I suspect this is due to this line:

    if (typeof error === 'string' || error instanceof Error) {
      return error;
    }

I am really not convinced an instanceof should be used here. Having a type guard to check for an error like object (it could also have a quick instanceof under the hood), would make more sense and also be more in line with the philosophy of TypeScript where types are checked as structures, instead of instances.

---

The second problem I see is the difference of handling when an error is thrown, captured by Sentry manually or passed to the error handler.

That there is a difference between Sentry.captureException() and ErrorHandler.handle() makes sense since the latter is using the former under the hood. But I think having a difference of handling for when an error is thrown instead of passed to the error handler is a lot more surprising.

I however could not find where the global listener is registered and check the problematic code or figure out a way to fix this.

Actual Result

// Case1: capture string
Nothing captured!

// Case2: capture error-like object

captureException
Non-Error exception captured with keys: message, name

// Case3: throw string
Nothing captured!

// Case4: throw error-like object

Error resolvePromise(polyfills)
Uncaught (in promise): Object: {"name":"FooError","message":"foo"}

// Case5: capture string with the SentryAngular ErrorHandler
Nothing captured!

// Case6: capture error-like object with the SentryAngular ErrorHandler
Nothing captured!

[AbhiPrasad]

Hey @theofidry thanks for writing in. I think you're right that we can re-evaluate the angular error handler logic here.

We can change the instance check, but ideally we make sure that errors have a error.stack as well. If you have any ideas, PRs are welcome.

Also cc @Lms24 here to comment on some of the Angular specific things.

[Lms24]

I agree, we should get rid of the instance check (type guard sounds like a good suggestion) and try to make the behaviour similar to captureException.

No idea though, why the output looks so different between captureException and throwing the object. @theofidry just fyi, here's where we attach our handler to window.onerror and how we handle such errors:

sentry-javascript/packages/browser/src/integrations/globalhandlers.ts

Lines 76 to 106 in a93cd23

	/** JSDoc */
	function _installGlobalOnErrorHandler(): void {
	addInstrumentationHandler(
	'error',
	// eslint-disable-next-line @typescript-eslint/no-explicit-any
	(data: { msg: any; url: any; line: any; column: any; error: any }) => {
	const [hub, stackParser, attachStacktrace] = getHubAndOptions();
	if (!hub.getIntegration(GlobalHandlers)) {
	return;
	}
	const { msg, url, line, column, error } = data;
	if (shouldIgnoreOnError() || (error && error.__sentry_own_request__)) {
	return;
	}
	const event =
	error === undefined && isString(msg)
	? _eventFromIncompleteOnError(msg, url, line, column)
	: _enhanceEventWithInitialFrame(
	eventFromUnknownInput(stackParser, error || msg, undefined, attachStacktrace, false),
	url,
	line,
	column,
	);
	event.level = 'error';
	addMechanismAndCapture(hub, error, event, 'onerror');
	},
	);
	}

Admittedly without looking too closely, my guess is that the difference comes from the eventFromUnknownInput function.

Gonna backlog this for now for the Errorhandler part at least.

[theofidry]

Thanks a lot for the replies!

We can change the instance check, but ideally we make sure that errors have a error.stack as well. If you have any ideas, PRs are welcome.

Is it really necessary? At the very least from this code and that one, it looks like that strings at least are fine.

So if I understand correct, it could be an error-like object without the stack, but it needs at the very least the message.

Admittedly without looking too closely, my guess is that the difference comes from the eventFromUnknownInput function.

@Lms24 is there any chance/recommendation as too how to hook the Angular error handler instead of this? Or do they need to be different?

It might not be too much of a problem if a parity is provided with the native code (the one attached to the window and the angular error handler), but this sounds hard and I also struggle to see how it would work if one extends the error handler.

[Lms24]

Is it really necessary?

I think what @AbhiPrasad meant was that having a stack trace is better than not having one, as it just provides a lot more context around the captured error. The Angular ErrorHandler should handle strings fine. But according to your description, cases 1, 3 and 5 regardless of the method, string "errors" aren't captured at all. This is weird because when I try to capture strings (Sentry.captureException('Test String Error');), the error shows up in Sentry.

is there any chance/recommendation as too how to hook the Angular error handler instead of this? Or do they need to be different?

You mean you'd like to capture errors captured in window.onerror via the Angular ErrorHandler? Not sure if this works but you could try disabling the GlobalHandlers integration and adding your own handler to window.onerror which does that?

Btw, if you want to change how errors are extracted from the Angular ErrorHandler, you can pass a custom extractor to the ErrorHandler's constructor

[theofidry]

Thanks a lot for the reply.

I'll give another try at the handling of strings. If it works on your side, since the code is up to date, the last possible culprit is that our Sentry instance is faulty or there is a weird ignore rule or something. I didn't really think of that when I posted the issue, I will check.

Otherwise it looks at the very least that we all agree on the undesired instance of Error. So I can do a PR for that.

[lforst]

We just released this change with version 7.30.0 of the SDK.