Add special case to ignore Binder leaks #351

dandc87 · 2015-12-30T23:39:36Z

The Android OS will retain a reference to Binder after a Service is
unbound. This adds logic to ignore any paths where a Binder is at a
root. We cannot simply add Binder to ExcludedRef because there could
be legitimate leaks where the Binder isn't the root.

dandc87 · 2015-12-30T23:58:11Z

Relevant issue: https://code.google.com/p/android/issues/detail?id=1797

vanniktech · 2015-12-31T00:10:35Z

leakcanary-analyzer/src/main/java/com/squareup/leakcanary/ShortestPathFinder.java

@@ -176,6 +176,13 @@ private void visitRootObj(LeakNode node) {
    RootObj rootObj = (RootObj) node.instance;
    Instance child = rootObj.getReferredInstance();

+    //We should ignore leaks where an android.os.Binder is the root of the leak.
+    //When you bind and unbind from a Service, yhe OS will tend to keep a reference to the Binder


little typo here. should be the and not yhe

pyricau · 2016-01-04T17:21:04Z

The referenced bug claims that the problems comes from the service process not being GCed.

shaybarak · 2016-01-04T17:42:50Z

Correct.
The server-side IBinder instance can only be GC'ed once the client-side IBinder instance has been GC'ed, even if the client process has disconnected.
Here's the issue:

Client process binds.
Server process Service does onCreate, onBind.
Client process unbinds.
Server process Service does onUnbind, onDestroy. But IBinder returned from onBind is set as a GC root, and likely holds a reference to the Service after Service.onDestroy was called, which looks like a leak.

The server process can't force the client process to do a GC, and it can't be notified when the client process has GC'ed either. Therefore the only way to not trigger false positives for LeakCanary notifications is to filter out instances of classes extending android.os.Binder as GC roots.

pyricau · 2016-01-04T18:06:27Z

I'll do a close review a bit later.

dandc87 · 2016-01-04T18:30:56Z

Here's a branch where I modified the sample app to show the binder leaking: https://github.com/dandc87/leakcanary/compare/master...binder_leak_demo?expand=1

Just click the button and wait!

pyricau · 2016-01-05T06:45:27Z

Just to clarify: that branch you are linking to is a binder "real leak" that should be detected, correct?

pyricau · 2016-01-05T06:48:47Z

leakcanary-analyzer/src/main/java/com/squareup/leakcanary/HahaHelper.java

@@ -163,6 +163,20 @@ static boolean hasField(List<ClassInstance.FieldValue> values, String fieldName)
    return false;
  }

+  static boolean extendsFrom(Instance instance, String className) {
+    if (instance == null) {


Why check for null here, have you seen it happen?

It happened when I ran unit tests

pyricau · 2016-01-05T07:01:17Z

I like it a lot overall, let's see if we can make it a bit more generic.

shaybarak · 2016-01-05T17:40:26Z

Thanks Pierre for the review!
Yes, the branch does demonstrate a real leak, but LeakCanary (before Adam's change) will find the wrong chain of references (leading up to the Binder instead of leading up to the static ArrayList). If you fix the leak by commenting out the static ArrayList, it still shows up as a leak.
With Adam's change, the leak is detected by pointing all the way to the static ArrayList, and when the leak is fixed, no false alerts are generated.

dandc87 · 2016-01-05T21:20:20Z

Made changes based on your feedback @pyricau. I added ExcludedRefs.Builder.rootClass() and modeled it's exclusion behavior off of clazz(). Does this work for you?

The Android OS will retain a reference to `Binder` after a `Service` is unbound. This adds logic to ignore any paths where a `Binder` is at a root. We cannot simply add `Binder` with `ExcludedRef.Builder.clazz()` because there could be legitimate leaks where the `Binder` isn't the root.

pyricau · 2016-01-06T14:12:05Z

leakcanary-analyzer/src/test/java/com/squareup/leakcanary/TestUtil.java

+import java.io.File;
+import java.net.URL;
+
+class TestUtil {


pyricau · 2016-01-06T14:18:05Z

Thanks @dandc87 ! I'll take it from here, change one or two nits, and merge.

* binder_leak: PR nits for #351 Add special case to ignore Binder leaks

jrodbx · 2016-09-09T20:30:03Z

@dandc87, it looks like 3d1f524 is resulting in false negatives, such as the case reported in #482. See d28f920 for an example test case.

Revisiting the example you provided in https://github.com/dandc87/leakcanary/commit/5a293dbf8098c04adbfd4a2e13699e4a90bc0feb, it looks like the leak occurs because an anonymous subclass of Binder references the Service installed to the RefWatcher. Consider https://github.com/jrodbx/leakcanary/commit/c9dce97aedca03a4fee443330094e3b32175338d which doesn't result in a leak as mentioned in the original cited Google issue. Am I missing something?

We can continue the dialog on this, but, in the meantime, I'll revert this commit since it's causing false negatives.

dandc87 · 2016-09-09T21:58:39Z

The false-positive happens when the Binder has a reference to the Service, be that an explicit reference or implicit from being an anonymous sublcass. If you don't need the Service's context, you can refactor the 'leak' away. However, if you need access to the Service from the Binder, LeakCanary will report the Service as leaking because the Android system is holding onto the Binder instance after onDestroy() (but it will eventually get released and GC'd).

You may want to just revert the changes made to AndroidExcludedRefs.java, I believe @pyricau did some cleanup after this merged, so I'm not sure how effective a full revert would be.

* binder_leak: PR nits for square#351 Add special case to ignore Binder leaks

vanniktech reviewed Dec 31, 2015
View reviewed changes

pyricau added the status: needs more info label Jan 4, 2016

pyricau added status: accepted and removed status: needs more info labels Jan 4, 2016

pyricau reviewed Jan 5, 2016
View reviewed changes

pyricau reviewed Jan 6, 2016
View reviewed changes

pyricau added a commit that referenced this pull request Jan 6, 2016

PR nits for #351

bf29548

pyricau merged commit 12c69bb into square:master Jan 6, 2016

pyricau added a commit that referenced this pull request Jan 6, 2016

Merge branch 'binder_leak'

3d1f524

* binder_leak: PR nits for #351 Add special case to ignore Binder leaks

pyricau added this to the 1.4 milestone Jan 6, 2016

Pengchengxiang pushed a commit to XLibrarys/leakcanary that referenced this pull request Jan 2, 2017

PR nits for square#351

df9abf4

Pengchengxiang pushed a commit to XLibrarys/leakcanary that referenced this pull request Jan 2, 2017

Merge branch 'binder_leak'

47d5c46

* binder_leak: PR nits for square#351 Add special case to ignore Binder leaks

Pengchengxiang pushed a commit to XLibrarys/leakcanary that referenced this pull request Jan 2, 2017

PR nits for square#351

5612e1e

Pengchengxiang pushed a commit to XLibrarys/leakcanary that referenced this pull request Jan 2, 2017

PR nits for square#351

fe7b558

Pengchengxiang pushed a commit to XLibrarys/leakcanary that referenced this pull request Jan 2, 2017

Merge branch 'binder_leak'

2a04119

* binder_leak: PR nits for square#351 Add special case to ignore Binder leaks

jpd236 mentioned this pull request Aug 12, 2020

Leak detected in standard AIDL service pattern #1906

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add special case to ignore Binder leaks #351

Add special case to ignore Binder leaks #351

dandc87 commented Dec 30, 2015

dandc87 commented Dec 30, 2015

vanniktech Dec 31, 2015

pyricau commented Jan 4, 2016

shaybarak commented Jan 4, 2016

pyricau commented Jan 4, 2016

dandc87 commented Jan 4, 2016

pyricau commented Jan 5, 2016

pyricau Jan 5, 2016

dandc87 Jan 5, 2016

pyricau commented Jan 5, 2016

shaybarak commented Jan 5, 2016

dandc87 commented Jan 5, 2016

pyricau Jan 6, 2016

pyricau commented Jan 6, 2016

jrodbx commented Sep 9, 2016

dandc87 commented Sep 9, 2016

Add special case to ignore Binder leaks #351

Add special case to ignore Binder leaks #351

Conversation

dandc87 commented Dec 30, 2015

dandc87 commented Dec 30, 2015

vanniktech Dec 31, 2015

Choose a reason for hiding this comment

pyricau commented Jan 4, 2016

shaybarak commented Jan 4, 2016

pyricau commented Jan 4, 2016

dandc87 commented Jan 4, 2016

pyricau commented Jan 5, 2016

pyricau Jan 5, 2016

Choose a reason for hiding this comment

dandc87 Jan 5, 2016

Choose a reason for hiding this comment

pyricau commented Jan 5, 2016

shaybarak commented Jan 5, 2016

dandc87 commented Jan 5, 2016

pyricau Jan 6, 2016

Choose a reason for hiding this comment

pyricau commented Jan 6, 2016

jrodbx commented Sep 9, 2016

dandc87 commented Sep 9, 2016