CreateCopy is now the default for locals.

This follows from the discussion on #207. Values passed through JNI are seemingly owned by the calling context and should not be deleted. This changes behaviour in existing applications, but at most should harmlessly leak a single local reference to a jobject. This *could* be a break a breaking change if locally created JNI objects were being wrapped by a `LocalObject` in an inner loop, but can be obviated by using `AdoptLocal` anywhere this is done. This also could break if you are passing nulls to objects (which is not allowed but would passively fail). Because `LocalObject<kClass>{input_obj}` is a "default path", this is going to become the default. All existing usages can add `AdoptLocal{}` to duplicate existing behaviour. PiperOrigin-RevId: 577476928
google · Nov 13, 2023 · 8e8f56c · 8e8f56c
1 parent 58e8b53
commit 8e8f56c
Show file tree

Hide file tree

Showing 27 changed files with 319 additions and 137 deletions.
diff --git a/BUILD b/BUILD
@@ -56,6 +56,7 @@ cc_library(
         "//implementation:no_idx",
         "//implementation:params",
         "//implementation:promotion_mechanics",
+        "//implementation:promotion_mechanics_tags",
         "//implementation:return",
         "//implementation:selector_static_info",
         "//implementation:self",

diff --git a/implementation/BUILD b/implementation/BUILD
@@ -29,10 +29,13 @@ cc_library(
         ":array",
         ":array_view",
         ":class",
+        ":class_ref",
         ":default_class_loader",
+        ":forward_declarations",
         ":jni_type",
         ":local_object",
         ":object_ref",
+        ":promotion_mechanics_tags",
         ":ref_base",
         "//:jni_dep",
         "//implementation/jni_helper:jni_array_helper",
@@ -292,7 +295,10 @@ cc_library(
         ":field_selection",
         ":id",
         ":id_type",
+        ":promotion_mechanics_tags",
         ":proxy",
+        ":proxy_convenience_aliases",
+        ":ref_base",
         "//:jni_dep",
         "//implementation/jni_helper",
         "//implementation/jni_helper:field_value_getter",
@@ -526,6 +532,7 @@ cc_library(
         ":jvm",
         ":no_idx",
         ":object",
+        ":promotion_mechanics_tags",
         "//:jni_dep",
         "//implementation/jni_helper:jni_array_helper",
         "//implementation/jni_helper:lifecycle_object",
@@ -594,10 +601,12 @@ cc_library(
         ":class",
         ":class_loader",
         ":class_loader_ref",
+        ":forward_declarations",
         ":jvm",
         ":jvm_ref",
         ":local_object",
         ":promotion_mechanics",
+        ":promotion_mechanics_tags",
         "//:jni_dep",
         "//class_defs:java_lang_classes",
         "//implementation/jni_helper:lifecycle_object",
@@ -610,11 +619,13 @@ cc_library(
     deps = [
         ":class",
         ":class_loader",
+        ":forward_declarations",
         ":jni_type",
         ":jvm",
         ":jvm_ref",
         ":object_ref",
         ":promotion_mechanics",
+        ":promotion_mechanics_tags",
         "//:jni_dep",
         "//implementation/jni_helper:lifecycle_object",
     ],
@@ -635,8 +646,10 @@ cc_library(
     name = "local_string",
     hdrs = ["local_string.h"],
     deps = [
+        ":forward_declarations",
         ":local_object",
         ":promotion_mechanics",
+        ":promotion_mechanics_tags",
         ":ref_base",
         ":string_ref",
         "//:jni_dep",
@@ -663,6 +676,7 @@ cc_library(
         ":jni_type",
         ":method",
         ":params",
+        ":promotion_mechanics_tags",
         ":proxy",
         ":proxy_convenience_aliases",
         ":proxy_definitions",
@@ -819,6 +833,7 @@ cc_library(
         ":forward_declarations",
         ":jni_type",
         ":object_ref",
+        ":promotion_mechanics_tags",
         ":ref_base",
         "//:jni_dep",
         "//implementation/jni_helper",
@@ -829,6 +844,11 @@ cc_library(
     ],
 )
 
+cc_library(
+    name = "promotion_mechanics_tags",
+    hdrs = ["promotion_mechanics_tags.h"],
+)
+
 cc_library(
     name = "proxy",
     hdrs = ["proxy.h"],

diff --git a/implementation/array_ref.h b/implementation/array_ref.h
@@ -21,13 +21,16 @@
 #include "implementation/array.h"
 #include "implementation/array_view.h"
 #include "implementation/class.h"
+#include "implementation/class_ref.h"
 #include "implementation/default_class_loader.h"
+#include "implementation/forward_declarations.h"
 #include "implementation/jni_helper/jni_array_helper.h"
 #include "implementation/jni_helper/lifecycle.h"
 #include "implementation/jni_helper/lifecycle_object.h"
 #include "implementation/jni_type.h"
 #include "implementation/local_object.h"
 #include "implementation/object_ref.h"
+#include "implementation/promotion_mechanics_tags.h"
 #include "implementation/ref_base.h"
 #include "jni_dep.h"
 
@@ -48,7 +51,12 @@ class ArrayRef : public ScopedArrayImpl<JniT> {
   using SpanType = typename JniT::SpanType;
 
   ArrayRef(std::size_t size)
-      : Base(JniArrayHelper<SpanType, JniT::kRank>::NewArray(size)) {}
+      : Base(AdoptLocal{},
+             JniArrayHelper<SpanType, JniT::kRank>::NewArray(size)) {}
+
+  template <typename T>
+  ArrayRef(const ArrayViewHelper<T>& array_view_helper)
+      : Base(AdoptLocal{}, array_view_helper.val) {}
 
   explicit ArrayRef(int size) : ArrayRef(static_cast<std::size_t>(size)) {}
 
@@ -79,17 +87,18 @@ class ArrayRefBase : public ScopedArrayImpl<JniT> {
 
   // Construct array with given size and null values.
   explicit ArrayRefBase(std::size_t size)
-      : Base(JniArrayHelper<jobject, JniT::kRank>::NewArray(
-            size, ClassRef_t<JniT>::GetAndMaybeLoadClassRef(nullptr),
-            static_cast<jobject>(nullptr))) {}
+      : Base(AdoptLocal{},
+             JniArrayHelper<jobject, JniT::kRank>::NewArray(
+                 size, ClassRef_t<JniT>::GetAndMaybeLoadClassRef(nullptr),
+                 static_cast<jobject>(nullptr))) {}
 
   // Construct from jobject lvalue (object is used as template).
   explicit ArrayRefBase(std::size_t size, jobject obj)
-      : Base(JniArrayHelper<jobject, JniT::kRank>::NewArray(
-            size,
-            ClassRef_t<JniT>::GetAndMaybeLoadClassRef(
-                static_cast<jobject>(obj)),
-            static_cast<jobject>(obj))) {}
+      : Base(AdoptLocal{}, JniArrayHelper<jobject, JniT::kRank>::NewArray(
+                               size,
+                               ClassRef_t<JniT>::GetAndMaybeLoadClassRef(
+                                   static_cast<jobject>(obj)),
+                               static_cast<jobject>(obj))) {}
 
   // Object arrays cannot be efficiently pinned like primitive types can.
   ArrayView<SpanType, JniT::kRank> Pin() {
@@ -108,8 +117,8 @@ class ArrayRefBase : public ScopedArrayImpl<JniT> {
   void Set(
       std::size_t idx,
       LocalObject<JniT::class_v, JniT::class_loader_v, JniT::jvm_v>&& val) {
-    JniArrayHelper<jobject, JniT::kRank>::SetArrayElement(Base::object_ref_,
-                                                          idx, val.Release());
+    AdoptLocal{}, JniArrayHelper<jobject, JniT::kRank>::SetArrayElement(
+                      Base::object_ref_, idx, val.Release());
   }
 };
 
@@ -158,9 +167,11 @@ class ArrayRef<JniT, std::enable_if_t<(JniT::kRank > 1)>>
 
   LocalArray<typename JniT::SpanType, JniT::kRank - 1, clazz, class_loader, jvm>
   Get(std::size_t idx) {
-    return {static_cast<jarray>(
-        JniArrayHelper<typename JniT::SpanType, JniT::kRank>::GetArrayElement(
-            Base::object_ref_, idx))};
+    return {AdoptLocal{},
+            static_cast<jarray>(
+                JniArrayHelper<typename JniT::SpanType,
+                               JniT::kRank>::GetArrayElement(Base::object_ref_,
+                                                             idx))};
   }
 
   template <typename SpanType, std::size_t kRank_, const auto& class_v_,

diff --git a/implementation/array_view.h b/implementation/array_view.h
@@ -24,6 +24,14 @@
 
 namespace jni {
 
+template <typename T>
+struct ArrayViewHelper {
+  const T& val_;
+  operator T() const { return val_; }
+
+  ArrayViewHelper(const T& val) : val_(val) {}
+};
+
 // Primitive Rank 1 Arrays.
 template <typename SpanType, std::size_t kRank = 1, typename Enable = void>
 class ArrayView {
@@ -145,12 +153,12 @@ class ArrayView<
       return tmp;
     }
 
-    PinHelper_t operator*() const {
+    ArrayViewHelper<PinHelper_t> operator*() const {
       if constexpr (kRank >= 2) {
-        return static_cast<PinHelper_t>(
-            JniArrayHelper<jobject, kRank>::GetArrayElement(arr_, idx_));
+        return {static_cast<PinHelper_t>(
+            JniArrayHelper<jobject, kRank>::GetArrayElement(arr_, idx_))};
       } else {
-        return JniArrayHelper<SpanType, kRank>::GetArrayElement(arr_, idx_);
+        return {JniArrayHelper<SpanType, kRank>::GetArrayElement(arr_, idx_)};
       }
     }
 

diff --git a/implementation/array_view_test.cc b/implementation/array_view_test.cc
@@ -23,6 +23,7 @@
 
 namespace {
 
+using ::jni::AdoptLocal;
 using ::jni::ArrayView;
 using ::jni::CDecl_t;
 using ::jni::Class;
@@ -95,14 +96,14 @@ TEST_F(JniTest, ArrayView_GetsAndReleaseArrayBuffer) {
                          Eq(Fake<jdoubleArray>()),
                          Eq(Fake<jdouble*>()), 0));
 
-  LocalArray<jboolean> boolean_array{Fake<jbooleanArray>()};
-  LocalArray<jbyte> byte_array{Fake<jbyteArray>()};
-  LocalArray<jchar> char_array{Fake<jcharArray>()};
-  LocalArray<jshort> short_array{Fake<jshortArray>()};
-  LocalArray<jint> int_array{Fake<jintArray>()};
-  LocalArray<jlong> long_array{Fake<jlongArray>()};
-  LocalArray<jfloat> float_array{Fake<jfloatArray>()};
-  LocalArray<jdouble> double_array{Fake<jdoubleArray>()};
+  LocalArray<jboolean> boolean_array{AdoptLocal{}, Fake<jbooleanArray>()};
+  LocalArray<jbyte> byte_array{AdoptLocal{}, Fake<jbyteArray>()};
+  LocalArray<jchar> char_array{AdoptLocal{}, Fake<jcharArray>()};
+  LocalArray<jshort> short_array{AdoptLocal{}, Fake<jshortArray>()};
+  LocalArray<jint> int_array{AdoptLocal{}, Fake<jintArray>()};
+  LocalArray<jlong> long_array{AdoptLocal{}, Fake<jlongArray>()};
+  LocalArray<jfloat> float_array{AdoptLocal{}, Fake<jfloatArray>()};
+  LocalArray<jdouble> double_array{AdoptLocal{}, Fake<jdoubleArray>()};
 
   ArrayView<jboolean, 1> boolean_array_pin = {boolean_array.Pin()};
   ArrayView<jbyte, 1> byte_array_pin = {byte_array.Pin()};
@@ -121,7 +122,7 @@ TEST_F(JniTest, LocalArrayView_AllowsCTAD) {
                          Eq(Fake<jbooleanArray>()),
                          Eq(Fake<jboolean*>()), 0));
 
-  LocalArray<jboolean> boolean_array{Fake<jbooleanArray>()};
+  LocalArray<jboolean> boolean_array{AdoptLocal{}, Fake<jbooleanArray>()};
   ArrayView ctad_array_view{boolean_array.Pin()};
 
   // Despite supporting construction from xvalue, move ctor is deleted (good).
@@ -300,7 +301,7 @@ TEST_F(JniTest, ArrayView_ShallowObjectsAreIterable) {
       .WillOnce(::testing::Return(Fake<jobject>(2)))
       .WillOnce(::testing::Return(Fake<jobject>(3)));
 
-  LocalArray<jobject> obj_arr{Fake<jobjectArray>()};
+  LocalArray<jobject> obj_arr{AdoptLocal{}, Fake<jobjectArray>()};
   ArrayView<jobject, 1> obj_view = obj_arr.Pin();
 
   EXPECT_TRUE(std::equal(obj_view.begin(), obj_view.end(), fake_vals.begin(),
@@ -318,7 +319,7 @@ TEST_F(JniTest, ArrayView_RichObjectsAreIterable) {
       .WillOnce(::testing::Return(Fake<jobject>(2)))
       .WillOnce(::testing::Return(Fake<jobject>(3)));
 
-  LocalArray<jobject, 1, kClass> obj_arr{Fake<jobjectArray>()};
+  LocalArray<jobject, 1, kClass> obj_arr{AdoptLocal{}, Fake<jobjectArray>()};
   auto obj_view = obj_arr.Pin();
 
   // Note: GlobalObject will fail to compile here. This is good, the user
@@ -350,7 +351,7 @@ TEST_F(JniTest, ArrayView_Rank2IntArraysAreIterable) {
   EXPECT_CALL(*env_, GetObjectArrayElement(Fake<jobjectArray>(), 2))
       .WillOnce(::testing::Return(Fake<jintArray>(3)));
 
-  LocalArray<jint, 2> int_arr_rank_2{Fake<jobjectArray>()};
+  LocalArray<jint, 2> int_arr_rank_2{AdoptLocal{}, Fake<jobjectArray>()};
   ArrayView<jint, 2> int_rank2_view = int_arr_rank_2.Pin();
 
   EXPECT_TRUE(std::equal(int_rank2_view.begin(), int_rank2_view.end(),
@@ -375,7 +376,7 @@ TEST_F(JniTest, ArrayView_Rank2ObjectkArraysAreIterable) {
   EXPECT_CALL(*env_, GetObjectArrayElement(Fake<jobjectArray>(), 2))
       .WillOnce(::testing::Return(Fake<jobjectArray>(3)));
 
-  LocalArray<jobject, 2> int_arr_rank_2{Fake<jobjectArray>()};
+  LocalArray<jobject, 2> int_arr_rank_2{AdoptLocal{}, Fake<jobjectArray>()};
   ArrayView<jobject, 2> int_rank2_view = int_arr_rank_2.Pin();
 
   EXPECT_TRUE(std::equal(int_rank2_view.begin(), int_rank2_view.end(),
@@ -403,7 +404,7 @@ TEST_F(JniTest, ArrayView_Rank3IntArraysAreIterable) {
   EXPECT_CALL(*env_, GetObjectArrayElement(Fake<jobjectArray>(), 2))
       .WillOnce(::testing::Return(Fake<jobjectArray>()));
 
-  LocalArray<jint, 3> int_arr_rank_3{Fake<jobjectArray>()};
+  LocalArray<jint, 3> int_arr_rank_3{AdoptLocal{}, Fake<jobjectArray>()};
   ArrayView<jint, 3> int_rank_3_view = int_arr_rank_3.Pin();
 
   EXPECT_TRUE(std::equal(int_rank_3_view.begin(), int_rank_3_view.end(),
@@ -426,7 +427,7 @@ TEST_F(JniTest, ArrayView_Rank3ObjectkArraysAreIterable) {
   EXPECT_CALL(*env_, GetObjectArrayElement(Fake<jobjectArray>(0), 2))
       .WillOnce(::testing::Return(Fake<jobjectArray>(3)));
 
-  LocalArray<jobject, 3> object_arr_rank_3{Fake<jobjectArray>(0)};
+  LocalArray<jobject, 3> object_arr_rank_3{AdoptLocal{}, Fake<jobjectArray>(0)};
   ArrayView<jobject, 3> object_rank_3_view = object_arr_rank_3.Pin();
 
   EXPECT_TRUE(std::equal(object_rank_3_view.begin(), object_rank_3_view.end(),

diff --git a/implementation/class_loader_ref_second_test.cc b/implementation/class_loader_ref_second_test.cc
@@ -6,6 +6,7 @@
 
 namespace {
 
+using ::jni::AdoptLocal;
 using ::jni::Class;
 using ::jni::ClassLoader;
 using ::jni::Constructor;
@@ -83,7 +84,7 @@ TEST_F(JniTestWithNoDefaultJvmRef,
   // Code under test.
   JvmRef<kClassLoaderJvm> jvm_ref{jvm_.get()};
   LocalClassLoader<kClassLoader, kClassLoaderJvm> class_loader{
-      Fake<jobject>(1)};
+      AdoptLocal{}, Fake<jobject>(1)};
 
   auto custom_loader_object = class_loader.BuildLocalObject<kClass>(jint{1});
 

diff --git a/implementation/class_loader_ref_test.cc b/implementation/class_loader_ref_test.cc
@@ -25,6 +25,7 @@
 namespace {
 
 using ::jni::AdoptGlobal;
+using ::jni::AdoptLocal;
 using ::jni::Class;
 using ::jni::ClassLoader;
 using ::jni::Constructor;
@@ -157,7 +158,8 @@ TEST_F(JniTestForClassLoaders,
        ClassLoaderRefTest_DefaultLoadedObjectBuildsWithClassLoadedObject) {
   JvmRef<kJvm> jvm_ref{jvm_.get()};
 
-  LocalClassLoader<kClassLoader, kJvm> local_class_loader{Fake<jobject>()};
+  LocalClassLoader<kClassLoader, kJvm> local_class_loader{AdoptLocal{},
+                                                          Fake<jobject>()};
   LocalObject<kClass1, kClassLoader> a =
       local_class_loader.BuildLocalObject<kClass1>();
   LocalObject<kClass2> b{a};
@@ -308,7 +310,7 @@ TEST_F(JniTestWithNoDefaultJvmRef,
   // Code under test.
   jni::JvmRef<atypical_jvm_definition> jvm_ref{jvm_.get()};
   jni::LocalObject<class_under_test, class_loader, atypical_jvm_definition>
-      obj1{Fake<jobject>(1)};
+      obj1{AdoptLocal{}, Fake<jobject>(1)};
   obj1("Foo");
 
   this->TearDown();

diff --git a/implementation/field_ref.h b/implementation/field_ref.h
@@ -29,7 +29,10 @@
 #include "implementation/jni_helper/field_value.h"
 #include "implementation/jni_helper/jni_helper.h"
 #include "implementation/jni_helper/static_field_value.h"
+#include "implementation/promotion_mechanics_tags.h"
 #include "implementation/proxy.h"
+#include "implementation/proxy_convenience_aliases.h"
+#include "implementation/ref_base.h"
 #include "jni_dep.h"
 #include "metaprogramming/double_locked_value.h"
 #include "metaprogramming/optional_wrap.h"
@@ -91,9 +94,16 @@ class FieldRef {
   }
 
   ReturnProxied Get() {
-    return {FieldHelper<CDecl_t<typename IdT::RawValT>, IdT::kRank,
-                        IdT::kIsStatic>::GetValue(SelfVal(),
-                                                  GetFieldID(class_ref_))};
+    if constexpr (std::is_base_of_v<RefBaseBase, ReturnProxied>) {
+      return {AdoptLocal{},
+              FieldHelper<CDecl_t<typename IdT::RawValT>, IdT::kRank,
+                          IdT::kIsStatic>::GetValue(SelfVal(),
+                                                    GetFieldID(class_ref_))};
+    } else {
+      return {FieldHelper<CDecl_t<typename IdT::RawValT>, IdT::kRank,
+                          IdT::kIsStatic>::GetValue(SelfVal(),
+                                                    GetFieldID(class_ref_))};
+    }
   }
 
   template <typename T>