Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(react): Handle case where error.cause already defined #7557

Merged
merged 4 commits into from Mar 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 20 additions & 1 deletion packages/react/src/errorboundary.tsx
Expand Up @@ -66,6 +66,25 @@ const INITIAL_STATE = {
eventId: null,
};

function setCause(error: Error & { cause?: Error }, cause: Error): void {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlikely that it is gonna happen but this could infinitely recurse. Are we worried about that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right, let me be a little more defensive and track the visited nodes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done with d0c99b2

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks weird to me:

  • fn signature: setCause(error, cause)
  • usage: setCause(error, errorBoundaryError);

errorBoundaryError is NOT the cause of the error being captured by the boundary. errorBoundaryError is an artificial error that you created at the top of the tree.

I have no idea why you try to put this error as a cause deeply in the error stack, but that looks really suspicious to me. errorBoundaryError should rather be the parent top-level error, not the most deeply nested cause.

const seenErrors = new WeakMap<Error, boolean>();

function recurse(error: Error & { cause?: Error }, cause: Error): void {
// If we've already seen the error, there is a recursive loop somewhere in the error's
// cause chain. Let's just bail out then to prevent a stack overflow.
if (seenErrors.has(error)) {
return;
}
if (error.cause) {
seenErrors.set(error, true);
return recurse(error.cause, cause);
}
error.cause = cause;
}

recurse(error, cause);
}

/**
* A ErrorBoundary component that logs errors to Sentry. Requires React >= 16.
* NOTE: If you are a Sentry user, and you are seeing this stack frame, it means the
Expand Down Expand Up @@ -93,7 +112,7 @@ class ErrorBoundary extends React.Component<ErrorBoundaryProps, ErrorBoundarySta
errorBoundaryError.stack = componentStack;

// Using the `LinkedErrors` integration to link the errors together.
error.cause = errorBoundaryError;
setCause(error, errorBoundaryError);
}

if (beforeCapture) {
Expand Down
101 changes: 91 additions & 10 deletions packages/react/test/errorboundary.test.tsx
Expand Up @@ -3,14 +3,8 @@ import { fireEvent, render, screen } from '@testing-library/react';
import * as React from 'react';
import { useState } from 'react';

import type {
ErrorBoundaryProps} from '../src/errorboundary';
import {
ErrorBoundary,
isAtLeastReact17,
UNKNOWN_COMPONENT,
withErrorBoundary,
} from '../src/errorboundary';
import type { ErrorBoundaryProps } from '../src/errorboundary';
import { ErrorBoundary, isAtLeastReact17, UNKNOWN_COMPONENT, withErrorBoundary } from '../src/errorboundary';

const mockCaptureException = jest.fn();
const mockShowReportDialog = jest.fn();
Expand Down Expand Up @@ -39,7 +33,13 @@ function Bam(): JSX.Element {
return <Boo title={title} />;
}

const TestApp: React.FC<ErrorBoundaryProps> = ({ children, ...props }) => {
interface TestAppProps extends ErrorBoundaryProps {
errorComp?: JSX.Element;
}

const TestApp: React.FC<TestAppProps> = ({ children, errorComp, ...props }) => {
// eslint-disable-next-line no-param-reassign
const customErrorComp = errorComp || <Bam />;
const [isError, setError] = React.useState(false);
return (
<ErrorBoundary
Expand All @@ -51,7 +51,7 @@ const TestApp: React.FC<ErrorBoundaryProps> = ({ children, ...props }) => {
}
}}
>
{isError ? <Bam /> : children}
{isError ? customErrorComp : children}
<button
data-testid="errorBtn"
onClick={() => {
Expand Down Expand Up @@ -299,6 +299,87 @@ describe('ErrorBoundary', () => {
expect(error.cause).not.toBeDefined();
});

it('handles when `error.cause` is nested', () => {
const mockOnError = jest.fn();

function CustomBam(): JSX.Element {
const firstError = new Error('bam');
const secondError = new Error('bam2');
const thirdError = new Error('bam3');
// @ts-ignore Need to set cause on error
secondError.cause = firstError;
// @ts-ignore Need to set cause on error
thirdError.cause = secondError;
throw thirdError;
}

render(
<TestApp fallback={<p>You have hit an error</p>} onError={mockOnError} errorComp={<CustomBam />}>
<h1>children</h1>
</TestApp>,
);

expect(mockOnError).toHaveBeenCalledTimes(0);
expect(mockCaptureException).toHaveBeenCalledTimes(0);

const btn = screen.getByTestId('errorBtn');
fireEvent.click(btn);

expect(mockCaptureException).toHaveBeenCalledTimes(1);
expect(mockCaptureException).toHaveBeenLastCalledWith(expect.any(Error), {
contexts: { react: { componentStack: expect.any(String) } },
});

expect(mockOnError.mock.calls[0][0]).toEqual(mockCaptureException.mock.calls[0][0]);

const thirdError = mockCaptureException.mock.calls[0][0];
const secondError = thirdError.cause;
const firstError = secondError.cause;
const cause = firstError.cause;
expect(cause.stack).toEqual(mockCaptureException.mock.calls[0][1].contexts.react.componentStack);
expect(cause.name).toContain('React ErrorBoundary');
expect(cause.message).toEqual(thirdError.message);
Comment on lines +338 to +341
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The errorBoundaryError shouldn't be deeply nested, it should be the parent instead. The fact that an error has been captured by the boundary is not the "root cause" of the problem, it's the consequence.

The root cause (deeply nested cause) is something low level like "undefined is not a function", not "Sentry Error Boundary captured an error"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We patch the error boundary error at the end so that it does not affect issue grouping - since there are some flaws to the synthetic stacktrace generated by the error boundary error (since componenStack is generated under the hood by throwing errors and walking up the component tree, it can sometimes point to the wrong line of code).

This is the most ergonomic way to get the error into sentry (and sourcemapped so you can see actual filenames/source context) without affecting existing/new grouping configurations.

You're right that this is conceptually wrong - but IMO the drawback is acceptable to make sure users get the best experience possible while staying within the limitations of both the componentStack and the sentry event schema.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: the imperfections in component stacks, see: facebook/react#18561

The idea is to throw as early as possible. Ideally it's the first line such as the built-in super call of a class or the destructuring of props in the argument position. It could however be slightly later which might be slightly confusing to see the line of the first hook for example. Source code aware tooling could adjust this number to the previous function definition before the offset. If it doesn't throw at all such as if no hooks and no props are used, then we can't figure out the line number.

Relying on the original error is the best way we can get the best issue grouping for our users.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explaining. Not sure exactly what's going on here but if you know what you are doing 👍 . Will test this on Sentry soon and see if it works as I expect it to.

});

it('handles when `error.cause` is recursive', () => {
const mockOnError = jest.fn();

function CustomBam(): JSX.Element {
const firstError = new Error('bam');
const secondError = new Error('bam2');
// @ts-ignore Need to set cause on error
firstError.cause = secondError;
// @ts-ignore Need to set cause on error
secondError.cause = firstError;
throw firstError;
}

render(
<TestApp fallback={<p>You have hit an error</p>} onError={mockOnError} errorComp={<CustomBam />}>
<h1>children</h1>
</TestApp>,
);

expect(mockOnError).toHaveBeenCalledTimes(0);
expect(mockCaptureException).toHaveBeenCalledTimes(0);

const btn = screen.getByTestId('errorBtn');
fireEvent.click(btn);

expect(mockCaptureException).toHaveBeenCalledTimes(1);
expect(mockCaptureException).toHaveBeenLastCalledWith(expect.any(Error), {
contexts: { react: { componentStack: expect.any(String) } },
});

expect(mockOnError.mock.calls[0][0]).toEqual(mockCaptureException.mock.calls[0][0]);

const error = mockCaptureException.mock.calls[0][0];
const cause = error.cause;
// We need to make sure that recursive error.cause does not cause infinite loop
expect(cause.stack).not.toEqual(mockCaptureException.mock.calls[0][1].contexts.react.componentStack);
expect(cause.name).not.toContain('React ErrorBoundary');
});

it('calls `beforeCapture()` when an error occurs', () => {
const mockBeforeCapture = jest.fn();

Expand Down