Fix lock visiting for synchronized native methods.

The `GetGenericJniSynchronizationObject()` function was used
in the wrong context. As documented, it can be used only for
a method with a GenericJni frame and also on the top of the
stack. When visiting locks, we can have a non-GenericJni
method frame as well as a method deeper in the stack.
Replace the wrong use with specialized code.

Test: Added regression test to 178-app-image-native-methods
Test: testrunner.py --host --debug --ndebug
Bug: 172332525
Bug: 189235039
Change-Id: Ia26f0b980c04a766e31b1588a1c011bcf46c90d8

4 files changed, 90 insertions, 3 deletions

diff --git a/runtime/entrypoints/entrypoint_utils-inl.h b/runtime/entrypoints/entrypoint_utils-inl.h
index 01e8911611..84299d5077 100644
--- a/runtime/entrypoints/entrypoint_utils-inl.h
+++ b/runtime/entrypoints/entrypoint_utils-inl.h
@@ -765,10 +765,13 @@ inline jobject GetGenericJniSynchronizationObject(Thread* self, ArtMethod* calle
   DCHECK(self->GetManagedStack()->GetTopQuickFrame() != nullptr);
   DCHECK_EQ(*self->GetManagedStack()->GetTopQuickFrame(), called);
   if (called->IsStatic()) {
+    // Static methods synchronize on the declaring class object.
     // The `jclass` is a pointer to the method's declaring class.
     return reinterpret_cast<jobject>(called->GetDeclaringClassAddressWithoutBarrier());
   } else {
+    // Instance methods synchronize on the `this` object.
     // The `this` reference is stored in the first out vreg in the caller's frame.
+    // The `jobject` is a pointer to the spill slot.
     uint8_t* sp = reinterpret_cast<uint8_t*>(self->GetManagedStack()->GetTopQuickFrame());
     size_t frame_size = RuntimeCalleeSaveFrame::GetFrameSize(CalleeSaveType::kSaveRefsAndArgs);
     return reinterpret_cast<jobject>(sp + frame_size + static_cast<size_t>(kRuntimePointerSize));
diff --git a/runtime/monitor.cc b/runtime/monitor.cc
index 2f590227af..f2189e16ac 100644
--- a/runtime/monitor.cc
+++ b/runtime/monitor.cc
@@ -1452,9 +1452,21 @@ void Monitor::VisitLocks(StackVisitor* stack_visitor,
   // TODO: use the JNI implementation's table of explicit MonitorEnter calls and dump those too.
   if (m->IsNative()) {
     if (m->IsSynchronized()) {
-      Thread* thread = stack_visitor->GetThread();
-      jobject lock = GetGenericJniSynchronizationObject(thread, m);
-      callback(thread->DecodeJObject(lock), callback_context);
+      DCHECK(!m->IsCriticalNative());
+      DCHECK(!m->IsFastNative());
+      ObjPtr<mirror::Object> lock;
+      if (m->IsStatic()) {
+        // Static methods synchronize on the declaring class object.
+        lock = m->GetDeclaringClass();
+      } else {
+        // Instance methods synchronize on the `this` object.
+        // The `this` reference is stored in the first out vreg in the caller's frame.
+        uint8_t* sp = reinterpret_cast<uint8_t*>(stack_visitor->GetCurrentQuickFrame());
+        size_t frame_size = stack_visitor->GetCurrentQuickFrameInfo().FrameSizeInBytes();
+        lock = reinterpret_cast<StackReference<mirror::Object>*>(
+            sp + frame_size + static_cast<size_t>(kRuntimePointerSize))->AsMirrorPtr();
+      }
+      callback(lock, callback_context);
     }
     return;
   }
diff --git a/test/178-app-image-native-method/native_methods.cc b/test/178-app-image-native-method/native_methods.cc
index 0669deb4b3..709c5dfe64 100644
--- a/test/178-app-image-native-method/native_methods.cc
+++ b/test/178-app-image-native-method/native_methods.cc
@@ -14,8 +14,16 @@
  * limitations under the License.
  */
 
+#include <sstream>
+
 #include "jni.h"
 
+#include "arch/context.h"
+#include "monitor.h"
+#include "scoped_thread_state_change-inl.h"
+#include "stack.h"
+#include "thread-current-inl.h"
+
 namespace art {
 
 static inline bool VerifyManyParameters(
@@ -638,4 +646,60 @@ extern "C" JNIEXPORT jint JNICALL Java_CriticalClinitCheck_nativeMethodWithManyP
   return ok ? 42 : -1;
 }
 
+extern "C" JNIEXPORT jint JNICALL Java_Main_b189235039CallThrough(JNIEnv* env, jobject m) {
+  jclass main_klass = env->GetObjectClass(m);
+  jmethodID checkLocks = env->GetStaticMethodID(main_klass, "b189235039CheckLocks", "(ILMain;)I");
+  if (checkLocks == nullptr) {
+    return -1;
+  }
+  return env->CallStaticIntMethod(main_klass, checkLocks, 42, m);
+}
+
+extern "C" JNIEXPORT jint JNICALL Java_Main_b189235039CheckLocks(JNIEnv*,
+                                                                 jclass,
+                                                                 int arg,
+                                                                 jobject lock) {
+  // Check that we do not crash when visiting locks and that we find the right lock.
+  ScopedObjectAccess soa(Thread::Current());
+  class VisitLocks : public StackVisitor {
+   public:
+    VisitLocks(Thread* thread, Context* context, jobject expected_lock)
+        : StackVisitor(thread, context, StackWalkKind::kIncludeInlinedFrames),
+          expected_lock_(expected_lock) {
+    }
+
+    bool VisitFrame() override REQUIRES_SHARED(Locks::mutator_lock_) {
+      ArtMethod* m = GetMethod();
+
+      // Ignore runtime methods.
+      if (m == nullptr || m->IsRuntimeMethod()) {
+        return true;
+      }
+
+      if (m->IsSynchronized()) {
+        // Interesting frame.
+        Monitor::VisitLocks(this, Callback, expected_lock_);
+        return false;
+      }
+
+      return true;
+    }
+
+   private:
+    static void Callback(ObjPtr<mirror::Object> obj, void* expected_lock)
+        REQUIRES_SHARED(Locks::mutator_lock_) {
+      CHECK(obj != nullptr);
+      CHECK(obj == Thread::Current()->DecodeJObject(reinterpret_cast<jobject>(expected_lock)));
+    }
+
+    jobject expected_lock_;
+  };
+  {
+    std::unique_ptr<Context> context(Context::Create());
+    VisitLocks vl(soa.Self(), context.get(), lock);
+    vl.WalkStack();
+  }
+  return arg;
+}
+
 }  // namespace art
diff --git a/test/178-app-image-native-method/src/Main.java b/test/178-app-image-native-method/src/Main.java
index e6c76ffc5f..294ad4739b 100644
--- a/test/178-app-image-native-method/src/Main.java
+++ b/test/178-app-image-native-method/src/Main.java
@@ -52,6 +52,7 @@ public class Main {
     $noinline$opt$testCriticalSignatures();
 
     $noinline$regressionTestB181736463();
+    $noinline$regressionTestB189235039();
 
     new CriticalClinitCheck();
     sTestCriticalClinitCheckOtherThread.join();
@@ -380,6 +381,10 @@ public class Main {
     }
   }
 
+  static void $noinline$regressionTestB189235039() {
+    assertEquals(42, new Main().b189235039CallThrough());
+  }
+
   static void initializingCriticalClinitCheck() {
     // Called from CriticalClinitCheck.<clinit>().
     // Test @CriticalNative calls on the initializing thread.
@@ -425,6 +430,9 @@ public class Main {
   }
 
   public static native void makeVisiblyInitialized();
+
+  public native synchronized int b189235039CallThrough();
+  public static native int b189235039CheckLocks(int placeholder, Main m);
 }
 
 class Test {