From 57355699048cb905772754325b4ec554e9e14203 Mon Sep 17 00:00:00 2001 From: Yohei Yukawa Date: Mon, 22 Mar 2021 18:29:42 -0700 Subject: [PATCH] Add "cmd input_method tracing save-for-bugreport" This CL reworks our previous CL [1], which enabled "adb bugreport" to include ime-tracing files into bugreport with some unpleasant side-effect described in Bug 177462676. Basically what this CL aims to do is to mirror my CL [2] for InputMethodManagerService. Instead of relying on heuristics, this CL introduces a clear and dedicated command adb shell cmd input_method tracing save-for-bugreport so that "adb bugreport" can explicitly tell IMMS to dump the tracing files. See the corresponding CL for frameworks/native [3] about how it's triggered from "adb bugreport". [1]: Ie87eb8423e2bb70f28c330983d45b95e2e07062d ac24994aaa5bd1f70608cb8c72b93b5ae00d90eb [2]: I887ae6941b4844a606675b447f67ecee88d7f192 96a51f9971ed7559c17d4f9aff98c83b2fa28f4a [3]: I62105334e4efbb4514390ffa8be5416482ca3b29 02b1d2c70b2f4703a9dd51ba69d78604d66916e1 Bug: 177462676 Test: Manually done as follows. 1. adb shell cmd input_method tracing start 2. adb logcat -d -s imeTracing:* Make sure ime tracing is running 3. adb shell dumpsys input_method \ --dump-priority CRITICAL --proto > /dev/null 4. adb logcat -d -s imeTracing:* Make sure ime tracing has not been stopped. 5. adb shell cmd input_method tracing save-for-bugreport 6. adb logcat -d -s imeTracing:* Make sure ime-tracing was dumped and is still running. 7. adb root && adb shell ls /data/misc/wmtrace/ -al Make sure ime tracing files are saved. Test: Manually done as follows. 1. adb shell cmd input_method tracing start 2. adb logcat -d -s imeTracing:* Make sure ime tracing is running 3. adb bugreport bugreport.zip 4. adb logcat -d -s imeTracing:* Make sure ime-tracing was dumped and is still running. 5. unzip -v bugreport.zip | grep ime_trace_ Make sure ime_trace_{clients,service,managerservice}.pb are included. Change-Id: Ib106e077a10eb6a4f37b38aa9172e7c99a81f898 --- .../android/util/imetracing/ImeTracing.java | 23 +++++----- .../util/imetracing/ImeTracingClientImpl.java | 8 ---- .../util/imetracing/ImeTracingServerImpl.java | 44 ++++++++++++------- .../InputMethodManagerService.java | 21 +++------ 4 files changed, 45 insertions(+), 51 deletions(-) diff --git a/core/java/android/util/imetracing/ImeTracing.java b/core/java/android/util/imetracing/ImeTracing.java index b28cfb87e28dc..2fcaec91833d6 100644 --- a/core/java/android/util/imetracing/ImeTracing.java +++ b/core/java/android/util/imetracing/ImeTracing.java @@ -129,6 +129,16 @@ public abstract class ImeTracing { */ public abstract void triggerManagerServiceDump(String where); + /** + * Being called while taking a bugreport so that tracing files can be included in the bugreport + * when the IME tracing is running. Does nothing otherwise. + * + * @param pw Print writer + */ + public void saveForBugreport(@Nullable PrintWriter pw) { + // does nothing by default. + } + /** * Sets whether ime tracing is enabled. * @@ -152,11 +162,6 @@ public abstract class ImeTracing { return mService != null; } - /** - * Writes the current tracing data to the specific output proto file. - */ - public abstract void writeTracesToFiles(); - /** * Starts a new IME trace if one is not already started. * @@ -171,14 +176,6 @@ public abstract class ImeTracing { */ public abstract void stopTrace(@Nullable PrintWriter pw); - /** - * Stops the IME trace if one was previously started. - * - * @param pw Print writer - * @param writeToFile If the current buffer should be written to disk or not - */ - public abstract void stopTrace(@Nullable PrintWriter pw, boolean writeToFile); - private static boolean isSystemProcess() { return ActivityThread.isSystem(); } diff --git a/core/java/android/util/imetracing/ImeTracingClientImpl.java b/core/java/android/util/imetracing/ImeTracingClientImpl.java index 35a81b7aeea53..17cdc46876601 100644 --- a/core/java/android/util/imetracing/ImeTracingClientImpl.java +++ b/core/java/android/util/imetracing/ImeTracingClientImpl.java @@ -98,10 +98,6 @@ class ImeTracingClientImpl extends ImeTracing { // Intentionally left empty, this is implemented in ImeTracingServerImpl } - @Override - public void writeTracesToFiles() { - } - @Override public void startTrace(PrintWriter pw) { } @@ -109,8 +105,4 @@ class ImeTracingClientImpl extends ImeTracing { @Override public void stopTrace(PrintWriter pw) { } - - @Override - public void stopTrace(PrintWriter pw, boolean writeToFile) { - } } diff --git a/core/java/android/util/imetracing/ImeTracingServerImpl.java b/core/java/android/util/imetracing/ImeTracingServerImpl.java index 77f017a4654a8..06e4c5002776e 100644 --- a/core/java/android/util/imetracing/ImeTracingServerImpl.java +++ b/core/java/android/util/imetracing/ImeTracingServerImpl.java @@ -139,14 +139,6 @@ class ImeTracingServerImpl extends ImeTracing { } } - @GuardedBy("mEnabledLock") - @Override - public void writeTracesToFiles() { - synchronized (mEnabledLock) { - writeTracesToFilesLocked(); - } - } - private void writeTracesToFilesLocked() { try { ProtoOutputStream clientsProto = new ProtoOutputStream(); @@ -192,12 +184,6 @@ class ImeTracingServerImpl extends ImeTracing { @Override public void stopTrace(@Nullable PrintWriter pw) { - stopTrace(pw, true /* writeToFile */); - } - - @GuardedBy("mEnabledLock") - @Override - public void stopTrace(@Nullable PrintWriter pw, boolean writeToFile) { if (IS_USER) { Log.w(TAG, "Warn: Tracing is not supported on user builds."); return; @@ -213,9 +199,35 @@ class ImeTracingServerImpl extends ImeTracing { + TRACE_FILENAME_CLIENTS + ", " + TRACE_FILENAME_IMS + ", " + TRACE_FILENAME_IMMS); sEnabled = false; - if (writeToFile) { - writeTracesToFilesLocked(); + writeTracesToFilesLocked(); + } + } + + /** + * {@inheritDoc} + */ + @Override + public void saveForBugreport(@Nullable PrintWriter pw) { + if (IS_USER) { + return; + } + synchronized (mEnabledLock) { + if (!isAvailable() || !isEnabled()) { + return; } + // Temporarily stop accepting logs from trace event providers. There is a small chance + // that we may drop some trace events while writing the file, but we currently need to + // live with that. Note that addToBuffer() also has a bug that it doesn't do + // read-acquire so flipping sEnabled here doesn't even guarantee that addToBuffer() will + // temporarily stop accepting incoming events... + // TODO(b/175761228): Implement atomic snapshot to avoid downtime. + // TODO(b/175761228): Fix synchronization around sEnabled. + sEnabled = false; + logAndPrintln(pw, "Writing traces in " + TRACE_DIRNAME + ": " + + TRACE_FILENAME_CLIENTS + ", " + TRACE_FILENAME_IMS + ", " + + TRACE_FILENAME_IMMS); + writeTracesToFilesLocked(); + sEnabled = true; } } diff --git a/services/core/java/com/android/server/inputmethod/InputMethodManagerService.java b/services/core/java/com/android/server/inputmethod/InputMethodManagerService.java index d0dfacb83da24..b0b810e5c51d5 100644 --- a/services/core/java/com/android/server/inputmethod/InputMethodManagerService.java +++ b/services/core/java/com/android/server/inputmethod/InputMethodManagerService.java @@ -175,7 +175,6 @@ import com.android.internal.inputmethod.StartInputReason; import com.android.internal.inputmethod.UnbindReason; import com.android.internal.messages.nano.SystemMessageProto.SystemMessage; import com.android.internal.notification.SystemNotificationChannels; -import com.android.internal.os.BackgroundThread; import com.android.internal.os.HandlerCaller; import com.android.internal.os.SomeArgs; import com.android.internal.os.TransferPipe; @@ -5230,15 +5229,6 @@ public class InputMethodManagerService extends IInputMethodManager.Stub if (!DumpUtils.checkDumpPermission(mContext, TAG, pw)) return; if (ArrayUtils.contains(args, PROTO_ARG)) { - final ImeTracing imeTracing = ImeTracing.getInstance(); - if (imeTracing.isEnabled()) { - imeTracing.stopTrace(null, false /* writeToFile */); - BackgroundThread.getHandler().post(() -> { - imeTracing.writeTracesToFiles(); - imeTracing.startTrace(null); - }); - } - final ProtoOutputStream proto = new ProtoOutputStream(fd); dumpDebug(proto, InputMethodManagerServiceTraceProto.INPUT_METHOD_MANAGER_SERVICE); proto.flush(); @@ -5816,7 +5806,7 @@ public class InputMethodManagerService extends IInputMethodManager.Stub } /** - * Handles {@code adb shell ime tracing start/stop}. + * Handles {@code adb shell cmd input_method tracing start/stop/save-for-bugreport}. * @param shellCommand {@link ShellCommand} object that is handling this command. * @return Exit code of the command. */ @@ -5828,16 +5818,19 @@ public class InputMethodManagerService extends IInputMethodManager.Stub switch (cmd) { case "start": ImeTracing.getInstance().getInstance().startTrace(pw); - break; + break; // proceed to the next step to update the IME client processes. case "stop": ImeTracing.getInstance().stopTrace(pw); - break; + break; // proceed to the next step to update the IME client processes. + case "save-for-bugreport": + ImeTracing.getInstance().saveForBugreport(pw); + return ShellCommandResult.SUCCESS; // no need to update the IME client processes. default: pw.println("Unknown command: " + cmd); pw.println("Input method trace options:"); pw.println(" start: Start tracing"); pw.println(" stop: Stop tracing"); - return ShellCommandResult.FAILURE; + return ShellCommandResult.FAILURE; // no need to update the IME client processes. } boolean isImeTraceEnabled = ImeTracing.getInstance().isEnabled(); ArrayMap clients;