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

Many libraries can cause "Error.stack getter called with an invalid receiver" #1496

Closed
1 task done
thomasttvo opened this issue Aug 27, 2024 · 16 comments
Closed
1 task done
Labels
enhancement New feature or request

Comments

@thomasttvo
Copy link

thomasttvo commented Aug 27, 2024

Bug Description

When an error is thrown while rendering, RN crashes of course as expected. However, for some errors, when ExceptionsManager calls error.stack, it throws new errors, masking the original errors, which makes it impossible to know what the original error was for users in production (using tools like sentry).

This happens when libraries using a technique like this to create their error classes:

    const MyError = function(message) {
      this.message = message;
    };
    MyError.prototype = new Error;
    MyError.prototype.name = 'MyError';

I can find at least 5 popular libraries that use this technique, and if it works in classic JS, I think it's supposed to work with hermes?

  • The issue is reproducible with the latest version of React Native.

React Native version: 0.73.5
OS: iOS, Android

Steps To Reproduce

 useEffect(() => {
    const MyError = function(message) {
      this.message = message;
    };
    MyError.prototype = new Error;
    MyError.prototype.name = 'MyError';

    throw new MyError('1234')
  }, []);

The Expected Behavior

Calling new MyError().stack doesn't throw Error.stack getter called with an invalid receiver

@thomasttvo thomasttvo added the bug Something isn't working label Aug 27, 2024
@tmikov
Copy link
Contributor

tmikov commented Aug 27, 2024

@thomasttvo hi, Error.stack is not part of the ECMAScript spec, so any behavior is valid. However, the example you have shared doesn't seem to work in any engine. I just tried the following:

const MyError = function(message) {
      this.message = message;
    };
    MyError.prototype = new Error;
    MyError.prototype.name = 'MyError';


function foo() {
    throw new MyError('1234')
}
function bar() {
    foo();
}


try {
    bar();
} catch (e) {
    print(e.stack);
}

If you run it with v8 or jsc, you get a useless stack trace:

v8 test.js
MyError
    at test.js:4:25

So, why is this a useful?

@tmikov tmikov added need more info Awating additional info before proceeding and removed bug Something isn't working labels Aug 27, 2024
@thomasttvo
Copy link
Author

thomasttvo commented Aug 27, 2024

@tmikov Even though the error doesn't report the stack trace that points to where it is actually thrown, it's useful enough to find out which library it comes from. We have no control over what libraries do with their errors, some throw plain objects, some throw strings. In any of those cases, some info is good enough (When you run with v8, the stack actually points to the library where the error was initially defined), whereas the stack trace and data of Error.stack getter called with an invalid receiver provides no useful info. We don't even have the message of the original error.

image

@tmikov
Copy link
Contributor

tmikov commented Aug 27, 2024

@thomasttvo Do you have an explanation of what the point is of putting an Error object in the prototype? What does it accomplish? More importantly, is there a formal specification anywhere of how this should work?

@thomasttvo
Copy link
Author

thomasttvo commented Aug 27, 2024

@tmikov I believe these libraries originated from a time pre-ES6 when class inheritance wasn't a feature. That was how you a bunch of libraries do class inheritance back then.

Here's a non-error pre-es6 inheritance snippet from Bluebird
image

Here's a custom error in handlebars.js
image

base-64
image

What's worse, sometimes we may not even know we're using these libraries because they're dependencies of dependencies of dependencies

@tmikov
Copy link
Contributor

tmikov commented Aug 27, 2024

@thomasttvo this is most definitely not how inheritance was implemented pre-ES6. At the very least it should call the parent constructor.

@tmikov
Copy link
Contributor

tmikov commented Aug 27, 2024

We understand this is a real problem users of Hermes are having and we want to help. But we need to understand what the solution is.

@thomasttvo
Copy link
Author

thomasttvo commented Aug 27, 2024

@tmikov you're right, that's not how you're supposed to do inheritance, but many extremely popular libraries use that method, and it didn't throw any error. I think the fix is to display the stack trace of the original error in the prototype and avoid throwing further errors when you perform operations on Error. If that's impossible, then just return the string "<Error.stack getter called with an invalid receiver>" for error.stack would be great. The goal I'm trying to get to is to not mask the original error with another error.

@tmikov
Copy link
Contributor

tmikov commented Aug 27, 2024

@thomasttvo yes, I think I am beginning to get the picture. One way or another we will make sure the stack getter doesn't throw.

@tmikov tmikov added enhancement New feature or request and removed need more info Awating additional info before proceeding labels Aug 27, 2024
@thomasttvo
Copy link
Author

thomasttvo commented Aug 27, 2024

thank you! with that said, if the stack getter returns at test.js:4:25 like above, it would still be really really helpful 🙏 although I know this might be hard to achieve.

tmikov pushed a commit to tmikov/hermes that referenced this issue Aug 27, 2024
Summary:
Look for stack data in the entire prototype chain in order to accommodate
usage like the one in the test.

See facebook#1496

Differential Revision: D61870728
@tmikov
Copy link
Contributor

tmikov commented Aug 27, 2024

This is the fix, landing in Static Hermes. The getter returns the stack from the prototype.
#1497

tmikov pushed a commit to tmikov/hermes that referenced this issue Aug 28, 2024
Summary:
Pull Request resolved: facebook#1497

Look for stack data in the entire prototype chain in order to accommodate
usage like the one in the test.

See facebook#1496

Reviewed By: avp

Differential Revision: D61870728
facebook-github-bot pushed a commit that referenced this issue Aug 28, 2024
Summary:
Pull Request resolved: #1497

Look for stack data in the entire prototype chain in order to accommodate
usage like the one in the test.

See #1496

Reviewed By: avp

Differential Revision: D61870728

fbshipit-source-id: 9a1008cddcd8a87f96363922691460aa7f85224a
@wobsoriano
Copy link

@tmikov looks like the fix was cancelled?

@tmikov
Copy link
Contributor

tmikov commented Sep 6, 2024

@tmikov looks like the fix was cancelled?

It wasn't cancelled. As can be seen clearly above, it was committed in #1497

@wobsoriano
Copy link

I see, so it was just labeled "Closed" but it's merged?

@tmikov
Copy link
Contributor

tmikov commented Sep 8, 2024

@wobsoriano yeah, I am not sure why it doesn't say "Merged", when it clearly was:

Screenshot 2024-09-08 at 9 03 03 AM

@wobsoriano
Copy link

wobsoriano commented Sep 8, 2024

Thanks @tmikov! and this is not yet available in the latest RN release right?

facebook-github-bot pushed a commit that referenced this issue Sep 8, 2024
Summary:
Imported from static_h
Original Author: [email protected]
Original Git: 8b7a9f8
Original Reviewed By: avp
Original Revision: D61870728

Look for stack data in the entire prototype chain in order to accommodate
usage like the one in the test.

See #1496

Reviewed By: fbmal7

Differential Revision: D62357838

fbshipit-source-id: 3873bfd8bcb5c16998dbec67f44ad4c098179758
@tmikov
Copy link
Contributor

tmikov commented Sep 9, 2024

@wobsoriano it will be in RN 0.76

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants