search

vickychijwani / quill

:ghost: [MOVED TO https://github.com/TryGhost/Ghost-Android] The beautiful Android app for your Ghost blog.
MIT License
553 stars 84 forks source link

Memory leak in WebViewFragment #75

Closed vickychijwani closed 8 years ago

vickychijwani commented 9 years ago

Steps to reproduce:

  1. Open a post preview (the new WebView-based one)
  2. Select some text
  3. Tap back twice to come back to the post list
  4. Open up the post again and watch LeakCanary dump a heap

Note:

Unfortunately the heap trace (below) is unhelpful:

me.vickychijwani.spectre D/LeakCanary﹕ In me.vickychijwani.spectre:1.0:1.
    * me.vickychijwani.spectre.view.PostViewActivity has leaked:
    * GC ROOT org.chromium.content.browser.input.PopupTouchHandleDrawable.mContext
    * leaks me.vickychijwani.spectre.view.PostViewActivity instance
    * Reference Key: c5347412-4c57-49a2-9df7-9760955cedeb
    * Device: LGE google Nexus 4 occam
    * Android Version: 5.1.1 API: 22
    * Durations: watch=5124ms, gc=162ms, heap dump=11630ms, analysis=43875ms

Maybe analyzing the actual heap dump will help.

vickychijwani commented 9 years ago

Got the heap dump, here: https://www.dropbox.com/s/zodudiuacbctjcr/quill_issue_75.hprof?dl=0 (needs to be converted using hprof-conv)

vickychijwani commented 9 years ago

Dammit, this is back to haunt me in a different form. Now it happens consistently with PostViewActivity and BrowserActivity, even without selecting text! :confused: 

me.vickychijwani.spectre D/LeakCanary﹕ In me.vickychijwani.spectre:1.0:1.
    * me.vickychijwani.spectre.view.PostViewActivity has leaked:
    * GC ROOT static me.vickychijwani.spectre.SpectreApplication.sInstance
    * references me.vickychijwani.spectre.DebugSpectreApplication.mComponentCallbacks
    * references java.util.ArrayList.array
    * references array java.lang.Object[].[10]
    * references org.chromium.android_webview.AwContents$AwComponentCallbacks.this$0
    * references org.chromium.android_webview.AwContents.mContext
    * references com.android.webview.chromium.ResourcesContextWrapperFactory$WebViewContextWrapper.mBase
    * leaks me.vickychijwani.spectre.view.PostViewActivity instance
    * Reference Key: e08d6645-ddf7-411c-9898-d4575efd6cd9
    * Device: LGE google Nexus 4 occam
    * Android Version: 5.1.1 API: 22
    * Durations: watch=5022ms, gc=508ms, heap dump=13897ms, analysis=39075ms
vickychijwani commented 9 years ago

"Fix" courtesy http://stackoverflow.com/a/27253968/504611

vickychijwani commented 9 years ago

WebView strikes again:

me.vickychijwani.spectre.debug D/LeakCanary﹕ In me.vickychijwani.spectre.debug:1.0.0-beta:1.
    * me.vickychijwani.spectre.view.PostViewActivity has leaked:
    * GC ROOT android.view.accessibility.AccessibilityManager$1.this$0 (anonymous class extends android.view.accessibility.IAccessibilityManagerClient$Stub)
    * references android.view.accessibility.AccessibilityManager.mAccessibilityStateChangeListeners
    * references java.util.concurrent.CopyOnWriteArrayList.elements
    * references array java.lang.Object[].[1]
    * references org.chromium.content.browser.ContentViewCore.mContext
    * references com.android.webview.chromium.ResourcesContextWrapperFactory$WebViewContextWrapper.mBase
    * leaks me.vickychijwani.spectre.view.PostViewActivity instance
    * Reference Key: 4e7b339e-303c-46a5-bf89-76d165a77dc3
    * Device: LGE google Nexus 4 occam
    * Android Version: 5.1.1 API: 22
    * Durations: watch=5040ms, gc=249ms, heap dump=12780ms, analysis=46400ms

Perhaps the previous fix (read: hack) can be modified to plug this leak as well?

vickychijwani commented 9 years ago

The forceUnregisterComponentCallbacks hack seems to be of no help here. I'll have to analyze the heap dump.

vickychijwani commented 9 years ago

This may be of use (directly or source code-wise): https://github.com/jjhesk/EZWebView

bjenning04 commented 8 years ago

My team is also running into this issue in our project. According to https://github.com/square/leakcanary/issues/92, this is related to https://code.google.com/p/chromium/issues/detail?id=473146, which appears to be fixed in Marshmallow. That said, I'd love to know if you ever found a solution to this for Jelly Bean or greater.

vickychijwani commented 8 years ago

@bjenning04 no, unfortunately. Thanks for letting me know of the fix in Marshmallow! Although my latest heap trace above seems to be quite different from the one in the issue you linked to, doesn't it? Which one are you getting?

vickychijwani commented 8 years ago

Oh wait, I see your point now. After reading through the Chromium issue comments I found this:

We were leaking activity Contexts in ResourcesContextWrapperFactory's cache of wrapped contexts, because WeakHashMap only references its keys weakly, and the ContextWrapper objects used as values in the hashmap have a strong reference to the Context being used as a key, so nothing was ever removed from the map.

So all ResourcesContextWrapperFactory leaks should indeed be fixed in Marshmallow. Thanks again!

vickychijwani commented 8 years ago

@bjenning04 It turns out this was programmer error after all. WebView expects to be detached from the view hierarchy before being destroyed, which was not happening. Check out the docs for WebView#destroy(): https://developer.android.com/reference/android/webkit/WebView.html#destroy%28%29

yuanzhuohao commented 8 years ago

Did you solve the memory leaked?

vickychijwani commented 8 years ago

@jessyuan24 yes, you can see the code here. Basically you have to make sure you call ((ViewGroup) mWebView.getParent()).removeView(mWebView) before mWebView.destroy(). This is also pointed out in the documentation for WebView#destroy.

yuanzhuohao commented 8 years ago

Okay, I try it.

yuanzhuohao commented 8 years ago

It doesn't work. I have same problem as you.

`me.vickychijwani.spectre.debug D/LeakCanary﹕ In me.vickychijwani.spectre.debug:1.0.0-beta:1.

vickychijwani commented 8 years ago

Maybe you can post a question on StackOverflow with more details? (your exact code and stack trace). I would be happy to take a look there. This thread is really not the right place to discuss this :confused:

yuanzhuohao commented 8 years ago

No, It works. Hahahaha