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

[Live Markdown Issues] Android - Chat - App isn't responding on pasting email repeatedly in compose box #36354

Closed
1 of 6 tasks
lanitochka17 opened this issue Feb 12, 2024 · 49 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff

Comments

@lanitochka17
Copy link

lanitochka17 commented Feb 12, 2024

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: 1.4.90
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

  1. Launch app
  2. Tap on 1: 1 report of expensifail user eg: applausetester++0411km@applause.expensifail.com
  3. Tap header
  4. Copy email
  5. Navigate to 1:1 page
  6. Repeatedly paste email in compose

Expected Result:

From header copy email and paste in compose repeatedly, app isn't responding message must note be displayed

Actual Result:

From header copy email and paste in compose repeatedly, app isn't responding message is displayed

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

Add any screenshot/video evidence

Bug6376901_1707757873412.not.mp4

logs (2).txt

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01c622c8bf192a870b
  • Upwork Job ID: 1757099852323344384
  • Last Price Increase: 2024-02-12
  • Automatic offers:
    • aimane-chnaif | Reviewer | 0
@lanitochka17 lanitochka17 added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Feb 12, 2024
@melvin-bot melvin-bot bot changed the title Android - Chat - App isn't responding on pasting email repeatedly in compose box [$500] Android - Chat - App isn't responding on pasting email repeatedly in compose box Feb 12, 2024
Copy link

melvin-bot bot commented Feb 12, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01c622c8bf192a870b

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

melvin-bot bot commented Feb 12, 2024

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

Copy link

melvin-bot bot commented Feb 12, 2024

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

@lanitochka17
Copy link
Author

We think that this bug might be related to #vip-vsp
CC @quinthar

@nrajyaguru
Copy link

nrajyaguru commented Feb 12, 2024

When user paste the text
There is change method getting invoked further which has series of actions internally and finally lead to no class found exception. Let me know if this is okay to go ahead
CC: @mallenexpensify @lanitochka17

Copy link

melvin-bot bot commented Feb 12, 2024

📣 @nrajyaguru! 📣
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>

@nayabatir1
Copy link

Hey @lanitochka17,
Is this issue specifically related to expensifail user?
If so how to create one? I've tested locally with normal user and I'm unable to reproduce.

Copy link

melvin-bot bot commented Feb 12, 2024

📣 @nayabatir1! 📣
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>

@mallenexpensify
Copy link
Contributor

Unable to reproduce on iOS, @aimane-chnaif can you test on android? Thx

@aimane-chnaif
Copy link
Contributor

reproduced on android

On 5th repeat, it took 10s to be pasted into composer. Seems related to live markdown

@aimane-chnaif
Copy link
Contributor

After copy pasting email several times, composer is super slow when try to edit message and almost unusable.
Huge performance lag

@mallenexpensify
Copy link
Contributor

Checking on in the markdown room https://expensify.slack.com/archives/C06BDSWLDPB/p1707866403183029
Thanks Aimane.

@tomekzaw
Copy link
Contributor

tomekzaw commented Feb 15, 2024

Thanks for reporting this issue, I will investigate it, please assign me.

@thienlnam Could we also add this issue in #36071?

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 15, 2024
Copy link

melvin-bot bot commented Feb 15, 2024

📣 @aimane-chnaif 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

@melvin-bot melvin-bot bot added the Overdue label Feb 19, 2024
Copy link

melvin-bot bot commented Feb 19, 2024

@tomekzaw, @mallenexpensify, @thienlnam, @aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick!

@tomekzaw
Copy link
Contributor

Working on it

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Feb 19, 2024
@melvin-bot melvin-bot bot removed the Overdue label Mar 4, 2024
@SzymczakJ
Copy link
Contributor

Still working at it. Current status: trying to profile Hermes runtime to find out which part of ExpensiMark parser is working too slow. With that knowledge maybe we will be able to make it faster.

@SzymczakJ
Copy link
Contributor

Current update: investigated performance of ExpensiMark on Hermes. The input for profiling ExpensiMark was "applausetester++0411km@applause.expensifail.comapplausetester++0411km@applause.expensifail.comapplausetester++0411km@applause.expensifail.comapplausetester++0411km@applause.expensifail.com" which is basically and email from this issue reproduction multiplied 4 times.
Screenshot 2024-03-07 at 10 02 34
As we can see execution of regExpPrototypeExec takes more than 2 seconds, so we can be sure that the RegExp implementation for Hermes is the problem here. Digging deeper into Hermes internals to further investigate the problem.

@mallenexpensify
Copy link
Contributor

@SzymczakJ is the bug reproducible with other text or data being pasted repeatedly in the compose box? I want to add this to a VIP/wave project and I'm thinking #vip-vsb. Wanting to know if small business users will be affected by this or if it's an edge case that only happens with pasted emails.

@SzymczakJ
Copy link
Contributor

@mallenexpensify I didn't manage to reproduce it with anything other than emails.
I've also measured time of Expensimark parsing which is a bottleneck in this issue, these are result for pretty small inputs:
something.someone@somedomain.com - 4ms
something.someone@somedomain.comsomething.someone@somedomain.com - 5595ms!
testEmail@testDomain.com - 1ms
testEmail@testDomain.comtestEmail@testDomain.com - 7ms
testEmail.testEmail@testDomain.com - 4ms
testEmail.testEmail@testDomain.comtestEmail.testEmail@testDomain.com -5405ms!
Keep in mind that we want to keep parse time at the maximum of 3 ms for the experience to be smooth, so even parsing single mail like something.someone@somedomain.com takes too long.

@mallenexpensify
Copy link
Contributor

Thanks @SzymczakJ , appreciate the context, I had no idea that it took longer to parse emails with more characters.

@thienlnam , lil help here. with this only affecting users who paste a bunch of emails in at once, do you think it's worth fixing right now or should we close then maybe revisit later?

One reason TO potentially fix now would be if email parsing updates also benefited or sped up other places in the app. (I have no clue)

@SzymczakJ
Copy link
Contributor

SzymczakJ commented Mar 11, 2024

@mallenexpensify after spending some time on this issue these are my thoughts:

  • it's not worth to spend so much time on such edge case that doesn't happen when using app without trying to purposefully break it
  • even if we fix this edge case there will be other 'attack strings' that also cause problems for our regexes and more such problems may arise

Maybe we should think of some more general and long-term solution for this "live-markdown working slow with some inputs" issue. Do you have other ideas?
cc @roryabraham @thienlnam

Copy link

melvin-bot bot commented Mar 11, 2024

@tomekzaw @mallenexpensify @thienlnam @SzymczakJ @aimane-chnaif this issue is now 4 weeks old and preventing us from maintaining WAQ. This should now be your highest priority. Please post below what your plan is to get a PR in review ASAP. Thanks!

@melvin-bot melvin-bot bot added the Internal Requires API changes or must be handled by Expensify staff label Mar 11, 2024
Copy link

melvin-bot bot commented Mar 11, 2024

Current assignee @aimane-chnaif is eligible for the Internal assigner, not assigning anyone new.

@thienlnam
Copy link
Contributor

Yeah I don't think this is an urgent bug we need to fix - it's definitely a malicious edge case so we can add it to the list but can prioritize other issues before this

@thienlnam thienlnam added Monthly KSv2 and removed Daily KSv2 labels Mar 11, 2024
@thienlnam
Copy link
Contributor

I added this to the main tracking issue #36071, and adjusted priority

@mallenexpensify mallenexpensify changed the title [$500] Android - Chat - App isn't responding on pasting email repeatedly in compose box [HOLD #36071 Live Markdown Issues][$500] Android - Chat - App isn't responding on pasting email repeatedly in compose box Mar 20, 2024
@mallenexpensify
Copy link
Contributor

Added to #vip-vsb because the bug doesn't have to do with money. I wonder, since there's also the tracking issue, if there's another/better way to manage this ¯_(ツ)_/¯

@melvin-bot melvin-bot bot added the Overdue label Apr 29, 2024
@mallenexpensify
Copy link
Contributor

It looks like Jack and gang are going through issues, from high > low and this one is medium.

@melvin-bot melvin-bot bot removed the Overdue label May 1, 2024
@melvin-bot melvin-bot bot added the Overdue label Jun 3, 2024
@mallenexpensify mallenexpensify changed the title [HOLD #36071 Live Markdown Issues][$500] Android - Chat - App isn't responding on pasting email repeatedly in compose box [Live Markdown Issues][$500] Android - Chat - App isn't responding on pasting email repeatedly in compose box Jun 5, 2024
@mallenexpensify
Copy link
Contributor

#36071 was closed

Closing this! We've worked through all the critical issues, I think every issue from here on out can just be addressed independently. We're still working through #39518 but there's no longer a need for this main tracking issue

So.. now... do we want to do anything here? Fix it? add retest-weekly label? Start a discussion in the #react-native-live-markdown?

From @SzymczakJ above

after spending some time on this issue these are my thoughts:

  • it's not worth to spend so much time on such edge case that doesn't happen when using app without trying to purposefully break it
  • even if we fix this edge case there will be other 'attack strings' that also cause problems for our regexes and more such problems may arise

Maybe we should think of some more general and long-term solution for this "live-markdown working slow with some inputs" issue. Do you have other ideas?

@melvin-bot melvin-bot bot removed the Overdue label Jun 5, 2024
@mallenexpensify mallenexpensify added Daily KSv2 and removed Monthly KSv2 labels Jun 5, 2024
@mallenexpensify mallenexpensify changed the title [Live Markdown Issues][$500] Android - Chat - App isn't responding on pasting email repeatedly in compose box [Live Markdown Issues] Android - Chat - App isn't responding on pasting email repeatedly in compose box Jun 5, 2024
@thienlnam
Copy link
Contributor

I think we can close for now and re-open if this comes up again. Realistically this doesn't seem like a common use-case and the time tradeoff does not seem worth it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff
Projects
Archived in project
Development

No branches or pull requests

9 participants