[HOLD for payment 2024-02-26][$500] [MEDIUM] Erroring thumbnail images try to reload infinitely

[lindboe]

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

---

Version Number: 1.4.25-4
Reproducible in staging?: Yes
Reproducible in production?: It's worse on production, the app fails to load completely once I sign in.
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers): [email protected]
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @lindboe
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1705427206739379?thread_ts=1705360862.915459&cid=C01GTK53T8Q

Action Performed:

1. Have a request with a receipt image that produces a 404 when requested. (I am not sure how to reproduce this part outside of development, where you can give a thumbnail component an intentionally wrong URL.)
2. Open the app in a web browser and open the devtools. Observe that the requests for the image that can't be loaded are continuously made.

Expected Result:

After the request returns a 404, the requests should stop being made.

Actual Result:

Requests for the image are infinitely repeated.

Workaround:

If this issue is encountered in production in its current state, the app can't be used at all, although I believe that particular issue may have been fixed by @cead22.

If this issue is encountered in staging, I don't think this prevents usage.

In development, it causes a connection/file descriptor leak as webpack generates more and more connections it never clears. This eventually leads to the developer's computer encountering errors and not working properly, including affecting internet access. (This must also be caused by a second issue with the app not clearing closed connections correctly).

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

In development:

receipt404.mov

Production screenshot:

Root cause and considerations for proposals:

1. Image, which handles the load request, is memoized based on whether the identity of the source prop stays the same
2. ImageWithSizeCalculation is reconstructing source each render because it's creating the object in the prop. Pretty easy to fix with a useMemo instead:

    const source = useMemo(() => {
        return {uri: url};
    }, [url]);

This does NOT occur for the Image/index.native.js implementation.

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~010386784e89b6acc5
• Upwork Job ID: 1749005349524606976
• Last Price Increase: 2024-02-04
• Automatic offers:
• paultsimura | Contributor | 28145121

[melvin-bot]

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

[lindboe]

Update: I've started to be able to load data on production again, now behavior matches staging/dev.

[jliexpensify]

Hi @lindboe - cool, so all good here? Can I close this?

[lindboe]

@jliexpensify I'm not sure I understand the question? The original bug still exists, if you're referring to my update, note that in the original ticket production was breaking worse than the other environments for me, in that it refused to load at all, but now all environments have the issue of constantly attempting to reload the 404ing image.

[jliexpensify]

Sorry @lindboe I misread your comment as the issue was fixed. I'll re-review this and see if I can reproduce.

EDIT: I don't know how to repro step 1, so asking here: https://expensify.slack.com/archives/C01SKUP7QR0/p1705458078466679 and here: https://expensify.slack.com/archives/C01GTK53T8Q/p1705532780265849

[jliexpensify]

@tranvantoan-qn responded and can reproduce the 404 request issue. Do you want to make a proposal in this GH?

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~010386784e89b6acc5

[melvin-bot]

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

[tranvantoan-qn]

@jliexpensify Sorry, I'm not a developer (I used to be a test contributor when the program was still open to testers). However, I've experienced that trying to reproduce issues can sometimes be impossible due to different conditions, so using some support tools can be very helpful. If you want to know more about how to use these tools to mimic scenarios like this one, I'm happy to help.

Once we can reproduce it, either in a normal way or by using the tool, I'm sure that we can fix it and verify the solution

[paultsimura]

Proposal

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

When an image cannot be loaded, requests are constantly being made.

This can be easily (for now) reproduced when modifying the Distance request offline. The Distance receipt will be fixed in the #34287, but for now, it allows us to test this endless requests loop.

What is the root cause of that problem?

The root cause is in the ImageWithSizeCalculation component, which builds the source inside the render body:

App/src/components/ImageWithSizeCalculation.tsx

Line 76 in 15338b9

source={{uri: url}}

So the component is re-rendered each time the request fails, and the request is sent again. This creates a loop.

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

Instead of building the source in-place, we should use the useMemo hook:

const source = useMemo(() => ({uri: url}), [url]);

And use this source for the Image.source.

Then, we should handle the fallback image.
We've decided that it will depend on 2 things: network status and the image type (purpose).
For that, we should:

1. Make use of the {isOffline} = useNetwork()
2. Pass a new parameter type or purpose (or whatever) to the ImageWithSizeCalculation to indicate whether it's a receipt/attachment/file.

To display this fallbackImage, we will add a new var and will set it on the onError:

const hasFailedToLoadRef = useRef<boolean | null>(null);
const onError = () => {
        Log.hmmm('Unable to fetch image to calculate size', {url});
        hasFailedToLoadRef.current = true
};

Then, we'll need to alter the above-mentioned memo to handle the failed state:

const source = useMemo(() => {
  if (!hasFailedToLoadRef.current) {
    return {uri: url};
  }
  
  if (isOffline) {
    return OFFLINE_IMG;
  }

  return FALLBACK_BASED_ON_TYPE
}, [url, hasFailedToLoadRef, isOffline, type]);

The last thing would be to handle the network change: when coming back online after being offline, reset the hasFailedToLoadRef once to allow re-fetching of the image from the server.

What alternative solutions did you explore? (Optional)

[jliexpensify]

Got it, thanks for the context @tranvantoan-qn! My biggest question is to understand how these 404 errors are generated and if they can be generated by actual users. The reason I ask is that we have pivoted to now focusing on project-specific bugs that impact performance/the UI so if it's going to affect actual users, we'd want to get it fixed. It's just unusual because I don't think anyone on our team, who use the app in real life, have experienced this with our requests.

[paultsimura]

@jliexpensify I have described the easiest way to reproduce the issue at the moment:

This can be easily (for now) reproduced when modifying the Distance request offline. The Distance receipt will be fixed in the #34287, but for now, it allows us to test this endless requests loop.

#34601 (comment)

[jliexpensify]

It looks like your proposal got posted as I was writing mine - it's great that we have a way to easily repro this one! If it's associated with Distance requests atm, we can fold this into #wave5 - https://expensify.slack.com/archives/C05DWUDHVK7/p1705872590528689

[parasharrajat]

Proposal looks correct. I will do some testing on it and let you all know.

[allroundexperts]

Can someone please assign this to me since I reviewed the PR? Thanks!

[jliexpensify]

I assume we unassign @parasharrajat ?

[paultsimura]

Deployed to prod: #36214 (comment)

[allroundexperts]

Checklist

1. Migrate Image/index.js to function component #24159
2. https://github.com/Expensify/App/pull/24159/files#r1493106476
3. N/A
4. I think this was an edge case that is not easily reproducible without altering the generated HTML or the source code. As such, a regression test might not make complete sense here.

[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.4.42-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: Add fallback icons for thumbnails #36214

If no regressions arise, payment will be issued on 2024-02-26. 🎊

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

• @paultsimura requires payment automatic offer (Contributor)
• @allroundexperts requires payment through NewDot Manual Requests
• @lindboe does not require payment (Contractor)

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

• [@allroundexperts] The PR that introduced the bug has been identified. Link to the PR:
• [@allroundexperts] 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:
• [@allroundexperts] 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:
• [@allroundexperts] Determine if we should create a regression test for this bug.
• [@allroundexperts] 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.
• [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[jliexpensify]

No checklist needed

Payment Summary:

• Contributor: @paultsimura $500 (PAID)
• C+: @allroundexperts $500 (New.Dot)

[JmillsExpensify]

$500 approved for @allroundexperts based on summary above.