Add lint rule which warns about View.setTag(int, Object)

This changeset adds a lint rule which looks for potential
leaks when using View#setTag(int, Object) where the Object
is likely to contain a strong reference to the context,
such as views and view holders.

http://code.google.com/p/android/issues/detail?id=26984

Change-Id: Ib606485d2b875d2129c339b9b89be0e444629408

7 files changed, 286 insertions, 1 deletions

diff --git a/eclipse/dictionary.txt b/eclipse/dictionary.txt
index c4d9089..e2c4340 100644
--- a/eclipse/dictionary.txt
+++ b/eclipse/dictionary.txt
@@ -190,6 +190,7 @@ newfound
 nexus
 ninepatch
 nodpi
+nulling
 num
 ok
 op
diff --git a/lint/libs/lint_checks/src/com/android/tools/lint/checks/BuiltinIssueRegistry.java b/lint/libs/lint_checks/src/com/android/tools/lint/checks/BuiltinIssueRegistry.java
index e1325b5..27073e7 100644
--- a/lint/libs/lint_checks/src/com/android/tools/lint/checks/BuiltinIssueRegistry.java
+++ b/lint/libs/lint_checks/src/com/android/tools/lint/checks/BuiltinIssueRegistry.java
@@ -54,7 +54,7 @@ public class BuiltinIssueRegistry extends IssueRegistry {
     private static final List<Issue> sIssues;
 
     static {
-        final int initialCapacity = 102;
+        final int initialCapacity = 103;
         List<Issue> issues = new ArrayList<Issue>(initialCapacity);
 
         issues.add(AccessibilityDetector.ISSUE);
@@ -85,6 +85,7 @@ public class BuiltinIssueRegistry extends IssueRegistry {
         issues.add(TooManyViewsDetector.TOO_DEEP);
         issues.add(GridLayoutDetector.ISSUE);
         issues.add(OnClickDetector.ISSUE);
+        issues.add(ViewTagDetector.ISSUE);
         issues.add(RegistrationDetector.ISSUE);
         issues.add(HandlerDetector.ISSUE);
         issues.add(FragmentDetector.ISSUE);
diff --git a/lint/libs/lint_checks/src/com/android/tools/lint/checks/ViewTagDetector.java b/lint/libs/lint_checks/src/com/android/tools/lint/checks/ViewTagDetector.java
new file mode 100644
index 0000000..0eeb448
--- /dev/null
+++ b/lint/libs/lint_checks/src/com/android/tools/lint/checks/ViewTagDetector.java
@@ -0,0 +1,174 @@
+/*
+ * Copyright (C) 2012 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.tools.lint.checks;
+
+import com.android.annotations.NonNull;
+import com.android.annotations.Nullable;
+import com.android.tools.lint.client.api.LintDriver;
+import com.android.tools.lint.client.api.SdkInfo;
+import com.android.tools.lint.detector.api.Category;
+import com.android.tools.lint.detector.api.ClassContext;
+import com.android.tools.lint.detector.api.Detector;
+import com.android.tools.lint.detector.api.Detector.ClassScanner;
+import com.android.tools.lint.detector.api.Issue;
+import com.android.tools.lint.detector.api.Location;
+import com.android.tools.lint.detector.api.Scope;
+import com.android.tools.lint.detector.api.Severity;
+import com.android.tools.lint.detector.api.Speed;
+
+import org.objectweb.asm.Type;
+import org.objectweb.asm.tree.ClassNode;
+import org.objectweb.asm.tree.InsnList;
+import org.objectweb.asm.tree.MethodInsnNode;
+import org.objectweb.asm.tree.MethodNode;
+import org.objectweb.asm.tree.analysis.Analyzer;
+import org.objectweb.asm.tree.analysis.AnalyzerException;
+import org.objectweb.asm.tree.analysis.BasicInterpreter;
+import org.objectweb.asm.tree.analysis.BasicValue;
+import org.objectweb.asm.tree.analysis.Frame;
+
+import java.util.Arrays;
+import java.util.EnumSet;
+import java.util.List;
+
+/**
+ * Checks for missing view tag detectors
+ */
+public class ViewTagDetector extends Detector implements ClassScanner {
+    /** Missing onClick handlers */
+    public static final Issue ISSUE = Issue.create(
+            "ViewTag", //$NON-NLS-1$
+            "Finds potential leaks when using View.setTag",
+
+            "Prior to Android 4.0, the implementation of View.setTag(int, Object) would " +
+            "store the objects in a static map, where the values were strongly referenced. " +
+            "This means that if the object contains any references pointing back to the " +
+            "context, the context (which points to pretty much everything else) will leak. " +
+            "If you pass a view, the view provides a reference to the context " +
+            "that created it. Similarly, view holders typically contain a view, and cursors " +
+            "are sometimes also associated with views.",
+
+            Category.PERFORMANCE,
+            6,
+            Severity.WARNING,
+            ViewTagDetector.class,
+            EnumSet.of(Scope.ALL_RESOURCE_FILES, Scope.CLASS_FILE));
+
+    /** Constructs a new {@link ViewTagDetector} */
+    public ViewTagDetector() {
+    }
+
+    @Override
+    public @NonNull Speed getSpeed() {
+        return Speed.FAST;
+    }
+
+    // ---- Implements ClassScanner ----
+
+    @Override
+    @Nullable
+    public List<String> getApplicableCallNames() {
+        return Arrays.asList("setTag"); //$NON-NLS-1$
+    }
+
+    @Override
+    public void checkCall(@NonNull ClassContext context, @NonNull ClassNode classNode,
+            @NonNull MethodNode method, @NonNull MethodInsnNode call) {
+        // The leak behavior is fixed in ICS:
+        // http://code.google.com/p/android/issues/detail?id=18273
+        if (context.getMainProject().getMinSdk() >= 14) {
+            return;
+        }
+
+        String owner = call.owner;
+        String desc = call.desc;
+        if (owner.equals("android/view/View")                 //$NON-NLS-1$
+                && desc.equals("(ILjava/lang/Object;)V")) {   //$NON-NLS-1$
+            Analyzer analyzer = new Analyzer(new BasicInterpreter() {
+                @Override
+                public BasicValue newValue(Type type) {
+                    if (type == null) {
+                        return BasicValue.UNINITIALIZED_VALUE;
+                    } else if (type.getSort() == Type.VOID) {
+                        return null;
+                    } else {
+                        return new BasicValue(type);
+                    }
+                }
+            });
+            try {
+                Frame[] frames = analyzer.analyze(classNode.name, method);
+                InsnList instructions = method.instructions;
+                Frame frame = frames[instructions.indexOf(call)];
+                if (frame.getStackSize() < 3) {
+                    return;
+                }
+                BasicValue stackValue = (BasicValue) frame.getStack(2);
+                Type type = stackValue.getType();
+                if (type == null) {
+                    return;
+                }
+
+                String internalName = type.getInternalName();
+                String className = type.getClassName();
+                LintDriver driver = context.getDriver();
+
+                SdkInfo sdkInfo = context.getClient().getSdkInfo(context.getMainProject());
+                String objectType = null;
+                while (className != null) {
+                    if (className.equals("android.view.View")) {         //$NON-NLS-1$
+                        objectType = "views";
+                        break;
+                    } else if (className.endsWith("ViewHolder")) {       //$NON-NLS-1$
+                        objectType = "view holders";
+                        break;
+                    } else if (className.endsWith("Cursor")              //$NON-NLS-1$
+                                && className.startsWith("android.")) {   //$NON-NLS-1$
+                        objectType = "cursors";
+                        break;
+                    }
+
+                    // TBD: Bitmaps, drawables? That's tricky, because as explained in
+                    // http://android-developers.blogspot.com/2009/01/avoiding-memory-leaks.html
+                    // apparently these are used along with nulling out the callbacks,
+                    // and that's harder to detect here
+
+                    String parent = sdkInfo.getParentViewClass(className);
+                    if (parent == null) {
+                        if (internalName == null) {
+                            internalName = className.replace('.', '/');
+                        }
+                        assert internalName != null;
+                        className = driver.getSuperClass(internalName);
+                    }
+                    className = parent;
+                    internalName = null;
+                }
+
+                if (objectType != null) {
+                    Location location = context.getLocation(call);
+                    String message = String.format("Avoid setting %1$s as values for setTag: " +
+                        "Can lead to memory leaks in versions older than Android 4.0",
+                        objectType);
+                    context.report(ISSUE, method, location, message, null);
+                }
+            } catch (AnalyzerException e) {
+                context.log(e, null);
+            }
+        }
+    }
+}
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/AbstractCheckTest.java b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/AbstractCheckTest.java
index 25e60a8..46efe57 100644
--- a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/AbstractCheckTest.java
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/AbstractCheckTest.java
@@ -343,6 +343,10 @@ public abstract class AbstractCheckTest extends TestCase {
                 sb.append(exception.toString());
             }
             System.err.println(sb);
+
+            if (exception != null) {
+                fail(exception.toString());
+            }
         }
 
         @Override
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/ViewTagDetectorTest.java b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/ViewTagDetectorTest.java
new file mode 100644
index 0000000..8d9e2d5
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/ViewTagDetectorTest.java
@@ -0,0 +1,68 @@
+/*
+ * Copyright (C) 2012 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.tools.lint.checks;
+
+import com.android.tools.lint.detector.api.Detector;
+
+@SuppressWarnings("javadoc")
+public class ViewTagDetectorTest extends AbstractCheckTest {
+    @Override
+    protected Detector getDetector() {
+        return new ViewTagDetector();
+    }
+
+    public void test() throws Exception {
+        assertEquals(
+            "src/test/pkg/ViewTagTest.java:21: Warning: Avoid setting views as values for setTag: Can lead to memory leaks in versions older than Android 4.0 [ViewTag]\n" +
+            "        view.setTag(android.R.id.button1, group); // ERROR\n" +
+            "             ~~~~~~\n" +
+            "src/test/pkg/ViewTagTest.java:22: Warning: Avoid setting views as values for setTag: Can lead to memory leaks in versions older than Android 4.0 [ViewTag]\n" +
+            "        view.setTag(android.R.id.icon, view.findViewById(android.R.id.icon)); // ERROR\n" +
+            "             ~~~~~~\n" +
+            "src/test/pkg/ViewTagTest.java:23: Warning: Avoid setting cursors as values for setTag: Can lead to memory leaks in versions older than Android 4.0 [ViewTag]\n" +
+            "        view.setTag(android.R.id.icon1, cursor1); // ERROR\n" +
+            "             ~~~~~~\n" +
+            "src/test/pkg/ViewTagTest.java:24: Warning: Avoid setting cursors as values for setTag: Can lead to memory leaks in versions older than Android 4.0 [ViewTag]\n" +
+            "        view.setTag(android.R.id.icon2, cursor2); // ERROR\n" +
+            "             ~~~~~~\n" +
+            "src/test/pkg/ViewTagTest.java:25: Warning: Avoid setting view holders as values for setTag: Can lead to memory leaks in versions older than Android 4.0 [ViewTag]\n" +
+            "        view.setTag(android.R.id.copy, new MyViewHolder()); // ERROR\n" +
+            "             ~~~~~~\n" +
+            "0 errors, 5 warnings\n",
+
+            lintProject(
+                    "bytecode/.classpath=>.classpath",
+                    "bytecode/AndroidManifest.xml=>AndroidManifest.xml",
+                    "res/layout/onclick.xml=>res/layout/onclick.xml",
+                    "bytecode/ViewTagTest.java.txt=>src/test/pkg/ViewTagTest.java",
+                    "bytecode/ViewTagTest.class.data=>bin/classes/test/pkg/ViewTagTest.class"
+                    ));
+    }
+
+    public void testICS() throws Exception {
+        assertEquals(
+            "No warnings.",
+
+            lintProject(
+                    "bytecode/.classpath=>.classpath",
+                    "apicheck/minsdk14.xml=>AndroidManifest.xml",
+                    "res/layout/onclick.xml=>res/layout/onclick.xml",
+                    "bytecode/ViewTagTest.java.txt=>src/test/pkg/ViewTagTest.java",
+                    "bytecode/ViewTagTest.class.data=>bin/classes/test/pkg/ViewTagTest.class"
+                    ));
+    }
+}
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/ViewTagTest.class.data b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/ViewTagTest.class.data
new file mode 100644
index 0000000..f26032c
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/ViewTagTest.class.data
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/ViewTagTest.java.txt b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/ViewTagTest.java.txt
new file mode 100644
index 0000000..8e72fd0
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/ViewTagTest.java.txt
@@ -0,0 +1,37 @@
+package test.pkg;
+
+import android.annotation.SuppressLint;
+import android.content.Context;
+import android.database.Cursor;
+import android.database.MatrixCursor;
+import android.view.LayoutInflater;
+import android.view.View;
+import android.view.ViewGroup;
+import android.widget.CursorAdapter;
+import android.widget.ImageView;
+import android.widget.TextView;
+
+@SuppressWarnings("unused")
+public abstract class ViewTagTest {
+    public View newView(Context context, ViewGroup group, Cursor cursor1,
+            MatrixCursor cursor2) {
+        LayoutInflater inflater = LayoutInflater.from(context);
+        View view = inflater.inflate(android.R.layout.activity_list_item, null);
+        view.setTag(android.R.id.background, "Some random tag"); // OK
+        view.setTag(android.R.id.button1, group); // ERROR
+        view.setTag(android.R.id.icon, view.findViewById(android.R.id.icon)); // ERROR
+        view.setTag(android.R.id.icon1, cursor1); // ERROR
+        view.setTag(android.R.id.icon2, cursor2); // ERROR
+        view.setTag(android.R.id.copy, new MyViewHolder()); // ERROR
+        return view;
+    }
+
+    @SuppressLint("ViewTag")
+    public void checkSuppress(Context context, View view) {
+        view.setTag(android.R.id.icon, view.findViewById(android.R.id.icon));
+    }
+
+    private class MyViewHolder {
+        View view;
+    }
+}