iOS - Profile - The App crashes when saving the user's avatar

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

---

Version Number: 1.4.49-0
Reproducible in staging?: y
Reproducible in production?: n
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4410248
Issue reported by: Applause - Internal Team

Action Performed:

1. Open iOS App
2. Login with any account
3. Go to Account Settings
4. Tap to User Avatar
5. Select "Upload photo"
6. Select "Take photo" or "Choose from gallery"
7. Take/select photo and tap "Save"

Expected Result:

Photo was successfully saved as the user's avatar.

Actual Result:

App crashes after user taps on "Save" button

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

Bug6406847_1709896582377.iOS-crash-Avatar.mp4

View all open jobs on GitHub

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[kbecciv]

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

[melvin-bot]

Triggered auto assignment to @madmax330 (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[shubham1206agra]

@kbecciv Can you share the crash logs here?

[kbecciv]

@shubham1206agra Please attached
logs crash.docx

[roryabraham]

not able to reproduce on dev

[roryabraham]

this appears to be related to this crash, which was first caught on 1.4.47-10. So I'm not sure if it's a blocker (but it was my mistake not noticing this earlier this week, I had been looking only at Android crashes by mistake)

[roryabraham]

was able to reproduce on staging. I wonder if it has to be an HEIC?

[roryabraham]

need to test on a physical device...

[situchan]

I reproduced in iOS Simulator.
The crash happens only on release build. It's pretty bad that once reproduced following OP, crash happens on app launch until uninstall and reinstall the app.
After reverting #36019, not able to reproduce.

[situchan]

I managed to debug more.

After reproducing crash on release build, switch to debug scheme and build, console error on app launch:

[roryabraham]

I wonder if this can be fixed by removing deprecated useWorkletCallback: https://github.com/software-mansion/react-native-reanimated/pull/5377/files#r1501883320

[roryabraham]

cool, was able to reproduce on a release build. Going to try nixing useWorkletCallback and see if that helps

[roryabraham]

I am very stuck on trying to fix this, mostly because it's very difficult to debug or even get logs in a release build, which seems to be the only place where this is reproducible.

[roryabraham]

#36019 is already on production. Have we verified that this is not reproducible on prod?

[roryabraham]

Requested retest: https://expensify.slack.com/archives/C9YU7BX5M/p1709996985170449

[roryabraham]

Noodling about in #38018, but I'm not confident that I'm any closer to a fix

[roryabraham]

Ok, this was reproduced in prod, so not a deploy blocker. But it's very bad, maybe a fire

[kavimuru]

Bug is reproduced in production too.

avatar.update.mp4

[roryabraham]

I confirmed that commenting out this line prevents the crash. This doesn't really tell us much, other than that the crash doesn't appear to be happening inside cropOrRotateImage, or manipulateAsync. It's still possible that the crash is caused by a problem there though, such as a file not being saved correctly.

[roryabraham]

some logs: https://staging.expensify.com/_devportal/tools/logSearch/#query=request_id:(%22861c7ee5cdbbef73-PDX%22)+AND+timestamp:[2024-03-09T15:38:30.081Z+TO+2024-03-09T17:38:30.081Z]&index=logs_expensify-027211

[roryabraham]

ok, so I confirmed that the crash happens after this point. I tried just removing the Onyx data from API.write(WRITE_COMMANDS.UPDATE_USER_AVATAR, parameters, {});, but the crash still happens in this case.

[roryabraham]

ok, commented out API.write() entirely and put Onyx.update(optimisticData) directly in PersonalDetails.updateAvatar and that worked. I'm seeing the new avatar image, which tells me the file is being saved locally.

So next up I need to figure out where the actual image upload code is and try to narrow down what's going wrong.

[roryabraham]

I found a random article which does this to upload a file:

const createFormData = (photo, body = {}) => {
  const data = new FormData();

  data.append('photo', {
    name: photo.fileName,
    type: photo.type,
    uri: Platform.OS === 'ios' ? photo.uri.replace('file://', '') : photo.uri,
  });

  Object.keys(body).forEach((key) => {
    data.append(key, body[key]);
  });

  return data;
};

note the Platform.OS === 'ios' ? photo.uri.replace('file://', '') : photo.uri - the uri we have definitely starts with file://...

[hungvu193]

Hey @roryabraham hope this can help, I can see the JS log. You need to run the scheme "New Expensify" form xcode.

[hungvu193]

The crash happened right after you clicked Send button and after reseting the app (later), so I think it must be something with applying optimisticData.

[roryabraham]

ok, commented out API.write() entirely and put Onyx.update(optimisticData) directly in PersonalDetails.updateAvatar and that worked

I'm not sure. I think it's the file upload that's failing?

[roryabraham]

I tried adding a line here like result.uri = result.uri.replace('file://', '');, no dice 😞

[hungvu193]

I can confirm revert expo-image-manipulator to use @oguzhnatly/react-native-image-manipulator will fix the issue. So the problem will be with expo-image-manipulator.

[roryabraham]

Going to proceed with a revert due to the severity of the bug. I'd like to CP the revert to staging so we can fix prod ASAP

[roryabraham]

reopening to verify the fix in 1.4.49-4

[situchan]

ok so as I said here, app still crashes on launch for the users who already triggered crash on avatar upload in previous build.
They should uninstall and reinstall to continue using the app.

[roryabraham]

yeah, that's unfortunate but I don't think there's anything we can do about it really ☹️

[roryabraham]

well, maybe if we found a "restorative" fix to the crash rather than a "preventative" fix, but idk if we'll be able to do that since it seems to be a low-level crash in the native network request code, presumably caused by some bad url or something?

[roryabraham]

verified this is fixed after you uninstall and reinstall

[kosmydel]

Hey, I've investigated this issue today.

---

The reason behind it was that we were passing the base64=null param inside the file parameter when calling the API:

App/src/libs/actions/PersonalDetails.ts

Line 385 in 95a774e

const parameters: UpdateUserAvatarParams = {file};

The RCTNetworking networking library didn't like the base64 nullish value, and early returned causing the crash:

https://github.com/facebook/react-native/blob/a7fde7c885a3827c03b058a21023b18501e2a57a/packages/react-native/Libraries/Network/RCTNetworking.mm#L425-L428

Why did we pass the base64 value?
The expo-image-manipulator library by default sets the base64 option to false, meaning that we don't need to generate it. But the library instead of skipping this param, adds base64=null in this line:

https://github.com/expo/expo/blob/a05bfbc3a907177bca2dd8a2dd22073763daa895/packages/expo-image-manipulator/ios/ImageManipulatorModule.swift#L33

Commenting out this line fixes the problem.

So I come up with two solutions (without touching the library):

• We can pass here base64: true option to generate the base64 (instead of passing null)
• We can remove the base64 prop from the parameters when calling the API command:

const {base64, ...fileWithoutBase64} = file;
const parameters: UpdateUserAvatarParams = {file: fileWithoutBase64};

I will also contact with the internal expo maintainers at SWM, as they might want to fix it in the library.

---

I will prepare a PR with the fix tomorrow.

Thanks @roryabraham for the initial investigation, as it helped a lot!

[roryabraham]

sounds good @kosmydel and thanks for figuring that out. Glad my initial investigation was helpful! If we're not using the base64 encoding I imagine it's likely better not to generate it unnecessarily. So let's go with the 2nd solution to remove the base64 from the parameters when calling the API command.

Alternatively, maybe we can update expo-image-manipulator to remove base64: null from the response if no base64 encoding is created. That seems like the best solution if they're down for that

[kosmydel]

Alternatively, maybe we can update expo-image-manipulator to remove base64: null from the response if no base64 encoding is created. That seems like the best solution if they're down for that

I've contacted an Expo maintainer, so I will keep you updated.

For now, I think to unblock the issue, I can prepare a PR with the 2nd solution.

[kosmydel]

sounds good @kosmydel and thanks for figuring that out. Glad my initial investigation was helpful! If we're not using the base64 encoding I imagine it's likely better not to generate it unnecessarily. So let's go with the 2nd solution to remove the base64 from the parameters when calling the API command.

Alternatively, maybe we can update expo-image-manipulator to remove base64: null from the response if no base64 encoding is created. That seems like the best solution if they're down for that

Quick update: After contacting Expo maintainers they told me, that this is expected, and consistent across other parameters. They will improve the types, so the base64 will also include the null type (currently it is string | undefined). So in that way, we would know, that base64 could be null, and we would be able to handle it appropriately.

For now, I've created a PR with 2nd solution.