ART: Do not use proxy method as owner of lock

During lock inflation, it is possible to see a proxy method on
top of the stack, e.g., when it is allocating its args array.
However, proxy methods are not helpful for lock profiling, as
they do not contain specific caller data we could use.

Instead, try to walk the stack for the proxy's caller method,
and use that as the owner. This is acceptable as proxies can
never be synchronized methods (as interfaces methods aren't),
and the caller frame cannot be a proxy method itself.

Add a test. But the repro rate is really low.

Bug: 68871592
Test: m test-art-host
Change-Id: I02cf38c7d05e2c963a5dc489c57b6dd8009dccd4

5 files changed, 165 insertions, 0 deletions

diff --git a/runtime/monitor.cc b/runtime/monitor.cc
index 32201d97dd..cdc55bd45c 100644
--- a/runtime/monitor.cc
+++ b/runtime/monitor.cc
@@ -177,6 +177,42 @@ bool Monitor::Install(Thread* self) {
     // Do not abort on dex pc errors. This can easily happen when we want to dump a stack trace on
     // abort.
     locking_method_ = owner_->GetCurrentMethod(&locking_dex_pc_, false);
+    if (locking_method_ != nullptr && UNLIKELY(locking_method_->IsProxyMethod())) {
+      // Grab another frame. Proxy methods are not helpful for lock profiling. This should be rare
+      // enough that it's OK to walk the stack twice.
+      struct NextMethodVisitor FINAL : public StackVisitor {
+        explicit NextMethodVisitor(Thread* thread) REQUIRES_SHARED(Locks::mutator_lock_)
+            : StackVisitor(thread,
+                           nullptr,
+                           StackVisitor::StackWalkKind::kIncludeInlinedFrames,
+                           false),
+              count_(0),
+              method_(nullptr),
+              dex_pc_(0) {}
+        bool VisitFrame() OVERRIDE REQUIRES_SHARED(Locks::mutator_lock_) {
+          ArtMethod* m = GetMethod();
+          if (m->IsRuntimeMethod()) {
+            // Continue if this is a runtime method.
+            return true;
+          }
+          count_++;
+          if (count_ == 2u) {
+            method_ = m;
+            dex_pc_ = GetDexPc(false);
+            return false;
+          }
+          return true;
+        }
+        size_t count_;
+        ArtMethod* method_;
+        uint32_t dex_pc_;
+      };
+      NextMethodVisitor nmv(owner_);
+      nmv.WalkStack();
+      locking_method_ = nmv.method_;
+      locking_dex_pc_ = nmv.dex_pc_;
+    }
+    DCHECK(locking_method_ == nullptr || !locking_method_->IsProxyMethod());
   }
   return success;
 }
@@ -337,6 +373,8 @@ bool Monitor::TryLockLocked(Thread* self) {
     // acquisition failures to use in sampled logging.
     if (lock_profiling_threshold_ != 0) {
       locking_method_ = self->GetCurrentMethod(&locking_dex_pc_);
+      // We don't expect a proxy method here.
+      DCHECK(locking_method_ == nullptr || !locking_method_->IsProxyMethod());
     }
   } else if (owner_ == self) {  // Recursive.
     lock_count_++;
diff --git a/test/165-lock-owner-proxy/expected.txt b/test/165-lock-owner-proxy/expected.txt
new file mode 100644
index 0000000000..e69de29bb2
--- /dev/null
+++ b/test/165-lock-owner-proxy/expected.txt
diff --git a/test/165-lock-owner-proxy/info.txt b/test/165-lock-owner-proxy/info.txt
new file mode 100644
index 0000000000..0964d33897
--- /dev/null
+++ b/test/165-lock-owner-proxy/info.txt
@@ -0,0 +1 @@
+Regression test for b/68871592
diff --git a/test/165-lock-owner-proxy/run b/test/165-lock-owner-proxy/run
new file mode 100644
index 0000000000..93654113e6
--- /dev/null
+++ b/test/165-lock-owner-proxy/run
@@ -0,0 +1,18 @@
+#!/bin/bash
+#
+# Copyright (C) 2017 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.
+
+# Use a smaller heap so it's easier to potentially fill up.
+exec ${RUN} $@ --runtime-option -Xmx2m
diff --git a/test/165-lock-owner-proxy/src/Main.java b/test/165-lock-owner-proxy/src/Main.java
new file mode 100644
index 0000000000..1b1694d2c1
--- /dev/null
+++ b/test/165-lock-owner-proxy/src/Main.java
@@ -0,0 +1,108 @@
+/*
+ * Copyright (C) 2017 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.
+ */
+
+import java.lang.reflect.InvocationHandler;
+import java.lang.reflect.Method;
+import java.lang.reflect.Proxy;
+
+public class Main {
+    static final int numberOfThreads = 5;
+    static final int totalOperations = 10000;
+
+    final static Object lockObject = new Object();
+    static SimpleInterface inf;
+    static volatile boolean finish = false;
+
+    public static void main(String[] args) throws Exception {
+        inf = (SimpleInterface)Proxy.newProxyInstance(SimpleInterface.class.getClassLoader(),
+            new Class[] { SimpleInterface.class }, new EmptyInvocationHandler());
+
+        Thread garbageThread = new Thread(new GarbageRunner());
+        garbageThread.start();
+
+        final Thread[] threads = new Thread[numberOfThreads];
+        for (int t = 0; t < threads.length; t++) {
+            threads[t] = new Thread((t % 2 == 0) ? new ProxyRunner() : new SyncRunner());
+        }
+        for (Thread t : threads) {
+            t.start();
+        }
+
+        // Now wait.
+        for (Thread t : threads) {
+            t.join();
+        }
+        finish = true;
+        garbageThread.join();
+    }
+
+    private static interface SimpleInterface {
+        // Add some primitives to force some allocation when calling.
+        public void foo(int i1, int i2, int i3, int i4, int i5, int i6);
+    }
+
+    private static class EmptyInvocationHandler implements InvocationHandler {
+        public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
+            return null;
+        }
+    }
+
+    private static class ProxyRunner implements Runnable {
+        public void run() {
+            int count = totalOperations;
+            while (count > 0) {
+                synchronized (lockObject) {
+                    inf.foo(10000 - count, 11000 - count, 12000 - count, 13000 - count,
+                          14000 - count, 15000 - count);
+              }
+              count--;
+            }
+        }
+    }
+
+    private static class SyncRunner implements Runnable {
+        public void run() {
+            int count = totalOperations;
+            while (count > 0) {
+                synchronized (lockObject) {
+                    // "Wait" a small amount of time.
+                    long start = System.nanoTime();
+                    long delta = 10 * 1000;  // 10 us.
+                    long elapsed;
+                    do {
+                      elapsed = System.nanoTime();
+                    } while (elapsed - start < delta);
+                }
+                count--;
+            }
+        }
+    }
+
+    private static class GarbageRunner implements Runnable {
+        public void run() {
+            while (!finish) {
+                // Some random allocations adding up to almost 2M.
+                for (int i = 0; i < 188; i++) {
+                    byte b[] = new byte[i * 100 + 10];
+                }
+                try {
+                    Thread.sleep(10);
+                } catch (Exception e) {
+                }
+            }
+        }
+    }
+}