You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
[$1000] Attachments- App crashes when user navigate attachments #24337
👋 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:
Identify the pull request that introduced this issue and revert it.
I am also unable to reproduce this issue. Can we get more specifics on the environment like the version of Android being tested on? Also, if we could get the exact same set of images used to test, that may prove to be helpful. Thanks.
Hey, it might be related to this issue.
Your workspace name contains HTML characters (&), which is also the cause of the mentioned issue. More details in my proposal.
Can you try to reproduce it on a new workspace, with a name without any special characters?
If so, it should be reproducible on production as well and not deploy blocker.
You are right. It was just a guess. I'm sorry for the confusion.
@Li357 Issue is still reproducible on Android
I have added an attachment that is causing the app to crash and there is one attachment that needs to be added that is larger than 10MB
changed the title [-][$500] Attachments- App crashes when user navigate attachments[/-][+][$1000] Attachments- App crashes when user navigate attachments[/+]on Nov 13, 2023
I spent some time trying to figure this out and was finally able to reproduce (you have to test with the attached image in #24337 (comment)). The crash is due to an out of memory error due to the number of pixels in the image, I think it's some insanely huge number (9000x9000, iirc but it may be higher). The solution would probably be to try to scale the image down before displaying it.
This was proposed as solution with 31204 that can fix crash with reproducible image attached
Proposal
Please re-state the problem that we are trying to solve in this issue.
App crashes after attempting to view a large uploaded image attachment that is beyond the size supported for the device.
What is the root cause of that problem?
The image size (below), which is 7724 x 5148, is too large for the rendering on the screen.
This causes the app to crash with the following in logcat on Android:
Canvas: trying to draw too large(159052608bytes) bitmap.
This image is too large for the container it is being rendered on the device.
What changes do you think we should make in order to solve the problem?
Make sure the image height and size are not out of bounds (for the canvas size) what is supported by the device display within AttachmentCarouselPage
Within AttachmentCarouselPage, the components' onLoad should size dimensions similar to existing approach in ImageView. This component is used for the initial image preview during adding the attachment:
// Resize the image to max dimensions possible on the Native platforms to prevent crashes on Android. To keep the same behavior, apply to IOS as well.
const maxDimensionsScale = 11;
imageZoomWidth = Math.min(imageZoomWidth, roundedContainerWidth * maxDimensionsScale);
imageZoomHeight = Math.min(imageZoomHeight, roundedContainerHeight * maxDimensionsScale);
What alternative solutions did you explore? (Optional)
Additionally, if there are an intended maximum dimensions for general image attachments this validation can be added before the image is uploaded. (personally don't like this solution unless there is a different need to scale down images)
In AttachmentPicker, we can add a function to check the size dimensions for an image (can check by height, width, and overall size):
@giltron There should be no need for this change anymore. We're completely overhauling the native ImageView component with a new MultiGestureCanvas/Lightbox component in this PR.
The crash with large images has also been fixed by a patch: #28159 (comment)
Activity
OSBotify commentedon Aug 9, 2023
👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open
StagingDeployCashdeploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:melvin-bot commentedon Aug 9, 2023
Triggered auto assignment to @Li357 (
Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.puneetlath commentedon Aug 9, 2023
@lanitochka17 is this an android-only issue? I'm not able to reproduce it on iOS.
akinwale commentedon Aug 10, 2023
I am also unable to reproduce this issue. Can we get more specifics on the environment like the version of Android being tested on? Also, if we could get the exact same set of images used to test, that may prove to be helpful. Thanks.
kosmydel commentedon Aug 10, 2023
Hey, it might be related to this issue.
Your workspace name contains HTML characters (
&), which is also the cause of the mentioned issue. More details in my proposal.Can you try to reproduce it on a new workspace, with a name without any special characters?
aimane-chnaif commentedon Aug 10, 2023
If so, it should be reproducible on production as well and not deploy blocker.
kosmydel commentedon Aug 10, 2023
You are right. It was just a guess. I'm sorry for the confusion.
Li357 commentedon Aug 10, 2023
Can't reproduce on iOS and Android either. I'm going to close this out for now, if you can provide extra details that would be great!
lanitochka17 commentedon Sep 21, 2023
@Li357 Issue is still reproducible on Android
I have added an attachment that is causing the app to crash and there is one attachment that needs to be added that is larger than 10MB
Screen_Recording_20230921_135617_New.Expensify.mp4
56 remaining items
[-][$500] Attachments- App crashes when user navigate attachments[/-][+][$1000] Attachments- App crashes when user navigate attachments[/+]melvin-bot commentedon Nov 13, 2023
Upwork job price has been updated to $1000
Li357 commentedon Nov 13, 2023
Bounty increased, I think this is valuable to work on because images get to this size pretty easily
chrispader commentedon Nov 15, 2023
@ArekChr was working on a PR in
react-native-fast-imagea few months ago, which should fix this problem by downsampling images.@ArekChr do you fix your PR could fix this problem and what's the status on the PR? :)
mvtglobally commentedon Nov 18, 2023
Issue not reproducible during KI retests. (First week)
melvin-bot commentedon Nov 20, 2023
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
giltron commentedon Nov 20, 2023
This was proposed as solution with 31204 that can fix crash with reproducible image attached
Proposal
Please re-state the problem that we are trying to solve in this issue.
App crashes after attempting to view a large uploaded image attachment that is beyond the size supported for the device.
What is the root cause of that problem?
The image size (below), which is 7724 x 5148, is too large for the rendering on the screen.
This causes the app to crash with the following in logcat on Android:
This image is too large for the container it is being rendered on the device.
What changes do you think we should make in order to solve the problem?
Make sure the image height and size are not out of bounds (for the canvas size) what is supported by the device display within AttachmentCarouselPage
Within AttachmentCarouselPage, the
components' onLoad should size
dimensionssimilar to existing approach in ImageView. This component is used for the initial image preview during adding the attachment:The solution is to adjust the imageHeight and imageWidth calculation within the onLoad events in [AttachmentCarouselPage](https://github.com/Expensify/App/blob/5e7022d8cc519ab8787aa0e0d70d5692c6c27945/src/components/Attachments/AttachmentCarousel/Pager/AttachmentCarouselPage.js( with similar approach as ImageView. AttachmentCarouselPage to ensure the maximum dimensions are not too large for the canvas:
What alternative solutions did you explore? (Optional)
Additionally, if there are an intended maximum dimensions for general image attachments this validation can be added before the image is uploaded. (personally don't like this solution unless there is a different need to scale down images)
In
AttachmentPicker, we can add a function to check the size dimensions for an image (can check by height, width, and overall size):For native Android/iOS, we can also configure react-native-image-picker to automatically resize the image:
For informing the user, a similar message could be displayed (if it is not desirable to automatically resize) as with the Avatar image upload
chrispader commentedon Nov 21, 2023
@giltron There should be no need for this change anymore. We're completely overhauling the native
ImageViewcomponent with a newMultiGestureCanvas/Lightboxcomponent in this PR.The crash with large images has also been fixed by a patch: #28159 (comment)
cc @akinwale @Li357 @mvtglobally
mvtglobally commentedon Nov 23, 2023
Issue not reproducible during KI retests. (Second week)
melvin-bot commentedon Nov 27, 2023
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
Li357 commentedon Nov 27, 2023
Awesome @chrispader! I'll close this out then!