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

[$250] Request Money - App crashed for specific account when requesting the money #10272

Closed
kbecciv opened this issue Aug 5, 2022 · 29 comments
Closed
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Weekly KSv2

Comments

@kbecciv
Copy link

kbecciv commented Aug 5, 2022

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


Action Performed:

  1. Launch the app
  2. Log in with applausetester+0901abb@applause.expensifail.com/Feya86Katya
  3. Select any chat
  4. Click on plus button - Request Money

Expected Result:

App is not crash for specific account when requesting the money

Actual Result:

App crashed for specific account when requesting the money

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.1.88.1

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers): applausetester+0901abb@applause.expensifail.com/Feya86Katya

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation
Screen Shot 2022-08-05 at 3 22 11 PM

Image.from.iOS.45.MP4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

@melvin-bot
Copy link

melvin-bot bot commented Aug 5, 2022

Triggered auto assignment to @AndrewGable (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@melvin-bot melvin-bot bot added the Overdue label Aug 8, 2022
@AndrewGable AndrewGable added the External Added to denote the issue can be worked on by a contributor label Aug 9, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 9, 2022

Triggered auto assignment to @mallenexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@AndrewGable AndrewGable added Weekly KSv2 and removed Daily KSv2 labels Aug 9, 2022
@melvin-bot melvin-bot bot removed the Overdue label Aug 9, 2022
@mallenexpensify
Copy link
Contributor

@melvin-bot
Copy link

melvin-bot bot commented Aug 9, 2022

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 9, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 9, 2022

Current assignee @AndrewGable is eligible for the Exported assigner, not assigning anyone new.

@melvin-bot melvin-bot bot changed the title Request Money - App crashed for specific account when requesting the money [$250] Request Money - App crashed for specific account when requesting the money Aug 9, 2022
@aimane-chnaif
Copy link
Contributor

Proposal

This issue happens when ONYXKEYS.IOU value removed from storage unexpectedly. (I think when out of storage because the reporter's account (applausetester+0901abb@applause.expensifail.com) has lots of data)
And app crashes on this line:

if (prevProps.iou.creatingIOUTransaction && !this.props.iou.creatingIOUTransaction && !this.props.iou.error) {

because of prevProps.iou = undefined
crash

Solution:
add null safe condition to avoid crash

        if (!prevProps.iou || !this.props.iou) {
            return;
        }

updated code:
IOU

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Aug 10, 2022
@rushatgabhane
Copy link
Member

rushatgabhane commented Aug 10, 2022

@aimane-chnaif if props.iou is undefined then IOUModal will be non functional right? It is also a required prop.

One could argue that crash is a good outcome. Because a refresh might fix temporarily fix the issue.

I think when out of storage because the reporter's account (applausetester+0901abb@applause.expensifail.com) has lots of data

We should attempt to make sure that props.iou is never undefined in the first place. What do you say?
cc: @AndrewGable

@aimane-chnaif
Copy link
Contributor

@rushatgabhane this is called in componentDidMount, which means props.iou is set immediately as {selectedCurrencyCode: xxx} after IOUModal component is rendered first time.

IOU.setIOUSelectedCurrency(this.props.currentUserPersonalDetails.localCurrencyCode);

So IOUModal is still functional after setting value.
The crash happens only because componentDidUpdate is called after updating this value, with these props: (prevProps.iou: undefined, this.props.iou: {selectedCurrencyCode: xxx})

These calls are not needed on first render of IOUModal so it's safe to return if !prevProps.iou || !this.props.iou
componentDidUpdate

@aimane-chnaif
Copy link
Contributor

aimane-chnaif commented Aug 11, 2022

@rushatgabhane @AndrewGable I just noticed this crash already fixed by @marcaaron in internal team
issue: #10275
pr: #10340

@marcaaron I think iou value is removed from storage and becomes undefined when out of storage. Only happening to the account with tons of data (workspaces, chats, etc)

@marcaaron
Copy link
Contributor

@marcaaron I think iou value is removed from storage and becomes undefined when out of storage. Only happening to the account with tons of data (workspaces, chats, etc)

Ah hmm why would it be removed from storage? My first thought was that it was not removed, but just hasn't loaded yet. Did we observe somewhere in the code where it's possible for that to happen or just a guess?

@aimane-chnaif
Copy link
Contributor

@marcaaron I think iou value is removed from storage and becomes undefined when out of storage. Only happening to the account with tons of data (workspaces, chats, etc)

Ah hmm why would it be removed from storage? My first thought was that it was not removed, but just hasn't loaded yet. Did we observe somewhere in the code where it's possible for that to happen or just a guess?

Theoretically, iou cannot be undefined. It's always set https://github.com/Expensify/App/blob/main/src/setup/index.js#L33 when app starts. Other than that, no code of setting or removing this value. Always do merge.
please check this video:
https://drive.google.com/file/d/1wtm19M_Lp-VDN66aoxOhe3mcrvo3sarH/view?usp=sharing
You will see that iou set to undefined unexpectedly
Also, app is too slow to load

@rushatgabhane
Copy link
Member

Okay, I don't have anything to add on this.

@marcaaron
Copy link
Contributor

Theoretically, iou cannot be undefined. It's always set https://github.com/Expensify/App/blob/main/src/setup/index.js#L33 when app starts.

Maybe there is some bug related to how that initial data is getting set or possibly a race condition related to the sign outs. Have we tried debugging?

I don't quite understand the conclusion that the data is somehow related to running out of storage space. My suggestion would be to find some evidence that this happening and otherwise stay open to the possibility that the issue could be related to something else.

@marcaaron
Copy link
Contributor

I think we can close this issue though since it's a duplicate and should be fixed on my other branch. Feel free to reopen if we disagree.

@melvin-bot
Copy link

melvin-bot bot commented Sep 27, 2022

@AndrewGable, @mallenexpensify, @rushatgabhane Huh... This is 4 days overdue. Who can take care of this?

@mallenexpensify
Copy link
Contributor

@rushatgabhane and @marcaaron , can you help here plz? The issue is reproducible so I'm hesitant to close

I think we can close this issue though since it's a duplicate and should be fixed on my other branch

What's link to other branch? Is it on staging yet?

@melvin-bot melvin-bot bot removed the Overdue label Sep 27, 2022
@marcaaron
Copy link
Contributor

@marcaaron what do you recommend we do with this issue since the bug is reproducible?

I would recommend that someone log into these tester accounts provided while debugging.

What's link to other branch? Is it on staging yet?

This one -> #10340

I think it should be on production by now... not sure how to figure that out for sure.

@melvin-bot melvin-bot bot added the Overdue label Sep 29, 2022
@mallenexpensify mallenexpensify removed their assignment Sep 30, 2022
@melvin-bot melvin-bot bot removed the Overdue label Sep 30, 2022
@mallenexpensify mallenexpensify added Overdue External Added to denote the issue can be worked on by a contributor and removed External Added to denote the issue can be worked on by a contributor labels Sep 30, 2022
@melvin-bot
Copy link

melvin-bot bot commented Sep 30, 2022

Triggered auto assignment to @adelekennedy (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@melvin-bot
Copy link

melvin-bot bot commented Sep 30, 2022

Current assignee @rushatgabhane is eligible for the External assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Sep 30, 2022

Current assignee @AndrewGable is eligible for the External assigner, not assigning anyone new.

@mallenexpensify mallenexpensify added Weekly KSv2 and removed Daily KSv2 labels Sep 30, 2022
@melvin-bot melvin-bot bot removed the Overdue label Sep 30, 2022
@mallenexpensify
Copy link
Contributor

@adelekennedy auto-assigned to you, I'm going to be OOO for a month. Thx

@mallenexpensify
Copy link
Contributor

@adelekennedy assigned myself and unassigned you cuz I'm NOT going to be OOO the next month ¯_(ツ)_/¯

@melvin-bot melvin-bot bot added the Overdue label Oct 12, 2022
@mallenexpensify
Copy link
Contributor

Just tried to reprod with test credentials in web/Firefox and was unable to. Closing, comment/reopen if disagree

@mvtglobally
Copy link

@mallenexpensify @AndrewGable Looks like @adeel0202 is still able to repro this issue. We are not able to reproduce. Asked the team to double check as well.
Should we reopen this issue?
https://expensify.slack.com/archives/C01GTK53T8Q/p1662020087722169

@mallenexpensify
Copy link
Contributor

@adeel0202 is there anything different that you're doing to reproduce, vs using the steps in the OP?

@adeel0202
Copy link
Contributor

@mallenexpensify I believe the bug I reported is different from this one, considering the fact that this bug occurs only for some specific accounts while the one I reported occurs for all the accounts. The other difference between the two bugs is that the one I reported occurs when you request money quickly i.e. after entering the amount, pressing the Next button quickly two times so that you press some account on the next screen at the same time.

@adeel0202
Copy link
Contributor

adeel0202 commented Oct 17, 2022

Update: Just check again today and I'm no longer able to reproduce the bug I reported. I was able to reproduce when this ticket was closed and I commented on slack. But now looks like it got fixed somehow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Weekly KSv2
Projects
None yet
Development

No branches or pull requests

10 participants