-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix(messaging, android): prevent OOM by limiting stored notifications #8771
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@sean5940 is attempting to deploy a commit to the Invertase Team on Vercel. A member of the Team first needs to authorize it. |
mikehardy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned in more detail in specific comments but a summary:
1- I really like the idea of making the serialized notification count configurable
But, I believe strongly this should use our existing code though, and if you want to make existing boot-crash-loop devices recoverable you'll need to add error handling to that common code
2- I do not believe we should change the default, we should avoid a potentially breaking change
3- I like the trim-until-we-are-the-value vs one-at-a-time change
I think there's the core of something that can go in here if you work through the info I put out for item 1 here in this list
| private static final String S_KEY_ALL_NOTIFICATION_IDS = "all_notification_ids"; | ||
| private static final int MAX_SIZE_NOTIFICATIONS = 100; | ||
| private final String DELIMITER = ","; | ||
| private static int MAX_SIZE_NOTIFICATIONS = 20; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe changing this is a good idea - not for 100% technical reasons - 100 may not be practical for every case - you obviously have an OOM so this is critical to address somehow - but this feature has existed for 5 years (ref: sean5940@a7cafc9) and you are the first report of a problem.
So while 100 may be impractical sometimes, it's once in 5 years out of maybe tens of thousands of apps and millions and millions of users.
Weigh that versus even the slim possibility of some app relying on this behavior (or some unknown amount less than 100 and greater than 20) and that implies this would be a breaking change.
I don't think a fix here is worth a breaking change given it has existed for 5 years without any other issues.
So I am a "no" on changing this constant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to maintain a maximum value of 100, based on comments
41ca8ef
...oid/src/main/java/io/invertase/firebase/messaging/ReactNativeFirebaseMessagingStoreImpl.java
Outdated
Show resolved
Hide resolved
...oid/src/main/java/io/invertase/firebase/messaging/ReactNativeFirebaseMessagingStoreImpl.java
Outdated
Show resolved
Hide resolved
...oid/src/main/java/io/invertase/firebase/messaging/ReactNativeFirebaseMessagingStoreImpl.java
Outdated
Show resolved
Hide resolved
While preferences.remove(firstRemoteMessageId) is invoked when the notification list exceeds MAX_SIZE_NOTIFICATIONS, the editor never calls apply()/commit(). 2025-11-25.10.53.27.mov |
|
@sean5940 your analysis appears correct, regardless of the other feedback I gave, your conversion of the "if" to a "while", and adding an |
|
@sean5940 I can merge and release the PR quickly, but review always helps. If you could take a look? I would still like to pursue the idea of making the storage limit configurable here, and working through the idea of OOM recovery by catching an OOM error. Although I think the 5th commit in my PR - "purge old messages before storing new" may actually recover people sufficiently,. |
|
This was mostly superceded by #8776 which I wanted to get out as soon as possible because it was in "crash fix" territory The remaining part of this is the configurable maximum message parameter - what do you think @sean5940 is it still worth it to implement that, or do you want to see if #8776 clears out the problem? |
One concern I have is that the shared preferences file is already extremely large, which significantly increases the risk of OOM errors. I’m exploring ways to deal with this — do you have any suggestions on how we might mitigate the issue? For example, if we assume a situation where the XML file manages only 100 IDs while several thousand FCM messages are stored, the cleanup process can operate only on those 100 known IDs. As a result, any data associated with IDs outside that range cannot be deleted. I’m not seeing a clear way to address this at the library level. In this situation, would it make sense for the app to check the file size in Application.onCreate() and delete it when it becomes abnormally large? |
c5caa92 to
fae6843
Compare
e86d62a to
56553db
Compare
|
🤔
Don't send huge data chunks in your messages, have the data in some other location and send unambiguous references to it, for fetch after getting the notification? But that's a design issue and not really your question
Ah, but if you mean that somehow the "accounting" (that is, the assumption in the code that each and every actual message stored has an id stored as well, with no ids for messages that don't exist and no messages without an id entry) is broken then I can see what you mean and we would need some sort of cleanup process. Indeed, this is the easiest to understand representation of the logic that is currently deployed it is after my refactor for clarity but before any logic changes from the PR I just merged Lines 31 to 43 in 2ca86df
So, 1- store the message as messageId + contents key/value pair We can assume that 1 may work, and 2 may fail which would lead to an "accounting" failure and an orphaned message yes. I believe a combination of However, the Judging from your recording above, a regex like (untested...) Then you can do a look up in the notificationIds list, and if it is not there, This would be a positive change in my opinion, if you implemented it I would be happy to merge it. If you didn't I would probably implement it myself but with a considerable delay - it could be weeks |
mikehardy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the config change - nice to give people control over this one
As mentioned I don't think a clamp is a positive move but that's an easy change
And if you wanted to implement an all-messageIds-iterator in a separate method like preenMessageStorage or similar then call it before storing a new message, I'd be open to that
...oid/src/main/java/io/invertase/firebase/messaging/ReactNativeFirebaseMessagingStoreImpl.java
Outdated
Show resolved
Hide resolved
…er review - Rename config key from 'rn_firebase_messaging_max_stored_notifications' to 'messaging_max_stored_notifications' for consistency - Remove clamping logic (min/max restrictions) to allow developers full control - Add logging for final setting value and source (SharedPreferences/firebase.json/AndroidManifest/default) for diagnostics - Update firebase-schema.json: remove minimum/maximum constraints and update description - Update tests/firebase.json to use new key name Addresses review feedback: developers should have full control over the limit without artificial restrictions, with appropriate warnings in documentation.
|
@mikehardy Key changes:
|
…etAll() The getAll() method was only handling null, String, and Boolean values when converting metadata to a WritableMap. With the new getIntValue() method supporting integer configuration values (e.g., messaging_max_stored_notifications), Integer values in metadata were being silently skipped. Added Integer type check to properly include Integer values in the returned WritableMap.
Description
This PR addresses an
OutOfMemoryError(OOM) crash on Android caused byReactNativeFirebaseMessagingStoreImplstoring up to 100 notifications inSharedPreferences.When a device receives many notifications with large payloads, the
SharedPreferencesfile grows significantly. SinceSharedPreferencesloads the entire file into memory, this can exceed the heap size on some devices, leading to a crash.Related Issue:
Closes #8770
Changes
Reduced Default Limit: The default
MAX_SIZE_NOTIFICATIONShas been reduced from 100 to 20.getInitialNotificationtypically only needs the single notification that launched the app. A buffer of 20 is sufficient for most use cases while significantly reducing memory pressure.Configurable Limit: Added support for configuring the limit via
AndroidManifest.xml.rn_firebase_messaging_max_stored_notificationsin theirAndroidManifest.xmlto adjust this limit without forking the library.Safety Cap: Added a hard cap of 100 to the configurable limit.
Aggressive Cleanup: Changed the cleanup logic from an
ifstatement to awhileloop.whileloop ensures the storage is immediately drained down to the limit upon receiving the next message.Verification
getInitialNotificationstill functions correctly with the reduced limit.AndroidManifest.xml.whileloop correctly cleans up excess notifications.Stack Trace (Reference)