Suspects LocalString would cause memory leak when created with a native string

[YiRanMushroom]

I was writing a Kotlin Scripting Engine and when I was trying to implement a native function wrapper, I wrote many tests to check if my implementation was correct.

Unfortunately, I found out there are some memory leaks, after over 5 hours of debugging, I found out that LocalStrings created by a native C++ String would not be released in time (if any of them would ever get released).

My question is whether this is the expected behaviour (I suspect not, since LocalObject and LocalArray have no such problem), and if it is the expected behaviour, what is the best way to pass a string to Java? Or whether and when this would be fixed if there is a bug.

Thank you for your attention to this matter.

[YiRanMushroom]

Here is the test case I was using:

int i = 0;
while (true) {
    for (int j = 0; j < 10000; j++) {
        LocalString str = "A very Long String.";
    }

    i++;
    if (i % 100 == 0) {
        std::cout << "Iteration: " << i << std::endl;
        // I even tried to manually call gc, but it has no effect in this case.
    }
}

This will crash in about 1 minute.

[jwhpryor]

Hi Yiran, thanks for filing this, I will take a look ASAP.

[YiRanMushroom]

After investigating the issue, I discovered that creating a LocalString involves two separate allocations: one for constructing a UTF8String, and another for creating the actual Java String instance. However, the local UTF8String created in the process is never released, leading to a potential memory leak.

I tried to fix this issue, but the template code was too complex to fully understand. Honestly, I think understanding someone else's C++ template metaprogramming code—or even my own after some time—is really hard unless one is already familiar with the whole system. There's basically no level of intelligence that makes it easy. That said, I tracked the possible leak to jni_bind_release.h at line 4517. The function there seems to take a jstring and create a Java String object, but it looks like the reference to the jstring is lost after that point, which causes a memory leak.

I made some changes to the function, and in all my tests, the memory leak seems to be resolved without causing any regressions.

Here is the modified version of the function:

  template <typename... CtorArgs>
  static inline jobject Construct(jclass clazz, jmethodID ctor_method,
                                  CtorArgs&&... ctor_args) {
    Trace(metaprogramming::LambdaToStr(STR("NewObject")), clazz, ctor_method,
          ctor_args...);

#ifdef DRY_RUN
    return Fake<jobject>();
#else
    // I changed some code here.
    auto obj = JniEnv::GetEnv()->NewObject(clazz, ctor_method, ctor_args...);

    ([]<typename T>(const T& obj){
      if constexpr (std::is_same_v<T, jstring>) {
        JniEnv::GetEnv()->DeleteLocalRef(obj);
      }
    }(ctor_args) , ...);

    return obj;
#endif  // DRY_RUN
  }

Please let me know when you have time whether this fix looks appropriate. I really appreciate your help.

[jwhpryor]

I took some time to try to repro this today and bizarrely I can't. I tried running your test under both unit and robolectric test, but it ran fine until the test fully timed out (which would be expected if it wasn't failing).

I'm going to look into your fix and backport it to the implementation. I'm both shocked and impressed you were able to modify the release header directly. I appreciate your help, and if I can confirm your failure by inspection I'll integrate your fix.

Thanks for your help!

[YiRanMushroom]

That is indeed strange. I’ve already applied this fix in my fork of jni-bind, and unfortunately, I’m not at home right now, so I can’t reproduce the issue using the exact code at the moment. However, I can still share a bug report from my original test case:

https://gist.github.com/YiRanMushroom/3453d0211994ad69a70cf57d56e423fc

In this report, you can see that the heap usage contains a significant amount of untracked memory, which suggests a memory leak. And JVM has unhandled exceptions of OutOfMemoryError.

I’m sorry I don’t have the report for the simplified test case, but I’m fairly confident that the issue isn’t caused by my implementation of NativeFunctionN. In fact, I observed this behavior (when testing using the case that I provided) by monitoring memory usage through Windows Task Manager. In that case, even after manually triggering GC (which usually keeps memory usage under 50MB), the memory usage kept increasing. It eventually went past 8GB, at which point I terminated the process because I believed something went wrong. I suspect the original tests failed quickly on my machine due to the powerful CPU, so when you’re running your test, could you try letting it run for at least 2–3 minutes, or monitor the memory usage when it is running?

Although I do call some low-level initialization code, I don’t believe it affects jni-bind’s allocation behavior.

If you're still unable to reproduce the issue, I’d really appreciate it if you could share the exact test setup you're using. I’d be happy to look into it further and help identify the root cause.

Thanks again for your attention to this!

[jwhpryor]

Ah, OK, when I observe the heap and make the allocations much bigger I do indeed see a growing heap. I'm taking a look right now, I'll get back to you when I have some update.

[jwhpryor]

So, I took a dive and I think what's happening is that OverloadRef is trying to always construct an object when calling Invoke as a constructor. It's causing it to take the object lifecycle path and not the string path. Why this is works at all is a bit confusing.

I tried patching your code in locally and I see all E2E tests passing but unit tests are failing. This might be OK, but it might not be so I have to dig. I think this is just leaking local references so if you returned to Java they would be cleaned up (which is likely why I haven't noticed this yet).

I'll let you know when I have an update.

[jwhpryor]

Ok, I think I have a more robust fix. The issue was an oversight in overload_ref.

The Invoke method falsely presumed that if it was being called as a constructor the constructing object would be a jobject, because, how could any object constructing itself be anything but an object?

This is unfortunately not true, because JNI differentiates jstring and jobject. Behind the scenes, JNI Bind was recognizing this wasn't correct and translating one to the other.

It never appeared on any memory sanitizer tests because it's not technically leaking memory, it's leaking objects. Unit tests enfore balancing of any newly created local as well as global, but NewStringUtf creates a local, but there's no enforcement to balance it with a DeleteLocalRef.

I was surprised your fix worked because I would have expected deleting a local reference would drop the reference count to 0, effectively making the object unusable. It was tremendously helpful in diagnosing the bug to have this as a lead, so I'm very appreciative for your effort.

I've created a pull request where you can preview the fix: https://github.com/google/jni-bind/compare/fix_bad_string_ctor?expand=1

It will take a bit for me to fix outstanding unit tests and regain permissions to commit this. I will update this bug when I have an update. I'd also like to ensure this behaviour isn't occurring with other types so I want to see if I can come up with a reliable test.

[YiRanMushroom]

Thanks a lot for your commitment to this issue!

I agree this seems like the correct approach to fix the problem. My workaround at the time was really just to confirm if the leak truly originated here — I was also actually surprised that deleting the local reference didn’t break any of my E2E tests.

I had suspected this area (the Invoke function) based on the difference in behavior between general object construction and jstring, but I wasn’t confident enough to modify it, especially since I wasn’t any familiar with how "Span" was working internally.

I’ll go through your PR carefully — I’m quite interested in template metaprogramming and this is a great learning opportunity. Thanks again for your effort and detailed explanation!

[jwhpryor]

You jumped way into the deep end starting with JNI Bind, but I'm happy to help! Feel free to add me on LinkedIn.

If you want a fun place to start, the metaprogramming directory has lot's of interesting self contained algorithms with test cases.

To debug your failure I downloaded Clion which has Bazel support and works great. It's extremely helpful for debugging, because you can see the flow.

Unfortunately, making changes can be difficult, especially in a place like OverloadRef because you're frequently compiling multiple code paths at once. It's not always obvious, but if multiple types go down the path, they all have to compile. constexpr can be a nice way to constrain this, but it's not always enough.

[jwhpryor]

Okay, I dug into this quite a bit more and my original fix was not quite right. I think a better solution than what I originally proposed is #416.

I'll give some details when I have the commit more polished.

[jwhpryor]

Hoo boy, alright, I think I finally have a proper solution.

It looks like I actually anticipated this a while back and then just hand waved assuming this would never be an issue because I didn't anticipate a usage like your own.

I'm still polishing, and it will be a little bit before I can submit it, but PTAL at #415.

Basically, NewStringUTF was allocating new strings in ProxyAsArg and never being released. I'm still a little unclear as to why your fix worked, but I"m confident the issue is the same.

I've added a special wrapped for proxy invocations that will conditionally extend the lifetime of the arg beyond its use, and then will properly tear it down. It appears to be a unique issue for strings because they alone will create new objects at proxy time.

Thanks again for bringing this to my attention, this was indeed a real and important bug.