Fix parseLogBoxLog test that test hermes component stacks#43281
Closed
rickhanlonii wants to merge 2 commits into
Closed
Fix parseLogBoxLog test that test hermes component stacks#43281rickhanlonii wants to merge 2 commits into
rickhanlonii wants to merge 2 commits into
Conversation
facebook-github-bot
added
the
CLA Signed
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D54423252 |
…cebook#43165) Summary: Fixes component stacks without source info, and duplication in the error message string. ## Before No component stacks, and error message duplication: {F1459400803} ## After Back to normal, albeit without the component source location: {F1459401148} Changelog: [General][Fixed] - Support component stacks without source info. Reviewed By: yungsters Differential Revision: D53981672
Summary: ## Overview This diff fixes a bug in the hermes component stack location parser, and fixes the hermes component stack tests which were not using hermes stack parsing, which is why the bug wasn't caught. The bug fix is that React component stacks may not all have stack frame locations. For example, this stack: ``` at MyComponent (/path/to/filename.js:1:2) at MyOtherComponent <-- no location at MyAppComponent (/path/to/app.js:100:20) ``` This can happen when we're unable to make a component throw (e.g. it doesn't use a hook or access props). We have plans to fix these frames, but currently they can exist. The bug was when `parseHermesStack` finds a frame without an `entry`, it would reset the `entries`. But if entries is already non-null, or if the current frame is a frame without a source, we should continue. ### Caveats The handling here fixes the behavior to go back to skipping these frames. I'm not sure what the best way to handle these cases are, since these frames do not have source location and should skip symbolication. We should follow up with handling for these frames. ## Why it wasn't caught In D18627930 we changed the hermes component stack parsing to check `global.HermesInternal`, but the tests for the hermes component stacks were still using the `stacktrace-parser`. I updated the tests to set/reset the global, which caught the bug. Changelog: [General][Fixed] - Support hermes component stacks with missing source info. Reviewed By: yungsters Differential Revision: D54423252
rickhanlonii
force-pushed
the
export-D54423252
branch
from
23d723f to
bee2b78
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D54423252 |
Contributor
|
This pull request has been merged in 82db330. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
Overview
This diff fixes a bug in the hermes component stack location parser, and fixes the hermes component stack tests which were not using hermes stack parsing, which is why the bug wasn't caught.
The bug fix is that React component stacks may not all have stack frame locations. For example, this stack:
This can happen when we're unable to make a component throw (e.g. it doesn't use a hook or access props). We have plans to fix these frames, but currently they can exist.
The bug was when
parseHermesStackfinds a frame without anentry, it would reset theentries. But if entries is already non-null, or if the current frame is a frame without a source, we should continue.Caveats
The handling here fixes the behavior to go back to skipping these frames. I'm not sure what the best way to handle these cases are, since these frames do not have source location and should skip symbolication. We should follow up with handling for these frames.
Why it wasn't caught
In D18627930 we changed the hermes component stack parsing to check
global.HermesInternal, but the tests for the hermes component stacks were still using thestacktrace-parser. I updated the tests to set/reset the global, which caught the bug.Changelog:
[General][Fixed] - Support hermes component stacks with missing source info.
Differential Revision: D54423252