From bb4ff3f6456082380bfcfd3fb8295a19f2d8aa35 Mon Sep 17 00:00:00 2001 From: Jernej Virag Date: Thu, 10 Mar 2022 17:52:11 +0000 Subject: Add an Android Lint check to prevent sendBroadcast calls on Main thread This lint check verifies that BroadcastSender is always used in SystemUI for sending broadcasts. It should prevent main thread jank due to broadcast calls. Bug: 223606115 Test: Included unit test for the linter. Change-Id: I16b8b3471389ff2c59053c686c94cb9cb694ec42 --- packages/SystemUI/Android.bp | 4 + packages/SystemUI/checks/Android.bp | 52 +++++++ .../lint/BroadcastSentViaContextDetector.kt | 83 ++++++++++++ .../systemui/lint/SystemUIIssueRegistry.kt | 43 ++++++ .../lint/BroadcastSentViaContextDetectorTest.kt | 150 +++++++++++++++++++++ 5 files changed, 332 insertions(+) create mode 100644 packages/SystemUI/checks/Android.bp create mode 100644 packages/SystemUI/checks/src/com/android/internal/systemui/lint/BroadcastSentViaContextDetector.kt create mode 100644 packages/SystemUI/checks/src/com/android/internal/systemui/lint/SystemUIIssueRegistry.kt create mode 100644 packages/SystemUI/checks/tests/com/android/systemui/lint/BroadcastSentViaContextDetectorTest.kt diff --git a/packages/SystemUI/Android.bp b/packages/SystemUI/Android.bp index 8cc9e004f758..f05c1e2e76f2 100644 --- a/packages/SystemUI/Android.bp +++ b/packages/SystemUI/Android.bp @@ -128,6 +128,10 @@ android_library { kotlincflags: ["-Xjvm-default=enable"], plugins: ["dagger2-compiler"], + + lint: { + extra_check_modules: ["SystemUILintChecker"], + }, } filegroup { diff --git a/packages/SystemUI/checks/Android.bp b/packages/SystemUI/checks/Android.bp new file mode 100644 index 000000000000..8457312dc403 --- /dev/null +++ b/packages/SystemUI/checks/Android.bp @@ -0,0 +1,52 @@ +// Copyright (C) 2021 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 { + // See: http://go/android-license-faq + // A large-scale-change added 'default_applicable_licenses' to import + // all of the 'license_kinds' from "frameworks_base_license" + // to get the below license kinds: + // SPDX-license-identifier-Apache-2.0 + default_applicable_licenses: ["frameworks_base_license"], +} + +java_library_host { + name: "SystemUILintChecker", + srcs: [ + "src/**/*.kt", + "src/**/*.java", + ], + plugins: ["auto_service_plugin"], + libs: [ + "auto_service_annotations", + "lint_api", + ], +} + +java_test_host { + name: "SystemUILintCheckerTest", + srcs: [ + "tests/**/*.kt", + "tests/**/*.java", + ], + static_libs: [ + "SystemUILintChecker", + "junit", + "lint", + "lint_tests", + ], + test_options: { + unit_test: true, + }, +} diff --git a/packages/SystemUI/checks/src/com/android/internal/systemui/lint/BroadcastSentViaContextDetector.kt b/packages/SystemUI/checks/src/com/android/internal/systemui/lint/BroadcastSentViaContextDetector.kt new file mode 100644 index 000000000000..8d48f0957be4 --- /dev/null +++ b/packages/SystemUI/checks/src/com/android/internal/systemui/lint/BroadcastSentViaContextDetector.kt @@ -0,0 +1,83 @@ +/* + * Copyright (C) 2022 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.internal.systemui.lint + +import com.android.tools.lint.detector.api.Category +import com.android.tools.lint.detector.api.Detector +import com.android.tools.lint.detector.api.Implementation +import com.android.tools.lint.detector.api.Issue +import com.android.tools.lint.detector.api.JavaContext +import com.android.tools.lint.detector.api.Scope +import com.android.tools.lint.detector.api.Severity +import com.android.tools.lint.detector.api.SourceCodeScanner +import com.intellij.psi.PsiMethod +import org.jetbrains.uast.UCallExpression +import org.jetbrains.uast.UClass +import org.jetbrains.uast.getParentOfType + +/** + * Checks if anyone is calling sendBroadcast / sendBroadcastAsUser on a Context (or subclasses) and + * directs them to using com.android.systemui.broadcast.BroadcastSender instead. + */ +@Suppress("UnstableApiUsage") +class BroadcastSentViaContextDetector : Detector(), SourceCodeScanner { + + override fun getApplicableMethodNames(): List { + return listOf("sendBroadcast", "sendBroadcastAsUser") + } + + override fun visitMethodCall(context: JavaContext, node: UCallExpression, method: PsiMethod) { + if (node.getParentOfType(UClass::class.java)?.qualifiedName == + "com.android.systemui.broadcast.BroadcastSender" + ) { + // Don't warn for class we want the developers to use. + return + } + + val evaulator = context.evaluator + if (evaulator.isMemberInSubClassOf(method, "android.content.Context")) { + context.report( + ISSUE, + method, + context.getNameLocation(node), + "Please don't call sendBroadcast/sendBroadcastAsUser directly on " + + "Context, use com.android.systemui.broadcast.BroadcastSender instead." + ) + } + } + + companion object { + @JvmField + val ISSUE: Issue = + Issue.create( + id = "BroadcastSentViaContext", + briefDescription = "Broadcast sent via Context instead of BroadcastSender.", + explanation = + "Broadcast was sent via " + + "Context.sendBroadcast/Context.sendBroadcastAsUser. Please use " + + "BroadcastSender.sendBroadcast/BroadcastSender.sendBroadcastAsUser " + + "which will schedule dispatch of broadcasts on background thread. " + + "Sending broadcasts on main thread causes jank due to synchronous " + + "Binder calls.", + category = Category.PERFORMANCE, + priority = 8, + severity = Severity.WARNING, + implementation = + Implementation(BroadcastSentViaContextDetector::class.java, Scope.JAVA_FILE_SCOPE) + ) + } +} diff --git a/packages/SystemUI/checks/src/com/android/internal/systemui/lint/SystemUIIssueRegistry.kt b/packages/SystemUI/checks/src/com/android/internal/systemui/lint/SystemUIIssueRegistry.kt new file mode 100644 index 000000000000..397a110f4bc7 --- /dev/null +++ b/packages/SystemUI/checks/src/com/android/internal/systemui/lint/SystemUIIssueRegistry.kt @@ -0,0 +1,43 @@ +/* + * Copyright (C) 2022 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.internal.systemui.lint + +import com.android.tools.lint.client.api.IssueRegistry +import com.android.tools.lint.client.api.Vendor +import com.android.tools.lint.detector.api.CURRENT_API +import com.android.tools.lint.detector.api.Issue +import com.google.auto.service.AutoService + +@AutoService(IssueRegistry::class) +@Suppress("UnstableApiUsage") +class SystemUIIssueRegistry : IssueRegistry() { + + override val issues: List + get() = listOf(BroadcastSentViaContextDetector.ISSUE) + + override val api: Int + get() = CURRENT_API + override val minApi: Int + get() = 8 + + override val vendor: Vendor = + Vendor( + vendorName = "Android", + feedbackUrl = "http://b/issues/new?component=78010", + contact = "jernej@google.com" + ) +} diff --git a/packages/SystemUI/checks/tests/com/android/systemui/lint/BroadcastSentViaContextDetectorTest.kt b/packages/SystemUI/checks/tests/com/android/systemui/lint/BroadcastSentViaContextDetectorTest.kt new file mode 100644 index 000000000000..da010212f211 --- /dev/null +++ b/packages/SystemUI/checks/tests/com/android/systemui/lint/BroadcastSentViaContextDetectorTest.kt @@ -0,0 +1,150 @@ +package com.android.internal.systemui.lint + +import com.android.tools.lint.checks.infrastructure.LintDetectorTest +import com.android.tools.lint.checks.infrastructure.TestFiles +import com.android.tools.lint.checks.infrastructure.TestFile +import com.android.tools.lint.checks.infrastructure.TestLintTask +import com.android.tools.lint.detector.api.Detector +import com.android.tools.lint.detector.api.Issue +import org.junit.Test + +class BroadcastSentViaContextDetectorTest : LintDetectorTest() { + + override fun getDetector(): Detector = BroadcastSentViaContextDetector() + override fun lint(): TestLintTask = super.lint().allowMissingSdk(true) + + override fun getIssues(): List = listOf( + BroadcastSentViaContextDetector.ISSUE) + + @Test + fun testSendBroadcast() { + lint().files( + TestFiles.java( + """ + package test.pkg; + import android.content.Context; + + public class TestClass1 { + public void send(Context context) { + Intent intent = new Intent(Intent.ACTION_VIEW); + context.sendBroadcast(intent); + } + } + """ + ).indented(), + *stubs) + .issues(BroadcastSentViaContextDetector.ISSUE) + .run() + .expectWarningCount(1) + .expectContains( + "Please don't call sendBroadcast/sendBroadcastAsUser directly on " + + "Context, use com.android.systemui.broadcast.BroadcastSender instead.") + } + + @Test + fun testSendBroadcastAsUser() { + lint().files( + TestFiles.java( + """ + package test.pkg; + import android.content.Context; + import android.os.UserHandle; + + public class TestClass1 { + public void send(Context context) { + Intent intent = new Intent(Intent.ACTION_VIEW); + context.sendBroadcastAsUser(intent, UserHandle.ALL, "permission"); + } + } + """).indented(), + *stubs) + .issues(BroadcastSentViaContextDetector.ISSUE) + .run() + .expectWarningCount(1) + .expectContains( + "Please don't call sendBroadcast/sendBroadcastAsUser directly on " + + "Context, use com.android.systemui.broadcast.BroadcastSender instead.") + } + + @Test + fun testSendBroadcastInActivity() { + lint().files( + TestFiles.java( + """ + package test.pkg; + import android.app.Activity; + import android.os.UserHandle; + + public class TestClass1 { + public void send(Activity activity) { + Intent intent = new Intent(Intent.ACTION_VIEW); + activity.sendBroadcastAsUser(intent, UserHandle.ALL, "permission"); + } + + } + """).indented(), + *stubs) + .issues(BroadcastSentViaContextDetector.ISSUE) + .run() + .expectWarningCount(1) + .expectContains( + "Please don't call sendBroadcast/sendBroadcastAsUser directly on " + + "Context, use com.android.systemui.broadcast.BroadcastSender instead.") + } + + @Test + fun testNoopIfNoCall() { + lint().files( + TestFiles.java( + """ + package test.pkg; + import android.content.Context; + + public class TestClass1 { + public void sendBroadcast() { + Intent intent = new Intent(Intent.ACTION_VIEW); + context.startActivity(intent); + } + } + """).indented(), + *stubs) + .issues(BroadcastSentViaContextDetector.ISSUE) + .run() + .expectClean() + } + + private val contextStub: TestFile = java( + """ + package android.content; + import android.os.UserHandle; + + public class Context { + public void sendBroadcast(Intent intent) {}; + public void sendBroadcast(Intent intent, String receiverPermission) {}; + public void sendBroadcastAsUser(Intent intent, UserHandle userHandle, + String permission) {}; + } + """ + ) + + private val activityStub: TestFile = java( + """ + package android.app; + import android.content.Context; + + public class Activity extends Context {} + """ + ) + + private val userHandleStub: TestFile = java( + """ + package android.os; + + public enum UserHandle { + ALL + } + """ + ) + + private val stubs = arrayOf(contextStub, activityStub, userHandleStub) +} -- cgit v1.2.3-59-g8ed1b