From 8c26f2ecf226f7d16a48f7a94006b801e609c3ee Mon Sep 17 00:00:00 2001
From: Jamieson Pryor <jwhpryor@google.com>
Date: Mon, 26 Jun 2023 17:58:08 -0700
Subject: [PATCH] Fix broken promotion mechanics for richly decorated locals to
 globals.

**THIS IS A BREAKING CHANGE** Locals were being blindly absorbed by globals which is wrong. Clients are unlikely to be broken because this logic would cause crashes on use, however, I can't guarantee there isn't a path that relied on this.

PiperOrigin-RevId: 543592694
---
 implementation/global_object_test.cc | 29 ++++++++++++++++++++++++++--
 implementation/promotion_mechanics.h | 28 +++++++++++++++++++++++----
 2 files changed, 51 insertions(+), 6 deletions(-)

diff --git a/implementation/global_object_test.cc b/implementation/global_object_test.cc
index dcff448f..2da5c013 100644
--- a/implementation/global_object_test.cc
+++ b/implementation/global_object_test.cc
@@ -30,6 +30,7 @@ using ::jni::Constructor;
 using ::jni::CreateCopy;
 using ::jni::Field;
 using ::jni::GlobalObject;
+using ::jni::LocalObject;
 using ::jni::Method;
 using ::jni::Params;
 using ::jni::PromoteToGlobal;
@@ -75,7 +76,7 @@ TEST_F(JniTest, GlobalObject_DoesNotDeleteAnyLocalsForAdoptedGlobalJobject) {
 
   GlobalObject<kClass> global_object{AdoptGlobal{}, Fake<jobject>()};
 
-  EXPECT_NE(jobject{global_object}, nullptr);
+  EXPECT_EQ(jobject{global_object}, Fake<jobject>());
 }
 
 TEST_F(JniTest, GlobalObject_PromotesJobjectsOnConstruction) {
@@ -85,8 +86,32 @@ TEST_F(JniTest, GlobalObject_PromotesJobjectsOnConstruction) {
 
   static constexpr Class kClass{"kClass"};
   GlobalObject<kClass> global_object{PromoteToGlobal{}, Fake<jobject>()};
+  EXPECT_EQ(jobject{global_object}, AsGlobal(Fake<jobject>()));
+}
 
-  EXPECT_NE(jobject{global_object}, nullptr);
+TEST_F(JniTest, GlobalObject_PromotesDecoratedLocals) {
+  EXPECT_CALL(*env_, NewObjectV).Times(0);
+  EXPECT_CALL(*env_, DeleteLocalRef).Times(1);
+  EXPECT_CALL(*env_, DeleteGlobalRef(AsGlobal(Fake<jobject>())));
+
+  static constexpr Class kClass{"kClass"};
+  LocalObject<kClass> local_obj{Fake<jobject>()};
+  // GlobalObject<kClass> global_object{local_obj}; // doesn't compile (good).
+  GlobalObject<kClass> global_object{std::move(local_obj)};
+
+  EXPECT_EQ(jobject{global_object}, AsGlobal(Fake<jobject>()));
+}
+
+// Identical to above but Local constructed in place.
+TEST_F(JniTest, GlobalObject_PromotesDecoratedLocalsFromXValue) {
+  EXPECT_CALL(*env_, NewObjectV).Times(0);
+  EXPECT_CALL(*env_, DeleteLocalRef).Times(1);
+  EXPECT_CALL(*env_, DeleteGlobalRef(AsGlobal(Fake<jobject>())));
+
+  static constexpr Class kClass{"kClass"};
+  GlobalObject<kClass> global_object{LocalObject<kClass>{Fake<jobject>()}};
+
+  EXPECT_EQ(jobject{global_object}, AsGlobal(Fake<jobject>()));
 }
 
 TEST_F(JniTest, GlobalObject_CallsOnlyDeleteOnWrapCtor) {
diff --git a/implementation/promotion_mechanics.h b/implementation/promotion_mechanics.h
index 63267e66..9287c4c6 100644
--- a/implementation/promotion_mechanics.h
+++ b/implementation/promotion_mechanics.h
@@ -43,8 +43,7 @@ struct AdoptGlobal {};
 // Marks the end of `ScopeEntry` daisy chain.
 struct ScopedTerminalTag {};
 
-// Shared implementation common to all `Entry`.
-// template <typename Base, LifecycleType lifecycleType, typename JniT>
+// Shared implementation common to all *local* `Entry`.
 template <typename Base, LifecycleType lifecycleType, typename JniT,
           typename ViableSpan>
 struct EntryBase : public Base {
@@ -52,9 +51,9 @@ struct EntryBase : public Base {
   using Span = typename JniT::SpanType;
 
   // `RefBaseTag` move constructor for object of same span type.
-  template <typename T, typename = std::enable_if_t<
+  template <typename T, typename = std::enable_if_t<(
                             ::jni::metaprogramming::DeepEqual_v<EntryBase, T> ||
-                            std::is_base_of_v<RefBaseTag<Span>, T>>>
+                            std::is_base_of_v<RefBaseTag<Span>, T>)>>
   EntryBase(T&& rhs) : Base(rhs.Release()) {}
 
   // "Copy" constructor: Additional reference to object will be created.
@@ -79,6 +78,27 @@ struct Entry
       : Base(static_cast<typename JniT::StorageType>(object)) {}
 };
 
+// Shared implementation common to all *global* `Entry`.
+template <typename Base, typename JniT, typename ViableSpan>
+struct EntryBase<Base, LifecycleType::GLOBAL, JniT, ViableSpan> : public Base {
+  using Base::Base;
+  using Span = typename JniT::SpanType;
+
+  // `RefBaseTag` move constructor for object of same span type.
+  template <typename T, typename = std::enable_if_t<(
+                            ::jni::metaprogramming::DeepEqual_v<EntryBase, T> ||
+                            std::is_base_of_v<RefBaseTag<Span>, T>)>>
+  EntryBase(T&& rhs)
+      : Base(LifecycleHelper<typename JniT::StorageType,
+                             LifecycleType::GLOBAL>::Promote(rhs.Release())) {}
+
+  // "Copy" constructor: Additional reference to object will be created.
+  EntryBase(CreateCopy, ViableSpan object)
+      : Base(static_cast<Span>(
+            LifecycleHelper<Span, LifecycleType::GLOBAL>::NewReference(
+                static_cast<Span>(object)))) {}
+};
+
 // Global scoped entry augmentation.
 template <typename JniT, typename ViableSpan, typename... ViableSpans>
 struct Entry<LifecycleType::GLOBAL, JniT, ViableSpan, ViableSpans...>