Add custom Error Prone check for rethrowing.

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.

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.

Disable this check on managers that we know are hosted outside the
system process.

Bug: 155703208
Test: ./build/soong/soong_ui.bash --make-mode framework-minus-apex services RUN_ERROR_PRONE=true
Exempt-From-Owner-Approval: trivial annotations
Change-Id: I04b4daf7c92251a14bcc3ebb1e18cd00f6a7f283

3 files changed, 101 insertions, 2 deletions

diff --git a/errorprone/Android.bp b/errorprone/Android.bp
index 016b85510a94..098f4bfa74ac 100644
--- a/errorprone/Android.bp
+++ b/errorprone/Android.bp
@@ -20,6 +20,4 @@ java_library_host {
     plugins: [
         "//external/dagger2:dagger2-auto-service",
     ],
-
-    javacflags: ["-verbose"],
 }
diff --git a/errorprone/java/com/google/errorprone/bugpatterns/android/RethrowFromSystemChecker.java b/errorprone/java/com/google/errorprone/bugpatterns/android/RethrowFromSystemChecker.java
new file mode 100644
index 000000000000..48123abd26cb
--- /dev/null
+++ b/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;
+    }
+}
diff --git a/errorprone/java/com/google/errorprone/bugpatterns/android/TargetSdkChecker.java b/errorprone/java/com/google/errorprone/bugpatterns/android/TargetSdkChecker.java
index 1ce816c34990..232cf3f0d677 100644
--- a/errorprone/java/com/google/errorprone/bugpatterns/android/TargetSdkChecker.java
+++ b/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",