fix(MM-62375): OOM on gif #8573

rahimrahman · 2025-02-08T18:48:05Z

Summary

Ticket Link

Fixes: https://mattermost.atlassian.net/browse/MM-62375

See changes in my own fork of APNG4Android (until I have permission to post to mattermost/APNG4Android): rahimrahman/APNG4Android@74dd31c

Checklist

Added or updated unit tests (required for all new features)
Has UI changes
Includes text changes and localization file updates
Have tested against the 5 core themes to ensure consistency between them.
Have run E2E tests by adding label E2E iOS tests for PR.

Device Information

This PR was tested on:

Google Pixel 6a, Android 14

Screenshots

Release Note

rahimrahman · 2025-02-08T18:54:23Z

patches/expo-image+2.0.4.patch

+  implementation project(':frameanimation')
+  implementation project(':gif')
+  implementation("com.github.penfeizhou.android.animation:glide-plugin:3.0.2") {
+    exclude group: 'com.github.penfeizhou.android.animation', module: 'frameanimation'
+    exclude group: 'com.github.penfeizhou.android.animation', module: 'gif'
+  }


The fixes are in frameanimation & gif.

Now in this PR (that will not get merged).

rahimrahman · 2025-02-08T18:55:39Z

scripts/postinstall.sh

+APNG4Android="node_modules/APNG4Android"
+if [ ! -z "$APNG4Android" ]; then
+    git clone -b "fix/MM-62375-oom-on-gif" --single-branch https://github.com/rahimrahman/APNG4Android.git "$APNG4Android";
+fi


Can't do npm i since APNG4Android doesn't have package.json.

TODO: will replace with mattermost/APNG4Android (as soon as permission to write is granted)

TODO: figure out if I can use SHA instead of branch.

No you cannot clone a specific commit, but you can clone the branch and then do a checkout

You could also use --depth=1 so that it only clones the latest one

rahimrahman · 2025-02-08T18:56:31Z

android/settings.gradle

+include ':frameanimation'
+project(':frameanimation').projectDir = new File('../node_modules/APNG4Android/frameanimation')
+include ':gif'
+project(':gif').projectDir = new File('../node_modules/APNG4Android/gif')


In order to get this as project in expo-image/android/build.gradle, need to declare frameanimation and gif here.

rahimrahman · 2025-02-08T18:57:37Z

patches/expo-image+2.0.4.patch

+    exclude group: 'com.github.penfeizhou.android.animation', module: 'frameanimation'
+    exclude group: 'com.github.penfeizhou.android.animation', module: 'gif'


Without exclusions, it will cause duplicate symbols error. Since glide-plugin will have frameanimation (required module) in other modules (apng, webp, etc).

rahimrahman · 2025-02-08T20:38:29Z

.github/actions/prepare-node-deps/action.yaml

+      run: |
+        echo "::group::clone-APNG4Android"
+        if [ ! -d "$APNG4Android" ]; then
+          git clone -b "fix/MM-62375-oom-on-gif" --single-branch https://github.com/rahimrahman/APNG4Android.git "$APNG4Android";


Is there any benefit of putting it into node_modules instead of a custom folder on the root path of the app?

TODO: Will replace this with the correct repo (mattermost/APNG4Android)

Irrelevant in my opinion, but of it works here no need to change

I like node_modules because it's easier to do the npm run clean and it'll auto-clean this library as well. But I could also have added an extra line to clean the folder wherever it might be in scripts/clean.sh as well.

I'll stick with node_modules.

enahum · 2025-02-10T14:42:27Z

package.json

@@ -54,7 +54,7 @@
    "expo-application": "6.0.1",
    "expo-crypto": "14.0.1",
    "expo-device": "7.0.1",
-    "expo-image": "2.0.3",
+    "expo-image": "2.0.4",


Any reason to bump this? Just curious

I made a mistake when reading the changelog. And looking at the changes, I thought the addition of try..catch was going to help, but it didn't. Looking deeper, I noticed that it was already implemented in 2.0.3, not a new change in 2.0.4.

Changelog for 2.0.4:

## 2.0.4 — 2025-01-10 _This version does not introduce any user-facing changes._

So it's pretty moot.

enahum · 2025-02-10T14:48:28Z

scripts/postinstall.sh

+APNG4Android="node_modules/APNG4Android"
+if [ ! -z "$APNG4Android" ]; then
+    git clone -b "fix/MM-62375-oom-on-gif" --single-branch https://github.com/rahimrahman/APNG4Android.git "$APNG4Android";
+fi


No you cannot clone a specific commit, but you can clone the branch and then do a checkout

enahum · 2025-02-10T14:49:13Z

scripts/postinstall.sh

+APNG4Android="node_modules/APNG4Android"
+if [ ! -z "$APNG4Android" ]; then
+    git clone -b "fix/MM-62375-oom-on-gif" --single-branch https://github.com/rahimrahman/APNG4Android.git "$APNG4Android";
+fi


You could also use --depth=1 so that it only clones the latest one

rahimrahman · 2025-02-10T18:24:56Z

@yasserfaraazkhan I'm flagging you for QA review, but @enzowritescode has done the test with his Android device and ✅ it in our private DM.

You're welcome to test this as well for redundancy.

rahimrahman · 2025-02-10T18:53:56Z

@yasserfaraazkhan I take that back. I think you should run this app on Android, and find or create channel with lots of images (gif specifically, and animated gif if possible or both). Particularly as big images resolution as you can post (starting with 100x100 to as high as you can think of). I'm curious if the images will show up at all.

Ugh.

yasserfaraazkhan · 2025-02-11T06:57:55Z

Thank you @rahimrahman I could verify the changes work.
I saw current PR display the gifs without crashing.

rahimrahman · 2025-02-11T16:16:09Z

@amyblais Hi. Since this is a security vuln issue, it should be included in 2.26. I await @enzowritescode to either move with the go ahead to have this as a patch 2.25 and to be release immediately.

rahimrahman · 2025-02-11T21:15:31Z

@amyblais based on this discussion, I think we'd need to cherry-pick this as a 2.25.1 as well.

mattermost-build · 2025-02-12T15:18:57Z

Cherry pick is scheduled.

* fix(MM-62375): OOM on gif * add APNG4Android into github action script to download * change to mattermost/APNG4Android (cherry picked from commit 985fa69)

* fix(MM-62375): OOM on gif * add APNG4Android into github action script to download * change to mattermost/APNG4Android (cherry picked from commit 985fa69) Co-authored-by: Rahim Rahman <rahim.rahman@mattermost.com>

amyblais · 2025-02-14T08:45:14Z

/cherry-pick release-2.25

mattermost-build · 2025-02-14T08:45:17Z

Cherry pick is scheduled.

* fix(MM-62375): OOM on gif * add APNG4Android into github action script to download * change to mattermost/APNG4Android (cherry picked from commit 985fa69)

* fix(MM-62375): OOM on gif * add APNG4Android into github action script to download * change to mattermost/APNG4Android (cherry picked from commit 985fa69) Co-authored-by: Rahim Rahman <rahim.rahman@mattermost.com>

* fix(MM-62375): OOM on gif * add APNG4Android into github action script to download * change to mattermost/APNG4Android

fix(MM-62375): OOM on gif

12b3757

mm-cloud-bot added kind/bug Categorizes issue or PR as related to a bug. release-note labels Feb 8, 2025

rahimrahman commented Feb 8, 2025

View reviewed changes

rahimrahman requested review from enahum and larkox February 8, 2025 18:58

rahimrahman added the Build Apps for PR Build the mobile app for iOS and Android to test label Feb 8, 2025

add APNG4Android into github action script to download

ea591bf

rahimrahman added Build App for Android Build the mobile app for Android and removed Build Apps for PR Build the mobile app for iOS and Android to test labels Feb 8, 2025

rahimrahman commented Feb 8, 2025

View reviewed changes

rahimrahman added Build App for Android Build the mobile app for Android and removed Build App for Android Build the mobile app for Android labels Feb 9, 2025

rahimrahman requested a review from enzowritescode February 9, 2025 17:23

larkox approved these changes Feb 10, 2025

View reviewed changes

enahum approved these changes Feb 10, 2025

View reviewed changes

rahimrahman added the 3: Security Review Review requested from Security Team label Feb 10, 2025

rahimrahman mentioned this pull request Feb 10, 2025

fix(MM-62375) OOM on gif with build updates mattermost/APNG4Android#1

Open

change to mattermost/APNG4Android

24cf5bd

rahimrahman added Build Apps for PR Build the mobile app for iOS and Android to test and removed Build App for Android Build the mobile app for Android labels Feb 10, 2025

enzowritescode approved these changes Feb 10, 2025

View reviewed changes

amyblais added this to the v2.26.0 milestone Feb 11, 2025

amyblais added the CherryPick/Approved Meant for the quality or patch release tracked in the milestone label Feb 11, 2025

amyblais removed the 3: Security Review Review requested from Security Team label Feb 12, 2025

rahimrahman merged commit 985fa69 into main Feb 12, 2025
24 checks passed

rahimrahman deleted the fix/MM-62375-OOM-on-gif branch February 12, 2025 15:18

mattermost-build pushed a commit that referenced this pull request Feb 12, 2025

fix(MM-62375): OOM on gif (#8573)

c8b1037

* fix(MM-62375): OOM on gif * add APNG4Android into github action script to download * change to mattermost/APNG4Android (cherry picked from commit 985fa69)

mattermost-build mentioned this pull request Feb 12, 2025

Automated cherry pick of #8573 #8586

Merged

mattermost-build added CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone and removed CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Feb 12, 2025

mattermost-build pushed a commit that referenced this pull request Feb 14, 2025

fix(MM-62375): OOM on gif (#8573)

dd41c67

* fix(MM-62375): OOM on gif * add APNG4Android into github action script to download * change to mattermost/APNG4Android (cherry picked from commit 985fa69)

mattermost-build mentioned this pull request Feb 14, 2025

Automated cherry pick of #8573 #8589

Merged

Willyfrog pushed a commit that referenced this pull request Feb 19, 2025

fix(MM-62375): OOM on gif (#8573)

6a696be

* fix(MM-62375): OOM on gif * add APNG4Android into github action script to download * change to mattermost/APNG4Android

Rajat-Dabade pushed a commit that referenced this pull request Feb 25, 2025

fix(MM-62375): OOM on gif (#8573)

f54c2db

* fix(MM-62375): OOM on gif * add APNG4Android into github action script to download * change to mattermost/APNG4Android

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(MM-62375): OOM on gif #8573

fix(MM-62375): OOM on gif #8573

rahimrahman commented Feb 8, 2025

rahimrahman Feb 8, 2025 •

edited

Loading

rahimrahman Feb 8, 2025 •

edited

Loading

enahum Feb 10, 2025

enahum Feb 10, 2025

rahimrahman Feb 8, 2025

rahimrahman Feb 8, 2025

rahimrahman Feb 8, 2025 •

edited

Loading

enahum Feb 10, 2025

rahimrahman Feb 10, 2025 •

edited

Loading

enahum Feb 10, 2025

rahimrahman Feb 10, 2025

enahum Feb 10, 2025

enahum Feb 10, 2025

rahimrahman commented Feb 10, 2025

rahimrahman commented Feb 10, 2025 •

edited

Loading

yasserfaraazkhan commented Feb 11, 2025

rahimrahman commented Feb 11, 2025

rahimrahman commented Feb 11, 2025

mattermost-build commented Feb 12, 2025

amyblais commented Feb 14, 2025

mattermost-build commented Feb 14, 2025

		+ exclude group: 'com.github.penfeizhou.android.animation', module: 'frameanimation'
		+ exclude group: 'com.github.penfeizhou.android.animation', module: 'gif'

fix(MM-62375): OOM on gif #8573

fix(MM-62375): OOM on gif #8573

Conversation

rahimrahman commented Feb 8, 2025

Summary

Ticket Link

Checklist

Device Information

Screenshots

Release Note

rahimrahman Feb 8, 2025 • edited Loading

Choose a reason for hiding this comment

rahimrahman Feb 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahimrahman Feb 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahimrahman Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahimrahman commented Feb 10, 2025

rahimrahman commented Feb 10, 2025 • edited Loading

yasserfaraazkhan commented Feb 11, 2025

rahimrahman commented Feb 11, 2025

rahimrahman commented Feb 11, 2025

mattermost-build commented Feb 12, 2025

amyblais commented Feb 14, 2025

mattermost-build commented Feb 14, 2025

rahimrahman Feb 8, 2025 •

edited

Loading

rahimrahman Feb 8, 2025 •

edited

Loading

rahimrahman Feb 8, 2025 •

edited

Loading

rahimrahman Feb 10, 2025 •

edited

Loading

rahimrahman commented Feb 10, 2025 •

edited

Loading