[HOLD for payment 2022-02-08] Android - Chat - App crashes when attaching a large size image

[kavimuru]

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 app and login
2. Open any chat
3. Tap on + > Add Attachment > Choose from galery
4. Select a very long image (use the one attached to the issue)
5. Wait a moment

Expected Result:

Preview of the image is displayed and able to send the image

Actual Result:

The app closes / crashes

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Android

Version Number: 1.1.22-0

Reproducible in staging?: Yes

Reproducible in production?: No

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos:

Expensify/Expensify Issue URL:

https://user-images.githubusercontent.com/43996225/146659084-e91b859c-3284-4a90-b06b-95669082764b.mp4

Issue reported by: Applause

Slack conversation:

View all open jobs on GitHub

[OSBotify]

:wave: 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.

[MelvinBot]

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

[sketchydroide]

I'm going to CC me as this is a deploy blocker and I'm the deployer for mobile this week

[sketchydroide]

I think Max is still ooo, due to time sensitivity, I'm reassigning this GH

[Jag96]

Looks like this is still happening on main, I can reproduce on my emulator but there aren't any obvious error messages in the metro logs or Expensify logs, so going to see if I can reproduce on production (1.1.18-3) and narrow it down from there. Also one thing I noticed, if I re-open the app after the crash and try to upload the same image again it works without any crash.

[Jag96]

I can still reproduce this on 1.1.18-3 which has been the production version since December 8th:

@kavimuru can you confirm this works on production consistently? Also, what device was this tested on? Getting the device info could help in finding the crash from the list in crashlytics.

One crash in crashlyitcs that is showing up is this one (note - internal link only), and it only started in 1.1.22-0 so going to look into the deploy checklist at potential issues

Fatal Exception: java.lang.RuntimeException
Canvas: trying to draw too large(159088608bytes) bitmap.

[Jag96]

I'm able to also reproduce this on 1.1.17-7 which was the previous production version, so I don't think this is a deploy blocker. Removing the deploy blocker label, adding Daily, and setting External.

[MelvinBot]

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

[jliexpensify]

Posted!

Internal - https://www.upwork.com/ab/applicants/1473083482261811200/job-details External - https://www.upwork.com/jobs/~010f941ebf54ffb6c7

[MelvinBot]

Triggered auto assignment to @parasharrajat (Exported)

[MelvinBot]

Current assignee @Jag96 is eligible for the Exported assigner, not assigning anyone new.

[ahmdshrif]

Proposal

this a react-native picker known issue and the workaround for this is to add maxWidth and maxHeight properties to options.

so going to /src/components/AttachmentPicker/index.native.js and add them the recommendation value is maxWidth: 1200, maxHeight: 800, but I try with this bigger value and the issue is not happening will start to happen if I increase any of these values.

another solution is to define quality as 0.7 also works with me but the image does not get a preview, also it will affect the small image quality.

Code change

const imagePickerOptions = {
    includeBase64: false,
    saveToPhotos: false,
    selectionLimit: 1,
+  maxWidth: 2000,
+  maxHeight: 8000,
};

Result

https://user-images.githubusercontent.com/21364901/146987801-f30eff0d-b0d9-443f-bb67-2f70c198caf0.mov

[parasharrajat]

Interesting @ahmdshrif. Do you have a link so that I can verify the issue? What is the maximum size that we can use?

[ahmdshrif]

@parasharrajat its recommended by some user not library so I try too many values with myself now you can update any large image and they will get resized to the max width and height we define in the code width : 2000 height : 8000

[ahmdshrif]

this issue link on react-native-image-picker https://github.com/react-native-image-picker/react-native-image-picker/issues/1073

[Jag96]

now you can update any large image and they will get resized to the max width and height we define in the code

Does this affect the way that smaller images show up? Or does it only re-size if the image is larger than one of those values (2000/8000)?

[parasharrajat]

I didn't find any information about resizing the image that you have proposed. That issue talks more about the size of the image file. How did you reach to that conclusion @ahmdshrif ?

[ahmdshrif]

@Jag96 this is the max-width and max-height, it only affects the image more than the threshold we provide.

@parasharrajat the conclusion comes from some searches and tests. the small Image did not have the issue and the big image had the issue so if we resize the big image the issue is solved. and buting this approach in test it works.

I know this is a workaround but I think the real solution will be from SDK itself.

[parasharrajat]

I know this is a workaround but I think the real solution will be from SDK itself.

Do you know what is causing the issue at the SDK level? and which SDK are you talking about?

Ok. Another question would be Will downsizing the image blur it or will it affect the image visually? If the Answer is No, Then I think we can adopt this solution as app crash is a critical issue.

cc: @Jag96

:ribbon: :eyes: :ribbon: C+ reviewed

[ahmdshrif]

@parasharrajat by SDK I mean the react-native-image-picker SDK.

I think the big image will have some quality downgrade it's relative to its size since more big images more a quality downgrade. you can see this effect in a video on the proposal. I do the same test with WhatsApp and I find it downgrades the quality with the same quality I get in the video.

[parasharrajat]

Ok. Thanks for pointing that out. As far as, we can zoom in on the image and clearly see the information, I am good with that.

@Jag96 Over to you.

[K4tsuki]

Telegram sends the image as file/document and let other applications open it properly.

[Jag96]

As far as, we can zoom in on the image and clearly see the information, I am good with that.

Agreed, and if there isn't any quality degradation for images smaller than the maxHeight/maxWidth values we set then I think that should be fine.

@ahmdshrif your proposal sounds good to me, can you please apply on upwork so @jliexpensify can hire you? Once you are hired feel free to start on a PR! Also in your PR please be sure to include a comment in the code explaining why maxWidth/maxHeight are being added, and be sure to confirm that there are no regressions on iOS.

[MelvinBot]

📣 @ahmdshrif You have been assigned to this job by @Jag96! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[jliexpensify]

Hi @ahmdshrif - I'm having some trouble finding you on Upworks, could you link me your profile? Cheers!

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[mvtglobally]

Issue not reproducible during KI retests. (Second week)

[Julesssss]

Taking over from Joe.

[ahmdshrif]

@Julesssss do you will review the pr or joe will review it ?

[Julesssss]

Just reviewed it, left a comment

[murataka]

Proposal

Hello, I know that this issue is closed , or just about to being closed. But I have some news ...

I saw that ,this happens even using our high-end phones , but the hardcoded max displayable image size is bigger than the shared long jpeg above. So I did a detailed research ( so please read it all ,more than 10 hours gone on my side ) , and will share just the summary here, but please feel free to ask any details .

RecordingCanjas.java // android 30 public static final int MAX_BITMAP_SIZE = 100 * 1024 * 1024; // 100 MB

Here is the latest release (play store version) of the app's logs ( A quick look ) :

And this one is just before the crash :

About the yellow underlined line : when using hermes, it is supposed to not call jsc library at all.

The expected behavior is to use hermes , and jsc is to be used as a fallback. That being said , In release mode , we possibly never could enable hermes vm . By the way, jsc uses a lot more memory than hermes, and that is why we can not display big images. Also , hermes is known to work faster.

Solution : There are things to do with the build-deploy system . And be sure that proguard does keep hermes related libraries , and the release should not try to load debug versions of hermes libraries.

You can download the tested app from this link :

https://play.google.com/store/apps/details?id=com.expensify.chat&hl=en&gl=US

[parasharrajat]

@murataka long story short, you are suggesting using Hermes.

If yes, we already do that.

[murataka]

@murataka long story short, you are suggesting using Hermes.

If yes, we already do that.

The real story is way longer for sure 😄 This is already the short one what I shared above .

The expected behavior is to use hermes , and jsc is to be used as a fallback. That being said , In release mode , we possibly never could enable hermes vm .

I said "could" if you read care fully.

It already works in debug mode. I checked every single version what is being used, what is not being used . also debugged much older versions of the app .

even the much older versions of expensify does not crash with big images ( debug build ) .

For some reason , it is not working on the release, but works on debugbuild ( Possible reasons-solutions explained in proposal ) . Here is a similar case :

https://github.com/facebook/react-native/pull/32683 https://github.com/facebook/react-native/issues/26400#issuecomment-833055441 https://github.com/facebook/react-native/issues/26400

This is why I asked you to read carefully. The above error logs clearly tell hermes is there, but not working .

[murataka]

@parasharrajat , evaluating this proposal would require following steps :

1. This is the commit I used for this : ( you can create a fork from this commit for the same version ) 2503fb4
2. Select an old branch which does not include the previous fixes
3. Create bundle in release mode
4. publish in play store for internal testing ( alpha or pre alpha )
5. Run the application and watch the logcat (after installing the alpha release )
6. Create a debug build
7. Check if the issue appears on debug build and compare the logcats
8. Check if the app still crashes after removing all libjsc and libherm files from the release build .
9. Check if the app still crashes when enablehermes = false ( see below emulator post )

As this may require some more priviledges, If you don't already have those priviledges to follow these steps, you can still evaluate it by running from the debug build ( npm run android ). The release build is already in playstore , until the next version is released.

Note : I'm not sure if we are using bundles while publishing to playstore. The exact way should be to repeat the same .

Also this would take much time to test. But I believe that native apps are capable to work better than browser versions of them , and we should not be compromising on the user experience on such an app ( by using workarounds )... Let me know if you want me to continue working on that issue or not , because further time spent on my side on this issue should not feel waste of time to me .

[murataka]

I did everything to crash the app, attached, downloaded , zoomed etc. It does not crash. The current solution does not let me pan and zoom to the image when it is large, if I'm not wrong.

This is the commit I used for this : ( you can create a fork from this commit for the same version ) https://github.com/Expensify/App/commit/2503fb40cc6a1dfbb6e602cf98cc48e520bf1969

Changes made :

project.ext.react = [
    enableHermes: false,  // clean and rebuild if changing
]

Emulator with 512 mb memory configured. 1024x600 resolution.

https://user-images.githubusercontent.com/5358438/151718330-0a6c49d0-fe12-4ff9-8c4c-adf0d2260678.mov

[Julesssss]

@murataka first, thank you for taking the time to investigate this.

I thought that we weren't using ProGuard currently (I have had the 'Introduce ProGuard' issue on my todo list for a while). But, it would troubling if Heremes is indeed not being used as we expect (even if it is just in some cases).

I'm going to follow your steps tomorrow morning, and I will let you know if I see the same results.

[murataka]

The reason may ( not ) be proguard, but the above steps would help to find the core issue . Configurations are too complicated to investigate directly. I hope at least one of your test builds will not crash and we shall be able to compare with the current release.

[Julesssss]

Hi @murataka. I read through all the linked issues and ran some tests today. I get slightly different results, so would you mind clarifying a few things for me?

I saw that this happens even using our high-end phones , but the hardcoded max displayable image size is bigger than the shared long jpeg above.

• Does this mean that you are seeing the crash when uploading a file larger than 100MB? It's not clear to me whether you see this with smaller images too.
• Is this crash handled by the app currently? Is it closing the app for users, or is the crash just seen in LogCat?

I wasn't able to reproduce any crash on any build today, but maybe my conditions don't match yours? I tested with a 5.6MB image (the one shown above). and built the following to Pixel 2 & Nexus 5x devices:

• AppStore signed App bundler build
• Local debug build
• Local signed APK build

The only thing I noticed was that my Nexus 5X would not render the image on the AppStore signed build (using Android App Bundler). But, as this was a production, release build no debug logs are available in LogCat.

If we are able to reproduce this then I would be happy to create a new issue, but we'll first need some more information and reproduction steps. Please let me know what you think!

[murataka]

• Does this mean that you are seeing the crash when uploading a file larger than 100MB? It's not clear to me whether you see this with smaller images too.

No. I test with same image, with the current release version from play store, because It does not crash with the above image in debug mode , even before the workaround that was merged a few days ago. Link : https://play.google.com/store/apps/details?id=com.expensify.chat&hl=en&gl=US No, getting crash logs is not a problem sometimes. The app crashes.

• Is this crash handled by the app currently? Is it closing the app for users, or is the crash just seen in LogCat?

The app on playstore crashes currently. The accepted solution limits large images (downscales) , in fact there are many issues with the latest merge about the accepted solution to this issue. I can add screen recordings in next post if you really need them. Also I shall possibly show it crashing again after the next release .

I wasn't able to reproduce any crash on any build today, but maybe my conditions don't match yours? I tested with a 5.6MB image (the one shown above). and built the following to Pixel 2 & Nexus 5x devices:

Yes, as the accepted solution is a workaround , and creates other issues, and even may crash for some other sized images , I tested and searched what the proper solution would be by using :

This is the commit I used for this : ( you can create a fork from this commit for the same version ) 2503fb4

Changes made :

project.ext.react = [
    enableHermes: false,  // clean and rebuild if changing
]

Emulator with 512 mb memory configured. 1024x600 resolution.

[murataka]

I. edited the steps, please be sure about you completely understand why I'm further investigating this before spending time for creating those test builds, as all these are taking additional time , and it is not possible to test before creating the builds.

[Julesssss]

I understand why you are investigating.

No. I test with same image, with the current release version from play store

Okay. So I did the same thing as you, but I am not seeing a crash. I am on the app version1.1.33-3 maybe newer than yours as I am on the beta channel. What (App Store) version are you testing, please?

there are many issues with the latest merge about the accepted solution to this issue. I can add screen recordings in next post if you really need them

Reproducible steps and information about the exact device and app version you are seeing the crash on is preferable at this point. I don't want to create a new issue for this before someone else has been able to reproduce.

[murataka]

1.1.32-0 is what I can download in release mode .

I don't want to create a new issue for this before someone else has been able to reproduce

There's no new issue , I suspect the actual one is not solved already .

So I did the same thing as you, but I am not seeing a crash. I am on the app version1.1.33-3 maybe newer than yours

When you test the above fix ( workaround ) and the same image , it would not crash.

This was the accepted proposal , the reviewers may not think this is an issue , but I think there are many issues with that.

I saw there are many opened issues about much smaller bugs than that workaround creates. But again , if maintainers say they don't think they need a proper fix, and the workaround is fine , we should not spend more time on this . However , when I shall have time , I shall share some video recordings about the issues this workaround creates.

As being said, solving those issues will require that workaround to be deleted , and the actual fix to be implemented .

Proposal

this a react-native picker known issue and the workaround for this is to add maxWidth and maxHeight properties to options.

so going to /src/components/AttachmentPicker/index.native.js and add them the recommendation value is maxWidth: 1200, maxHeight: 800, but I try with this bigger value and the issue is not happening will start to happen if I increase any of these values.

another solution is to define quality as 0.7 also works with me but the image does not get a preview, also it will affect the small image quality.

[Julesssss]

As being said, solving those issues will require that workaround to be deleted , and the actual fix to be implemented .

I understand that.

There's no new issue

By issue, I mean a Github issue. The previous fix was acceptable at the time, so I don't think this should be a regression, but a new followup issue.

You have made some interesting points and if we are able to come up with a proper proposal I would like for us to create a new issue, else we will not be able to pay you for your work 😄

As far as we are concerned, this previous solution was okay. Please forget about that for now. I would like you to approach the proposal as a new problem to solve -- an improvement to the previous solution. Does that make sense?

[murataka]

As far as we are concerned, this previous solution was okay. Please forget about that for now. I would like you to approach the proposal as a new problem to solve -- an improvement to the previous solution. Does that make sense?

Yes , but the improvement will require deletion of the actual fix :D If I will get the same amount of the payment , I shall include what the problems the actual solution will create. And I would not have problem with both being paid.

But again , I shall be a bit busy for this week, so I hope someone will not report the same bug with a different title with 250 , and someone else solve the actual issue properly for 250 again at the mean time. That would be an interesting situation.

I shall be active to answer any further questions , but won't be working on the bug for now .

Does that make sense?

Yes ! 💯

[Julesssss]

I hope someone will not report the same bug with a different title with 250 , and someone else solve the actual issue for 250 again at the mean time.

This will not happen. It is clearly your proposal, you have the comments above to prove it.

Yes , but the improvement will require deletion of the actual fix

If PRs could only add code and not remove existing code it would be a very different world 😆

No rush! Please do not start on this until we have 1) clarified what your exact proposal is, 2) Created a new issue and hired you.

[mvtglobally]

Issue not reproducible during KI retests. (Third week) @Julesssss Do we need to adjust any steps to repro this? I see you discuss that issue is persisting.

[Julesssss]

Hi @mvtglobally, please ignore this for now.

[botify]

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

• https://github.com/Expensify/App/pull/7209

If no regressions arise, payment will be issued on 2022-02-08. :confetti_ball:

[murataka]

I would like to continue discussion on slack about this topic. You can still continue here if you want.

I shall attach the test image to the next post.

[murataka]