Revert^3 "Change the way tombstones are added to dropbox."

This reverts commit 225bfb760a3dade91aa599a86ecc3e818fa96b20.

Reason for revert: We are possibly losing tombstones

Change-Id: I8372ae3b7b5db63bc48155eca63eb3cae41239c8

Android.bp

@@ -204,7 +204,6 @@ java_library {
         "apex_aidl_interface-java",
         "packagemanager_aidl-java",
         "framework-protos",
-        "libtombstone_proto_java",
         "updatable-driver-protos",
         "ota_metadata_proto_java",
         "android.hidl.base-V1.0-java",

ProtoLibraries.bp

@@ -34,6 +34,7 @@ gensrcs {
         ":ipconnectivity-proto-src",
         ":libstats_atom_enum_protos",
         ":libstats_atom_message_protos",
+        ":libtombstone_proto-src",
         "core/proto/**/*.proto",
         "libs/incident/**/*.proto",
     ],

STABILITY_OWNERS

@@ -1,2 +0,0 @@
-gaillard@google.com
-

services/core/java/com/android/server/BootReceiver.java

@@ -48,8 +48,6 @@ import com.android.internal.util.XmlUtils;
 import com.android.modules.utils.TypedXmlPullParser;
 import com.android.modules.utils.TypedXmlSerializer;
 import com.android.server.am.DropboxRateLimiter;
-import com.android.server.os.TombstoneProtos;
-import com.android.server.os.TombstoneProtos.Tombstone;
 
 import org.xmlpull.v1.XmlPullParser;
 import org.xmlpull.v1.XmlPullParserException;
@@ -62,14 +60,11 @@ import java.io.FileOutputStream;
 import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.attribute.PosixFilePermissions;
-import java.util.AbstractMap;
 import java.util.HashMap;
 import java.util.Iterator;
-import java.util.Map;
 import java.util.concurrent.locks.ReentrantLock;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
-import java.util.stream.Collectors;
 
 /**
  * Performs a number of miscellaneous, non-system-critical actions
@@ -332,12 +327,12 @@ public class BootReceiver extends BroadcastReceiver {
      *
      * @param ctx Context
      * @param tombstone path to the tombstone
-     * @param tombstoneProto the parsed proto tombstone
+     * @param proto whether the tombstone is stored as proto
      * @param processName the name of the process corresponding to the tombstone
      * @param tmpFileLock the lock for reading/writing tmp files
      */
     public static void addTombstoneToDropBox(
-                Context ctx, File tombstone, Tombstone tombstoneProto, String processName,
+                Context ctx, File tombstone, boolean proto, String processName,
                 ReentrantLock tmpFileLock) {
         final DropBoxManager db = ctx.getSystemService(DropBoxManager.class);
         if (db == null) {
@@ -347,34 +342,32 @@ public class BootReceiver extends BroadcastReceiver {
 
         // Check if we should rate limit and abort early if needed.
         DropboxRateLimiter.RateLimitResult rateLimitResult =
-                sDropboxRateLimiter.shouldRateLimit(TAG_TOMBSTONE_PROTO_WITH_HEADERS, processName);
+                sDropboxRateLimiter.shouldRateLimit(
+                        proto ? TAG_TOMBSTONE_PROTO_WITH_HEADERS : TAG_TOMBSTONE, processName);
         if (rateLimitResult.shouldRateLimit()) return;
 
         HashMap<String, Long> timestamps = readTimestamps();
         try {
-            // Remove the memory data from the proto.
+            if (proto) {
-            Tombstone tombstoneProtoWithoutMemory = removeMemoryFromTombstone(tombstoneProto);
-
-            final byte[] tombstoneBytes = tombstoneProtoWithoutMemory.toByteArray();
-
-            // Use JNI to call the c++ proto to text converter and add the headers to the tombstone.
-            String tombstoneWithoutMemory = new StringBuilder(getBootHeadersToLogAndUpdate())
-                    .append(rateLimitResult.createHeader())
-                    .append(getTombstoneText(tombstoneBytes))
-                    .toString();
-
-            // Add the tombstone without memory data to dropbox.
-            db.addText(TAG_TOMBSTONE, tombstoneWithoutMemory);
-
-            // Add the tombstone proto to dropbox.
                 if (recordFileTimestamp(tombstone, timestamps)) {
+                    // We need to attach the count indicating the number of dropped dropbox entries
+                    // due to rate limiting. Do this by enclosing the proto tombsstone in a
+                    // container proto that has the dropped entry count and the proto tombstone as
+                    // bytes (to avoid the complexity of reading and writing nested protos).
                     tmpFileLock.lock();
                     try {
-                    addAugmentedProtoToDropbox(tombstone, tombstoneBytes, db, rateLimitResult);
+                        addAugmentedProtoToDropbox(tombstone, db, rateLimitResult);
                     } finally {
                         tmpFileLock.unlock();
                     }
                 }
+            } else {
+                // Add the header indicating how many events have been dropped due to rate limiting.
+                final String headers = getBootHeadersToLogAndUpdate()
+                        + rateLimitResult.createHeader();
+                addFileToDropBox(db, timestamps, headers, tombstone.getPath(), LOG_SIZE,
+                                 TAG_TOMBSTONE);
+            }
         } catch (IOException e) {
             Slog.e(TAG, "Can't log tombstone", e);
         }
@@ -382,8 +375,11 @@ public class BootReceiver extends BroadcastReceiver {
     }
 
     private static void addAugmentedProtoToDropbox(
-                File tombstone, byte[] tombstoneBytes, DropBoxManager db,
+                File tombstone, DropBoxManager db,
                 DropboxRateLimiter.RateLimitResult rateLimitResult) throws IOException {
+        // Read the proto tombstone file as bytes.
+        final byte[] tombstoneBytes = Files.readAllBytes(tombstone.toPath());
+
         final File tombstoneProtoWithHeaders = File.createTempFile(
                 tombstone.getName(), ".tmp", TOMBSTONE_TMP_DIR);
         Files.setPosixFilePermissions(
@@ -416,8 +412,6 @@ public class BootReceiver extends BroadcastReceiver {
         }
     }
 
-    private static native String getTombstoneText(byte[] tombstoneBytes);
-
     private static void addLastkToDropBox(
             DropBoxManager db, HashMap<String, Long> timestamps,
             String headers, String footers, String filename, int maxSize,
@@ -435,31 +429,6 @@ public class BootReceiver extends BroadcastReceiver {
         addFileWithFootersToDropBox(db, timestamps, headers, footers, filename, maxSize, tag);
     }
 
-    /** Removes memory information from the Tombstone proto. */
-    @VisibleForTesting
-    public static Tombstone removeMemoryFromTombstone(Tombstone tombstoneProto) {
-        Tombstone.Builder tombstoneBuilder = tombstoneProto.toBuilder()
-                .clearMemoryMappings()
-                .clearThreads()
-                .putAllThreads(tombstoneProto.getThreadsMap().entrySet()
-                        .stream()
-                        .map(BootReceiver::clearMemoryDump)
-                        .collect(Collectors.toMap(e->e.getKey(), e->e.getValue())));
-
-        if (tombstoneProto.hasSignalInfo()) {
-            tombstoneBuilder.setSignalInfo(
-                    tombstoneProto.getSignalInfo().toBuilder().clearFaultAdjacentMetadata());
-        }
-
-        return tombstoneBuilder.build();
-    }
-
-    private static AbstractMap.SimpleEntry<Integer, TombstoneProtos.Thread> clearMemoryDump(
-            Map.Entry<Integer, TombstoneProtos.Thread> e) {
-        return new AbstractMap.SimpleEntry<Integer, TombstoneProtos.Thread>(
-            e.getKey(), e.getValue().toBuilder().clearMemoryDump().build());
-    }
-
     private static void addFileToDropBox(
             DropBoxManager db, HashMap<String, Long> timestamps,
             String headers, String filename, int maxSize, String tag) throws IOException {

services/core/java/com/android/server/os/NativeTombstoneManager.java

@@ -41,13 +41,14 @@ import android.system.Os;
 import android.system.StructStat;
 import android.util.Slog;
 import android.util.SparseArray;
+import android.util.proto.ProtoInputStream;
+import android.util.proto.ProtoParseException;
 
 import com.android.internal.annotations.GuardedBy;
 import com.android.server.BootReceiver;
 import com.android.server.ServiceThread;
 import com.android.server.os.TombstoneProtos.Cause;
 import com.android.server.os.TombstoneProtos.Tombstone;
-import com.android.server.os.protobuf.CodedInputStream;
 
 import libcore.io.IoUtils;
 
@@ -127,21 +128,18 @@ public final class NativeTombstoneManager {
             return;
         }
 
+        String processName = "UNKNOWN";
         final boolean isProtoFile = filename.endsWith(".pb");
-        if (!isProtoFile) {
+        File protoPath = isProtoFile ? path : new File(path.getAbsolutePath() + ".pb");
-            return;
-        }
 
-        Optional<ParsedTombstone> parsedTombstone = handleProtoTombstone(path, true);
+        Optional<TombstoneFile> parsedTombstone = handleProtoTombstone(protoPath, isProtoFile);
         if (parsedTombstone.isPresent()) {
-            BootReceiver.addTombstoneToDropBox(
+            processName = parsedTombstone.get().getProcessName();
-                    mContext, path, parsedTombstone.get().getTombstone(),
-                    parsedTombstone.get().getProcessName(), mTmpFileLock);
         }
+        BootReceiver.addTombstoneToDropBox(mContext, path, isProtoFile, processName, mTmpFileLock);
     }
 
-    private Optional<ParsedTombstone> handleProtoTombstone(
+    private Optional<TombstoneFile> handleProtoTombstone(File path, boolean addToList) {
-            File path, boolean addToList) {
         final String filename = path.getName();
         if (!filename.endsWith(".pb")) {
             Slog.w(TAG, "unexpected tombstone name: " + path);
@@ -171,7 +169,7 @@ public final class NativeTombstoneManager {
             return Optional.empty();
         }
 
-        final Optional<ParsedTombstone> parsedTombstone = TombstoneFile.parse(pfd);
+        final Optional<TombstoneFile> parsedTombstone = TombstoneFile.parse(pfd);
         if (!parsedTombstone.isPresent()) {
             IoUtils.closeQuietly(pfd);
             return Optional.empty();
@@ -184,7 +182,7 @@ public final class NativeTombstoneManager {
                     previous.dispose();
                 }
 
-                mTombstones.put(number, parsedTombstone.get().getTombstoneFile());
+                mTombstones.put(number, parsedTombstone.get());
             }
         }
 
@@ -332,27 +330,6 @@ public final class NativeTombstoneManager {
         }
     }
 
-    static class ParsedTombstone {
-        TombstoneFile mTombstoneFile;
-        Tombstone mTombstone;
-        ParsedTombstone(TombstoneFile tombstoneFile, Tombstone tombstone) {
-            mTombstoneFile = tombstoneFile;
-            mTombstone = tombstone;
-        }
-
-        public String getProcessName() {
-            return mTombstoneFile.getProcessName();
-        }
-
-        public TombstoneFile getTombstoneFile() {
-            return mTombstoneFile;
-        }
-
-        public Tombstone getTombstone() {
-            return mTombstone;
-        }
-    }
-
     static class TombstoneFile {
         final ParcelFileDescriptor mPfd;
 
@@ -435,21 +412,67 @@ public final class NativeTombstoneManager {
             }
         }
 
-        static Optional<ParsedTombstone> parse(ParcelFileDescriptor pfd) {
+        static Optional<TombstoneFile> parse(ParcelFileDescriptor pfd) {
-            Tombstone tombstoneProto;
+            final FileInputStream is = new FileInputStream(pfd.getFileDescriptor());
-            try (FileInputStream is = new FileInputStream(pfd.getFileDescriptor())) {
+            final ProtoInputStream stream = new ProtoInputStream(is);
-                final byte[] tombstoneBytes = is.readAllBytes();
 
-                tombstoneProto = Tombstone.parseFrom(
+            int pid = 0;
-                        CodedInputStream.newInstance(tombstoneBytes));
+            int uid = 0;
-            } catch (IOException ex) {
+            String processName = null;
+            String crashReason = "";
+            String selinuxLabel = "";
+
+            try {
+                while (stream.nextField() != ProtoInputStream.NO_MORE_FIELDS) {
+                    switch (stream.getFieldNumber()) {
+                        case (int) Tombstone.PID:
+                            pid = stream.readInt(Tombstone.PID);
+                            break;
+
+                        case (int) Tombstone.UID:
+                            uid = stream.readInt(Tombstone.UID);
+                            break;
+
+                        case (int) Tombstone.COMMAND_LINE:
+                            if (processName == null) {
+                                processName = stream.readString(Tombstone.COMMAND_LINE);
+                            }
+                            break;
+
+                        case (int) Tombstone.CAUSES:
+                            if (!crashReason.equals("")) {
+                                // Causes appear in decreasing order of likelihood. For now we only
+                                // want the most likely crash reason here, so ignore all others.
+                                break;
+                            }
+                            long token = stream.start(Tombstone.CAUSES);
+                        cause:
+                            while (stream.nextField() != ProtoInputStream.NO_MORE_FIELDS) {
+                                switch (stream.getFieldNumber()) {
+                                    case (int) Cause.HUMAN_READABLE:
+                                        crashReason = stream.readString(Cause.HUMAN_READABLE);
+                                        break cause;
+
+                                    default:
+                                        break;
+                                }
+                            }
+                            stream.end(token);
+                            break;
+
+                        case (int) Tombstone.SELINUX_LABEL:
+                            selinuxLabel = stream.readString(Tombstone.SELINUX_LABEL);
+                            break;
+
+                        default:
+                            break;
+                    }
+                }
+            } catch (IOException | ProtoParseException ex) {
                 Slog.e(TAG, "Failed to parse tombstone", ex);
                 return Optional.empty();
             }
 
-            int pid = tombstoneProto.getPid();
-            int uid = tombstoneProto.getUid();
-
             if (!UserHandle.isApp(uid)) {
                 Slog.e(TAG, "Tombstone's UID (" + uid + ") not an app, ignoring");
                 return Optional.empty();
@@ -466,7 +489,6 @@ public final class NativeTombstoneManager {
             final int userId = UserHandle.getUserId(uid);
             final int appId = UserHandle.getAppId(uid);
 
-            String selinuxLabel = tombstoneProto.getSelinuxLabel();
             if (!selinuxLabel.startsWith("u:r:untrusted_app")) {
                 Slog.e(TAG, "Tombstone has invalid selinux label (" + selinuxLabel + "), ignoring");
                 return Optional.empty();
@@ -478,30 +500,11 @@ public final class NativeTombstoneManager {
             result.mAppId = appId;
             result.mPid = pid;
             result.mUid = uid;
-            result.mProcessName = getCmdLineProcessName(tombstoneProto);
+            result.mProcessName = processName == null ? "" : processName;
             result.mTimestampMs = timestampMs;
-            result.mCrashReason = getCrashReason(tombstoneProto);
+            result.mCrashReason = crashReason;
 
-            return Optional.of(new ParsedTombstone(result, tombstoneProto));
+            return Optional.of(result);
-        }
-
-        private static String getCmdLineProcessName(Tombstone tombstoneProto) {
-            for (String cmdline : tombstoneProto.getCommandLineList()) {
-                if (cmdline != null) {
-                    return cmdline;
-                }
-            }
-            return "";
-        }
-
-        private static String getCrashReason(Tombstone tombstoneProto) {
-            for (Cause cause : tombstoneProto.getCausesList()) {
-                if (cause.getHumanReadable() != null
-                        && !cause.getHumanReadable().equals("")) {
-                    return cause.getHumanReadable();
-                }
-            }
-            return "";
         }
 
         public IParcelFileDescriptorRetriever getPfdRetriever() {

services/core/jni/Android.bp

@@ -37,7 +37,6 @@ cc_library_static {
         "com_android_server_adb_AdbDebuggingManager.cpp",
         "com_android_server_am_BatteryStatsService.cpp",
         "com_android_server_biometrics_SurfaceToNativeHandleConverter.cpp",
-        "com_android_server_BootReceiver.cpp",
         "com_android_server_ConsumerIrService.cpp",
         "com_android_server_companion_virtual_InputController.cpp",
         "com_android_server_devicepolicy_CryptoTestHelper.cpp",
@@ -92,16 +91,6 @@ cc_library_static {
     header_libs: [
         "bionic_libc_platform_headers",
     ],
-
-    static_libs: [
-        "libunwindstack",
-    ],
-
-    whole_static_libs: [
-        "libdebuggerd_tombstone_proto_to_text",
-    ],
-
-    runtime_libs: ["libdexfile"],
 }
 
 cc_defaults {

services/core/jni/OWNERS

@@ -32,7 +32,3 @@ per-file com_android_server_companion_virtual_InputController.cpp = file:/servic
 
 # Bug component : 158088 = per-file *AnrTimer*
 per-file *AnrTimer* = file:/PERFORMANCE_OWNERS
-
-# Bug component : 158088 = per-file com_android_server_utils_AnrTimer*.java
-per-file com_android_server_utils_AnrTimer*.java = file:/PERFORMANCE_OWNERS
-per-file com_android_server_BootReceiver.cpp = file:/STABILITY_OWNERS

services/core/jni/com_android_server_BootReceiver.cpp

@@ -1,57 +0,0 @@
-/*
- * Copyright (C) 2023 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.
- */
-
-#include <libdebuggerd/tombstone.h>
-#include <nativehelper/JNIHelp.h>
-
-#include <sstream>
-
-#include "jni.h"
-#include "tombstone.pb.h"
-
-namespace android {
-
-static void writeToString(std::stringstream& ss, const std::string& line, bool should_log) {
-    ss << line << std::endl;
-}
-
-static jstring com_android_server_BootReceiver_getTombstoneText(JNIEnv* env, jobject,
-                                                                jbyteArray tombstoneBytes) {
-    Tombstone tombstone;
-    tombstone.ParseFromArray(env->GetByteArrayElements(tombstoneBytes, 0),
-                             env->GetArrayLength(tombstoneBytes));
-
-    std::stringstream tombstoneString;
-
-    tombstone_proto_to_text(tombstone,
-                            std::bind(&writeToString, std::ref(tombstoneString),
-                                      std::placeholders::_1, std::placeholders::_2));
-
-    return env->NewStringUTF(tombstoneString.str().c_str());
-}
-
-static const JNINativeMethod sMethods[] = {
-        /* name, signature, funcPtr */
-        {"getTombstoneText", "([B)Ljava/lang/String;",
-         (jstring*)com_android_server_BootReceiver_getTombstoneText},
-};
-
-int register_com_android_server_BootReceiver(JNIEnv* env) {
-    return jniRegisterNativeMethods(env, "com/android/server/BootReceiver", sMethods,
-                                    NELEM(sMethods));
-}
-
-} // namespace android

services/core/jni/onload.cpp

@@ -63,7 +63,6 @@ int register_android_server_stats_pull_StatsPullAtomService(JNIEnv* env);
 int register_android_server_sensor_SensorService(JavaVM* vm, JNIEnv* env);
 int register_android_server_companion_virtual_InputController(JNIEnv* env);
 int register_android_server_app_GameManagerService(JNIEnv* env);
-int register_com_android_server_BootReceiver(JNIEnv* env);
 int register_com_android_server_wm_TaskFpsCallbackController(JNIEnv* env);
 int register_com_android_server_display_DisplayControl(JNIEnv* env);
 int register_com_android_server_SystemClockTime(JNIEnv* env);
@@ -123,7 +122,6 @@ extern "C" jint JNI_OnLoad(JavaVM* vm, void* /* reserved */)
     register_android_server_sensor_SensorService(vm, env);
     register_android_server_companion_virtual_InputController(env);
     register_android_server_app_GameManagerService(env);
-    register_com_android_server_BootReceiver(env);
     register_com_android_server_wm_TaskFpsCallbackController(env);
     register_com_android_server_display_DisplayControl(env);
     register_com_android_server_SystemClockTime(env);

services/tests/servicestests/src/com/android/server/BootReceiverTest.java

@@ -1,97 +0,0 @@
-/*
- * Copyright (C) 2023 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.server;
-
-import static com.google.common.truth.Truth.assertThat;
-
-import android.test.AndroidTestCase;
-
-import com.android.server.os.TombstoneProtos;
-import com.android.server.os.TombstoneProtos.Tombstone;
-
-public class BootReceiverTest extends AndroidTestCase {
-    private static final String TAG = "BootReceiverTest";
-
-    public void testRemoveMemoryFromTombstone() {
-        Tombstone tombstoneBase = Tombstone.newBuilder()
-                .setBuildFingerprint("build_fingerprint")
-                .setRevision("revision")
-                .setPid(123)
-                .setTid(23)
-                .setUid(34)
-                .setSelinuxLabel("selinux_label")
-                .addCommandLine("cmd1")
-                .addCommandLine("cmd2")
-                .addCommandLine("cmd3")
-                .setProcessUptime(300)
-                .setAbortMessage("abort")
-                .addCauses(TombstoneProtos.Cause.newBuilder()
-                        .setHumanReadable("cause1")
-                        .setMemoryError(TombstoneProtos.MemoryError.newBuilder()
-                                .setTool(TombstoneProtos.MemoryError.Tool.SCUDO)
-                                .setType(TombstoneProtos.MemoryError.Type.DOUBLE_FREE)))
-                .addLogBuffers(TombstoneProtos.LogBuffer.newBuilder().setName("name").addLogs(
-                        TombstoneProtos.LogMessage.newBuilder()
-                                .setTimestamp("123")
-                                .setMessage("message")))
-                .addOpenFds(TombstoneProtos.FD.newBuilder().setFd(1).setPath("path"))
-                .build();
-
-        Tombstone tombstoneWithoutMemory = tombstoneBase.toBuilder()
-                .putThreads(1, TombstoneProtos.Thread.newBuilder()
-                        .setId(1)
-                        .setName("thread1")
-                        .addRegisters(TombstoneProtos.Register.newBuilder().setName("r1").setU64(1))
-                        .addRegisters(TombstoneProtos.Register.newBuilder().setName("r2").setU64(2))
-                        .addBacktraceNote("backtracenote1")
-                        .addUnreadableElfFiles("files1")
-                        .setTaggedAddrCtrl(1)
-                        .setPacEnabledKeys(10)
-                        .build())
-                .build();
-
-        Tombstone tombstoneWithMemory = tombstoneBase.toBuilder()
-                .addMemoryMappings(TombstoneProtos.MemoryMapping.newBuilder()
-                        .setBeginAddress(1)
-                        .setEndAddress(100)
-                        .setOffset(10)
-                        .setRead(true)
-                        .setWrite(true)
-                        .setExecute(false)
-                        .setMappingName("mapping")
-                        .setBuildId("build")
-                        .setLoadBias(70))
-                .putThreads(1, TombstoneProtos.Thread.newBuilder()
-                        .setId(1)
-                        .setName("thread1")
-                        .addRegisters(TombstoneProtos.Register.newBuilder().setName("r1").setU64(1))
-                        .addRegisters(TombstoneProtos.Register.newBuilder().setName("r2").setU64(2))
-                        .addBacktraceNote("backtracenote1")
-                        .addUnreadableElfFiles("files1")
-                        .addMemoryDump(TombstoneProtos.MemoryDump.newBuilder()
-                                .setRegisterName("register1")
-                                .setMappingName("mapping")
-                                .setBeginAddress(10))
-                        .setTaggedAddrCtrl(1)
-                        .setPacEnabledKeys(10)
-                        .build())
-                .build();
-
-        assertThat(BootReceiver.removeMemoryFromTombstone(tombstoneWithMemory))
-                .isEqualTo(tombstoneWithoutMemory);
-    }
-}