Commit 5c8058b7 authored by ssid's avatar ssid Committed by Commit Bot

Add error-prone plugin to verify string literals for trace events

The java trace event strings cannot be checked for string literals at
runtime. So, add a compile time check to verify that all uses are
passing in string constants.
A few exceptions are marked with suppress warnings.

Total overhead of running the plugin is about 100ms with 20ms
measurement overhead.

BUG=984827

Change-Id: Ia10fe6f9f255c7402936a287bdde8b86684dbd71
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1705518Reviewed-by: default avataroysteine <oysteine@chromium.org>
Reviewed-by: default avatarTommy Nyquist <nyquist@chromium.org>
Commit-Queue: ssid <ssid@chromium.org>
Cr-Commit-Position: refs/heads/master@{#678527}
parent d202e167
...@@ -22,7 +22,8 @@ import java.util.Map; ...@@ -22,7 +22,8 @@ import java.util.Map;
import javax.annotation.concurrent.GuardedBy; import javax.annotation.concurrent.GuardedBy;
/** Support for early tracing, before the native library is loaded. /**
* Support for early tracing, before the native library is loaded.
* *
* This is limited, as: * This is limited, as:
* - Arguments are not supported * - Arguments are not supported
...@@ -37,6 +38,9 @@ import javax.annotation.concurrent.GuardedBy; ...@@ -37,6 +38,9 @@ import javax.annotation.concurrent.GuardedBy;
* as some are pending, then early tracing is permanently disabled after dumping the * as some are pending, then early tracing is permanently disabled after dumping the
* events. This means that if any early event is still pending when tracing is disabled, * events. This means that if any early event is still pending when tracing is disabled,
* all early events are dropped. * all early events are dropped.
*
* Like the TraceEvent, the event name of the trace events must be a string literal or a |static
* final String| class member. Otherwise NoDynamicStringsInTraceEventCheck error will be thrown.
*/ */
@JNINamespace("base::android") @JNINamespace("base::android")
@MainDex @MainDex
......
...@@ -23,6 +23,9 @@ import org.chromium.base.annotations.MainDex; ...@@ -23,6 +23,9 @@ import org.chromium.base.annotations.MainDex;
* } * }
* }</pre> * }</pre>
* *
* The event name of the trace events must be a string literal or a |static final String| class
* member. Otherwise NoDynamicStringsInTraceEventCheck error will be thrown.
*
* It is OK to use tracing before the native library has loaded, in a slightly restricted fashion. * It is OK to use tracing before the native library has loaded, in a slightly restricted fashion.
* @see EarlyTraceEvent for details. * @see EarlyTraceEvent for details.
*/ */
......
...@@ -276,6 +276,8 @@ public abstract class AsyncTask<Result> { ...@@ -276,6 +276,8 @@ public abstract class AsyncTask<Result> {
* while waiting. * while waiting.
*/ */
@DoNotInline @DoNotInline
// The string passed is safe since it is class and method name.
@SuppressWarnings("NoDynamicStringsInTraceEventCheck")
public final Result get() throws InterruptedException, ExecutionException { public final Result get() throws InterruptedException, ExecutionException {
Result r; Result r;
if (getStatus() != Status.FINISHED && ThreadUtils.runningOnUiThread()) { if (getStatus() != Status.FINISHED && ThreadUtils.runningOnUiThread()) {
......
...@@ -49,7 +49,7 @@ public class TaskRunnerImpl implements TaskRunner { ...@@ -49,7 +49,7 @@ public class TaskRunnerImpl implements TaskRunner {
/** /**
* @param traits The TaskTraits associated with this TaskRunnerImpl. * @param traits The TaskTraits associated with this TaskRunnerImpl.
* @param traceCategory Specifies which subclass is this instance for logging purposes. * @param traceCategory Specifies the name of this instance's subclass for logging purposes.
* @param taskRunnerType Specifies which subclass is this instance for initialising the correct * @param taskRunnerType Specifies which subclass is this instance for initialising the correct
* native scheduler. * native scheduler.
*/ */
...@@ -119,6 +119,8 @@ public class TaskRunnerImpl implements TaskRunner { ...@@ -119,6 +119,8 @@ public class TaskRunnerImpl implements TaskRunner {
/** /**
* Runs a single task and returns when its finished. * Runs a single task and returns when its finished.
*/ */
// The trace event name is derived from string literals.
@SuppressWarnings("NoDynamicStringsInTraceEventCheck")
protected void runPreNativeTask() { protected void runPreNativeTask() {
try (TraceEvent te = TraceEvent.scoped(mTraceEvent)) { try (TraceEvent te = TraceEvent.scoped(mTraceEvent)) {
Runnable task; Runnable task;
......
...@@ -52,7 +52,7 @@ public abstract class ConnectedTask<T extends ChromeGoogleApiClient> implements ...@@ -52,7 +52,7 @@ public abstract class ConnectedTask<T extends ChromeGoogleApiClient> implements
/** /**
* @param client * @param client
* @param logPrefix used for logging and tracing. * @param logPrefix used for logging and tracing. Must be string literal.
*/ */
public ConnectedTask(T client, String logPrefix) { public ConnectedTask(T client, String logPrefix) {
assert logPrefix != null; assert logPrefix != null;
...@@ -95,6 +95,8 @@ public abstract class ConnectedTask<T extends ChromeGoogleApiClient> implements ...@@ -95,6 +95,8 @@ public abstract class ConnectedTask<T extends ChromeGoogleApiClient> implements
@Override @Override
@VisibleForTesting @VisibleForTesting
// We always only pass in a string literal here.
@SuppressWarnings("NoDynamicStringsInTraceEventCheck")
public final void run() { public final void run() {
TraceEvent.begin("GCore:" + mLogPrefix + ":run"); TraceEvent.begin("GCore:" + mLogPrefix + ":run");
try { try {
......
...@@ -9,9 +9,10 @@ java_library("errorprone_plugin_java") { ...@@ -9,9 +9,10 @@ java_library("errorprone_plugin_java") {
# Turned off because of existing code which fails the check # Turned off because of existing code which fails the check
# "src/org/chromium/tools/errorprone/plugin/NoContextGetApplicationContext.java", # "src/org/chromium/tools/errorprone/plugin/NoContextGetApplicationContext.java",
"src/org/chromium/tools/errorprone/plugin/AndroidNullableCheck.java", "src/org/chromium/tools/errorprone/plugin/AndroidNullableCheck.java",
"src/org/chromium/tools/errorprone/plugin/NoSynchronizedMethodCheck.java",
"src/org/chromium/tools/errorprone/plugin/NoRedundantFieldInitCheck.java",
"src/org/chromium/tools/errorprone/plugin/NoAndroidAsyncTaskCheck.java", "src/org/chromium/tools/errorprone/plugin/NoAndroidAsyncTaskCheck.java",
"src/org/chromium/tools/errorprone/plugin/NoDynamicStringsInTraceEventCheck.java",
"src/org/chromium/tools/errorprone/plugin/NoRedundantFieldInitCheck.java",
"src/org/chromium/tools/errorprone/plugin/NoSynchronizedMethodCheck.java",
"src/org/chromium/tools/errorprone/plugin/NoSynchronizedThisCheck.java", "src/org/chromium/tools/errorprone/plugin/NoSynchronizedThisCheck.java",
] ]
......
// Copyright 2019 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
package org.chromium.tools.errorprone.plugin;
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.matchers.Description;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.BinaryTree;
import com.sun.source.tree.IdentifierTree;
import com.sun.source.tree.LiteralTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.Tree;
import com.sun.source.util.SimpleTreeVisitor;
import com.sun.tools.javac.code.Symbol;
import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import javax.lang.model.element.ElementKind;
import javax.lang.model.element.Modifier;
/**
* Triggers an error for {@link org.chromium.base.TraceEvent} usages with non string literals.
*/
@AutoService(BugChecker.class)
@BugPattern(name = "NoDynamicStringsInTraceEventCheck", category = BugPattern.Category.JDK,
summary = "Only use of string literals are allowed in trace events.",
severity = BugPattern.SeverityLevel.ERROR, linkType = BugPattern.LinkType.CUSTOM,
link = "https://crbug.com/984827")
public class NoDynamicStringsInTraceEventCheck
extends BugChecker implements BugChecker.MethodInvocationTreeMatcher {
private static final Set<String> sTracingFunctions = new HashSet<>(Arrays.asList(
"begin", "end", "scoped", "startAsync", "finishAsync", "instant", "TraceEvent"));
private static final ParameterVisitor sVisitor = new ParameterVisitor();
@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState visitorState) {
Symbol.MethodSymbol method = ASTHelpers.getSymbol(tree);
if (!sTracingFunctions.contains(method.name.toString())) return Description.NO_MATCH;
String className = method.enclClass().fullname.toString();
if (!"org.chromium.base.EarlyTraceEvent".equals(className)
&& !"org.chromium.base.TraceEvent".equals(className)) {
return Description.NO_MATCH;
}
// Allow the events added by tracing. Adding SuppressWarning in these files causes all
// caller warnings to be ignored.
String filename = visitorState.getPath().getCompilationUnit().getSourceFile().getName();
if (filename.endsWith("TraceEvent.java")) {
return Description.NO_MATCH;
}
List<? extends Tree> args = tree.getArguments();
Tree eventName_expr = args.get(0);
ParameterVisitor.Result r = eventName_expr.accept(sVisitor, null);
if (r.success) return Description.NO_MATCH;
return buildDescription(tree)
.setMessage("Calling TraceEvent.begin() without a constant String object. "
+ r.errorMessage)
.build();
}
static class ParameterVisitor extends SimpleTreeVisitor<ParameterVisitor.Result, Void> {
static class Result {
public boolean success;
public String errorMessage;
private Result(boolean successVal, String error) {
success = successVal;
errorMessage = error;
}
public static Result createError(String error) {
return new Result(false, error);
}
public static Result createOk() {
return new Result(true, null);
}
public Result append(Result other) {
success &= other.success;
if (errorMessage == null) {
errorMessage = other.errorMessage;
} else if (other.errorMessage != null) {
errorMessage += " " + other.errorMessage;
}
return this;
}
};
@Override
protected Result defaultAction(Tree tree, Void p) {
throw new RuntimeException("Unhandled expression tree type: " + tree.getKind());
}
@Override
public Result visitBinary(BinaryTree tree, Void p) {
return tree.getLeftOperand()
.accept(this, null)
.append(tree.getRightOperand().accept(this, null));
}
@Override
public Result visitLiteral(LiteralTree tree, Void p) {
return Result.createOk();
}
@Override
public Result visitIdentifier(IdentifierTree node, Void p) {
Symbol eventName = ASTHelpers.getSymbol(node);
if (eventName == null) {
return Result.createError("Identifier was not found: " + node + '.');
}
if (eventName.getKind() == ElementKind.FIELD) {
if (!"java.lang.String".equals(eventName.type.toString())) {
return Result.createError("Field: " + eventName + " should be of type string.");
}
Set<Modifier> modifiers = eventName.getModifiers();
if (!modifiers.contains(Modifier.FINAL) || !modifiers.contains(Modifier.STATIC)) {
return Result.createError(
"String literal: " + eventName + " is not static final.");
}
return Result.createOk();
} else if (eventName.getKind() == ElementKind.PARAMETER) {
return Result.createError(
"Passing in event name as parameter: " + eventName + " is not supported.");
}
return Result.createError("Unhandled identifier kind: " + node.getKind() + '.');
}
};
}
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment