Merge "Add custom Error Prone check for rethrowing." into rvc-dev

errorprone/Android.bp

@@ -20,6 +20,4 @@ java_library_host {
     plugins: [
         "//external/dagger2:dagger2-auto-service",
     ],
-
-    javacflags: ["-verbose"],
 }

errorprone/java/com/google/errorprone/bugpatterns/android/RethrowFromSystemChecker.java

@@ -0,0 +1,80 @@
+/*
+ * Copyright (C) 2020 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.google.errorprone.bugpatterns.android;
+
+import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
+import static com.google.errorprone.matchers.Matchers.enclosingClass;
+import static com.google.errorprone.matchers.Matchers.hasAnnotation;
+import static com.google.errorprone.matchers.Matchers.instanceMethod;
+import static com.google.errorprone.matchers.Matchers.isSameType;
+import static com.google.errorprone.matchers.Matchers.methodInvocation;
+import static com.google.errorprone.matchers.Matchers.throwStatement;
+import static com.google.errorprone.matchers.Matchers.variableType;
+
+import com.google.auto.service.AutoService;
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.bugpatterns.BugChecker.CatchTreeMatcher;
+import com.google.errorprone.matchers.Description;
+import com.google.errorprone.matchers.Matcher;
+import com.sun.source.tree.CatchTree;
+import com.sun.source.tree.StatementTree;
+import com.sun.source.tree.Tree;
+import com.sun.source.tree.VariableTree;
+
+import java.util.List;
+
+/**
+ * Apps making calls into the system server may end up persisting internal state
+ * or making security decisions based on the perceived success or failure of a
+ * call, or any default values returned. For this reason, we want to strongly
+ * throw when there was trouble with the transaction.
+ * <p>
+ * The rethrowFromSystemServer() method is the best-practice way of doing this
+ * correctly, so that we don't clutter logs with misleading stack traces, and
+ * this checker verifies that best-practice is used.
+ */
+@AutoService(BugChecker.class)
+@BugPattern(
+    name = "AndroidFrameworkRethrowFromSystem",
+    summary = "Verifies that system_server calls use rethrowFromSystemServer()",
+    severity = WARNING)
+public final class RethrowFromSystemChecker extends BugChecker implements CatchTreeMatcher {
+    private static final Matcher<Tree> INSIDE_MANAGER =
+            enclosingClass(hasAnnotation("android.annotation.SystemService"));
+    private static final Matcher<VariableTree> REMOTE_EXCEPTION = variableType(
+            isSameType("android.os.RemoteException"));
+    private static final Matcher<StatementTree> RETHROW_FROM_SYSTEM = throwStatement(
+            methodInvocation(instanceMethod().onExactClass("android.os.RemoteException")
+                    .named("rethrowFromSystemServer")));
+
+    @Override
+    public Description matchCatch(CatchTree tree, VisitorState state) {
+        if (INSIDE_MANAGER.matches(tree, state)
+                && REMOTE_EXCEPTION.matches(tree.getParameter(), state)) {
+            final List<? extends StatementTree> statements = tree.getBlock().getStatements();
+            if (statements.size() != 1 || !RETHROW_FROM_SYSTEM.matches(statements.get(0), state)) {
+                return buildDescription(tree)
+                        .setMessage("Must contain single "
+                                + "'throw e.rethrowFromSystemServer()' statement")
+                        .build();
+            }
+        }
+        return Description.NO_MATCH;
+    }
+}

errorprone/java/com/google/errorprone/bugpatterns/android/TargetSdkChecker.java

@@ -34,6 +34,27 @@ import com.sun.source.tree.BinaryTree;
 import com.sun.source.tree.ExpressionTree;
 import com.sun.source.tree.Tree.Kind;
 
+/**
+ * Over the years we've had several obscure bugs related to how SDK level
+ * comparisons are performed, specifically during the window of time where we've
+ * started distributing the "frankenbuild" to developers.
+ * <p>
+ * Consider the case where a framework developer shipping release "R" wants to
+ * only grant a specific behavior to modern apps; they could write this in two
+ * different ways:
+ * <ol>
+ * <li>if (targetSdkVersion > Build.VERSION_CODES.Q) {
+ * <li>if (targetSdkVersion >= Build.VERSION_CODES.R) {
+ * </ol>
+ * The safer of these two options is (2), which will ensure that developers only
+ * get the behavior when <em>both</em> the app and the platform agree on the
+ * specific SDK level having shipped.
+ * <p>
+ * Consider the breakage that would happen with option (1) if we started
+ * shipping APKs that are based on the final R SDK, but are then installed on
+ * earlier preview releases which still consider R to be CUR_DEVELOPMENT; they'd
+ * risk crashing due to behaviors that were never part of the official R SDK.
+ */
 @AutoService(BugChecker.class)
 @BugPattern(
     name = "AndroidFrameworkTargetSdk",