OSDN Git Service

Plumb real stack traces through StrictMode
authorKurt Nelson <kurtn@google.com>
Thu, 7 Sep 2017 17:56:56 +0000 (10:56 -0700)
committerKurt Nelson <kurtn@google.com>
Mon, 23 Oct 2017 17:04:19 +0000 (10:04 -0700)
Currently StrictMode uses a string representation of the entire stack
trace throughout. Switching to passing Throwables will allow callback
consumers to traverse an array.

It does not regress the performance test added in ag/3083879.

Test: adb shell am instrument -w -e class android.os.StrictModeTest \
    com.android.perftests.core/android.support.test.runner.AndroidJUnitRunner

timeThreadViolation_mean=332071
timeThreadViolation_median=328184
timeThreadViolation_min=311253
timeThreadViolation_standardDeviation=16106

timeCrossBinderThreadViolationNoStrictMode_mean=1843599
timeCrossBinderThreadViolationNoStrictMode_median=1824457
timeCrossBinderThreadViolationNoStrictMode_min=1810186
timeCrossBinderThreadViolationNoStrictMode_standardDeviation=43539

timeCrossBinderThreadViolation_mean=2300256
timeCrossBinderThreadViolation_median=2148796
timeCrossBinderThreadViolation_min=1792660
timeCrossBinderThreadViolation_standardDeviation=472271

timeVmViolationNoStrictMode_mean=27794864
timeVmViolationNoStrictMode_median=26617027
timeVmViolationNoStrictMode_min=23994153
timeVmViolationNoStrictMode_standardDeviation=3384362

timeVmViolation_mean=32878535
timeVmViolation_median=34775241
timeVmViolation_min=28373537
timeVmViolation_standardDeviation=3462046

timeThreadViolationNoStrictMode_mean=373863
timeThreadViolationNoStrictMode_median=388998
timeThreadViolationNoStrictMode_min=333664
timeThreadViolationNoStrictMode_standardDeviation=33219

Bug: 62458734
Change-Id: I6b3924be91f19654c502e0ec2f44cc07d6e86e3f
Test: cts-tradefed run cts-dev --module CtsOsTestCases --test
android.os.cts.StrictModeTest

api/test-current.txt
core/java/android/os/StrictMode.java
services/core/java/com/android/server/am/ActivityManagerService.java

index faccb6b..921cb82 100644 (file)
@@ -32044,17 +32044,18 @@ package android.os {
   public static final class StrictMode.ViolationInfo implements android.os.Parcelable {
     ctor public StrictMode.ViolationInfo();
     ctor public StrictMode.ViolationInfo(java.lang.Throwable, int);
-    ctor public StrictMode.ViolationInfo(java.lang.String, java.lang.Throwable, int);
+    ctor public deprecated StrictMode.ViolationInfo(java.lang.String, java.lang.Throwable, int);
     ctor public StrictMode.ViolationInfo(android.os.Parcel);
     ctor public StrictMode.ViolationInfo(android.os.Parcel, boolean);
     method public int describeContents();
     method public void dump(android.util.Printer, java.lang.String);
+    method public java.lang.String getMessagePrefix();
+    method public java.lang.String getStackTrace();
+    method public java.lang.String getViolationDetails();
     method public void writeToParcel(android.os.Parcel, int);
     field public static final android.os.Parcelable.Creator<android.os.StrictMode.ViolationInfo> CREATOR;
     field public java.lang.String broadcastIntentAction;
-    field public final android.app.ApplicationErrorReport.CrashInfo crashInfo;
     field public int durationMillis;
-    field public final java.lang.String message;
     field public int numAnimationsRunning;
     field public long numInstances;
     field public final int policy;
index 826ec1e..f52d94e 100644 (file)
@@ -20,7 +20,6 @@ import android.annotation.Nullable;
 import android.annotation.TestApi;
 import android.app.ActivityManager;
 import android.app.ActivityThread;
-import android.app.ApplicationErrorReport;
 import android.app.IActivityManager;
 import android.content.BroadcastReceiver;
 import android.content.Context;
@@ -48,8 +47,10 @@ import java.io.PrintWriter;
 import java.io.StringWriter;
 import java.net.InetAddress;
 import java.net.UnknownHostException;
+import java.util.ArrayDeque;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Deque;
 import java.util.HashMap;
 import java.util.concurrent.atomic.AtomicInteger;
 
@@ -352,8 +353,8 @@ public final class StrictMode {
                 } else {
                     msg = "StrictMode policy violation:";
                 }
-                if (info.crashInfo != null) {
-                    Log.d(TAG, msg + " " + info.crashInfo.stackTrace);
+                if (info.hasStackTrace()) {
+                    Log.d(TAG, msg + " " + info.getStackTrace());
                 } else {
                     Log.d(TAG, msg + " missing stack trace!");
                 }
@@ -1247,28 +1248,6 @@ public final class StrictMode {
         }
     }
 
-    /** Like parsePolicyFromMessage(), but returns the violation. */
-    private static int parseViolationFromMessage(String message) {
-        if (message == null) {
-            return 0;
-        }
-        int violationIndex = message.indexOf("violation=");
-        if (violationIndex == -1) {
-            return 0;
-        }
-        int numberStartIndex = violationIndex + "violation=".length();
-        int numberEndIndex = message.indexOf(' ', numberStartIndex);
-        if (numberEndIndex == -1) {
-            numberEndIndex = message.length();
-        }
-        String violationString = message.substring(numberStartIndex, numberEndIndex);
-        try {
-            return Integer.parseInt(violationString);
-        } catch (NumberFormatException e) {
-            return 0;
-        }
-    }
-
     private static final ThreadLocal<ArrayList<ViolationInfo>> violationsBeingTimed =
             new ThreadLocal<ArrayList<ViolationInfo>>() {
                 @Override
@@ -1516,7 +1495,7 @@ public final class StrictMode {
         // to people who push/pop temporary policy in regions of code,
         // hence the policy being passed around.
         void handleViolation(final ViolationInfo info) {
-            if (info == null || info.crashInfo == null || info.crashInfo.stackTrace == null) {
+            if (info == null || !info.hasStackTrace()) {
                 Log.wtf(TAG, "unexpected null stacktrace");
                 return;
             }
@@ -1530,7 +1509,7 @@ public final class StrictMode {
                     gatheredViolations.set(violations);
                 }
                 for (ViolationInfo previous : violations) {
-                    if (info.crashInfo.stackTrace.equals(previous.crashInfo.stackTrace)) {
+                    if (info.getStackTrace().equals(previous.getStackTrace())) {
                         // Duplicate. Don't log.
                         return;
                     }
@@ -1576,8 +1555,7 @@ public final class StrictMode {
             }
 
             if (violationMaskSubset != 0) {
-                int violationBit = parseViolationFromMessage(info.crashInfo.exceptionMessage);
-                violationMaskSubset |= violationBit;
+                violationMaskSubset |= info.getViolationBit();
                 final int savedPolicyMask = getThreadPolicyMask();
 
                 final boolean justDropBox = (info.policy & THREAD_PENALTY_MASK) == PENALTY_DROPBOX;
@@ -1622,8 +1600,7 @@ public final class StrictMode {
     }
 
     private static void executeDeathPenalty(ViolationInfo info) {
-        int violationBit = parseViolationFromMessage(info.crashInfo.exceptionMessage);
-        throw new StrictModeViolation(info.policy, violationBit, null);
+        throw new StrictModeViolation(info.policy, info.getViolationBit(), null);
     }
 
     /**
@@ -2027,21 +2004,14 @@ public final class StrictMode {
      * read back all the encoded violations.
      */
     /* package */ static void readAndHandleBinderCallViolations(Parcel p) {
-        // Our own stack trace to append
-        StringWriter sw = new StringWriter();
-        sw.append("# via Binder call with stack:\n");
-        PrintWriter pw = new FastPrintWriter(sw, false, 256);
-        new LogStackTrace().printStackTrace(pw);
-        pw.flush();
-        String ourStack = sw.toString();
-
+        LogStackTrace localCallSite = new LogStackTrace();
         final int policyMask = getThreadPolicyMask();
         final boolean currentlyGathering = (policyMask & PENALTY_GATHER) != 0;
 
         final int size = p.readInt();
         for (int i = 0; i < size; i++) {
             final ViolationInfo info = new ViolationInfo(p, !currentlyGathering);
-            info.crashInfo.appendStackTrace(ourStack);
+            info.addLocalStack(localCallSite);
             BlockGuard.Policy policy = BlockGuard.getThreadPolicy();
             if (policy instanceof AndroidBlockGuardPolicy) {
                 ((AndroidBlockGuardPolicy) policy).handleViolationWithTimingAttempt(info);
@@ -2366,10 +2336,18 @@ public final class StrictMode {
      */
     @TestApi
     public static final class ViolationInfo implements Parcelable {
-        public final String message;
+        /** Some VM violations provide additional information outside the throwable. */
+        @Nullable private final String mMessagePrefix;
 
-        /** Stack and other stuff info. */
-        @Nullable public final ApplicationErrorReport.CrashInfo crashInfo;
+        /** Stack and violation details. */
+        @Nullable private final Throwable mThrowable;
+
+        private final Deque<Throwable> mBinderStack = new ArrayDeque<>();
+
+        /** Memoized stack trace of full violation. */
+        @Nullable private String mStackTrace;
+        /** Memoized violation bit. */
+        private int mViolationBit;
 
         /** The strict mode policy mask at the time of violation. */
         public final int policy;
@@ -2404,19 +2382,25 @@ public final class StrictMode {
 
         /** Create an uninitialized instance of ViolationInfo */
         public ViolationInfo() {
-            message = null;
-            crashInfo = null;
+            mMessagePrefix = null;
+            mThrowable = null;
             policy = 0;
         }
 
+        /** Create an instance of ViolationInfo. */
         public ViolationInfo(Throwable tr, int policy) {
             this(null, tr, policy);
         }
 
-        /** Create an instance of ViolationInfo initialized from an exception. */
-        public ViolationInfo(String message, Throwable tr, int policy) {
-            this.message = message;
-            crashInfo = new ApplicationErrorReport.CrashInfo(tr);
+        /**
+         * Create an instance of ViolationInfo initialized from an exception with a message prefix.
+         *
+         * @deprecated prefixes belong in the Throwable.
+         */
+        @Deprecated
+        public ViolationInfo(String messagePrefix, Throwable tr, int policy) {
+            this.mMessagePrefix = messagePrefix;
+            this.mThrowable = tr;
             violationUptimeMillis = SystemClock.uptimeMillis();
             this.policy = policy;
             this.numAnimationsRunning = ValueAnimator.getCurrentAnimationsCount();
@@ -2446,11 +2430,101 @@ public final class StrictMode {
             }
         }
 
+        /** Equivalent output to {@link ApplicationErrorReport.CrashInfo#stackTrace}. */
+        public String getStackTrace() {
+            if (mThrowable != null && mStackTrace == null) {
+                StringWriter sw = new StringWriter();
+                PrintWriter pw = new FastPrintWriter(sw, false, 256);
+                mThrowable.printStackTrace(pw);
+                for (Throwable t : mBinderStack) {
+                    pw.append("# via Binder call with stack:\n");
+                    t.printStackTrace(pw);
+                }
+                pw.flush();
+                pw.close();
+                mStackTrace = sw.toString();
+            }
+            return mStackTrace;
+        }
+
+        /**
+         * Optional message describing this violation.
+         *
+         * @hide
+         */
+        @TestApi
+        public String getViolationDetails() {
+            if (mThrowable != null) {
+                return mThrowable.getMessage();
+            } else {
+                return "";
+            }
+        }
+
+        /**
+         * A handful of VM violations provide extra information that should be presented before
+         * {@link #getViolationDetails()}.
+         *
+         * @hide
+         */
+        @TestApi
+        public String getMessagePrefix() {
+            return mMessagePrefix != null ? mMessagePrefix : "";
+        }
+
+        /** If this violation has a useful stack trace.
+         *
+         * @hide
+         */
+        public boolean hasStackTrace() {
+            return mThrowable != null;
+        }
+
+        /**
+         * Add a {@link Throwable} from the current process that caused the underlying violation.
+         *
+         * @hide
+         */
+        void addLocalStack(Throwable t) {
+            mBinderStack.addFirst(t);
+        }
+
+        /**
+         * Retrieve the type of StrictMode violation.
+         *
+         * @hide
+         */
+        int getViolationBit() {
+            if (mThrowable == null || mThrowable.getMessage() == null) {
+                return 0;
+            }
+            if (mViolationBit != 0) {
+                return mViolationBit;
+            }
+            String message = mThrowable.getMessage();
+            int violationIndex = message.indexOf("violation=");
+            if (violationIndex == -1) {
+                return 0;
+            }
+            int numberStartIndex = violationIndex + "violation=".length();
+            int numberEndIndex = message.indexOf(' ', numberStartIndex);
+            if (numberEndIndex == -1) {
+                numberEndIndex = message.length();
+            }
+            String violationString = message.substring(numberStartIndex, numberEndIndex);
+            try {
+                mViolationBit = Integer.parseInt(violationString);
+                return mViolationBit;
+            } catch (NumberFormatException e) {
+                return 0;
+            }
+        }
+
         @Override
         public int hashCode() {
             int result = 17;
-            if (crashInfo != null) {
-                result = 37 * result + crashInfo.stackTrace.hashCode();
+            if (mThrowable != null) {
+                result = 37 * result + mThrowable.hashCode();
             }
             if (numAnimationsRunning != 0) {
                 result *= 37;
@@ -2478,11 +2552,11 @@ public final class StrictMode {
          *     should be removed.
          */
         public ViolationInfo(Parcel in, boolean unsetGatheringBit) {
-            message = in.readString();
-            if (in.readInt() != 0) {
-                crashInfo = new ApplicationErrorReport.CrashInfo(in);
-            } else {
-                crashInfo = null;
+            mMessagePrefix = in.readString();
+            mThrowable = (Throwable) in.readSerializable();
+            int binderStackSize = in.readInt();
+            for (int i = 0; i < binderStackSize; i++) {
+                mBinderStack.add((Throwable) in.readSerializable());
             }
             int rawPolicy = in.readInt();
             if (unsetGatheringBit) {
@@ -2502,12 +2576,11 @@ public final class StrictMode {
         /** Save a ViolationInfo instance to a parcel. */
         @Override
         public void writeToParcel(Parcel dest, int flags) {
-            dest.writeString(message);
-            if (crashInfo != null) {
-                dest.writeInt(1);
-                crashInfo.writeToParcel(dest, flags);
-            } else {
-                dest.writeInt(0);
+            dest.writeString(mMessagePrefix);
+            dest.writeSerializable(mThrowable);
+            dest.writeInt(mBinderStack.size());
+            for (Throwable t : mBinderStack) {
+                dest.writeSerializable(t);
             }
             int start = dest.dataPosition();
             dest.writeInt(policy);
@@ -2542,8 +2615,8 @@ public final class StrictMode {
 
         /** Dump a ViolationInfo instance to a Printer. */
         public void dump(Printer pw, String prefix) {
-            if (crashInfo != null) {
-                crashInfo.dump(pw, prefix);
+            if (mThrowable != null) {
+                pw.println(prefix + "stackTrace: " + getStackTrace());
             }
             pw.println(prefix + "policy: " + policy);
             if (durationMillis != -1) {
index e1e53b3..13f12e2 100644 (file)
@@ -14239,12 +14239,12 @@ public class ActivityManagerService extends IActivityManager.Stub
                 }
             }
             sb.append("\n");
-            if (info.crashInfo != null && info.crashInfo.stackTrace != null) {
-                sb.append(info.crashInfo.stackTrace);
+            if (info.hasStackTrace()) {
+                sb.append(info.getStackTrace());
                 sb.append("\n");
             }
-            if (info.message != null) {
-                sb.append(info.message);
+            if (info.getViolationDetails() != null) {
+                sb.append(info.getViolationDetails());
                 sb.append("\n");
             }