[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)