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.

Sign up for GitHub

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

Jump to bottom

[$250] iOS - Expense - App crashes when opening Description and saving it a few times #44437

Closed
1 of 6 tasks
izarutskaya opened this issue Jun 26, 2024 · 45 comments
Closed
1 of 6 tasks

[$250] iOS - Expense - App crashes when opening Description and saving it a few times #44437

izarutskaya opened this issue Jun 26, 2024 · 45 comments
Assignees
@MonilBhavsar @lschurr @brunovjk
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@izarutskaya
Copy link

izarutskaya commented Jun 26, 2024 •
edited by lschurr
Loading

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: 9.0.2-0
Reproducible in staging?: Y
Reproducible in production?: N
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

  1. Launch New Expensify app.
  2. Go to FAB > Submit expense > Manual.
  3. Submit a manual expense to any user.
  4. Go to transaction thread.
  5. Tap Description.
  6. Tap Save.
  7. Repeat Step 5 and 6 a few times.

Expected Result:

App will not crash.

Actual Result:

App crashes when opening Description and saving it a few times.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6524815_1719379123236.RPReplay_Final1719378986.mp4

Bug6524815_1719395555645!New_Expensify-2024-06-26-082221.txt

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0183701f9c89311311
  • Upwork Job ID: 1805978752685884090
  • Last Price Increase: 2024-06-26
  • Automatic offers:
    • brunovjk | Reviewer | 102888406
@izarutskaya izarutskaya added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. DeployBlocker Indicates it should block deploying the API labels Jun 26, 2024
@melvin-bot Melvin Bot
Copy link

melvin-bot bot commented Jun 26, 2024

Triggered auto assignment to @lschurr (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@melvin-bot Melvin Bot
Copy link

melvin-bot bot commented Jun 26, 2024

Triggered auto assignment to @MonilBhavsar (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Jun 26, 2024
@github-actions GitHub Actions
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@github-actions github-actions bot mentioned this issue Jun 26, 2024
Closed
92 tasks
@izarutskaya
Copy link
Author

@lschurr I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

@izarutskaya
Copy link
Author

We think this issue might be related to the #collect project.

@MonilBhavsar
Copy link
Contributor

able to reproduce

@MonilBhavsar
Copy link
Contributor

I'm able to reproduce on physical device and not on simulator. Client side logs didn't give much hints. Going to make it external

@MonilBhavsar MonilBhavsar added the External Added to denote the issue can be worked on by a contributor label Jun 26, 2024
@melvin-bot melvin-bot bot changed the title iOS - Expense - App crashes when opening Description and saving it a few times [$250] iOS - Expense - App crashes when opening Description and saving it a few times Jun 26, 2024
@melvin-bot Melvin Bot
Copy link

melvin-bot bot commented Jun 26, 2024

Job added to Upwork: https://www.upwork.com/jobs/~0183701f9c89311311

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 26, 2024
@melvin-bot Melvin Bot
Copy link

melvin-bot bot commented Jun 26, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @brunovjk (External)

@MonilBhavsar MonilBhavsar removed the DeployBlocker Indicates it should block deploying the API label Jun 26, 2024
@MonilBhavsar
Copy link
Contributor

Some more insights from crashalytics

This is indeed a deploy blocker as it started happening yesterday, and only happens on iOS

Screenshot 2024-06-26 at 8 50 44 PM

Stack trace of the crash

Screenshot 2024-06-26 at 8 51 56 PM

The error seems to be coming from this line in react native animated library https://github.com/software-mansion/react-native-reanimated/blame/main/packages/react-native-reanimated/Common/cpp/Fabric/ReanimatedCommitMarker.cpp#L12
but I'm unable to connect the dots between that and recent deploys

@MonilBhavsar MonilBhavsar mentioned this issue Jun 26, 2024
Merged
50 tasks
@MonilBhavsar
Copy link
Contributor

I commented on the PR that seemed little sus to me #43749

@melvin-bot melvin-bot bot mentioned this issue Jun 26, 2024
Closed
@rezkiy37
Copy link
Contributor

Hi, I’m Michael (Mykhailo) from Callstack and I would like to work on this issue.

@lschurr
Copy link
Contributor

lschurr commented Jul 18, 2024

How are we doing on this one @tomekzaw - are we holding on something?

@melvin-bot melvin-bot bot removed the Overdue label Jul 18, 2024
@tomekzaw
Copy link
Contributor

@bartlomiejbloniarz is actively investigating this in Reanimated.

@melvin-bot Melvin Bot
Copy link

melvin-bot bot commented Jul 24, 2024

@MonilBhavsar @lschurr @brunovjk this issue is now 4 weeks old, please consider:

  • Finding a contributor to fix the bug
  • Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

@tomekzaw
Copy link
Contributor

@bartlomiejbloniarz is still working on this.

@bartlomiejbloniarz
Copy link

I was able to reproduce the issue in the 9.0.2 version of the App. The reason for this issue is that when a TextInput is removed during a ShadowTree commit, a Keyboard event is triggered. react-native-keyboard-controller listens to events using the reanimated useEvent hook, which synchronously (while still being inside the original commit) triggers another commit. It can happen that the first commit was also triggered by reanimated, which would break on of our assumptions -> that's why the assert was failing.

I spent some time trying to figure out if nested Shadow Tree commits should be allowed, but according to this commit, it seems that they are fine.

Hopefully we will have a fix for this issue merged soon, I have prepared a PR to reanimated that should resolve it. From my tests it seems that newer versions of the Expensify App don't have this issue. I tried to reproduce it many times on main, but to no avail. If someone has reproduced the issue on newer version it would be good to know.

@melvin-bot Melvin Bot
Copy link

melvin-bot bot commented Jul 25, 2024

📣 @bartlomiejbloniarz! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@melvin-bot melvin-bot bot added the Overdue label Jul 29, 2024
@bartlomiejbloniarz bartlomiejbloniarz mentioned this issue Jul 29, 2024
Merged
@lschurr
Copy link
Contributor

lschurr commented Jul 29, 2024

Looks like there is a draft PR up - any other update for now @bartlomiejbloniarz?

@melvin-bot melvin-bot bot removed the Overdue label Jul 29, 2024
github-merge-queue bot pushed a commit to software-mansion/react-native-reanimated that referenced this issue Jul 30, 2024
@bartlomiejbloniarz
Modify reanimated props updates (#6330)
071e513 
## Summary

This PR changes the way `performOperations` and `ReanimatedCommitHook`
are synchronized. The current implementations faces 2 problems:

1. Updates that come through `performOperations` after
`pleaseSkipReanimatedCommit` is called in the commit hook will be
deferred until the next commit. Usually it's fine since the next commit
will be triggered by the next animation frame. But if there was a
singular update scheduled through Reanimated, we might not see the
change for a long time. This issue is more thoroughly explained in [this
issue](#6245).

2. In `ReanimatedCommitMarker` there is an assumption that there can be
at most one commit happening on a given thread (i.e. there can't be
nested commits). This isn't true, since there can be an event listener
that commits some changes in a reaction to a native mount/unmount (which
is a part of the commit function). [This exact scenario was observed in
the New Expensify App with
`react-native-keyboard-controller`](Expensify/App#44437 (comment)).
At first I thought this is a mistake, but [this PR in
RN](facebook/react-native@5f0435f)
seems to allow for scenarios like that.

Applied fixes:

ad 1. Now instead of resetting the skip flag in reanimated after a
transaction is mounted (via MountHook), we reset the flag whenever a
non-empty batch is read in `performOperations`. This ensures that we
don't make an unnecessary commit, but never skip any updates.

ad 2. Now we mark reanimated commits through
`ReanimatedCommitShadowNode`. This is a class derived from ShadowNode
that allows us to modify the root nodes traits_ (and add our custom
trait). This ensures that even if the root node gets cloned it will
retain the information. We couldn't derive from `RootShadowNode` since
it is a final class.

## Test plan
@bartlomiejbloniarz
Copy link

I think that's all. The PR was merged. I tested the New Expensify App with these changes and everything worked fine.

@MonilBhavsar
Copy link
Contributor

MonilBhavsar commented Jul 31, 2024 •
edited
Loading

Thank you for the help! 🙇
I think we can revert the patch now. ah nvm, we also need to bump reanimated version along with it

@MonilBhavsar MonilBhavsar mentioned this issue Jul 31, 2024
Closed
50 tasks
@MonilBhavsar MonilBhavsar added the Reviewing Has a PR in review label Jul 31, 2024
@melvin-bot melvin-bot bot removed the Weekly KSv2 label Aug 23, 2024
@melvin-bot Melvin Bot
Copy link

melvin-bot bot commented Aug 23, 2024

This issue has not been updated in over 15 days. @MonilBhavsar, @lschurr, @brunovjk eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@melvin-bot melvin-bot bot added the Monthly KSv2 label Aug 23, 2024
@MonilBhavsar MonilBhavsar added Weekly KSv2 and removed Reviewing Has a PR in review Monthly KSv2 labels Aug 23, 2024
@tomekzaw
Copy link
Contributor

tomekzaw commented Sep 2, 2024 •
edited
Loading

@bartlomiejbloniarz's PR software-mansion/react-native-reanimated#6330 was merged and released in 3.15.0.

E/App now runs on Reanimated 3.15.1: https://github.com/Expensify/App/blob/main/package.json#L172

We can safely close this issue now.

Thanks everyone for your help!

@melvin-bot melvin-bot bot added the Overdue label Sep 2, 2024
@brunovjk
Copy link
Contributor

brunovjk commented Sep 2, 2024

Thank you very much @tomekzaw.
@izarutskaya, @MonilBhavsar can you confirm the comment above? Thanks

@melvin-bot melvin-bot bot removed the Overdue label Sep 2, 2024
@MonilBhavsar
Copy link
Contributor

Sounds good. Let's close this. Thanks all for the help! 🚀

@github-project-automation github-project-automation bot moved this from Polish to Done in [#whatsnext] #wave-collect Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@MonilBhavsar MonilBhavsar

@lschurr lschurr

@brunovjk brunovjk

Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
No open projects
[#whatsnext] #wave-collect
Status: Done
Milestone
No milestone
Development

No branches or pull requests
9 participants
@tomekzaw @kirillzyusko @MonilBhavsar @lschurr @mountiny @bartlomiejbloniarz @rezkiy37 @brunovjk @izarutskaya