Fix bad string ctor

.clang-format

@@ -0,0 +1,12 @@
+BasedOnStyle: Google
+IndentWidth: 2
+ColumnLimit: 80
+AllowShortFunctionsOnASingleLine: Empty
+DerivePointerAlignment: false
+PointerAlignment: Left
+BreakBeforeBraces: Attach
+AllowShortIfStatementsOnASingleLine: false
+SpacesInAngles: false
+SpacesInCStyleCastParentheses: false
+Cpp11BracedListStyle: true
+Standard: Latest

.gitignore

@@ -1,5 +1,9 @@
 bazel*
-.vscode
+.vscode/
+.clwb/
+.ijwb/
+.idea/
+.vscode/
 MODULE.bazel.lock
 cmake_build
-cmake_install
+cmake_install

implementation/BUILD

@@ -294,6 +294,7 @@ cc_library(
     name = "field_ref",
     hdrs = ["field_ref.h"],
     deps = [
+        ":proxy_temporary",
         ":default_class_loader",
         ":field_selection",
         ":id",
@@ -788,6 +789,7 @@ cc_library(
     name = "method_ref",
     hdrs = ["overload_ref.h"],
     deps = [
+        ":proxy_temporary",
         ":configuration",
         ":id_type",
         ":promotion_mechanics_tags",
@@ -1052,6 +1054,7 @@ cc_library(
     name = "proxy_definitions_string",
     hdrs = ["proxy_definitions_string.h"],
     deps = [
+        ":proxy_temporary",
         ":default_class_loader",
         ":forward_declarations",
         ":jvm",
@@ -1076,6 +1079,14 @@ cc_test(
     ],
 )
 
+################################################################################
+# ProxyTemporary.
+################################################################################
+cc_library(
+    name = "proxy_temporary",
+    hdrs = ["proxy_temporary.h"],
+)
+
 ################################################################################
 # RefBase.
 ################################################################################

implementation/field_ref.h

@@ -22,8 +22,10 @@
 #include <cstddef>
 #include <type_traits>
 #include <utility>
+#include <utility>
 #include <vector>
 
+#include "implementation/proxy_temporary.h"
 #include "implementation/default_class_loader.h"
 #include "implementation/field_selection.h"
 #include "implementation/id.h"
@@ -115,8 +117,9 @@ class FieldRef {
   void Set(T&& value) {
     FieldHelper<CDecl_t<typename IdT::RawValT>, IdT::kRank,
                 IdT::kIsStatic>::SetValue(SelfVal(), GetFieldID(class_ref_),
-                                          Proxy_t<T>::ProxyAsArg(
-                                              std::forward<T>(value)));
+                                          ForwardWithProxyTemporaryStrip(
+                                              Proxy_t<T>::ProxyAsArg(
+                                                  std::forward<T>(value))));
   }
 
  private:

implementation/jni_helper/BUILD

@@ -26,22 +26,6 @@ cc_test(
     ],
 )
 
-################################################################################
-# DryRun.
-################################################################################
-cc_test(
-    name = "dry_run_test",
-    srcs = ["dry_run_test.cc"],
-    defines = [
-        "DRY_RUN",
-        "ENABLE_DEBUG_OUTPUT",
-    ],
-    deps = [
-        "//:jni_bind",
-        "@googletest//:gtest_main",
-    ],
-)
-
 ################################################################################
 # Fake Test Constants.
 ################################################################################

implementation/legacy/local_array_string_test.cc

@@ -109,9 +109,8 @@ TEST_F(JniTest, Array_LocalVanillaObjectRValuesCanBeSet) {
   // In the future, this should drop to 1.
   EXPECT_CALL(*env_, FindClass(StrEq("java/lang/String"))).Times(2);
 
-  EXPECT_CALL(*env_, DeleteLocalRef(_)).Times(2);  // array, in place obj
+  EXPECT_CALL(*env_, DeleteLocalRef(_)).Times(3);  // array, in place obj
   EXPECT_CALL(*env_, DeleteLocalRef(Fake<jclass>())).Times(2);  // FindClass
-  EXPECT_CALL(*env_, DeleteLocalRef(Fake<jstring>())).Times(0);
 
   LocalArray<jobject, 1, kJavaLangString> arr{
       3, LocalObject<kJavaLangString>{"Foo"}};

implementation/legacy/string_ref_test.cc

@@ -111,11 +111,10 @@ TEST_F(JniTest, LocalString_CreatesFromStringView) {
 
   // jclass for temp String class reference.
   EXPECT_CALL(*env_, DeleteLocalRef(Fake<jclass>()));
+  // Temporary xref created during construction from NewStringUTF.
+  EXPECT_CALL(*env_, DeleteLocalRef(Fake<jstring>()));
   // The variable str (which is itself an object).
   EXPECT_CALL(*env_, DeleteLocalRef(Fake<jobject>()));
-  // TODO(b/143908983): Currently strings leak one local during proxying.
-  // Temporary xref created during construction.
-  // EXPECT_CALL(*env_, DeleteLocalRef(Fake<jstring>()));
 
   LocalString str{std::string_view{char_ptr}};
 }
@@ -126,11 +125,11 @@ TEST_F(JniTest, LocalString_CreatesFromString) {
 
   // jclass for temp String class reference.
   EXPECT_CALL(*env_, DeleteLocalRef(Fake<jclass>()));
+  // Temporary xref created during construction from NewStringUTF.
+  EXPECT_CALL(*env_, DeleteLocalRef(Fake<jstring>()));
   // The variable str (which is itself an object).
   EXPECT_CALL(*env_, DeleteLocalRef(Fake<jobject>()));
-  // TODO(b/143908983): Currently strings leak one local during proxying.
-  // Temporary xref created during construction.
-  // EXPECT_CALL(*env_, DeleteLocalRef(Fake<jstring>()));
+
   LocalString str{std::string{"TestString"}};
 }
 

implementation/local_array_string_test.cc

@@ -107,9 +107,8 @@ TEST_F(JniTest, Array_LocalVanillaObjectRValuesCanBeSet) {
   // In the future, this should drop to 1.
   EXPECT_CALL(*env_, FindClass(StrEq("java/lang/String"))).Times(2);
 
-  EXPECT_CALL(*env_, DeleteLocalRef(_)).Times(2);  // array, in place obj
+  EXPECT_CALL(*env_, DeleteLocalRef(_)).Times(3);  // array, in place obj
   EXPECT_CALL(*env_, DeleteLocalRef(Fake<jclass>())).Times(2);  // FindClass
-  EXPECT_CALL(*env_, DeleteLocalRef(Fake<jstring>())).Times(0);
 
   LocalArray<jobject, 1, kJavaLangString> arr{
       3, LocalObject<kJavaLangString>{"Foo"}};

implementation/overload_ref.h

@@ -102,25 +102,33 @@ struct OverloadRef {
     if constexpr (std::is_same_v<ReturnProxied, void>) {
       return InvokeHelper<void, kRank, kStatic>::Invoke(
           object, clazz, mthd,
-          Proxy_t<Params>::ProxyAsArg(std::forward<Params>(params))...);
+          ForwardWithProxyTemporaryStrip(Proxy_t<Params>::ProxyAsArg(
+              std::forward<Params>(params)))...);
     } else if constexpr (IdT::kIsConstructor) {
       return ReturnProxied{
           AdoptLocal{},
           LifecycleHelper<jobject, LifecycleType::LOCAL>::Construct(
               clazz, mthd,
-              Proxy_t<Params>::ProxyAsArg(std::forward<Params>(params))...)};
+              ForwardWithProxyTemporaryStrip(
+                  Proxy_t<Params>::ProxyAsArg(
+                      std::forward<Params>(params)))...)};
     } else {
       if constexpr (std::is_base_of_v<RefBaseBase, ReturnProxied>) {
-        return ReturnProxied{
-            AdoptLocal{},
-            InvokeHelper<typename ReturnIdT::CDecl, kRank, kStatic>::Invoke(
-                object, clazz, mthd,
-                Proxy_t<Params>::ProxyAsArg(std::forward<Params>(params))...)};
+          return 
+              ReturnProxied{
+                  AdoptLocal{},
+                      InvokeHelper<typename ReturnIdT::CDecl, kRank, kStatic>::Invoke(
+                              object, clazz, mthd,
+                              ForwardWithProxyTemporaryStrip(
+                                  Proxy_t<Params>::ProxyAsArg(std::forward<Params>(params)))
+                              ...)};
       } else {
         return static_cast<ReturnProxied>(
-            InvokeHelper<typename ReturnIdT::CDecl, kRank, kStatic>::Invoke(
-                object, clazz, mthd,
-                Proxy_t<Params>::ProxyAsArg(std::forward<Params>(params))...));
+          InvokeHelper<typename ReturnIdT::CDecl, kRank, kStatic>::Invoke(
+              object, clazz, mthd,
+              ForwardWithProxyTemporaryStrip(
+                  Proxy_t<Params>::ProxyAsArg(
+                      std::forward<Params>(params)))...));
       }
     }
   }

implementation/proxy_definitions_string.h

@@ -32,12 +32,12 @@
 #include "implementation/jni_helper/lifecycle_string.h"
 #include "implementation/jvm.h"
 #include "implementation/proxy.h"
+#include "implementation/proxy_temporary.h"
 #include "implementation/proxy_convenience_aliases.h"
 #include "implementation/ref_base.h"
 #include "jni_dep.h"
 
 namespace jni {
-
 template <typename JString>
 struct Proxy<JString,
              typename std::enable_if_t<std::is_same_v<JString, jstring>>>
@@ -68,22 +68,36 @@ struct Proxy<JString,
       IsConvertibleKey<T>::template value<std::string_view> ||
       std::is_same_v<T, LocalString> || std::is_same_v<T, GlobalString>;
 
+  static constexpr auto DeleteLambda = [](const jstring& s) {
+    JniEnv::GetEnv()->DeleteLocalRef(static_cast<jobject>(s));
+  };
+
+  struct DeleteLocalRef {
+    static void Call(const jstring& s) {
+      JniEnv::GetEnv()->DeleteLocalRef(static_cast<jobject>(s));
+    }
+  };
+
   // These leak local instances of strings.  Usually, RAII mechanisms would
   // correctly release local instances, but here we are stripping that so it can
   // be used in a method.  This could be obviated by wrapping the calling scope
   // in a local stack frame.
-  static jstring ProxyAsArg(jstring s) { return s; }
+  static jstring ProxyAsArg(jstring s) {
+    return s;
+  }
 
+  // Note: Because a temporary is created `ProxyTemporary` is used to
+  // guarantee the release of the underlying local after use in `ProxyAsArg`.
   template <typename T,
             typename = std::enable_if_t<std::is_same_v<T, const char*> ||
                                         std::is_same_v<T, std::string> ||
                                         std::is_same_v<T, std::string_view>>>
-  static jstring ProxyAsArg(T s) {
+  static ProxyTemporary<jstring, DeleteLocalRef> ProxyAsArg(T s) {
     if constexpr (std::is_same_v<T, const char*>) {
-      return LifecycleHelper<jstring, LifecycleType::LOCAL>::Construct(s);
+      return {LifecycleHelper<jstring, LifecycleType::LOCAL>::Construct(s)};
     } else {
-      return LifecycleHelper<jstring, LifecycleType::LOCAL>::Construct(
-          s.data());
+      return {LifecycleHelper<jstring, LifecycleType::LOCAL>::Construct(
+          s.data())};
     }
   }
 
@@ -101,7 +115,6 @@ struct Proxy<JString,
     return t.Release();
   }
 };
+} // namespace jni
 
-}  // namespace jni
-
-#endif  // JNI_BIND_IMPLEMENTATION_PROXY_DEFINITIONS_STRING_H_
+#endif  // JNI_BIND_IMPLEMENTATION_PROXY_DEFINITIONS_STRING_H_

implementation/proxy_temporary.h

@@ -0,0 +1,53 @@
+#ifndef JNI_BIND_IMPLEMENTATION_PROXY_TEMPORARY_H_
+#define JNI_BIND_IMPLEMENTATION_PROXY_TEMPORARY_H_
+
+namespace jni {
+
+struct ProxyTemporaryBase {};
+
+// Some values built in proxy_definitions are intended to be ephemeral.
+// Because the full decorated LocalObject is not defined, we use this 
+// class to proxy the value for the duration of `ProxyAsArg`, but allow it to
+// immediately be destroyed after the call.
+//
+// See https://github.com/google/jni-bind/issues/414.
+template <typename T, typename DtorLambda>
+struct ProxyTemporary : ProxyTemporaryBase {
+  ProxyTemporary(T t) : t_(t) { }
+
+  ~ProxyTemporary() {
+    DtorLambda::Call(t_);
+  }
+
+  const T& Get() const { return t_; }
+
+  T t_;
+};
+
+
+namespace detail {
+
+template <typename T>
+const auto& ForwardWithProxyTemporaryStripImpl(T&& t, std::true_type /* is_ref_base */) {
+  return t.Get();
+}
+
+template <typename T>
+decltype(auto) ForwardWithProxyTemporaryStripImpl(
+    T&& t, std::false_type /* not ref base */) {
+  return std::forward<T>(t); // perfect forward
+}
+
+} // namespace detail
+
+template <typename T>
+decltype(auto) ForwardWithProxyTemporaryStrip(T&& t) {
+  using U = std::remove_reference_t<T>;
+  return detail::ForwardWithProxyTemporaryStripImpl(
+      std::forward<T>(t),
+      std::is_base_of<ProxyTemporaryBase, U>{});
+}
+
+} // namespace jni
+
+#endif // JNI_BIND_IMPLEMENTATION_PROXY_TEMPORARY_H_

implementation/string_ref_test.cc

@@ -109,11 +109,10 @@ TEST_F(JniTest, LocalString_CreatesFromStringView) {
 
   // jclass for temp String class reference.
   EXPECT_CALL(*env_, DeleteLocalRef(Fake<jclass>()));
+  // Temporary xref created during construction from NewStringUTF.
+  EXPECT_CALL(*env_, DeleteLocalRef(Fake<jstring>()));
   // The variable str (which is itself an object).
   EXPECT_CALL(*env_, DeleteLocalRef(Fake<jobject>()));
-  // TODO(b/143908983): Currently strings leak one local during proxying.
-  // Temporary xref created during construction.
-  // EXPECT_CALL(*env_, DeleteLocalRef(Fake<jstring>()));
 
   LocalString str{std::string_view{char_ptr}};
 }
@@ -124,11 +123,11 @@ TEST_F(JniTest, LocalString_CreatesFromString) {
 
   // jclass for temp String class reference.
   EXPECT_CALL(*env_, DeleteLocalRef(Fake<jclass>()));
+  // Temporary xref created during construction from NewStringUTF.
+  EXPECT_CALL(*env_, DeleteLocalRef(Fake<jstring>()));
   // The variable str (which is itself an object).
   EXPECT_CALL(*env_, DeleteLocalRef(Fake<jobject>()));
   // TODO(b/143908983): Currently strings leak one local during proxying.
-  // Temporary xref created during construction.
-  // EXPECT_CALL(*env_, DeleteLocalRef(Fake<jstring>()));
   LocalString str{std::string{"TestString"}};
 }
 

javatests/com/jnibind/test/StringTest.java

@@ -146,4 +146,11 @@ public void nullStringTests() {
   public void globalReturnsCorrectlyOverJniBoundary() {
     jniReturnsAGlobalString();
   }
+
+  native void nativeAllocationThrash();
+
+  @Test
+  public void allocationThrash() {
+    nativeAllocationThrash();
+  }
 }