Leak in SpellCheckerSession. about leakcanary HOT 19 CLOSED

Cool, I thought it was a motorola only thing: https://github.com/square/leakcanary/blob/master/library/leakcanary-android/src/main/java/com/squareup/leakcanary/AndroidExcludedRefs.java#L216

Next steps:

1. figure out what the leak is exactly in AOSP
2. File a bug on Android
3. (Optional) find a way to fix the leak in apps on existing versions of Android
4. Update AndroidExcludedRefs

from leakcanary.

Oh I should have to look at this file first.

from leakcanary.

Can't we update AndroidExcludedRefs at first?

from leakcanary.

Yes. I like to figure out the issue first (when it's worth it), so that I don't just hide it under the rug. But we can also just ignore this guy.

from leakcanary.

@pepyakin Before I file an issue on Android, what is InterceptEditText doing? Is it messing with the edit text content in some way, maybe the spell checker or the Editor?

This leak could happen if SpellChecker.close() isn't correctly called. It's normally called when the textview is detached, which itself calls Editor.onDetach which closes the SpellChecker which should end up closing the SpellCheckSession and clearing the mHandler field in SpellCheckerSessionListenerImpl

from leakcanary.

@pyricau , OK, I see.
InterceptEditText looks like just no-op.
https://github.com/brendanw/PaymentKit-Droid/blob/master/library/src/main/java/com/paymentkit/views/InterceptEditText.java

from leakcanary.

Thanks! Could you send me the heap dump file, maybe as a dropbox or google drive link?

I'm trying to figure this out. I need to access the state of some of the intermediary objects.

Here's my "bug report" draft :

SpellCheckerSessionListenerImpl.mHandler is leaking destroyed Activity

Android API 22

5 seconds after an activity was destroyed, it was still held in memory by a chain of references that originates in SpellCheckerSessionListenerImpl.

Here is the leak trace:

• GC ROOT android.view.textservice.SpellCheckerSession$SpellCheckerSessionListenerImpl.mHandler
• references android.view.textservice.SpellCheckerSession$1.this$0 (anonymous class extends android.os.Handler)
• references android.view.textservice.SpellCheckerSession.mSpellCheckerSessionListener
• references android.widget.SpellChecker.mTextView
• references android.widget.EditText.mContext
• leaks com.wheely.app.ui.home.profile.CardActivity instance

SpellCheckerSessionListenerImpl is held in memory for IPC calls. It has a reference to SpellCheckerSession.mHandler. That handler is an anonymous class that therefore has a reference SpellCheckerSession.

It's considered a bad practice to create an anonymous Handler class, as noted in the Handler constructor (https://github.com/android/platform_frameworks_base/blob/master/core/java/android/os/Handler.java#L189). That being said, it looks like SpellCheckerSessionListenerImpl.mHandler should get cleared when the session is closed, and here it wasn't.

From what I can see, the normal flow is:

• TextView.onDetachedFromWindowInternal() calls Editor.onDetachedFromWindow()
• Which calls SpellChecker.closeSession()
• Which calls SpellCheckerSession.close()
• Which calls SpellCheckerSessionListenerImpl.close()
• Which ends up calling SpellCheckerSessionListenerImpl.processTask() with scp.mWhat == TASK_CLOSE
• Which clears the SpellCheckerSessionListenerImpl.mHandler reference

So through this path, something wasn't called at some point.

This leak was found with LeakCanary: #4

from leakcanary.

Things I would check in the heap dump:

• InterceptEditText.mAttachInfo should be null
• InterceptEditText.mEditor should not be null
• InterceptEditText.mEditor.mSpellChecker should be null
• SpellChecker.mSpellCheckerSession should not be null
• SpellCheckerSession.SpellCheckerSessionListenerImpl.mISpellCheckerSession should not be null
• SpellCheckerSession.SpellCheckerSessionListenerImpl.mPendingTasks should be empty

I expect one of these conditions to not hold.

from leakcanary.

I'm sorry, can't give you the dump now: every time I try to share a dump, receiver app complains that there is no such file, but it is for another issue.

I'll try to reproduce this issue and make a dump tomorrow.

On 09 May 2015, at 19:10, Pierre-Yves Ricau [email protected] wrote:

Thanks! Could you send me the heap dump file, maybe as a dropbox or google drive link?

I'm trying to figure this out. I need to access the state of some of the intermediary objects.

Here's my "bug report" draft :

SpellCheckerSessionListenerImpl.mHandler is leaking destroyed Activity

Android API 22

5 seconds after an activity was destroyed, it was still held in memory by a chain of references that originates in SpellCheckerSessionListenerImpl.

Here is the leak trace:

• GC ROOT android.view.textservice.SpellCheckerSession$SpellCheckerSessionListenerImpl.mHandler
• references android.view.textservice.SpellCheckerSession$1.this$0 (anonymous class extends android.os.Handler)
• references android.view.textservice.SpellCheckerSession.mSpellCheckerSessionListener
• references android.widget.SpellChecker.mTextView
• references android.widget.EditText.mContext
• leaks com.wheely.app.ui.home.profile.CardActivity instance

SpellCheckerSessionListenerImpl is held in memory for IPC calls. It has a reference to SpellCheckerSession.mHandler. That handler is an anonymous class that therefore has a reference SpellCheckerSession.

It's considered a bad practice to create an anonymous Handler class, as noted in the Handler constructor (https://github.com/android/platform_frameworks_base/blob/master/core/java/android/os/Handler.java#L189). That being said, it looks like SpellCheckerSessionListenerImpl.mHandler should get cleared when the session is closed, and here it wasn't.

From what I can see, the normal flow is:

• TextView.onDetachedFromWindowInternal() calls Editor.onDetachedFromWindow()
• Which calls SpellChecker.closeSession()
• Which calls SpellCheckerSession.close()
• Which calls SpellCheckerSessionListenerImpl.close()
• Which ends up calling SpellCheckerSessionListenerImpl.processTask() with scp.mWhat == TASK_CLOSE
• Which clears the SpellCheckerSessionListenerImpl.mHandler reference

So through this path, something wasn't called at some point.

This leak was found with LeakCanary: #4
—
Reply to this email directly or view it on GitHub.

from leakcanary.

The dumps are on the device. If your device is rooted, you can get it with adb pull /data/data/com.myapp/PATH_TO_HEAPDUMP

from leakcanary.

There would not be a problem if device were rooted :) Anyway I'll manage to get the dump as soon as I get to my laptop.

On 09 May 2015, at 20:00, Pierre-Yves Ricau [email protected] wrote:

The dumps are on the device. If your device is rooted, you can get it with adb pull /data/data/com.myapp/PATH_TO_HEAPDUMP

—
Reply to this email directly or view it on GitHub.

from leakcanary.

You can run this command on a debug app if your phone is not rooted
adb shell "run-as com.your.packagename cp <source> <dest>"

from leakcanary.

@andhie wow! That's cool! If only I had known this before I rooted my phone 😔

from leakcanary.

Finally managed to get heapdump, here is a link.

from leakcanary.

Condition SpellCheckerSession.SpellCheckerSessionListenerImpl.mISpellCheckerSession should not be null doesn't hold. Also, SpellCheckerSession.SpellCheckerSessionListenerImpl.mPendingTasks.size == 2.

mPendingTasks looks like following:

[ 
  { mWhat       = TASK_GET_SUGGESTIONS_MULTIPLE_FOR_SENTENCE
  , mSessions = null
  , mTextInfos = [ { ... } ] 
  , // ...
  }, 
  { mWhat       = TASK_CLOSE
  , mSession   = null
  , mTextInfos = null
  }
]

from leakcanary.

Cool.

SpellCheckerSession.SpellCheckerSessionListenerImpl.mISpellCheckerSession is null, so that means android.view.textservice.SpellCheckerSession.SpellCheckerSessionListenerImpl#onServiceConnected was never called, which is confirmed by mOpened being false.

The two pending tasks are TASK_GET_SUGGESTIONS_MULTIPLE_FOR_SENTENCE and TASK_CLOSE.

So we're waiting for the service the connect here. When the service connects, it processes the pending tasks.

Looks like it never indeed connect, hence creating the leak.

from leakcanary.

Filed here: https://code.google.com/p/android/issues/detail?id=172542

from leakcanary.

Is there a hack for this issue?

from leakcanary.

It looks like this might have returned in 7.1. Here's a leak we've seen a few times recently:

* com.trello.feature.board.BoardActivity has leaked:
* GC ROOT android.view.textservice.SpellCheckerSession$SpellCheckerSessionListenerImpl.mHandler
* references android.view.textservice.SpellCheckerSession$1.this$0 (anonymous subclass of android.os.Handler)
* references android.view.textservice.SpellCheckerSession.mSpellCheckerSessionListener
* references android.widget.SpellChecker.mTextView
* references com.trello.<CENSORED>
* references com.trello.<CENSORED>
* leaks com.trello.feature.board.BoardActivity instance

* Retaining: 208 KB.
* Reference Key: 6d70f3ab-6679-4e79-8920-9facb1b888b3
* Device: Google google Pixel sailfish
* Android Version: 7.1 API: 25 LeakCanary: 1.5 00f37f5
* Durations: watch=5382ms, gc=675ms, heap dump=3468ms, analysis=564667ms

from leakcanary.