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
[expo-notifications] Fix exception sometimes being thrown if observer is cleared before it's registered #10640
Conversation
…k reference to the helper, leak the observer consciously
@@ -121,7 +121,7 @@ public static void addListener(NotificationManager listener) { | |||
} | |||
} | |||
|
|||
private boolean mIsAppInForeground = false; | |||
private boolean mIsAppInForeground = ProcessLifecycleOwner.get().getLifecycle().getCurrentState().isAtLeast(Lifecycle.State.RESUMED); |
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.
🙏
Hi @sjchmiela, thank you so much for the PR 🎉 I'm not an Android developer, so sorry if this question is dummy. I'm trying to test this PR but I'm getting the following error:
I tested adding the import but I get the same error.
Of course I created Sorry if this question is dummy and again, thank you so much for your support! |
Hey @RodolfoGS! Thank you so much for trying to test this PR! It it indeed related to Kotlin — on |
…vice delegate (#10666) # Why A prerequisite to single-threaded notifications handling. # How More refactoring, moving staff from `NotificationsHelper` to appropriate `NotificationsService`' delegates. This time to `ExpoHandlingDelegate`. In the meantime I have also removed a memory leak introduced in #10640 — we don't need to _observe_ the state, we can check for the state every time we need to access it. # Test Plan Push and scheduled notifications are presented.
Why
Probably fixes #10610.
How
Looked into the stacktrace, noticed that we may be trying to add a
null
observer to the registry (origin, the observer is saved in aWeakReference
).Since we really want the observer to be saved and we don't have infrastructure to notify
NotificationsHelper
of its destroy we need to leak the observer (a refactor removing need for this is currently in the works). If we need to leak the observer lets leak just as little as we can (just the observer, not the helper). So I created a simple class forwarding interesting calls to the helper.Test Plan
I have tested the code manually (verified that the callbacks are called correctly).