firebase-android-sdk icon indicating copy to clipboard operation
firebase-android-sdk copied to clipboard

remove dismiss listener

Open s2g090123 opened this issue 3 years ago • 5 comments

There is no way to remove dismiss listeners. So I add this part at removeDismissListener and removeAllListeners.

s2g090123 avatar Aug 10 '22 02:08 s2g090123

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

google-cla[bot] avatar Aug 10 '22 02:08 google-cla[bot]

Hi @s2g090123. Thanks for your PR.

I'm waiting for a firebase member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

google-oss-bot avatar Aug 10 '22 02:08 google-oss-bot

@s2g090123 Can you expand your use case into detail? Why would you like to have the ability to remove dismiss listeners?

thatfiredev avatar Aug 17 '22 16:08 thatfiredev

Hi @thatfiredev. Thanks for your reply. I am using FirebaseInAppMessaging and add a dismiss listener in a activity. But I get a memory leak from LeakCanary when the activity is destroyed after rotating screen. And I would like to remove the dismiss listener in Activity#onDestroyed.

Below is my code (it's very simple)

class MainActivity : AppCompatActivity() {
    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContentView(R.layout.activity_main)
        FirebaseInAppMessaging.getInstance().addDismissListener {
            onDismiss()
        }
    }

    private fun onDismiss() {
        // handle onDismiss
    }
}

Leak trace in LeakCanary

┬─── │ GC Root: System class │ ├─ android.provider.FontsContract class │ Leaking: NO (MyApplication↓ is not leaking and a class is never leaking) │ ↓ static FontsContract.sContext ├─ com.example.inappmessagetest.MyApplication instance │ Leaking: NO (Application is a singleton) │ mBase instance of android.app.ContextImpl │ ↓ Application.mActivityLifecycleCallbacks │ ~~~~~~~~~~~~~~~~~~~~~~~~~~~ ├─ java.util.ArrayList instance │ Leaking: UNKNOWN │ Retaining 1.4 kB in 44 objects │ ↓ ArrayList.elementData │ ~~~~~~~~~~~ ├─ java.lang.Object[] array │ Leaking: UNKNOWN │ Retaining 1.3 kB in 43 objects │ ↓ Object[].[3] │ ~~~ ├─ com.google.firebase.inappmessaging.display.FirebaseInAppMessagingDisplay │ instance │ Leaking: UNKNOWN │ Retaining 953 B in 37 objects │ application instance of com.example.inappmessagetest.MyApplication │ ↓ FirebaseInAppMessagingDisplay.headlessInAppMessaging │ ~~~~~~~~~~~~~~~~~~~~~~ ├─ com.google.firebase.inappmessaging.FirebaseInAppMessaging instance │ Leaking: UNKNOWN │ Retaining 100.0 kB in 1502 objects │ ↓ FirebaseInAppMessaging.developerListenerManager │ ~~~~~~~~~~~~~~~~~~~~~~~~ ├─ com.google.firebase.inappmessaging.internal.DeveloperListenerManager instance │ Leaking: UNKNOWN │ Retaining 98.2 kB in 1461 objects │ ↓ DeveloperListenerManager.registeredDismissListeners │ ~~~~~~~~~~~~~~~~~~~~~~~~~~ ├─ java.util.HashMap instance │ Leaking: UNKNOWN │ Retaining 98.0 kB in 1457 objects │ ↓ HashMap.table │ ~~~~~ ├─ java.util.HashMap$Node[] array │ Leaking: UNKNOWN │ Retaining 98.0 kB in 1456 objects │ ↓ HashMap$Node[].[0] │ ~~~ ├─ java.util.HashMap$Node instance │ Leaking: UNKNOWN │ Retaining 97.9 kB in 1452 objects │ ↓ HashMap$Node.key │ ~~~ ├─ com.example.inappmessagetest.MainActivity$$ExternalSyntheticLambda0 instance │ Leaking: UNKNOWN │ Retaining 97.8 kB in 1450 objects │ f$0 instance of com.example.inappmessagetest.MainActivity with mDestroyed │ = true │ ↓ MainActivity$$ExternalSyntheticLambda0.f$0 │ ~~~ ╰→ com.example.inappmessagetest.MainActivity instance Leaking: YES (ObjectWatcher was watching this because com.example. inappmessagetest.MainActivity received Activity#onDestroy() callback and Activity#mDestroyed is true) Retaining 97.8 kB in 1449 objects

If you require any further information, feel free to contact me. Thanks!

s2g090123 avatar Aug 18 '22 02:08 s2g090123

@s2g090123 Thank you for explaining. I'll wait for one of the engineers on the In-App Messaging team to review this.

thatfiredev avatar Aug 18 '22 10:08 thatfiredev

Is minebase a failed token

Aslamxtc avatar Dec 16 '22 01:12 Aslamxtc

Is minebase a failed token

Aslamxtc avatar Dec 16 '22 01:12 Aslamxtc

API review Approved: b/262402247

eldhosembabu avatar Dec 22 '22 19:12 eldhosembabu

@s2g090123 could you please correct the Unit tests and other checks?

eldhosembabu avatar Dec 22 '22 19:12 eldhosembabu

Since there is some more additional changes needed, I've created a new PR copying your change with the additional changes here: https://github.com/firebase/firebase-android-sdk/pull/4492

So closing this PR.

eldhosembabu avatar Dec 22 '22 19:12 eldhosembabu