-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -119,6 +119,12 @@ | |||||||||
| "description": "On iOS, indicating how to present a notification in a foreground app.", | ||||||||||
| "type": "array" | ||||||||||
| }, | ||||||||||
| "rn_firebase_messaging_max_stored_notifications": { | ||||||||||
| "description": "Controls how many notifications are retained on-device for background storage. Priority order is SharedPreferences -> firebase.json -> AndroidManifest. Values outside the 20-100 range are clamped to prevent excessive deletions or OOM.", | ||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not believe in restricting developers capabilities without reason. Someone may have reason to set it to zero for zero data retention for some reason. Someone may have reason to set it to 1000 (small messages!) for their particular use case. Our job is to pick a sensible default then allow people to do what they want, and just expose the risks. 100 was already selected as a reasonable default for most cases (and as mentioned has caused no problems for 5 years...) so we have done that. The rest is up to developers.
Suggested change
|
||||||||||
| "type": "number", | ||||||||||
| "minimum": 20, | ||||||||||
| "maximum": 100 | ||||||||||
|
Comment on lines
+124
to
+126
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
| }, | ||||||||||
| "perf_auto_collection_enabled": { | ||||||||||
| "description": "Disable or enable auto collection of performance monitoring data collection.\n This is useful for opt-in-first data flows, for example when dealing with GDPR compliance.\nThis can be overridden in JavaScript.", | ||||||||||
| "type": "boolean" | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,31 +8,66 @@ | |
| import com.facebook.react.bridge.ReadableMap; | ||
| import com.facebook.react.bridge.WritableMap; | ||
| import com.google.firebase.messaging.RemoteMessage; | ||
| import io.invertase.firebase.common.UniversalFirebasePreferences; | ||
|
|
||
| import org.json.JSONException; | ||
| import org.json.JSONObject; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.Arrays; | ||
| import java.util.List; | ||
| import org.json.JSONException; | ||
| import org.json.JSONObject; | ||
|
|
||
| import io.invertase.firebase.common.ReactNativeFirebaseJSON; | ||
| import io.invertase.firebase.common.ReactNativeFirebaseMeta; | ||
| import io.invertase.firebase.common.ReactNativeFirebasePreferences; | ||
| import io.invertase.firebase.common.UniversalFirebasePreferences; | ||
|
|
||
| public class ReactNativeFirebaseMessagingStoreImpl implements ReactNativeFirebaseMessagingStore { | ||
|
|
||
| private static final String KEY_MAX_STORED_NOTIFICATIONS = "rn_firebase_messaging_max_stored_notifications"; | ||
| 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 final int DEFAULT_MAX_SIZE_NOTIFICATIONS = 100; | ||
| private static final int MIN_SIZE_NOTIFICATIONS = 20; | ||
|
Comment on lines
-21
to
+30
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned in the edit for the key description - please do not restrict developers via "good intentions" as those are frequently just our idea of what a developer needs but sometimes restrict valid use cases. Just maintain the 100 original default, take the value in from configuration (if set) and log the final setting and source it came from for diagnostics just in case. No clamping |
||
| private static final int maxNotificationSize = resolveMaxNotificationSize(); | ||
|
|
||
| private static int resolveMaxNotificationSize() { | ||
| int maxSize = DEFAULT_MAX_SIZE_NOTIFICATIONS; | ||
| ReactNativeFirebaseJSON json = ReactNativeFirebaseJSON.getSharedInstance(); | ||
| ReactNativeFirebaseMeta meta = ReactNativeFirebaseMeta.getSharedInstance(); | ||
| ReactNativeFirebasePreferences prefs = ReactNativeFirebasePreferences.getSharedInstance(); | ||
|
|
||
| try { | ||
| // Priority: SharedPreferences -> firebase.json -> AndroidManifest | ||
| // Note: ReactNativeFirebaseMeta doesn't have getIntValue, so we check meta.contains | ||
| // and read from AndroidManifest directly if needed | ||
| if (prefs.contains(KEY_MAX_STORED_NOTIFICATIONS)) { | ||
| maxSize = prefs.getIntValue(KEY_MAX_STORED_NOTIFICATIONS, DEFAULT_MAX_SIZE_NOTIFICATIONS); | ||
| } else if (json.contains(KEY_MAX_STORED_NOTIFICATIONS)) { | ||
| maxSize = json.getIntValue(KEY_MAX_STORED_NOTIFICATIONS, DEFAULT_MAX_SIZE_NOTIFICATIONS); | ||
| } else if (meta.contains(KEY_MAX_STORED_NOTIFICATIONS)) { | ||
| maxSize = meta.getIntValue(KEY_MAX_STORED_NOTIFICATIONS, DEFAULT_MAX_SIZE_NOTIFICATIONS); | ||
| } | ||
|
|
||
| // Clamp to allowed range to avoid OOM (upper) or mass deletions (lower) | ||
| return Math.max(MIN_SIZE_NOTIFICATIONS, Math.min(maxSize, DEFAULT_MAX_SIZE_NOTIFICATIONS)); | ||
| } catch (Exception e) { | ||
| // Ignore and use default | ||
| return DEFAULT_MAX_SIZE_NOTIFICATIONS; | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void storeFirebaseMessage(RemoteMessage remoteMessage) { | ||
| try { | ||
| String remoteMessageString = | ||
| reactToJSON(remoteMessageToWritableMap(remoteMessage)).toString(); | ||
| reactToJSON(remoteMessageToWritableMap(remoteMessage)).toString(); | ||
| // Log.d("storeFirebaseMessage", remoteMessageString); | ||
| UniversalFirebasePreferences preferences = UniversalFirebasePreferences.getSharedInstance(); | ||
|
|
||
| // remove old notifications message before store to free space as needed | ||
| String notificationIds = preferences.getStringValue(S_KEY_ALL_NOTIFICATION_IDS, ""); | ||
| List<String> allNotificationList = convertToArray(notificationIds); | ||
| while (allNotificationList.size() > MAX_SIZE_NOTIFICATIONS - 1) { | ||
| while (allNotificationList.size() > maxNotificationSize - 1) { | ||
| clearFirebaseMessage(allNotificationList.get(0)); | ||
| allNotificationList.remove(0); | ||
| } | ||
|
|
@@ -61,7 +96,7 @@ public RemoteMessage getFirebaseMessage(String remoteMessageId) { | |
| @Override | ||
| public WritableMap getFirebaseMessageMap(String remoteMessageId) { | ||
| String remoteMessageString = | ||
| UniversalFirebasePreferences.getSharedInstance().getStringValue(remoteMessageId, null); | ||
| UniversalFirebasePreferences.getSharedInstance().getStringValue(remoteMessageId, null); | ||
| if (remoteMessageString != null) { | ||
| // Log.d("getFirebaseMessage", remoteMessageString); | ||
| try { | ||
|
|
||
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.
we prefix with the specific name traditionally - it is reasonable to argue that since SharedPreferences is a global namespace we should be more specific but we're using
messaging_so far for messaging items, and consistency does have value