Leak in SpellCheckerSession.

[pepyakin]

I have following leak on vanilla 5.1 on Nexus 5.

In com.wheely.wheely.dev:5.1.3:1014510.
* com.wheely.app.ui.home.profile.CardActivity has leaked:
* 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 com.paymentkit.views.InterceptEditText.mContext
* leaks com.wheely.app.ui.home.profile.CardActivity instance
D/LeakCanary( 7557):
* Reference Key: 3d9a5a01-4d51-472e-a459-717c21e1c7d1
* Device: LGE google Nexus 5 hammerhead
* Android Version: 5.1 API: 22
* Durations: watch=5027ms, gc=158ms, heap dump=2430ms, analysis=25573ms

[pyricau]

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

[pepyakin]

Oh I should have to look at this file first.

[pepyakin]

Can't we update AndroidExcludedRefs at first?

[pyricau]

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.

[pyricau]

@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

[pepyakin]

@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

[pyricau]

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

[pyricau]

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.

[pepyakin]

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 notifications@github.com 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.

[pyricau]

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

[pepyakin]

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 notifications@github.com 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.

[andhie]

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>"

[pepyakin]

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

[pepyakin]

Finally managed to get heapdump, here is a link.

[pepyakin]

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
  }
]

[pyricau]

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.

[pyricau]

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

[Yky]

Is there a hack for this issue?

[dlew]

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