[Bug][LeakCanary] (regression) CoordinatorLayout leaked

[cadeyrn]

Steps to reproduce

1. Start Firefox.

Expected behavior

No leak.

Actual behavior

┬───
│ GC Root: Global variable in native code
│
├─ dalvik.system.PathClassLoader instance
│    Leaking: NO (GlobalAddonDependencyProvider↓ is not leaking and A ClassLoader is never leaking)
│    ↓ PathClassLoader.runtimeInternalObjects
├─ java.lang.Object[] array
│    Leaking: NO (GlobalAddonDependencyProvider↓ is not leaking)
│    ↓ Object[].[2385]
├─ mozilla.components.feature.addons.update.GlobalAddonDependencyProvider class
│    Leaking: NO (a class is never leaking)
│    ↓ static GlobalAddonDependencyProvider.addonManager
│                                           ~~~~~~~~~~~~
├─ mozilla.components.feature.addons.AddonManager instance
│    Leaking: UNKNOWN
│    ↓ AddonManager.store
│                   ~~~~~
├─ mozilla.components.browser.state.store.BrowserStore instance
│    Leaking: UNKNOWN
│    ↓ BrowserStore.subscriptions
│                   ~~~~~~~~~~~~~
├─ java.util.Collections$SetFromMap instance
│    Leaking: UNKNOWN
│    ↓ Collections$SetFromMap.m
│                             ~
├─ java.util.concurrent.ConcurrentHashMap instance
│    Leaking: UNKNOWN
│    ↓ ConcurrentHashMap.table
│                        ~~~~~
├─ java.util.concurrent.ConcurrentHashMap$Node[] array
│    Leaking: UNKNOWN
│    ↓ ConcurrentHashMap$Node[].[5]
│                               ~~~
├─ java.util.concurrent.ConcurrentHashMap$Node instance
│    Leaking: UNKNOWN
│    ↓ ConcurrentHashMap$Node.key
│                             ~~~
├─ mozilla.components.lib.state.Store$Subscription instance
│    Leaking: UNKNOWN
│    ↓ Store$Subscription.observer
│                         ~~~~~~~~
├─ mozilla.components.lib.state.ext.StoreExtensionsKt$channel$subscription$1 instance
│    Leaking: UNKNOWN
│    Anonymous subclass of kotlin.jvm.internal.Lambda
│    ↓ StoreExtensionsKt$channel$subscription$1.$channel
│                                               ~~~~~~~~
├─ kotlin.jvm.internal.Ref$ObjectRef instance
│    Leaking: UNKNOWN
│    ↓ Ref$ObjectRef.element
│                    ~~~~~~~
├─ kotlinx.coroutines.channels.ConflatedChannel instance
│    Leaking: UNKNOWN
│    ↓ ConflatedChannel.queue
│                       ~~~~~
├─ kotlinx.coroutines.internal.LockFreeLinkedListHead instance
│    Leaking: UNKNOWN
│    ↓ LockFreeLinkedListHead._next
│                             ~~~~~
├─ kotlinx.coroutines.channels.AbstractChannel$ReceiveHasNext instance
│    Leaking: UNKNOWN
│    ↓ AbstractChannel$ReceiveHasNext.cont
│                                     ~~~~
├─ kotlinx.coroutines.CancellableContinuationImpl instance
│    Leaking: UNKNOWN
│    ↓ CancellableContinuationImpl.delegate
│                                  ~~~~~~~~
├─ kotlinx.coroutines.internal.DispatchedContinuation instance
│    Leaking: UNKNOWN
│    ↓ DispatchedContinuation.callerFrame
│                             ~~~~~~~~~~~
├─ mozilla.components.lib.state.ext.FragmentKt$consumeFrom$1 instance
│    Leaking: UNKNOWN
│    Anonymous subclass of kotlin.coroutines.jvm.internal.SuspendLambda
│    ↓ FragmentKt$consumeFrom$1.$block
│                               ~~~~~~
├─ org.mozilla.fenix.home.HomeFragment$onViewCreated$1$5 instance
│    Leaking: UNKNOWN
│    Anonymous subclass of kotlin.jvm.internal.Lambda
│    ↓ HomeFragment$onViewCreated$1$5.this$0
│                                     ~~~~~~
├─ org.mozilla.fenix.home.HomeFragment$onViewCreated$1 instance
│    Leaking: UNKNOWN
│    Anonymous subclass of kotlin.jvm.internal.Lambda
│    ↓ HomeFragment$onViewCreated$1.$view
│                                   ~~~~~
╰→ androidx.coordinatorlayout.widget.CoordinatorLayout instance
​     Leaking: YES (ObjectWatcher was watching this because org.mozilla.fenix.home.HomeFragment received Fragment#onDestroyView() callback (references to its views should be cleared to prevent leaks))
​     key = e70350da-c15b-4dd5-a824-897aa38bca19
​     watchDurationMillis = 8826
​     retainedDurationMillis = 3825
​     mContext instance of org.mozilla.fenix.HomeActivity with mDestroyed = false
​     View#mParent is null
​     View#mAttachInfo is null (view detached)
​     View.mID = R.id.homeLayout
​     View.mWindowAttachCount = 0

METADATA

Build.VERSION.SDK_INT: 30
Build.MANUFACTURER: OnePlus
LeakCanary version: 2.4
App process name: org.mozilla.fenix.debug
Analysis duration: 6577 ms

Device information

• Device vendor / model and Android version: OnePlus 7T Pro McLaren Edition / Oxygen OS 11 (Android 11)
• Firefox for Android version: master branch revision d0a45ba

┆Issue is synchronized with this Jira Task

[mcarare]

This is reproducible even without changes from d0a45ba.

[cadeyrn]

@mcarare I noticed it as well that you don't have to open the new submenu. I edited the steps to reproduce shortly before your comment. I was tricked by the fact that LeakCanary needs a few seconds to report the leak and because I tested the new submenu it was always reported when I opened the menu. But in fact it's already enough to start Firefox and not related to the new submenu. 😅

[mcarare]

After further investigation, this only happens when having opened tabs, and the HomeFragment is destroyed and the app navigates to browser fragment after cold start.

@mcomella Although the info from LeakCanary points to another direction, this leak seems to originate from measuring the duration of HomeFragment.onViewCreated.
funcToMeasure also contains a consumeFrom that keeps a reference to the HomeFragment view:

fenix/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt

Lines 412 to 413 in 62e2fb1

	consumeFrom(requireComponents.core.store) {
	updateTabCounter(it)

This can be fixed using different approaches:

• exclude the tab counter updating from measurement,
• create a new measurement specifically for this update and measure it separately from the rest of the code in HomeFragment.onViewCreated
• create a mechanism where measurement takes into account the view lifecycle scope.

I am not familiar with how these measurements are used so I am leaving this decision to you.

[mcomella]

Triage: action item is to check out the code before and after the probe is added (for theory from #18596 (comment)) and see if we can reproduce. If not, ask csadilek. If so, maybe implement probe an alternative way.

[mcomella]

Stand-up: let's do the quick solution: change to start/stop. Unfortunately, this means we won't identify the root cause (unless it continues) but we don't think understanding the root cause is a good use of our resources right now as we have been fixing a lot of bugs recently and generally not making perf improvements.

[mcomella]

I've been able to reproduce this on a local build from early April 6e5b4b3 with a slightly different STR. Something similar to:

• Launch the app view app link
• Press back to close the app (and the current tab)
• Open the app via the home screen icon
• Press back twice (once to the homescreen, once to close the app)
• Open the app

Wait a few seconds. Repeat if it didn't work, sometimes ending the process.

[mcomella]

I don't know what changed but I'm struggling to reproduce this in the latest master with a local build 3d22642. I'm not sure what changed. @cadeyrn Are you still able to reproduce this on the lastest master?

I could make the speculative fix suggested in #18596 (comment) or we can wait until we see this again.

[cadeyrn]

@mcomella I am no longer able to reproduce the leak. According to LeakCanary I saw the last leak on April, 13th (I don't compile a new build every day so it doesn't mean that the leak was fixed exactly on this day).

[mcomella]

@cadeyrn Thanks for the update. I'm wondering what fixed it… 🤔 I'm going to close this but please reopen if you see the leak again! At that point, maybe we can retry the speculative fix I made as it seemed to work for earlier commits.