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

Symbolicate unhandled promise rejections #41377

Merged
merged 1 commit into from Nov 10, 2023

Conversation

fortmarek
Copy link
Contributor

Summary:

Cherry pick of 18c9797. Doing a PR due to merge conflicts.

Changelog:

[GENERAL] [FIXED] - Show correct stack frame on unhandled promise rejections on development mode.

Test Plan:

See test plan in #40914

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 8, 2023
@fortmarek fortmarek changed the base branch from main to 0.72-stable November 8, 2023 09:46
@fortmarek
Copy link
Contributor Author

@ospfranco would you have time to look at the failed pipeline, so we can backport this to 0.72?

@ospfranco
Copy link
Contributor

None of the errors on the CI look like they have to do anything with my code. Don't know how to fix those.

Summary:
For a very long time when a promise rejects without an attached catch we get this warning screen without a correct stack trace, only some internal calls to the RN internals.

<img src="https://github.com/facebook/react-native/assets/1634213/75aa7615-ee3e-4229-80d6-1744130de6e5" width="200" />

I created [an issue for discussion](react-native-community/discussions-and-proposals#718) in the react-native-community repo and we figured out it was only a matter of symbolication. While it cannot be done on release without external packages and source maps, at least while developing we can provide a symbolicated stack-trace so developers can better debug the source of rejected promise.

I got the stack trace symbolicated and the correct code frame. I'm missing some help trying to display it in the warning view but at the very least I can now correctly show the line of the error and log the codeframe to the console.

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[GENERAL] [FIXED] - Show correct stack frame on unhandled promise rejections on development mode.

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests

Pull Request resolved: #40914

Test Plan:
I simply created a throwing function on a dummy app, and checked the output of the console and the warning view:

```ts
import React from 'react';
import {SafeAreaView, Text} from 'react-native';

async function throwme() {
  throw new Error('UNHANDLED');
}

function App(): JSX.Element {
  throwme();

  return (
    <SafeAreaView>
      <Text>Throw test</Text>
    </SafeAreaView>
  );
}

export default App;
```

Here is the output

<img src="https://github.com/facebook/react-native/assets/1634213/2c100e4d-618e-4143-8d64-4095e8370f4f" width="200" />

Edit: I got the warning window working properly:

<img src="https://github.com/facebook/react-native/assets/1634213/f02a2568-da3e-4daa-8132-e05cbe591737" width="200" />

Reviewed By: yungsters

Differential Revision: D50324344

Pulled By: javache

fbshipit-source-id: 66850312d444cf1ae5333b493222ae0868d47056
@fortmarek fortmarek force-pushed the fortmarek/symbolicate-rejections branch from 611ca7f to 9d67fdb Compare November 8, 2023 10:17
@fortmarek
Copy link
Contributor Author

@ospfranco you are right, sorry for the ping. I redid the cherry pick and kept the flow type we have in 0.72-stable instead of the one that's on main.

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,492,329 -9,149,518
android hermes armeabi-v7a 7810350 n/a
android hermes x86 8962359 n/a
android hermes x86_64 8823752 n/a
android jsc arm64-v8a 9,151,474 -11,870,121
android jsc armeabi-v7a 8341166 n/a
android jsc x86 9204652 n/a
android jsc x86_64 9463476 n/a

Base commit: bb4e940
Branch: main

@fortmarek fortmarek merged commit 55c2c33 into 0.72-stable Nov 10, 2023
64 checks passed
@swrobel
Copy link
Contributor

swrobel commented Nov 14, 2023

This should be working as of 0.72.7, correct? I just upgraded from 0.72.6, and the error screen doesn't seem to have improved:
Simulator Screenshot - iPhone SE (3rd generation) - 2023-11-14 at 15 17 19

yayvery pushed a commit to discord/react-native that referenced this pull request Dec 5, 2023
…1377)

Summary:
For a very long time when a promise rejects without an attached catch we get this warning screen without a correct stack trace, only some internal calls to the RN internals.

<img src="https://github.com/facebook/react-native/assets/1634213/75aa7615-ee3e-4229-80d6-1744130de6e5" width="200" />

I created [an issue for discussion](react-native-community/discussions-and-proposals#718) in the react-native-community repo and we figured out it was only a matter of symbolication. While it cannot be done on release without external packages and source maps, at least while developing we can provide a symbolicated stack-trace so developers can better debug the source of rejected promise.

I got the stack trace symbolicated and the correct code frame. I'm missing some help trying to display it in the warning view but at the very least I can now correctly show the line of the error and log the codeframe to the console.

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[GENERAL] [FIXED] - Show correct stack frame on unhandled promise rejections on development mode.

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests

Pull Request resolved: facebook#40914

Test Plan:
I simply created a throwing function on a dummy app, and checked the output of the console and the warning view:

```ts
import React from 'react';
import {SafeAreaView, Text} from 'react-native';

async function throwme() {
  throw new Error('UNHANDLED');
}

function App(): JSX.Element {
  throwme();

  return (
    <SafeAreaView>
      <Text>Throw test</Text>
    </SafeAreaView>
  );
}

export default App;
```

Here is the output

<img src="https://github.com/facebook/react-native/assets/1634213/2c100e4d-618e-4143-8d64-4095e8370f4f" width="200" />

Edit: I got the warning window working properly:

<img src="https://github.com/facebook/react-native/assets/1634213/f02a2568-da3e-4daa-8132-e05cbe591737" width="200" />

Reviewed By: yungsters

Differential Revision: D50324344

Pulled By: javache

fbshipit-source-id: 66850312d444cf1ae5333b493222ae0868d47056

Co-authored-by: Oscar Franco <ospfranco@gmail.com>
@joe-sam
Copy link

joe-sam commented Dec 8, 2023

In fact the bug fix does not actually appear to have been cherry pick incorporated in 0.72.7, because if you look at the letter casing of the warning "Promise Unhandled Rejection" it clearly should have changed to lower case --> "promise unhandled rejection". Are you sure you are on 0.72.7 @swrobel ? I have raised this on the react working group discussion as a possible regression and hopefully the react cherry picking team will take a look at this odd malfunction in time for the next release.

This should be working as of 0.72.7, correct? I just upgraded from 0.72.6, and the error screen doesn't seem to have improved: Simulator Screenshot - iPhone SE (3rd generation) - 2023-11-14 at 15 17 19

@swrobel
Copy link
Contributor

swrobel commented Dec 8, 2023

Are you sure you are on 0.72.7

Yes, positive

@joe-sam
Copy link

joe-sam commented Dec 9, 2023

@swrobel I created a reproducer with the test plan by initiating a new react native project on version 0.72.7 and it worked fine, as you can see the warning is updated successfully (lowercased) and the promise rejection stack trace appears to be symbolicated successfully. Unsure why it was not working earlier for me ( I may not have updated RN to 0.72.7). I am removing my pick request.
image

Flewp pushed a commit to discord/react-native that referenced this pull request Mar 9, 2024
facebook#41377)

Summary:
For a very long time when a promise rejects without an attached catch we get this warning screen without a correct stack trace, only some internal calls to the RN internals.

<img src="https://github.com/facebook/react-native/assets/1634213/75aa7615-ee3e-4229-80d6-1744130de6e5" width="200" />

I created [an issue for discussion](react-native-community/discussions-and-proposals#718) in the react-native-community repo and we figured out it was only a matter of symbolication. While it cannot be done on release without external packages and source maps, at least while developing we can provide a symbolicated stack-trace so developers can better debug the source of rejected promise.

I got the stack trace symbolicated and the correct code frame. I'm missing some help trying to display it in the warning view but at the very least I can now correctly show the line of the error and log the codeframe to the console.

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[GENERAL] [FIXED] - Show correct stack frame on unhandled promise rejections on development mode.

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests

Pull Request resolved: facebook#40914

Test Plan:
I simply created a throwing function on a dummy app, and checked the output of the console and the warning view:

```ts
import React from 'react';
import {SafeAreaView, Text} from 'react-native';

async function throwme() {
  throw new Error('UNHANDLED');
}

function App(): JSX.Element {
  throwme();

  return (
    <SafeAreaView>
      <Text>Throw test</Text>
    </SafeAreaView>
  );
}

export default App;
```

Here is the output

<img src="https://github.com/facebook/react-native/assets/1634213/2c100e4d-618e-4143-8d64-4095e8370f4f" width="200" />

Edit: I got the warning window working properly:

<img src="https://github.com/facebook/react-native/assets/1634213/f02a2568-da3e-4daa-8132-e05cbe591737" width="200" />

Reviewed By: yungsters

Differential Revision: D50324344

Pulled By: javache

fbshipit-source-id: 66850312d444cf1ae5333b493222ae0868d47056

Co-authored-by: Oscar Franco <ospfranco@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Shopify Partner: Shopify Partner Pick Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants