Skip to content

Commit

Permalink
Add reason for ignoring to analysis result
Browse files Browse the repository at this point in the history 
* When creating an ExcludedRef rule, we can now provide a name for that rule and a reason.
* When such a rule applies to an analysis result, it is stored in the leak trace element.
* Also moved the lengthy comments in AndroidExcludedRefs to now be ExcludedRef reasons, embedded in LeakCanary.

Fixes #365
  • Loading branch information
@pyricau
pyricau committed Jan 8, 2016
1 parent 3da4331 commit 8baa337
Show file tree
Hide file tree
Showing 17 changed files with 464 additions and 346 deletions.
11 changes: 6 additions & 5 deletions leakcanary-analyzer/src/main/java/com/squareup/leakcanary/HeapAnalyzer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ private boolean isIgnoredDominator(Instance dominator, Instance instance) {
private LeakTrace buildLeakTrace(LeakNode leakingNode) {
List<LeakTraceElement> elements = new ArrayList<>();
// We iterate from the leak to the GC root
LeakNode node = new LeakNode(null, leakingNode, null, null);
LeakNode node = new LeakNode(null, null, leakingNode, null, null);
while (node != null) {
LeakTraceElement element = buildLeakElement(node);
if (element != null) {
Expand Down Expand Up @@ -264,22 +264,23 @@ private LeakTraceElement buildLeakElement(LeakNode node) {
Class<?>[] interfaces = actualClass.getInterfaces();
if (interfaces.length > 0) {
Class<?> implementedInterface = interfaces[0];
extra = "(anonymous class implements " + implementedInterface.getName() + ")";
extra = "(anonymous implementation of " + implementedInterface.getName() + ")";
} else {
extra = "(anonymous class extends java.lang.Object)";
extra = "(anonymous subclass of java.lang.Object)";
}
} catch (ClassNotFoundException ignored) {
}
} else {
holderType = OBJECT;
// Makes it easier to figure out which anonymous class we're looking at.
extra = "(anonymous class extends " + parentClassName + ")";
extra = "(anonymous subclass of " + parentClassName + ")";
}
} else {
holderType = OBJECT;
}
}
return new LeakTraceElement(referenceName, type, holderType, className, extra, fields);
return new LeakTraceElement(referenceName, type, holderType, className, extra, node.exclusion,
fields);
}

private long since(long analysisStartNanoTime) {
Expand Down
7 changes: 5 additions & 2 deletions leakcanary-analyzer/src/main/java/com/squareup/leakcanary/LeakNode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,16 @@
import com.squareup.haha.perflib.Instance;

final class LeakNode {
/** May be null. */
final Exclusion exclusion;
final Instance instance;
final LeakNode parent;
final String referenceName;
final LeakTraceElement.Type referenceType;

LeakNode(Instance instance, LeakNode parent, String referenceName,
LeakTraceElement.Type referenceType) {
LeakNode(Exclusion exclusion, Instance instance, LeakNode parent,
String referenceName, LeakTraceElement.Type referenceType) {
this.exclusion = exclusion;
this.instance = instance;
this.parent = parent;
this.referenceName = referenceName;
Expand Down
11 changes: 10 additions & 1 deletion leakcanary-analyzer/src/main/java/com/squareup/leakcanary/LeakTraceElement.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,16 +48,20 @@ public enum Holder {
/** Additional information, may be null. */
public final String extra;

/** If not null, there was no path that could exclude this element. */
public final Exclusion exclusion;

/** List of all fields (member and static) for that object. */
public final List<String> fields;

LeakTraceElement(String referenceName, Type type, Holder holder, String className, String extra,
List<String> fields) {
Exclusion exclusion, List<String> fields) {
this.referenceName = referenceName;
this.type = type;
this.holder = holder;
this.className = className;
this.extra = extra;
this.exclusion = exclusion;
this.fields = unmodifiableList(new ArrayList<>(fields));
}

Expand All @@ -83,6 +87,11 @@ public enum Holder {
if (extra != null) {
string += " " + extra;
}

if (exclusion != null) {
string += " , matching exclusion " + exclusion.matching;
}

return string;
}

Expand Down
114 changes: 52 additions & 62 deletions leakcanary-analyzer/src/main/java/com/squareup/leakcanary/ShortestPathFinder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,9 @@ Result findPath(Snapshot snapshot, Instance leakingRef) {
node = toVisitQueue.poll();
} else {
node = toVisitIfNoPathQueue.poll();
if (node.exclusion == null) {
throw new IllegalStateException("Expected node to have an exclusion " + node);
}
excludingKnownLeaks = true;
}

Expand Down Expand Up @@ -131,9 +134,9 @@ private void enqueueGcRoots(Snapshot snapshot) {
case JAVA_LOCAL:
Instance thread = HahaSpy.allocatingThread(rootObj);
String threadName = threadName(thread);
Boolean alwaysIgnore = excludedRefs.threadNames.get(threadName);
if (alwaysIgnore == null || !alwaysIgnore) {
enqueue(alwaysIgnore == null, null, rootObj, null, null);
Exclusion params = excludedRefs.threadNames.get(threadName);
if (params == null || !params.alwaysExclude) {
enqueue(params, null, rootObj, null, null);
}
break;
case INTERNED_STRING:
Expand All @@ -160,7 +163,7 @@ private void enqueueGcRoots(Snapshot snapshot) {
// Input or output parameters in native code.
case NATIVE_STACK:
case JAVA_STATIC:
enqueue(true, null, rootObj, null, null);
enqueue(null, null, rootObj, null, null);
break;
default:
throw new UnsupportedOperationException("Unknown root type:" + rootObj.getRootType());
Expand All @@ -176,50 +179,48 @@ private void visitRootObj(LeakNode node) {
RootObj rootObj = (RootObj) node.instance;
Instance child = rootObj.getReferredInstance();

boolean visitRootNow = true;
if (child != null) {
// true = ignore root, false = visit root later, null = visit root now.
Boolean rootSuperClassAlwaysIgnored = rootSuperClassAlwaysIgnored(child);
if (rootSuperClassAlwaysIgnored != null) {
if (rootSuperClassAlwaysIgnored) {
return;
} else {
visitRootNow = false;
}
}
Exclusion exclusion = rootSuperClassAlwaysIgnored(child);

if (exclusion != null && exclusion.alwaysExclude) {
return;
}

if (rootObj.getRootType() == RootType.JAVA_LOCAL) {
Instance holder = HahaSpy.allocatingThread(rootObj);
// We switch the parent node with the thread instance that holds
// the local reference.
LeakNode parent = new LeakNode(holder, null, null, null);
enqueue(visitRootNow, parent, child, "<Java Local>", LOCAL);
LeakNode parent = new LeakNode(null, holder, null, null, null);
if (node.exclusion != null) {
exclusion = node.exclusion;
}
enqueue(exclusion, parent, child, "<Java Local>", LOCAL);
} else {
enqueue(visitRootNow, node, child, null, null);
enqueue(exclusion, node, child, null, null);
}
}

private Boolean rootSuperClassAlwaysIgnored(Instance child) {
Boolean alwaysIgnoreClassHierarchy = null;
private Exclusion rootSuperClassAlwaysIgnored(Instance child) {
if (child == null) {
return null;
}
Exclusion matchingParams = null;
ClassObj superClassObj = child.getClassObj();
while (superClassObj != null) {
Boolean alwaysIgnoreClass =
excludedRefs.rootSuperClassNames.get(superClassObj.getClassName());
if (alwaysIgnoreClass != null) {
Exclusion params = excludedRefs.rootClassNames.get(superClassObj.getClassName());
if (params != null) {
// true overrides null or false.
if (alwaysIgnoreClassHierarchy == null || !alwaysIgnoreClassHierarchy) {
alwaysIgnoreClassHierarchy = alwaysIgnoreClass;
if (matchingParams == null || !matchingParams.alwaysExclude) {
matchingParams = params;
}
}
superClassObj = superClassObj.getSuperClassObj();
}
return alwaysIgnoreClassHierarchy;
return matchingParams;
}

private void visitClassObj(LeakNode node) {
ClassObj classObj = (ClassObj) node.instance;
Map<String, Boolean> ignoredStaticFields =
Map<String, Exclusion> ignoredStaticFields =
excludedRefs.staticFieldNameByClassName.get(classObj.getClassName());
for (Map.Entry<Field, Object> entry : classObj.getStaticFieldValues().entrySet()) {
Field field = entry.getKey();
Expand All @@ -233,72 +234,60 @@ private void visitClassObj(LeakNode node) {
Instance child = (Instance) entry.getValue();
boolean visit = true;
if (ignoredStaticFields != null) {
Boolean alwaysIgnore = ignoredStaticFields.get(fieldName);
if (alwaysIgnore != null) {
Exclusion params = ignoredStaticFields.get(fieldName);
if (params != null) {
visit = false;
if (!alwaysIgnore) {
enqueue(false, node, child, fieldName, STATIC_FIELD);
if (!params.alwaysExclude) {
enqueue(params, node, child, fieldName, STATIC_FIELD);
}
}
}
if (visit) {
enqueue(true, node, child, fieldName, STATIC_FIELD);
enqueue(null, node, child, fieldName, STATIC_FIELD);
}
}
}

private void visitClassInstance(LeakNode node) {
ClassInstance classInstance = (ClassInstance) node.instance;
Map<String, Boolean> ignoredFields = null;
Map<String, Exclusion> ignoredFields = new LinkedHashMap<>();
ClassObj superClassObj = classInstance.getClassObj();
Boolean alwaysIgnoreClassHierarchy = null;
Exclusion classExclusion = null;
while (superClassObj != null) {
Boolean alwaysIgnoreClass = excludedRefs.classNames.get(superClassObj.getClassName());
if (alwaysIgnoreClass != null) {
Exclusion params = excludedRefs.classNames.get(superClassObj.getClassName());
if (params != null) {
// true overrides null or false.
if (alwaysIgnoreClassHierarchy == null || !alwaysIgnoreClassHierarchy) {
alwaysIgnoreClassHierarchy = alwaysIgnoreClass;
if (classExclusion == null || !classExclusion.alwaysExclude) {
classExclusion = params;
}
}
Map<String, Boolean> classIgnoredFields =
Map<String, Exclusion> classIgnoredFields =
excludedRefs.fieldNameByClassName.get(superClassObj.getClassName());
if (classIgnoredFields != null) {
if (ignoredFields == null) {
ignoredFields = new LinkedHashMap<>();
}
ignoredFields.putAll(classIgnoredFields);
}
superClassObj = superClassObj.getSuperClassObj();
}

if (alwaysIgnoreClassHierarchy != null && alwaysIgnoreClassHierarchy) {
if (classExclusion != null && classExclusion.alwaysExclude) {
return;
}

for (ClassInstance.FieldValue fieldValue : classInstance.getValues()) {
Exclusion fieldExclusion = classExclusion;
Field field = fieldValue.getField();
if (field.getType() != Type.OBJECT) {
continue;
}
Instance child = (Instance) fieldValue.getValue();
boolean visit = true;
boolean visitIfNoPath = false;
// We don't even get here if alwaysIgnoreClassHierarchy is false.
if (alwaysIgnoreClassHierarchy != null) {
visit = false;
visitIfNoPath = true;
}
String fieldName = field.getName();
if (ignoredFields != null) {
Boolean alwaysIgnore = ignoredFields.get(fieldName);
if (alwaysIgnore != null) {
visit = false;
visitIfNoPath = !alwaysIgnore;
}
}
if (visit || visitIfNoPath) {
enqueue(visit, node, child, fieldName, INSTANCE_FIELD);
Exclusion params = ignoredFields.get(fieldName);
// If we found a field exclusion and it's stronger than a class exclusion
if (params != null && (fieldExclusion == null || (params.alwaysExclude
&& !fieldExclusion.alwaysExclude))) {
fieldExclusion = params;
}
enqueue(fieldExclusion, node, child, fieldName, INSTANCE_FIELD);
}
}

Expand All @@ -309,12 +298,12 @@ private void visitArrayInstance(LeakNode node) {
Object[] values = arrayInstance.getValues();
for (int i = 0; i < values.length; i++) {
Instance child = (Instance) values[i];
enqueue(true, node, child, "[" + i + "]", ARRAY_ENTRY);
enqueue(null, node, child, "[" + i + "]", ARRAY_ENTRY);
}
}
}

private void enqueue(boolean visitNow, LeakNode parent, Instance child, String referenceName,
private void enqueue(Exclusion exclusion, LeakNode parent, Instance child, String referenceName,
LeakTraceElement.Type referenceType) {
if (child == null) {
return;
Expand All @@ -326,6 +315,7 @@ private void enqueue(boolean visitNow, LeakNode parent, Instance child, String r
if (toVisitSet.contains(child)) {
return;
}
boolean visitNow = exclusion == null;
if (!visitNow && toVisitIfNoPathSet.contains(child)) {
return;
}
Expand All @@ -335,7 +325,7 @@ private void enqueue(boolean visitNow, LeakNode parent, Instance child, String r
if (visitedSet.contains(child)) {
return;
}
LeakNode childNode = new LeakNode(child, parent, referenceName, referenceType);
LeakNode childNode = new LeakNode(exclusion, child, parent, referenceName, referenceType);
if (visitNow) {
toVisitSet.add(child);
toVisitQueue.add(childNode);
Expand Down
25 changes: 17 additions & 8 deletions leakcanary-analyzer/src/test/java/com/squareup/leakcanary/AsyncTaskLeakTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@
package com.squareup.leakcanary;

import java.lang.ref.WeakReference;
import java.util.Arrays;
import java.util.Collection;
import java.util.List;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand All @@ -29,6 +29,7 @@
import static com.squareup.leakcanary.TestUtil.HeapDumpFile.ASYNC_TASK_MPREVIEW2;
import static com.squareup.leakcanary.TestUtil.HeapDumpFile.ASYNC_TASK_M_POSTPREVIEW2;
import static com.squareup.leakcanary.TestUtil.analyze;
import static java.util.Arrays.asList;
import static org.hamcrest.core.StringContains.containsString;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
Expand All @@ -44,23 +45,25 @@ public class AsyncTaskLeakTest {
static final String EXECUTOR_FIELD_2 = "sDefaultExecutor";

@Parameterized.Parameters public static Collection<Object[]> data() {
return Arrays.asList(new Object[][] {
return asList(new Object[][] {
{ ASYNC_TASK }, //
{ ASYNC_TASK_MPREVIEW2 }, //
{ ASYNC_TASK_M_POSTPREVIEW2 } //
});
}

private final TestUtil.HeapDumpFile heapDumpFile;
ExcludedRefs.Builder excludedRefs;
ExcludedRefs.BuilderWithParams excludedRefs;

public AsyncTaskLeakTest(TestUtil.HeapDumpFile heapDumpFile) {
this.heapDumpFile = heapDumpFile;
}

@Before public void setUp() {
excludedRefs = new ExcludedRefs.Builder().clazz(WeakReference.class.getName(), true)
.clazz("java.lang.ref.FinalizerReference", true);
excludedRefs = new ExcludedRefs.BuilderWithParams().clazz(WeakReference.class.getName())
.alwaysExclude()
.clazz("java.lang.ref.FinalizerReference")
.alwaysExclude();
}

@Test public void leakFound() {
Expand All @@ -86,11 +89,17 @@ public AsyncTaskLeakTest(TestUtil.HeapDumpFile heapDumpFile) {
}

@Test public void excludeStatic() {
excludedRefs.thread(ASYNC_TASK_THREAD);
excludedRefs.staticField(ASYNC_TASK_CLASS, EXECUTOR_FIELD_1);
excludedRefs.staticField(ASYNC_TASK_CLASS, EXECUTOR_FIELD_2);
excludedRefs.thread(ASYNC_TASK_THREAD).named(ASYNC_TASK_THREAD);
excludedRefs.staticField(ASYNC_TASK_CLASS, EXECUTOR_FIELD_1).named(EXECUTOR_FIELD_1);
excludedRefs.staticField(ASYNC_TASK_CLASS, EXECUTOR_FIELD_2).named(EXECUTOR_FIELD_2);
AnalysisResult result = analyze(heapDumpFile, excludedRefs);
assertTrue(result.leakFound);
assertTrue(result.excludedLeak);
LeakTrace leakTrace = result.leakTrace;
List<LeakTraceElement> elements = leakTrace.elements;
Exclusion exclusion = elements.get(0).exclusion;

List<String> expectedExclusions = asList(ASYNC_TASK_THREAD, EXECUTOR_FIELD_1, EXECUTOR_FIELD_2);
assertTrue(expectedExclusions.contains(exclusion.name));
}
}
Loading

0 comments on commit 8baa337

Please sign in to comment.