🔴 [$500] App crashes when opening the second distance request as a reviewer #26925

hayata-suenaga · 2023-09-07T05:15:53Z

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

Action Performed:

Log in to User A's account.
Create a workspace and invite User B.
Open the workspace room from User B.
As User B, create a distance request (workspace chat -> + -> Request money -> Distance -> add waypoints -> Next -> Request
Return to User A's account.
Open the Requested Distance sent from User B. Confirm that the app doesn't crash
Switch back to User B's account and send a distance request again.
As User A, try to open the new distance request sent by User B.

Expected Result:

User A should be able to view the new distance request

Actual Result:

App crashes

Workaround:

N/A

Platforms:

Only tested on web. Not sure if the issue is present on other platforms, too

Version Number: v1.3.64-2
Reproducible in staging?: Yes
Reproducible in production?: Yes
**If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
**Email or phone of affected tester (no customers): N/A
Logs: N/A
Notes/Photos/Videos:
Please refer to this video (I couldn't upload the video)
**Expensify/Expensify Issue URL: N/A
Issue reported by: @ayazhussain79
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1694035685129389

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~014b20f43c302788fe
Upwork Job ID: 1699652736794378240
Last Price Increase: 2023-09-14

melvin-bot · 2023-09-07T05:15:57Z

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

melvin-bot · 2023-09-07T05:15:59Z

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

melvin-bot · 2023-09-07T05:16:06Z

Job added to Upwork: https://www.upwork.com/jobs/~014b20f43c302788fe

melvin-bot · 2023-09-07T05:16:10Z

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

melvin-bot · 2023-09-07T05:16:16Z

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

akinwale · 2023-09-07T05:19:22Z

Proposal

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

App crashes when opening the second distance request as a reviewer.

What is the root cause of that problem?

The crash is coming from the ReceiptUtils#getThumbnailAndImageURIs method while trying to check if the file is an image. In some cases, the filename passed as a parameter to the method can be null or undefined which causes a crash when trying to perform any kind of operation (split, etc) on the passed filename parameter.

App/src/libs/ReceiptUtils.js

Lines 40 to 52 in 46e6e20

    
           function getThumbnailAndImageURIs(path, filename) { 
        
               const isReceiptImage = Str.isImage(filename); 
        
               // For local files, we won't have a thumbnail yet 
        
               if (isReceiptImage && (path.startsWith('blob:') || path.startsWith('file:'))) { 
        
                   return {thumbnail: null, image: path}; 
        
               } 
        
               if (isReceiptImage) { 
        
                   return {thumbnail: `${path}.1024.jpg`, image: path}; 
        
               } 
        
               const {fileExtension} = FileUtils.splitExtensionFromFileName(filename);

It can also happen when opening the first distance request on a newly created workspace.

This can be reliably reproduced when the reviewing user attempts to open the request before the receipt preview image has actually loaded. It may occur if Onyx data from Pusher has not finished updating the reviewer's local database. The issue can stem from this line:

App/src/components/ReportActionItem/MoneyRequestView.js

Line 93 in 46e6e20

    
           receiptURIs = ReceiptUtils.getThumbnailAndImageURIs(transaction.receipt.source, transaction.filename);

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

Use the React state for hasReceipt, hasErrors and receiptURIs in the MoneyRequestView component and a useEffect hook with a dependency on the transaction prop to set these values. When the transaction gets updated with Onyx, the view will re-render properly with updated values.

const [hasReceipt, setHasReceipt] = useState(false);
const [hasErrors, setHasErrors] = useState(false);
const [receiptURIs, setReceiptURIs] = useState(receiptURIs);

useEffect(() => {
    const requestHasReceipt = TransactionUtils.hasReceipt(transaction);
    setHasReceipt(requestHasReceipt);
    if (requestHasReceipt) {
        setReceiptURIs(ReceiptUtils.getThumbnailAndImageURIs(transaction.receipt.source, transaction.filename));
        setHasErrors(TransactionUtils.hasMissingSmartscanFields(transaction));
    }
}, [transaction]);

Perform an early return in ReceiptUtils#getThumbnailAndImageURIs if filename is null or undefined. When the filename actually gets retrieved, the view will re-render and the image (if any) will be properly displayed.

function getThumbnailAndImageURIs(path, filename) {
+    if (!filename) {
+        return {thumbnail: null, image: ReceiptGeneric};
+    }
// ... rest of the method
}

What alternative solutions did you explore? (Optional)

None.

imranaalam · 2023-09-07T05:58:00Z

Problem Statement:
The application crashes when attempting to open the second distance request as a reviewer. This crash can also occur when opening the first distance request on a newly created workspace.

Root Cause:
The malfunction stems from the ReceiptUtils#getThumbnailAndImageURIs method. When this method tries to determine if the file is an image, it encounters cases where the filename passed as a parameter can be null or undefined. Performing operations (such as split) on this null or undefined filename causes the crash.

Proposed Solution:
Introduce a comprehensive validation mechanism for filenames. This validation will address not only null or undefined values but also empty strings, filenames with invalid characters, excessively long filenames, and filenames that start or end with a dot.

Here's the code for the proposed solution:

def isValidFilename(filename):
    INVALID_CHARS = set(r'\/:*?"<>|')
    MAX_FILENAME_LENGTH = 255  # Typically, most file systems limit filenames to 255 characters
    
    # Check if filename is None (null/undefined in JS context)
    if filename is None:
        return False
    
    # Check for empty filename or filename with just whitespace
    if filename.strip() == "":
        return False

    # Check for excessively long filenames
    if len(filename) > MAX_FILENAME_LENGTH:
        return False
    
    # Check for invalid characters
    if any(char in INVALID_CHARS for char in filename):
        return False

    # Check if filename starts or ends with a dot
    if filename.startswith('.') or filename.endswith('.'):
        return False
    
    return True

def getThumbnailAndImageURIs(path, filename):
    # If not a valid filename, return default values
    if not isValidFilename(filename):
        return {'thumbnail': None, 'image': 'ReceiptGeneric'}
    
    # ... [rest of the original function logic]

Alternative Solution:
An initial simpler solution was to only check for null or undefined filenames and return default values if either condition was met. However, this approach might not catch other potential filename anomalies that could arise in the future.

mountiny · 2023-09-07T08:11:02Z

@akinwale How will the money request action component look like until the receipt is loaded?

akinwale · 2023-09-07T08:12:49Z

@akinwale How will the money request action component look like until the receipt is loaded?

It displays the default generic receipt image (the green placeholder) until the image loads. I can make a video to demo this.

pradeepmdk · 2023-09-07T08:16:03Z

Proposal

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

App crashes when opening the second distance request as a reviewer.

What is the root cause of that problem?

App/src/libs/ReceiptUtils.js

Lines 40 to 52 in 46e6e20

    
           function getThumbnailAndImageURIs(path, filename) { 
        
               const isReceiptImage = Str.isImage(filename); 
        
               // For local files, we won't have a thumbnail yet 
        
               if (isReceiptImage && (path.startsWith('blob:') || path.startsWith('file:'))) { 
        
                   return {thumbnail: null, image: path}; 
        
               } 
        
               if (isReceiptImage) { 
        
                   return {thumbnail: `${path}.1024.jpg`, image: path}; 
        
               } 
        
               const {fileExtension} = FileUtils.splitExtensionFromFileName(filename);

when file name is empty its crashing and pusher send an different property 'receiptFilename'

we already updated in this PR
https://github.com/Expensify/App/pull/26704/files#diff-d8e6b69ad4461bebf91e940a67ddc07adf36a3f7c156d6066363c3bde06a0f8a
but miss to add

App/src/components/ReportActionItem/MoneyRequestView.js

Line 93 in 46e6e20

    
           receiptURIs = ReceiptUtils.getThumbnailAndImageURIs(transaction.receipt.source, transaction.filename);

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

we should update

ReceiptUtils.getThumbnailAndImageURIs(transaction.receipt.source, transaction.filename || transaction.receiptFilename || '');

and here we should declare the default param (in future avoid like this crash we should take default value in the param)

getThumbnailAndImageURIs(path, filename = '')

@mountiny @hayata-suenaga
we already discussed this issue backend should be sent as a filename property in pusher and API
#26585 (comment)

#26585 (comment)
#26585 (comment)
#26585 (comment)

akinwale · 2023-09-07T08:25:45Z

@mountiny Demo video. My connection is a bit slow so it takes a while to load.

26925-demo.mp4

hayata-suenaga · 2023-09-11T03:19:55Z

@ArekChr could you review the proposals?

neil-marcellini · 2023-09-11T16:28:30Z

I'm taking over as requested internally in Slack here

ArekChr · 2023-09-12T08:22:14Z

Are there steps on how to trigger this crash? I couldn't reproduce it

pradeepmdk · 2023-09-12T09:39:34Z

@ArekChr this will help you reproduce this video but now the issue was changed pusher not sending the transcation_ 2023636921105531794 data so its keeps loading

Untitled.mov

pradeepmdk · 2023-09-12T10:20:11Z

@neil-marcellini @ArekChr could you check this on the Backend side.

ArekChr · 2023-09-13T11:29:05Z

@pradeepmdk I couldn't load the video. Could you optimize it and send it in a smaller size?

pradeepmdk · 2023-09-13T11:40:12Z

@ArekChr which one

this one on slack bug reproduce video this video

I converted this video here
https://github.com/Expensify/App/assets/16595846/1e426bb9-84da-4316-ba70-2bf242c31d78

bug_repro.video-converter.com.mp4

akinwale · 2023-09-13T11:43:41Z

@pradeepmdk I couldn't load the video. Could you optimize it and send it in a smaller size?

@ArekChr You can also check out the demo video I posted here: #26925 (comment)

At the time, I was able to reliably reproduce if I tried to open the request before the image loaded.

ArekChr · 2023-09-14T10:32:19Z

@akinwale, @pradeepmdk Thanks for videos! I tested that scenario again and It seems the app is not crashing anymore on the main branch and staging, there is a different issue with no loaded distance request which is not related to this issue.

melvin-bot · 2023-09-14T16:00:07Z

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

ayazhussain79 · 2023-09-14T21:53:58Z

@ArekChr @akinwale Do you know which PR has fixed this bug?

pradeepmdk · 2023-09-15T00:47:19Z

@ayazhussain79 this is not yet fixed. previously pusher sent the wrong data so it crashed. now Pusher not sending that whole object so the issue has changed now.

ArekChr · 2023-09-15T08:33:27Z

@pradeepmdk, The bug with the crash issue, is not reproducible anymore

pradeepmdk · 2023-09-15T08:44:27Z

@ArekChr Yes I agree, I am just explaining @ayazhussain79 to the issue that has changed now. The crash issue is no more here. Maybe one pusher is fixed and again filename property is missed means this issue will reproduce. now this issue is closed or hold that is your choice @ArekChr.

ArekChr · 2023-09-15T09:26:02Z

I think we can close this issue, for the infinite skeleton loading bug, there is a separate issue created here

hayata-suenaga added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 7, 2023

hayata-suenaga self-assigned this Sep 7, 2023

melvin-bot bot assigned bfitzexpensify Sep 7, 2023

hayata-suenaga added the External Added to denote the issue can be worked on by a contributor label Sep 7, 2023

melvin-bot bot changed the title ~~App crashes when opening the second distance request as a reviewer~~ [$500] App crashes when opening the second distance request as a reviewer Sep 7, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 7, 2023

melvin-bot bot assigned ArekChr Sep 7, 2023

neil-marcellini assigned neil-marcellini and unassigned hayata-suenaga Sep 11, 2023

pradeepmdk mentioned this issue Sep 12, 2023

HOLD [$500] Web - IOU - When performing a split bill in a group, the skeleton view is stuck #27200

Closed

6 tasks

tgolen changed the title ~~[$500] App crashes when opening the second distance request as a reviewer~~ 🔴 [$500] App crashes when opening the second distance request as a reviewer Sep 14, 2023

tgolen closed this as completed Sep 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🔴 [$500] App crashes when opening the second distance request as a reviewer #26925

🔴 [$500] App crashes when opening the second distance request as a reviewer #26925

hayata-suenaga commented Sep 7, 2023 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Sep 7, 2023

melvin-bot bot commented Sep 7, 2023

melvin-bot bot commented Sep 7, 2023

melvin-bot bot commented Sep 7, 2023

melvin-bot bot commented Sep 7, 2023

akinwale commented Sep 7, 2023 •

edited

Loading

imranaalam commented Sep 7, 2023 •

edited

Loading

mountiny commented Sep 7, 2023

akinwale commented Sep 7, 2023

pradeepmdk commented Sep 7, 2023 •

edited

Loading

akinwale commented Sep 7, 2023

hayata-suenaga commented Sep 11, 2023

neil-marcellini commented Sep 11, 2023

ArekChr commented Sep 12, 2023

pradeepmdk commented Sep 12, 2023

pradeepmdk commented Sep 12, 2023

ArekChr commented Sep 13, 2023

pradeepmdk commented Sep 13, 2023 •

edited

Loading

akinwale commented Sep 13, 2023

ArekChr commented Sep 14, 2023

melvin-bot bot commented Sep 14, 2023

ayazhussain79 commented Sep 14, 2023

pradeepmdk commented Sep 15, 2023

ArekChr commented Sep 15, 2023

pradeepmdk commented Sep 15, 2023

ArekChr commented Sep 15, 2023

🔴 [$500] App crashes when opening the second distance request as a reviewer #26925

🔴 [$500] App crashes when opening the second distance request as a reviewer #26925

Comments

hayata-suenaga commented Sep 7, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

melvin-bot bot commented Sep 7, 2023

melvin-bot bot commented Sep 7, 2023

Bug0 Triage Checklist (Main S/O)

melvin-bot bot commented Sep 7, 2023

melvin-bot bot commented Sep 7, 2023

melvin-bot bot commented Sep 7, 2023

akinwale commented Sep 7, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

imranaalam commented Sep 7, 2023 • edited Loading

mountiny commented Sep 7, 2023

akinwale commented Sep 7, 2023

pradeepmdk commented Sep 7, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

akinwale commented Sep 7, 2023

hayata-suenaga commented Sep 11, 2023

neil-marcellini commented Sep 11, 2023

ArekChr commented Sep 12, 2023

pradeepmdk commented Sep 12, 2023

pradeepmdk commented Sep 12, 2023

ArekChr commented Sep 13, 2023

pradeepmdk commented Sep 13, 2023 • edited Loading

akinwale commented Sep 13, 2023

ArekChr commented Sep 14, 2023

melvin-bot bot commented Sep 14, 2023

ayazhussain79 commented Sep 14, 2023

pradeepmdk commented Sep 15, 2023

ArekChr commented Sep 15, 2023

pradeepmdk commented Sep 15, 2023

ArekChr commented Sep 15, 2023

hayata-suenaga commented Sep 7, 2023 •

edited by melvin-bot bot

Loading

akinwale commented Sep 7, 2023 •

edited

Loading

imranaalam commented Sep 7, 2023 •

edited

Loading

pradeepmdk commented Sep 7, 2023 •

edited

Loading

pradeepmdk commented Sep 13, 2023 •

edited

Loading