Fix "using JNI after critical" exceptions

Refactored ScopedJavaNioBuffer implementation to copy
directly to std::vector<uint8_t> instances and release
the jni critical array queries immediately after copying
into a vector. This is to avoid potential interleavings
of JNI method calls before destructor of ScopedJavaNioBuffer
is invoked. As per jni requirements no other JNI calls are
allowed after GetPrimitiveArrayCritical until a matching call
to ReleasePrimitiveArrayCritical is made.

Fixes: 271468824
Test: Added tests to MeshTest
Change-Id: Ia4afd8b8584ae43f021c79b6a341b05e80e0e205

5 files changed, 189 insertions, 180 deletions

diff --git a/libs/hwui/Android.bp b/libs/hwui/Android.bp
index 536bb49675f1..9c967a2290d0 100644
--- a/libs/hwui/Android.bp
+++ b/libs/hwui/Android.bp
@@ -338,6 +338,7 @@ cc_defaults {
         "jni/android_util_PathParser.cpp",
 
         "jni/Bitmap.cpp",
+        "jni/BufferUtils.cpp",
         "jni/HardwareBufferHelpers.cpp",
         "jni/BitmapFactory.cpp",
         "jni/ByteBufferStreamAdaptor.cpp",
diff --git a/libs/hwui/Mesh.h b/libs/hwui/Mesh.h
index 983681707415..13e3c8e7bf77 100644
--- a/libs/hwui/Mesh.h
+++ b/libs/hwui/Mesh.h
@@ -104,33 +104,31 @@ private:
 
 class Mesh {
 public:
-    Mesh(const sk_sp<SkMeshSpecification>& meshSpec, int mode, const void* vertexBuffer,
-         size_t vertexBufferSize, jint vertexCount, jint vertexOffset,
+    Mesh(const sk_sp<SkMeshSpecification>& meshSpec, int mode,
+         std::vector<uint8_t>&& vertexBufferData, jint vertexCount, jint vertexOffset,
          std::unique_ptr<MeshUniformBuilder> builder, const SkRect& bounds)
             : mMeshSpec(meshSpec)
             , mMode(mode)
+            , mVertexBufferData(std::move(vertexBufferData))
             , mVertexCount(vertexCount)
             , mVertexOffset(vertexOffset)
             , mBuilder(std::move(builder))
-            , mBounds(bounds) {
-        copyToVector(mVertexBufferData, vertexBuffer, vertexBufferSize);
-    }
+            , mBounds(bounds) {}
 
-    Mesh(const sk_sp<SkMeshSpecification>& meshSpec, int mode, const void* vertexBuffer,
-         size_t vertexBufferSize, jint vertexCount, jint vertexOffset, const void* indexBuffer,
-         size_t indexBufferSize, jint indexCount, jint indexOffset,
+    Mesh(const sk_sp<SkMeshSpecification>& meshSpec, int mode,
+         std::vector<uint8_t>&& vertexBufferData, jint vertexCount, jint vertexOffset,
+         std::vector<uint8_t>&& indexBuffer, jint indexCount, jint indexOffset,
          std::unique_ptr<MeshUniformBuilder> builder, const SkRect& bounds)
             : mMeshSpec(meshSpec)
             , mMode(mode)
+            , mVertexBufferData(std::move(vertexBufferData))
             , mVertexCount(vertexCount)
             , mVertexOffset(vertexOffset)
+            , mIndexBufferData(std::move(indexBuffer))
             , mIndexCount(indexCount)
             , mIndexOffset(indexOffset)
             , mBuilder(std::move(builder))
-            , mBounds(bounds) {
-        copyToVector(mVertexBufferData, vertexBuffer, vertexBufferSize);
-        copyToVector(mIndexBufferData, indexBuffer, indexBufferSize);
-    }
+            , mBounds(bounds) {}
 
     Mesh(Mesh&&) = default;
 
@@ -180,13 +178,6 @@ public:
     MeshUniformBuilder* uniformBuilder() { return mBuilder.get(); }
 
 private:
-    void copyToVector(std::vector<uint8_t>& dst, const void* src, size_t srcSize) {
-        if (src) {
-            dst.resize(srcSize);
-            memcpy(dst.data(), src, srcSize);
-        }
-    }
-
     sk_sp<SkMeshSpecification> mMeshSpec;
     int mMode = 0;
 
diff --git a/libs/hwui/jni/BufferUtils.cpp b/libs/hwui/jni/BufferUtils.cpp
new file mode 100644
index 000000000000..3eb08d7552da
--- /dev/null
+++ b/libs/hwui/jni/BufferUtils.cpp
@@ -0,0 +1,130 @@
+/*
+ * 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 "BufferUtils.h"
+
+#include "graphics_jni_helpers.h"
+
+static void copyToVector(std::vector<uint8_t>& dst, const void* src, size_t srcSize) {
+    if (src) {
+        dst.resize(srcSize);
+        memcpy(dst.data(), src, srcSize);
+    }
+}
+
+/**
+ * This code is taken and modified from com_google_android_gles_jni_GLImpl.cpp to extract data
+ * from a java.nio.Buffer.
+ */
+static void* getDirectBufferPointer(JNIEnv* env, jobject buffer) {
+    if (buffer == nullptr) {
+        return nullptr;
+    }
+
+    jint position;
+    jint limit;
+    jint elementSizeShift;
+    jlong pointer;
+    pointer = jniGetNioBufferFields(env, buffer, &position, &limit, &elementSizeShift);
+    if (pointer == 0) {
+        jniThrowException(env, "java/lang/IllegalArgumentException",
+                          "Must use a native order direct Buffer");
+        return nullptr;
+    }
+    pointer += position << elementSizeShift;
+    return reinterpret_cast<void*>(pointer);
+}
+
+static void releasePointer(JNIEnv* env, jarray array, void* data, jboolean commit) {
+    env->ReleasePrimitiveArrayCritical(array, data, commit ? 0 : JNI_ABORT);
+}
+
+static void* getPointer(JNIEnv* env, jobject buffer, jarray* array, jint* remaining, jint* offset) {
+    jint position;
+    jint limit;
+    jint elementSizeShift;
+
+    jlong pointer;
+    pointer = jniGetNioBufferFields(env, buffer, &position, &limit, &elementSizeShift);
+    *remaining = (limit - position) << elementSizeShift;
+    if (pointer != 0L) {
+        *array = nullptr;
+        pointer += position << elementSizeShift;
+        return reinterpret_cast<void*>(pointer);
+    }
+
+    *array = jniGetNioBufferBaseArray(env, buffer);
+    *offset = jniGetNioBufferBaseArrayOffset(env, buffer);
+    return nullptr;
+}
+
+/**
+ * This is a copy of
+ * static void android_glBufferData__IILjava_nio_Buffer_2I
+ * from com_google_android_gles_jni_GLImpl.cpp
+ */
+static void setIndirectData(JNIEnv* env, size_t size, jobject data_buf,
+                            std::vector<uint8_t>& result) {
+    jint exception = 0;
+    const char* exceptionType = nullptr;
+    const char* exceptionMessage = nullptr;
+    jarray array = nullptr;
+    jint bufferOffset = 0;
+    jint remaining;
+    void* data = 0;
+    char* dataBase = nullptr;
+
+    if (data_buf) {
+        data = getPointer(env, data_buf, (jarray*)&array, &remaining, &bufferOffset);
+        if (remaining < size) {
+            exception = 1;
+            exceptionType = "java/lang/IllegalArgumentException";
+            exceptionMessage = "remaining() < size < needed";
+            goto exit;
+        }
+    }
+    if (data_buf && data == nullptr) {
+        dataBase = (char*)env->GetPrimitiveArrayCritical(array, (jboolean*)0);
+        data = (void*)(dataBase + bufferOffset);
+    }
+
+    copyToVector(result, data, size);
+
+exit:
+    if (array) {
+        releasePointer(env, array, (void*)dataBase, JNI_FALSE);
+    }
+    if (exception) {
+        jniThrowException(env, exceptionType, exceptionMessage);
+    }
+}
+
+std::vector<uint8_t> copyJavaNioBufferToVector(JNIEnv* env, jobject buffer, size_t size,
+                                               jboolean isDirect) {
+    std::vector<uint8_t> data;
+    if (buffer == nullptr) {
+        jniThrowNullPointerException(env);
+    } else {
+        if (isDirect) {
+            void* directBufferPtr = getDirectBufferPointer(env, buffer);
+            if (directBufferPtr) {
+                copyToVector(data, directBufferPtr, size);
+            }
+        } else {
+            setIndirectData(env, size, buffer, data);
+        }
+    }
+    return data;
+}
diff --git a/libs/hwui/jni/BufferUtils.h b/libs/hwui/jni/BufferUtils.h
new file mode 100644
index 000000000000..b43c320b7771
--- /dev/null
+++ b/libs/hwui/jni/BufferUtils.h
@@ -0,0 +1,32 @@
+/*
+ * 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.
+ */
+#ifndef BUFFERUTILS_H_
+#define BUFFERUTILS_H_
+
+#include <jni.h>
+
+#include <vector>
+
+/**
+ * Helper method to load a java.nio.Buffer instance into a vector. This handles
+ * both direct and indirect buffers and promptly releases any critical arrays that
+ * have been retrieved in order to avoid potential jni exceptions due to interleaved
+ * jni calls between get/release primitive method invocations.
+ */
+std::vector<uint8_t> copyJavaNioBufferToVector(JNIEnv* env, jobject buffer, size_t size,
+                                               jboolean isDirect);
+
+#endif  // BUFFERUTILS_H_
diff --git a/libs/hwui/jni/android_graphics_Mesh.cpp b/libs/hwui/jni/android_graphics_Mesh.cpp
index 04339dc8b9a5..5cb43e54e499 100644
--- a/libs/hwui/jni/android_graphics_Mesh.cpp
+++ b/libs/hwui/jni/android_graphics_Mesh.cpp
@@ -14,172 +14,18 @@
  * limitations under the License.
  */
 
-#include <GrDirectContext.h>
 #include <Mesh.h>
 #include <SkMesh.h>
 #include <jni.h>
-#include <log/log.h>
 
 #include <utility>
 
+#include "BufferUtils.h"
 #include "GraphicsJNI.h"
 #include "graphics_jni_helpers.h"
 
 #define gIndexByteSize 2
 
-// A smart pointer that provides read only access to Java.nio.Buffer. This handles both
-// direct and indrect buffers, allowing access to the underlying data in both
-// situations. If passed a null buffer, we will throw NullPointerException,
-// and c_data will return nullptr.
-//
-// This class draws from com_google_android_gles_jni_GLImpl.cpp for Buffer to void *
-// conversion.
-class ScopedJavaNioBuffer {
-public:
-    ScopedJavaNioBuffer(JNIEnv* env, jobject buffer, size_t size, jboolean isDirect)
-            : mEnv(env), mBuffer(buffer) {
-        if (buffer == nullptr) {
-            mDataBase = nullptr;
-            mData = nullptr;
-            jniThrowNullPointerException(env);
-        } else {
-            mArray = (jarray) nullptr;
-            if (isDirect) {
-                mData = getDirectBufferPointer(mEnv, mBuffer);
-            } else {
-                mData = setIndirectData(size);
-            }
-        }
-    }
-
-    ScopedJavaNioBuffer(ScopedJavaNioBuffer&& rhs) noexcept { *this = std::move(rhs); }
-
-    ~ScopedJavaNioBuffer() { reset(); }
-
-    void reset() {
-        if (mDataBase) {
-            releasePointer(mEnv, mArray, mDataBase, JNI_FALSE);
-            mDataBase = nullptr;
-        }
-    }
-
-    ScopedJavaNioBuffer& operator=(ScopedJavaNioBuffer&& rhs) noexcept {
-        if (this != &rhs) {
-            reset();
-
-            mEnv = rhs.mEnv;
-            mBuffer = rhs.mBuffer;
-            mDataBase = rhs.mDataBase;
-            mData = rhs.mData;
-            mArray = rhs.mArray;
-            rhs.mEnv = nullptr;
-            rhs.mData = nullptr;
-            rhs.mBuffer = nullptr;
-            rhs.mArray = nullptr;
-            rhs.mDataBase = nullptr;
-        }
-        return *this;
-    }
-
-    const void* data() const { return mData; }
-
-private:
-    /**
-     * This code is taken and modified from com_google_android_gles_jni_GLImpl.cpp to extract data
-     * from a java.nio.Buffer.
-     */
-    void* getDirectBufferPointer(JNIEnv* env, jobject buffer) {
-        if (buffer == nullptr) {
-            return nullptr;
-        }
-
-        jint position;
-        jint limit;
-        jint elementSizeShift;
-        jlong pointer;
-        pointer = jniGetNioBufferFields(env, buffer, &position, &limit, &elementSizeShift);
-        if (pointer == 0) {
-            jniThrowException(mEnv, "java/lang/IllegalArgumentException",
-                              "Must use a native order direct Buffer");
-            return nullptr;
-        }
-        pointer += position << elementSizeShift;
-        return reinterpret_cast<void*>(pointer);
-    }
-
-    static void releasePointer(JNIEnv* env, jarray array, void* data, jboolean commit) {
-        env->ReleasePrimitiveArrayCritical(array, data, commit ? 0 : JNI_ABORT);
-    }
-
-    static void* getPointer(JNIEnv* env, jobject buffer, jarray* array, jint* remaining,
-                            jint* offset) {
-        jint position;
-        jint limit;
-        jint elementSizeShift;
-
-        jlong pointer;
-        pointer = jniGetNioBufferFields(env, buffer, &position, &limit, &elementSizeShift);
-        *remaining = (limit - position) << elementSizeShift;
-        if (pointer != 0L) {
-            *array = nullptr;
-            pointer += position << elementSizeShift;
-            return reinterpret_cast<void*>(pointer);
-        }
-
-        *array = jniGetNioBufferBaseArray(env, buffer);
-        *offset = jniGetNioBufferBaseArrayOffset(env, buffer);
-        return nullptr;
-    }
-
-    /**
-     * This is a copy of
-     * static void android_glBufferData__IILjava_nio_Buffer_2I
-     * from com_google_android_gles_jni_GLImpl.cpp
-     */
-    void* setIndirectData(size_t size) {
-        jint exception;
-        const char* exceptionType;
-        const char* exceptionMessage;
-        jint bufferOffset = (jint)0;
-        jint remaining;
-        void* tempData;
-
-        if (mBuffer) {
-            tempData =
-                    (void*)getPointer(mEnv, mBuffer, (jarray*)&mArray, &remaining, &bufferOffset);
-            if (remaining < size) {
-                exception = 1;
-                exceptionType = "java/lang/IllegalArgumentException";
-                exceptionMessage = "remaining() < size < needed";
-                goto exit;
-            }
-        }
-        if (mBuffer && tempData == nullptr) {
-            mDataBase = (char*)mEnv->GetPrimitiveArrayCritical(mArray, (jboolean*)0);
-            tempData = (void*)(mDataBase + bufferOffset);
-        }
-        return tempData;
-    exit:
-        if (mArray) {
-            releasePointer(mEnv, mArray, (void*)(mDataBase), JNI_FALSE);
-        }
-        if (exception) {
-            jniThrowException(mEnv, exceptionType, exceptionMessage);
-        }
-        return nullptr;
-    }
-
-    JNIEnv* mEnv;
-
-    // Java Buffer data
-    void* mData;
-    jobject mBuffer;
-
-    // Indirect Buffer Data
-    jarray mArray;
-    char* mDataBase;
-};
-
 namespace android {
 
 static jlong make(JNIEnv* env, jobject, jlong meshSpec, jint mode, jobject vertexBuffer,
@@ -187,9 +33,12 @@ static jlong make(JNIEnv* env, jobject, jlong meshSpec, jint mode, jobject verte
                   jfloat right, jfloat bottom) {
     auto skMeshSpec = sk_ref_sp(reinterpret_cast<SkMeshSpecification*>(meshSpec));
     size_t bufferSize = vertexCount * skMeshSpec->stride();
-    auto buff = ScopedJavaNioBuffer(env, vertexBuffer, bufferSize, isDirect);
+    auto buffer = copyJavaNioBufferToVector(env, vertexBuffer, bufferSize, isDirect);
+    if (env->ExceptionCheck()) {
+        return 0;
+    }
     auto skRect = SkRect::MakeLTRB(left, top, right, bottom);
-    auto meshPtr = new Mesh(skMeshSpec, mode, buff.data(), bufferSize, vertexCount, vertexOffset,
+    auto meshPtr = new Mesh(skMeshSpec, mode, std::move(buffer), vertexCount, vertexOffset,
                             std::make_unique<MeshUniformBuilder>(skMeshSpec), skRect);
     auto [valid, msg] = meshPtr->validate();
     if (!valid) {
@@ -205,11 +54,17 @@ static jlong makeIndexed(JNIEnv* env, jobject, jlong meshSpec, jint mode, jobjec
     auto skMeshSpec = sk_ref_sp(reinterpret_cast<SkMeshSpecification*>(meshSpec));
     auto vertexBufferSize = vertexCount * skMeshSpec->stride();
     auto indexBufferSize = indexCount * gIndexByteSize;
-    auto vBuf = ScopedJavaNioBuffer(env, vertexBuffer, vertexBufferSize, isVertexDirect);
-    auto iBuf = ScopedJavaNioBuffer(env, indexBuffer, indexBufferSize, isIndexDirect);
+    auto vBuf = copyJavaNioBufferToVector(env, vertexBuffer, vertexBufferSize, isVertexDirect);
+    if (env->ExceptionCheck()) {
+        return 0;
+    }
+    auto iBuf = copyJavaNioBufferToVector(env, indexBuffer, indexBufferSize, isIndexDirect);
+    if (env->ExceptionCheck()) {
+        return 0;
+    }
     auto skRect = SkRect::MakeLTRB(left, top, right, bottom);
-    auto meshPtr = new Mesh(skMeshSpec, mode, vBuf.data(), vertexBufferSize, vertexCount,
-                            vertexOffset, iBuf.data(), indexBufferSize, indexCount, indexOffset,
+    auto meshPtr = new Mesh(skMeshSpec, mode, std::move(vBuf), vertexCount, vertexOffset,
+                            std::move(iBuf), indexCount, indexOffset,
                             std::make_unique<MeshUniformBuilder>(skMeshSpec), skRect);
     auto [valid, msg] = meshPtr->validate();
     if (!valid) {