[HOLD for payment 2023-08-10] [$1000] Android - User is able to click on download button on share code page after modal shows up if user is clicks rapidly and app crashes

[kbecciv]

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. Open ND on android
2. Go to settings > share code
3. Click on the download button rapidly multiple times
4. Do step 3 until app crashes

Expected Result:

User should not be able to click on the download button more than once then the confirmation modal should show up. App should not crash if user clicks on download button multiple of times

Actual Result:

User is able to click on the download button more than once even after the confirmation modal shows up if user is clicking on the download button rapidly multiple of times. And if user continues to do this for a few seconds app crashes.

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.41-3
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
Notes/Photos/Videos: Any additional supporting documentation

Screen_Recording_20230718_100949_One.UI.Home.mp4

Screen_Recording_20230718_103253_One.UI.Home.mp4

Expensify/Expensify Issue URL:
Issue reported by: @Nathan-Mulugeta
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1689664399381639

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01ea9db9c8634d571d
• Upwork Job ID: 1681825807519490048
2023-07-20
• Automatic offers:
• | | 0
• | | 0
• Nathan-Mulugeta | Reporter | 25722312

[melvin-bot]

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

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[hungvu193]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Android - User is able to click on download button on share code page after modal shows up if user is clicks rapidly and app crashes

What is the root cause of that problem?

When user tried to press many times into download button, Android app will be crashed, since our handleDownload tried to create 2 file with the same name which caused this issue.

What changes do you think we should make in order to solve the problem?

We can simply wrap our download function with debounce to avoid this issue happened again.

  this.downLoadQrCode = _.debounce(this.downLoadQrCode.bind(this), 500);

   downLoadQrCode() {
             this.qrCodeRef.current?.download()
    }

// then use it

        {isNative && (
            <MenuItem
                isAnonymousAction
                title={this.props.translate('common.download')}
                icon={Expensicons.Download}
                // eslint-disable-next-line es/no-optional-chaining
                onPress={this.downLoadQrCode}
            />

What alternative solutions did you explore? (Optional)

Use Download.setDownload similar the way we used to download attachment, we only enable Download button again after attachment is downloaded.

[bernhardoj]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Downloading the Share code multiple times will crash the app on Android, starting on Android 10.

What is the root cause of that problem?

The download button will call fileDownload function. Here are the steps of how a file is downloaded on Android:

1. Check its permission, if pass, call handleDownload
   

   App/src/libs/fileDownload/index.android.js

   Lines 103 to 108 in 16a83ac

   	.then((hasPermission) => {
   	if (hasPermission) {
   	return handleDownload(url, fileName);
   	}
   	FileUtils.showPermissionErrorAlert();
   	})

2. Download the attachment
   

   App/src/libs/fileDownload/index.android.js

   Lines 51 to 62 in 16a83ac

   	if (!isLocalFile) {
   	// Fetching the attachment
   	fetchedAttachment = RNFetchBlob.config({
   	fileCache: true,
   	path: `${path}/${attachmentName}`,
   	addAndroidDownloads: {
   	useDownloadManager: true,
   	notification: false,
   	path: `${path}/Expensify/${attachmentName}`,
   	},
   	}).fetch('GET', url);
   	}

3. Copy the downloaded attachment to /Downloads/Expensify/
   

   App/src/libs/fileDownload/index.android.js

   Lines 65 to 82 in 16a83ac

   	fetchedAttachment
   	.then((attachment) => {
   	if (!isLocalFile && (!attachment || !attachment.info())) {
   	return Promise.reject();
   	}
   	if (!isLocalFile) attachmentPath = attachment.path();
   	return RNFetchBlob.MediaCollection.copyToMediaStore(
   	{
   	name: attachmentName,
   	parentFolder: 'Expensify',
   	mimeType: null,
   	},
   	'Download',
   	attachmentPath,
   	);
   	})

The error happens on the 3rd step.

First, we need to understand why the crash/error happens. Here is the error message of the crash:

Based on the stack trace above, the error comes from copyToMediaStore->createNewMediaFile->resolver.insert here:

Notice that the logic is only for Android 10 (Q) and above.

I don't find an official documentation stating why the error occurs, but I found a comment mentioning that this error happens when we save the same file name > 30 files. On my Android device, the error starts to happen when saving the 33rd image.

So, it's not about quickly pressing the download. We can even reproduce this issue by downloading the same report attachment and 2FA codes > 30 times.

Here is the video showing the error on the report attachment.

WhatsApp.Video.2023-07-19.at.21.37.17.mp4

What changes do you think we should make in order to solve the problem?

To allow users to keep downloading the same attachment over and over, we have 2 options:

1. Always append the attachment name with the current time. This way we won't have a file with the same name. (similar to WhatsApp)
   OR
2. Only append the attachment name after we get an error (similar to Chrome, see image below), but we currently unable to do this

We actually already have a catch block to catch any error here:

App/src/libs/fileDownload/index.android.js

Lines 73 to 89 in 16a83ac

	return RNFetchBlob.MediaCollection.copyToMediaStore(
	{
	name: attachmentName,
	parentFolder: 'Expensify',
	mimeType: null,
	},
	'Download',
	attachmentPath,
	);
	})
	.then(() => {
	RNFetchBlob.fs.unlink(attachmentPath);
	FileUtils.showSuccessAlert();
	})
	.catch(() => {
	FileUtils.showGeneralErrorAlert();
	})

The problem is, the library that we use, which is react-native-blob-util doesn't catch the native error, so we have no way to catch the error on the JS side. The solution is to raise a PR upstream to catch the error natively.
https://github.com/RonRadtke/react-native-blob-util/blob/48735c72ff8f442db215aff1bef68c53bc3c5582/android/src/main/java/com/ReactNativeBlobUtil/ReactNativeBlobUtilMediaCollection.java#L98-L99

+try {
    // Keeps a handle to the new file's URI in case we need to modify it later.
    return resolver.insert(mediauri, fileDetails);
+} catch (Exception ex) { return null }

By returning null on error, it will get rejected here:
https://github.com/RonRadtke/react-native-blob-util/blob/48735c72ff8f442db215aff1bef68c53bc3c5582/android/src/main/java/com/ReactNativeBlobUtil/ReactNativeBlobUtilImpl.java#L422-L425

After we did the above fix, we can now catch the error and retry the file copying with a different file name that is appended with the current time:

function copyToExpensifyDownloads(from, attachmentName) {
    return RNFetchBlob.MediaCollection.copyToMediaStore(
        {
            name: attachmentName,
            parentFolder: 'Expensify',
            mimeType: null,
        },
        'Download',
        from,
    );
}
return copyToExpensifyDownloads(attachmentPath, attachmentName).catch(() => {
    console.log('retrying copying the attchment iwth diff name')
    return copyToExpensifyDownloads(attachmentPath, `${attachmentName}_${(new Date()).toISOString()}`);
});

[twisterdotcom]

So, it's not about quickly pressing the download. We can even reproduce this issue by downloading the same report attachment and 2FA codes > 30 times.

Okay nice. This is a nice legit bug.

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[parasharrajat]

Ok, @bernhardoj's proposal make sense. I think we should update the upstream lib to throw an error as it will benefit the community and then we can follow your solution 2.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @madmax330, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[madmax330]

I think we should do solution 1.
If we're going to append the current time to the filename, let's do it from the start that way we don't have to deal with this try catch instance.
We can still submit a PR to the upstream repo to allow throwing errors, but I think we should just include the timestamp all the time, it's a cleaner solution.

[melvin-bot]

📣 @parasharrajat Please request via NewDot manual requests for the Reviewer role ($1000)

[melvin-bot]

📣 @bernhardoj You have been assigned to this job!
Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs!
Keep in mind: Code of Conduct | Contributing 📖

[melvin-bot]

📣 @Nathan-Mulugeta 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app!

Upwork job

[bernhardoj]

@madmax330 I have a question. Do we want to append it with the current time on all platforms, or just Android?

[madmax330]

Let's do it for all platforms to keep it consistent.

[bernhardoj]

Got it! Will come up with the PR in a few hours

[parasharrajat]

@madmax330 PR is ready for your review.

[parasharrajat]

Bump @madmax330

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @bernhardoj got assigned: 2023-07-24 16:40:14 Z
• when the PR got merged: 2023-08-01 10:21:21 UTC
• days elapsed: 5

On to the next one 🚀

[parasharrajat]

I think this is eligible for a merge bonus as PR was approved 5 days back and there was no new change request afterward.

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.49-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Append current time to the downloaded file name #23531

If no regressions arise, payment will be issued on 2023-08-10. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @parasharrajat does not require payment (Eligable for Manual Requests)
• @bernhardoj requires payment

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR:
• [@parasharrajat] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@parasharrajat] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@parasharrajat] Determine if we should create a regression test for this bug.
• [@parasharrajat] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@twisterdotcom] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[parasharrajat]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR: NA
• [@parasharrajat] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: NA
• [@parasharrajat] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: This is an edge case and couldn't be found under normal circumstances.
• [@parasharrajat] Determine if we should create a regression test for this bug. yes
• [@parasharrajat] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Steps

iOS/Android

1. Create a chat room if you don't have one
2. Open the chat room
3. Press the chat header
4. Press the Share code menu
5. Press the Download menu to download the QR code, repeat this > 30 times
6. Verify you can still download the QR code without any error
7. Verify the current time is appended to the QR code image name (except iOS)

2FA code download

1. Open Settings > Security > Two-factor authentication
2. Press the Download button
3. Verify the current time is appended to the 2fa code file name (on iOS, a Share modal will show instead)

Chat attachment download

1. Open any chat
2. Send 3 types of attachments, that is image, video, and other files (e.g., text)
3. Download the attachment
4. Verify the current time is appended to the attachment file name (except the image on iOS)

Note: Used library does not currently support changing the name of the file before saving on iOS. Thus you won't see timestamp being added on iOS.

Do you agree 👍 or 👎 ?

[parasharrajat]

Payment requested with bonus. Ref: #23094 (comment)

[twisterdotcom]

Payment summary:
@parasharrajat will be paid via newDot $1500
@bernhardoj will be paid via Upwork $1500: https://www.upwork.com/nx/wm/offer/26030844
@Nathan-Mulugeta will be paid via Upwork $250: https://www.upwork.com/nx/wm/workroom/34308785/overview

[twisterdotcom]

All paid.

[JmillsExpensify]

Review the details for @parasharrajat. Approved for payment in NewDot based on the BZ summary above.

[kidroca]

Hey team,

I've identified a bug that appears to be a regression linked to the changes introduced in the current ticket.

• Bug Report Slack Discussion
• Relevant Code Change: https://github.com/Expensify/App/pull/23531/files#r1299806796

[jasperhuangg]

@kidroca Thanks! Normally we leave regressions for the person who implemented the solution to fix. @bernhardoj Please fix the regression!

[bernhardoj]

I think @kidroca is fixing it along with the PR, do we want to create a separate PR for the fix? (Btw, I don't know how to receive an image from Concierge)