Skip to content

Commit

Permalink
CreateCopy is now the default for locals.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
jwhpryor authored and copybara-github committed Nov 12, 2023
1 parent 58e8b53 commit 0951bea
Show file tree
Hide file tree
Showing 25 changed files with 304 additions and 137 deletions.
13 changes: 13 additions & 0 deletions implementation/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ cc_library(
":array_view",
":class",
":default_class_loader",
":forward_declarations",
":jni_type",
":local_object",
":object_ref",
Expand Down Expand Up @@ -292,7 +293,9 @@ cc_library(
":field_selection",
":id",
":id_type",
":promotion_mechanics_tags",
":proxy",
":proxy_convenience_aliases",
"//:jni_dep",
"//implementation/jni_helper",
"//implementation/jni_helper:field_value_getter",
Expand Down Expand Up @@ -594,6 +597,7 @@ cc_library(
":class",
":class_loader",
":class_loader_ref",
":forward_declarations",
":jvm",
":jvm_ref",
":local_object",
Expand All @@ -610,6 +614,7 @@ cc_library(
deps = [
":class",
":class_loader",
":forward_declarations",
":jni_type",
":jvm",
":jvm_ref",
Expand All @@ -635,6 +640,7 @@ cc_library(
name = "local_string",
hdrs = ["local_string.h"],
deps = [
":forward_declarations",
":local_object",
":promotion_mechanics",
":ref_base",
Expand Down Expand Up @@ -663,6 +669,7 @@ cc_library(
":jni_type",
":method",
":params",
":promotion_mechanics_tags",
":proxy",
":proxy_convenience_aliases",
":proxy_definitions",
Expand Down Expand Up @@ -819,6 +826,7 @@ cc_library(
":forward_declarations",
":jni_type",
":object_ref",
":promotion_mechanics_tags",
":ref_base",
"//:jni_dep",
"//implementation/jni_helper",
Expand All @@ -829,6 +837,11 @@ cc_library(
],
)

cc_library(
name = "promotion_mechanics_tags",
hdrs = ["promotion_mechanics_tags.h"],
)

cc_library(
name = "proxy",
hdrs = ["proxy.h"],
Expand Down
37 changes: 23 additions & 14 deletions implementation/array_ref.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "implementation/array_view.h"
#include "implementation/class.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"
Expand All @@ -48,7 +49,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)) {}

Expand Down Expand Up @@ -79,17 +85,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() {
Expand All @@ -108,8 +115,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());
}
};

Expand Down Expand Up @@ -158,9 +165,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_,
Expand Down
16 changes: 12 additions & 4 deletions implementation/array_view.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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_)};
}
}

Expand Down
31 changes: 16 additions & 15 deletions implementation/array_view_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

namespace {

using ::jni::AdoptLocal;
using ::jni::ArrayView;
using ::jni::CDecl_t;
using ::jni::Class;
Expand Down Expand Up @@ -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()};
Expand All @@ -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).
Expand Down Expand Up @@ -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(),
Expand All @@ -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
Expand Down Expand Up @@ -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(),
Expand All @@ -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(),
Expand Down Expand Up @@ -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(),
Expand All @@ -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(),
Expand Down
3 changes: 2 additions & 1 deletion implementation/class_loader_ref_second_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

namespace {

using ::jni::AdoptLocal;
using ::jni::Class;
using ::jni::ClassLoader;
using ::jni::Constructor;
Expand Down Expand Up @@ -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});

Expand Down
6 changes: 4 additions & 2 deletions implementation/class_loader_ref_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
namespace {

using ::jni::AdoptGlobal;
using ::jni::AdoptLocal;
using ::jni::Class;
using ::jni::ClassLoader;
using ::jni::Constructor;
Expand Down Expand Up @@ -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};
Expand Down Expand Up @@ -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();
Expand Down
15 changes: 12 additions & 3 deletions implementation/field_ref.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@
#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 "jni_dep.h"
#include "metaprogramming/double_locked_value.h"
#include "metaprogramming/optional_wrap.h"
Expand Down Expand Up @@ -91,9 +93,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>
Expand Down
5 changes: 3 additions & 2 deletions implementation/field_ref_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

namespace {

using ::jni::AdoptLocal;
using ::jni::Class;
using ::jni::Field;
using ::jni::GlobalObject;
Expand Down Expand Up @@ -159,9 +160,9 @@ TEST_F(JniTest, Field_ObjectSet) {
Field{"classField", kClass2}};

LocalObject<kClass> obj{};
LocalObject<kClass2> some_obj{Fake<jobject>()};
LocalObject<kClass2> some_obj{AdoptLocal{}, Fake<jobject>()};
obj["classField"].Set(Fake<jobject>());
obj["classField"].Set(LocalObject<kClass2>{Fake<jobject>()});
obj["classField"].Set(LocalObject<kClass2>{AdoptLocal{}, Fake<jobject>()});
obj["classField"].Set(some_obj);
obj["classField"].Set(std::move(some_obj));
}
Expand Down
4 changes: 4 additions & 0 deletions implementation/forward_declarations.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ namespace jni {
#define JNI_BIND_C_ENTRYPOINT(class_name, return_type, method_name, ...) \
return_type class_name_##method_name(JNIEnv*, jclass, ##__VA_ARGS__)

// ArrayView Helper.
template <typename T>
struct ArrayViewHelper;

// Id.
template <typename JniT_, IdType kIdType_, std::size_t idx,
std::size_t secondary_idx, std::size_t tertiary_idx>
Expand Down
Loading

0 comments on commit 0951bea

Please sign in to comment.